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