> On Aug 4, 2020, at 5:30 PM, Samuel Thibault <samuel.thiba...@gnu.org> wrote: > > 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.
Please bear with me as I am slow here. The interrupt handler does not touch the queue. It works on the user_intr_t pointer. So we use simple_lock to protect again other threads, but we do not need splhigh/splx to protect us against the interrupt handler. >>>> @@ -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. We can protect __enable_irq and __disable_irq with a lock. But that is tricky, because __disable_irq is called in the interrupt handler. Another thread holding a lock while interrupt happens causes a deadlock. > >>>> 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. What other stuff though? The queue is is only manipulation by the intr_thread and registration. No other threads or interrupts mess with the queue. Junling