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

[Junling Ma]

> On Aug 4, 2020, at 5:30 PM, Samuel Thibault <samuel.thibault@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