bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] user_intr: a lock to protect main_intr_queue


From: Samuel Thibault
Subject: Re: [PATCH] user_intr: a lock to protect main_intr_queue
Date: Wed, 5 Aug 2020 02:30:30 +0200
User-agent: NeoMutt/20170609 (1.8.3)

Junling Ma, le mar. 04 août 2020 17:08:03 -0700, a ecrit:
> >> static user_intr_t *
> >> search_intr (struct irqdev *dev, ipc_port_t dst_port)
> >> {
> >>   user_intr_t *e;
> >> -  queue_iterate (dev->intr_queue, e, user_intr_t *, chain)
> >> +  simple_lock(&dev->lock);
> >> +  queue_iterate (&dev->intr_queue, e, user_intr_t *, chain)
> >>     {
> >>       if (e->dst_port == dst_port)
> >> -  return e;
> >> +        {
> >> +          simple_unlock(&dev->lock);
> >> +          return e;
> >> +        }
> >>     }
> >> +  simple_unlock(&dev->lock);
> >>   return NULL;
> >> }
> > 
> > I believe you want to make the *caller* lock, here. Otherwise another
> > CPU could be removing the entry you have just found. That's actually the
> > point of splhigh/splx in the callers.
> 
> I think splhigh/splx disable interrupts, but the interrupt handler will not 
> touch the queue any more, only the intr_thread does. Wouldn’t a simple lock 
> be sufficient?

You not only have the interrupt handler, but also other processors doing
stuff.

Note: simple_lock is optimized to void in uni-processor builds. simple
locks are there only to handle multi-processor cases. They just cannot
protect at all against an interrupt, that's what splhigh is for.

> >> @@ -64,7 +77,7 @@ irq_acknowledge (ipc_port_t receive_port)
> >>   if (irqtab.irqdev_ack)
> >>     (*(irqtab.irqdev_ack)) (&irqtab, e->id);
> >> 
> >> -  __enable_irq (irqtab.irq[e->id]);
> >> +  PROTECT(irqtab.lock, __enable_irq (irqtab.irq[e->id]));
> > 
> > I don't think we need a lock here. Yes, we need to protect
> > __enable_irq's stuff, but I don't think it's device/intr.c's duty to do
> > this. __enable_irq actually already has some of it, we'd just need to
> > add a simple lock in addition (and possibly introduce a simple+splhigh
> > helper to do this)
> 
> You are right. Actually, we probably do not need to lock at all, These ack 
> calls are serialized at the msg server, so they cannot mess each other. The 
> intr_thread does not enable irq unless the port is dead, in which case there 
> would be no ack calls.

I don't think we want to base safety over the precise behavior of other
code, but only over simple conventions that are properly documented.

> >>      if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
> >> @@ -231,6 +244,7 @@ intr_thread (void)
> >>          s = splhigh ();
> >>        }
> >>    }
> >> +      simple_unlock(&irqtab.lock);
> > 
> > We probably want to unlock earlier than this, before we call
> > deliver_intr. Otherwise we keep the lock busy while doing the delivery.
> > Even worse with the kfree call. Again, basically follow the existing
> > splhigh/splx, it already provides the uniprocessor concerns which
> > already have the problem of interrupts happening concurrently.
> 
> Yes we only need to protect the dead port deletion part to prevent device 
> registratio from appending to the queue while we delete here. The delivery 
> loop should be safe without locking.

No, it's not completely safe, you need to protect against other
processors doing other stuff.

Samuel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]