Re: [PATCH] irqdev: remove tot_num_intr

[Jessica Clarke]

On 4 Aug 2020, at 22:47, Junling Ma <junlingm@gmail.com> wrote:
> 
> The tot_num_intr field is a count of how many deliverable interrupts across 
> all lines. When we move
> to the scheme of blocking read for request and write for acking, it is 
> possible that an interrupt
> can happen during a small period that the interrupt is acked, but the read 
> has not happended yet.
> If it occurs, then the interrupt is not deliverable, so we cannot decrease 
> the interrupts counter
> and tot_num_intr, otherwise we lose interrupts. But not decreasing 
> tot_num_intr causes an infinit
> loop in intr_thread. Thus, instead, we remove the tot_num_intr cvounter, and 
> count the number of
> deliverable interrupts in intr_thread.
> 
> ---
> device/intr.c   | 10 ++++------
> device/intr.h   |  1 -
> i386/i386/irq.c |  1 -
> 3 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/device/intr.c b/device/intr.c
> index b60a6a28..ba86bc2d 100644
> --- a/device/intr.c
> +++ b/device/intr.c
> @@ -98,7 +98,6 @@ deliver_user_intr (int id, void *dev_id, struct pt_regs 
> *regs)
>   __disable_irq (irqtab.irq[id]);
>   e->n_unacked++;
>   e->interrupts++;
> -  irqtab.tot_num_intr++;
>   splx(s);
> 
>   thread_wakeup ((event_t) &intr_thread);
> @@ -182,15 +181,14 @@ intr_thread (void)
>             printf("irq handler [%d]: removed entry %p\n", id, e);
>             /* if e is the last handler registered for irq ID, then remove 
> the linux irq handler */
>             free_irq(id, e);
> -           if (e->interrupts != 0)
> -             irqtab.tot_num_intr -= e->interrupts;
>             kfree ((vm_offset_t) e, sizeof (*e));
>             e = next;
>           }
>       }
> 
>       /* Now check for interrupts */
> -      while (irqtab.tot_num_intr)
> +      int total = 0;

This is not at the start of a block.

> +      do
>       {
>         int del = 0;
> 
> @@ -210,8 +208,7 @@ intr_thread (void)
>                 clear_wait (current_thread (), 0, 0);
>                 id = e->id;
>                 dst_port = e->dst_port;
> -               e->interrupts--;
> -               irqtab.tot_num_intr--;
> +               total += --e->interrupts;
> 
>                 splx (s);
>                 deliver_intr (id, dst_port);
> @@ -237,6 +234,7 @@ intr_thread (void)
>             s = splhigh ();
>           }
>       }
> +      while (total != 0);

This is clearly broken if the loop ever needs to execute more than once.

Jess

>       simple_unlock(&irqtab.lock);
>       splx (s);
>       thread_block (NULL);
> diff --git a/device/intr.h b/device/intr.h
> index b1c09e6c..d4f98ca3 100644
> --- a/device/intr.h
> +++ b/device/intr.h
> @@ -44,7 +44,6 @@ struct irqdev {
> 
>   queue_head_t intr_queue;
>   decl_simple_lock_data(, lock);/* a lock to protect the intr_queue */
> -  int tot_num_intr; /* Total number of unprocessed interrupts */
> 
>   /* Machine dependent */
>   irq_t irq[NINTR];
> diff --git a/i386/i386/irq.c b/i386/i386/irq.c
> index 4ef1c43f..78375b07 100644
> --- a/i386/i386/irq.c
> +++ b/i386/i386/irq.c
> @@ -68,7 +68,6 @@ void irq_init()
>   irqtab.irqdev_ack = irq_eoi;
>   queue_init (&irqtab.intr_queue);
>   simple_lock_init(&irqtab.lock);
> -  irqtab.tot_num_intr = 0;
>   for (int i = 0; i < NINTR; ++i)
>     irqtab.irq[i] = i;
> }
> -- 
> 2.28.0.rc1
> 
>