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

[Junling Ma]

Hi Samual,

> On Aug 4, 2020, at 4:51 PM, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> 
> Junling Ma, le dim. 02 août 2020 21:04:24 -0700, a ecrit:
>> -struct irqdev irqtab = {
>> -  "irq", irq_eoi, &main_intr_queue, 0,
>> -  {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15},
>> -};
>> +struct irqdev irqtab;
>> +
>> +void irq_init()
>> +{  
>> +  irqtab.name = "irq";
>> +  irqtab.irqdev_ack = irq_eoi;
>> +  irqtab.tot_num_intr = 0;
> 
> Err, why doing this dynamically at initialization time while we can do
> it statically once for good at compile-time?
> 
>> +  queue_init (&irqtab.intr_queue);
>> +  simple_lock_init(&irqtab.lock);
> 
> Please rather introduce initializers in queue.h and lock.h, so that
> again it's initialized statically. Yes, for the queue you will need to
> pass the initializer the address of the structure field, that is fine
> and expected for such a macro.

Yes that makes sense.

> 
>> +  for (int i = 0; i < NINTR; ++i)
>> +    irqtab.irq[i] = i;
>> +}
> 
> I'd still rather see it initialized statically.

In the future, we may have LAPIC and IOAPIC, so the array may not be determined 
at compile time For the simple ASPIC, there would be no problem to statically 
init it.

>> +#define PROTECT(lock, critical_section) \
>> +{\
>> +  simple_lock(&lock);\
>> +  critical_section;\
>> +  simple_unlock(&lock);\
>> +}
> 
> As mentioned on IRC, please make this a SIMPLE_LOCKED macro in lock.h
> itself, it can be useful beyond this file.

Sure.

>> 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?

>> @@ -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.


>> @@ -163,6 +175,7 @@ intr_thread (void)
>>       spl_t s = splhigh ();
>> 
>>       /* Check for aborted processes */
>> +      simple_lock(&irqtab.lock);
>>       queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
> 
> Please move the lock above the "Check" comment. Otherwise we'd think we
> lock only for the aborted check, while we lock also for the interrupt
> check.

Ok.

>>      {
>>        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.

> Note: after the deliver_intr call, we will want to break as well, in
> order to restart iterating over the queue again, otherwise the current
> entry may have disappeared and you'd iterate in the space.

I guess we should simple check if the port is dead before delivery. Ah, that is 
why the old code marks for freeing, and free after the delivery loop. I 
didn’t;t get that. So, we check if the port is dead, and if so, we free the 
port and set it to NULL, and continue. After the delivery loop finishes, we 
unregister and free all the dead user_intr_t.

Junling