[PATCH] irqdev: remove tot_num_intr

[Junling Ma]

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;
+ 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);
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

[Jessica Clarke]

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

[Junling Ma]

Hi Jess,

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

[Samuel Thibault]

Junling Ma, le mar. 04 août 2020 14:47:09 -0700, a ecrit:

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

As discussed on IRC, this is rather a problem of the device_read/write
concept, that would require device_read() to be called before the
interrupt raises, just because on the kernel side the mach_device_t is
the same for all userland processes sharing the IRQ. I don't think it's
a path we want to follow, and we'd rather use register/ack IPC like we
have now.

Samuel

[Junling Ma]

In summary, there is no easy way for device_* interface to know who called. So registering a notification port is a simple and sane way, We only need to move device_intr_register/ack to irq_register/ack into something like irq.defs, so that other devices are freed from having to implement these two device_* called that are only meant for the irq device.

Best,
Junling