> 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
