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

[Samuel Thibault]

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.

> +  for (int i = 0; i < NINTR; ++i)
> +    irqtab.irq[i] = i;
> +}

I'd still rather see it initialized statically.

> +#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.

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

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

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

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

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.

Samuel