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
