Re: [PATCH] irqdev: remove tot_num_intr

[Junling Ma]

Hi Jess,

> On Aug 4, 2020, at 3:22 PM, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> 
> 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.

No this is not start of a block. It follows the previous queue_iterate loop. I 
just changed the while loop to a do-while loop. It depends on the previous 
patches that I sent.
> 
>> +      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.

You are right. I should have initialized the total *inside* the do loop. Will 
fix that.

Junling