[PATCH] Move user_intr handling to mach side

[Junling Ma]

1. make deliver_intr a linux irq handler, and do not let it touch the main_intr_queue. the queue
handling is solely in intr_thread (removing dead entries) and insert_intr_entry (insertion).
2. remove user_intr from struct linux_action and its handling on linux side.

---
device/intr.c | 156 +++++++++++++------------------
device/intr.h | 1 -
linux/dev/arch/i386/kernel/irq.c | 73 +--------------
3 files changed, 65 insertions(+), 165 deletions(-)

diff --git a/device/intr.c b/device/intr.c
index 705dc1c6..3962d7aa 100644
--- a/device/intr.c
+++ b/device/intr.c
@@ -23,10 +23,24 @@

#ifndef MACH_XEN

+#define SA_SHIRQ 0x04000000
+
extern struct irqdev irqtab;
#define main_intr_queue irqtab.intr_queue
static boolean_t deliver_intr (int id, ipc_port_t dst_port);

+struct pt_regs;
+extern int request_irq (unsigned int irq, void (*handler) (int, void *, struct pt_regs *),
+ unsigned long flags, const char *device, void *dev_id);
+extern void free_irq (unsigned int irq, void *dev_id);
+
+#define SPLHIGH(critical_section) \
+{\
+ spl_t s = splhigh();\
+ critical_section;\
+ splx(s);\
+}
+
#define PROTECT(lock, critical_section) \
{\
simple_lock(&lock);\
@@ -82,88 +96,52 @@ irq_acknowledge (ipc_port_t receive_port)
return D_SUCCESS;
}

-/* This function can only be used in the interrupt handler. */
-static void
-queue_intr (struct irqdev *dev, int id, user_intr_t *e)
+void
+deliver_user_intr (int id, void *dev_id, struct pt_regs *regs)
{
- /* Until userland has handled the IRQ in the driver, we have to keep it
- * disabled. Level-triggered interrupts would keep raising otherwise. */
- __disable_irq (dev->irq[id]);
-
- spl_t s = splhigh ();
- e->n_unacked++;
- e->interrupts++;
- dev->tot_num_intr++;
- splx (s);
-
+ SPLHIGH({
+ user_intr_t *e = dev_id;
+ /* Until userland has handled the IRQ in the driver, we have to keep it
+ * disabled. Level-triggered interrupts would keep raising otherwise. */
+ __disable_irq (irqtab.irq[id]);
+ e->n_unacked++;
+ e->interrupts++;
+ irqtab.tot_num_intr++;
+ });
thread_wakeup ((event_t) &intr_thread);
}

-int
-deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e)
-{
- /* The reference of the port was increased
- * when the port was installed.
- * If the reference is 1, it means the port should
- * have been destroyed and I destroy it now. */
- if (e->dst_port
- && e->dst_port->ip_references == 1)
- {
- printf ("irq handler [%d]: release a dead delivery port %p entry %p\n", id, e->dst_port, e);
- ipc_port_release (e->dst_port);
- e->dst_port = MACH_PORT_NULL;
- thread_wakeup ((event_t) &intr_thread);
- return 0;
- }
- else
- {
- queue_intr (dev, id, e);
- return 1;
- }
-}
-
/* insert an interrupt entry in the queue.
* This entry exists in the queue until
* the corresponding interrupt port is removed.*/
user_intr_t *
insert_intr_entry (struct irqdev *dev, int id, ipc_port_t dst_port)
{
- user_intr_t *e, *new, *ret;
- int free = 0;
-
- new = (user_intr_t *) kalloc (sizeof (*new));
- if (new == NULL)
- return NULL;
-
/* check whether the intr entry has been in the queue. */
- spl_t s = splhigh ();
- e = search_intr (dev, dst_port);
+ user_intr_t *e = search_intr (dev, dst_port);
if (e)
{
printf ("the interrupt entry for irq[%d] and port %p has already been inserted\n", id, dst_port);
- free = 1;
- ret = NULL;
- goto out;
+ return NULL;
}
- printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, new);
- ret = new;
- new->id = id;
- new->dst_port = dst_port;
- new->interrupts = 0;
- new->n_unacked = 0;
-
- PROTECT(dev->lock, queue_enter (&dev->intr_queue, new, user_intr_t *, chain));
-out:
- splx (s);
- if (free)
- kfree ((vm_offset_t) new, sizeof (*new));
- return ret;
+
+ e = (user_intr_t *) kalloc (sizeof (*e));
+ if (e == NULL)
+ return NULL;
+ e->id = id;
+ e->dst_port = dst_port;
+ e->interrupts = 0;
+ e->n_unacked = 0;
+ /* we do not need to disable irq here, because the irq handler does not touch the queue now. */
+ PROTECT(dev->lock, queue_enter (&dev->intr_queue, e, user_intr_t *, chain));
+ printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, e);
+ return e;
}

void
intr_thread (void)
{
- user_intr_t *e;
+ user_intr_t *e, *next;
int id;
ipc_port_t dst_port;

@@ -178,8 +156,11 @@ intr_thread (void)
simple_lock(&irqtab.lock);
queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
{
- if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
+ /* e cannot be NULL, as we check against it when installing the handler */
+ assert(e != NULL);
+ while (e->dst_port->ip_references == 1)
{
+ next = (user_intr_t *)queue_next(&e->chain);
printf ("irq handler [%d]: release dead delivery %d unacked irqs port %p entry %p\n", e->id, e->n_unacked, e->dst_port, e);
/* The reference of the port was increased
* when the port was installed.
@@ -193,25 +174,22 @@ intr_thread (void)
__enable_irq (irqtab.irq[e->id]);
e->n_unacked--;
}
+ ipc_port_release(e->dst_port);
+ assert (!queue_empty (&main_intr_queue));
+ queue_remove (&main_intr_queue, e, user_intr_t *, chain);
+ printf("irq handler [%d]: removed entry %p\n", e->id, e);
+ /* if e is the last handler registered for irq ID, then remove the linux irq handler */
+ free_irq(e->id, e);
+ kfree ((vm_offset_t) e, sizeof (*e));
+ e = next;
}
}

/* Now check for interrupts */
while (irqtab.tot_num_intr)
{
- int del = 0;
-
queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
{
- /* if an entry doesn't have dest port,
- * we should remove it. */
- if (e->dst_port == MACH_PORT_NULL)
- {
- clear_wait (current_thread (), 0, 0);
- del = 1;
- break;
- }
-
if (e->interrupts)
{
clear_wait (current_thread (), 0, 0);
@@ -225,24 +203,6 @@ intr_thread (void)
s = splhigh ();
}
}
-
- /* remove the entry without dest port from the queue and free it. */
- if (del)
- {
- assert (!queue_empty (&main_intr_queue));
- queue_remove (&main_intr_queue, e, user_intr_t *, chain);
- if (e->n_unacked)
- printf("irq handler [%d]: still %d unacked irqs in entry %p\n", e->id, e->n_unacked, e);
- while (e->n_unacked)
- {
- __enable_irq (irqtab.irq[e->id]);
- e->n_unacked--;
- }
- printf("irq handler [%d]: removed entry %p\n", e->id, e);
- splx (s);
- kfree ((vm_offset_t) e, sizeof (*e));
- s = splhigh ();
- }
}
simple_unlock(&irqtab.lock);
splx (s);
@@ -294,4 +254,16 @@ deliver_intr (int id, ipc_port_t dst_port)
return TRUE;
}

+int
+install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags,
+ user_intr_t *user_intr)
+{
+ if (id >= NINTR || user_intr == NULL)
+ return D_INVALID_OPERATION;
+ unsigned int irq = dev->irq[id];
+ if (irq >= NINTR)
+ return D_INVALID_OPERATION;
+ return request_irq (id, deliver_user_intr, SA_SHIRQ, NULL, user_intr);
+}
+
#endif /* MACH_XEN */
diff --git a/device/intr.h b/device/intr.h
index 54ddb331..b1c09e6c 100644
--- a/device/intr.h
+++ b/device/intr.h
@@ -51,7 +51,6 @@ struct irqdev {
};

extern int install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags, user_intr_t *e);
-extern int deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e);
extern user_intr_t *insert_intr_entry (struct irqdev *dev, int id, ipc_port_t receive_port);

void intr_thread (void);
diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
index 1e911f33..5879165f 100644
--- a/linux/dev/arch/i386/kernel/irq.c
+++ b/linux/dev/arch/i386/kernel/irq.c
@@ -78,7 +78,6 @@ struct linux_action
void *dev_id;
struct linux_action *next;
unsigned long flags;
- user_intr_t *user_intr;
};

static struct linux_action *irq_action[16] =
@@ -98,7 +97,6 @@ linux_intr (int irq)
{
struct pt_regs regs;
struct linux_action *action = *(irq_action + irq);
- struct linux_action **prev = &irq_action[irq];
unsigned long flags;

kstat.interrupts[irq]++;
@@ -110,32 +108,11 @@ linux_intr (int irq)

while (action)
{
- // TODO I might need to check whether the interrupt belongs to
- // the current device. But I don't do it for now.
- if (action->user_intr)
- {
- if (!deliver_user_intr(&irqtab, irq, action->user_intr))
- {
- *prev = action->next;
- linux_kfree(action);
- action = *prev;
- continue;
- }
- }
- else if (action->handler)
+ if (action->handler)
action->handler (irq, action->dev_id, &regs);
- prev = &action->next;
action = action->next;
}

- if (!irq_action[irq])
- {
- /* No handler any more, disable interrupt */
- mask_irq (irq);
- ivect[irq] = intnull;
- iunit[irq] = irq;
- }
-
restore_flags (flags);

intr_count--;
@@ -219,53 +196,6 @@ setup_x86_irq (int irq, struct linux_action *new)
return 0;
}

-int
-install_user_intr_handler (struct irqdev *dev, int id, unsigned long flags,
- user_intr_t *user_intr)
-{
- struct linux_action *action;
- struct linux_action *old;
- int retval;
-
- unsigned int irq = dev->irq[id];
-
- assert (irq < 16);
-
- /* Test whether the irq handler has been set */
- // TODO I need to protect the array when iterating it.
- old = irq_action[irq];
- while (old)
- {
- if (old->user_intr && old->user_intr->dst_port == user_intr->dst_port)
- {
- printk ("The interrupt handler has already been installed on line %d", irq);
- return linux_to_mach_error (-EAGAIN);
- }
- old = old->next;
- }
-
- /*
- * Hmm... Should I use `kalloc()' ?
- * By OKUJI Yoshinori.
- */
- action = (struct linux_action *)
- linux_kmalloc (sizeof (struct linux_action), GFP_KERNEL);
- if (action == NULL)
- return linux_to_mach_error (-ENOMEM);
-
- action->handler = NULL;
- action->next = NULL;
- action->dev_id = NULL;
- action->flags = SA_SHIRQ;
- action->user_intr = user_intr;
-
- retval = setup_x86_irq (irq, action);
- if (retval)
- linux_kfree (action);
-
- return linux_to_mach_error (retval);
-}
-
/*
* Attach a handler to an IRQ.
*/
@@ -294,7 +224,6 @@ request_irq (unsigned int irq, void (*handler) (int, void *, struct pt_regs *),
action->next = NULL;
action->dev_id = dev_id;
action->flags = flags;
- action->user_intr = NULL;

retval = setup_x86_irq (irq, action);
if (retval)
--
2.28.0.rc1

[Samuel Thibault]

Re,

Junling Ma, le dim. 02 août 2020 21:07:00 -0700, a ecrit:

> +struct pt_regs;
> +extern int request_irq (unsigned int irq, void (*handler) (int, void *, struct pt_regs *),
> + unsigned long flags, const char *device, void *dev_id);
> +extern void free_irq (unsigned int irq, void *dev_id);

Please put them in intr.h, with a comment saying that this for now plugs
into the Linux code, but will have to plug into the hardware-specific
code when we get rid of the Linux code.

> +#define SPLHIGH(critical_section) \
> +{\
> + spl_t s = splhigh();\
> + critical_section;\
> + splx(s);\
> +}

Mmm.

> - spl_t s = splhigh ();
> - e->n_unacked++;
> - e->interrupts++;
> - dev->tot_num_intr++;
> - splx (s);
> -
> + SPLHIGH({
> + user_intr_t *e = dev_id;
> + /* Until userland has handled the IRQ in the driver, we have to keep it
> + * disabled. Level-triggered interrupts would keep raising otherwise. */
> + __disable_irq (irqtab.irq[id]);
> + e->n_unacked++;
> + e->interrupts++;
> + irqtab.tot_num_intr++;
> + });

I don't think this makes it really more readable, actually. And it won't
protect from e.g. goto, so it'd bring a fake sense of security.

> + __disable_irq (irqtab.irq[id]);

I'd say add a struct irqdev * in the user_intr_t, so you don't have to
hardcode irqtab. Also, no need to keep it inside splhigh.

> -int
> -deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e)
> -{
> - /* The reference of the port was increased
> - * when the port was installed.
> - * If the reference is 1, it means the port should
> - * have been destroyed and I destroy it now. */
> - if (e->dst_port
> - && e->dst_port->ip_references == 1)
> - {
> - printf ("irq handler [%d]: release a dead delivery port %p entry %p\n", id, e->dst_port, e);
> - ipc_port_release (e->dst_port);
> - e->dst_port = MACH_PORT_NULL;
> - thread_wakeup ((event_t) &intr_thread);
> - return 0;

Did you actually test it? I don't remember exactly why we wanted to
release the port at this point. One of the reasons why we prefer patches
to be split is to be able to test each change separately, to be able to
determine which one breaks something.

> user_intr_t *
> insert_intr_entry (struct irqdev *dev, int id, ipc_port_t dst_port)
> {

[...]

> + /* we do not need to disable irq here, because the irq handler does not touch the queue now. */

??? How so?

> + PROTECT(dev->lock, queue_enter (&dev->intr_queue, e, user_intr_t *, chain));
> + printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, e);
> + return e;

> @@ -178,8 +156,11 @@ intr_thread (void)
> simple_lock(&irqtab.lock);
> queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
> {
> - if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
> + /* e cannot be NULL, as we check against it when installing the handler */
> + assert(e != NULL);

Errr, it can't be null because it's a member of the queue.

> + while (e->dst_port->ip_references == 1)

while?? belowe you ipc_port_release(), so the reference will necesarily
fall down to 0 anyway.

> {
> + next = (user_intr_t *)queue_next(&e->chain);

See my comment in my other mail. While doing free_irq() and kfree() you
have no idea what could happen. Don't bother to try to optimize, it's a
rare case, just restart iterating from the beginning of the queue.

> printf ("irq handler [%d]: release dead delivery %d unacked irqs port %p entry %p\n", e->id, e->n_unacked, e->dst_port, e);
> /* The reference of the port was increased
> * when the port was installed.
> @@ -193,25 +174,22 @@ intr_thread (void)
> __enable_irq (irqtab.irq[e->id]);
> e->n_unacked--;
> }
> + ipc_port_release(e->dst_port);
> + assert (!queue_empty (&main_intr_queue));
> + queue_remove (&main_intr_queue, e, user_intr_t *, chain);
> + printf("irq handler [%d]: removed entry %p\n", e->id, e);
> + /* if e is the last handler registered for irq ID, then remove the linux irq handler */

? We register a handler for each user of an irq, so we just always free
the irq, we don't need to care being "last handler".

> diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
> index 1e911f33..5879165f 100644
> --- a/linux/dev/arch/i386/kernel/irq.c
> +++ b/linux/dev/arch/i386/kernel/irq.c
>

> - if (!irq_action[irq])
> - {
> - /* No handler any more, disable interrupt */
> - mask_irq (irq);
> - ivect[irq] = intnull;
> - iunit[irq] = irq;
> - }
> -

I'd say we want to keep it?

Samuel

[Samuel Thibault]

Samuel Thibault, le mer. 05 août 2020 02:24:20 +0200, a ecrit:

> > user_intr_t *
> > insert_intr_entry (struct irqdev *dev, int id, ipc_port_t dst_port)
> > {
> [...]
> > + /* we do not need to disable irq here, because the irq handler does not touch the queue now. */
> ??? How so?

Ah, you mean with the modifications, interrupts handler don't modify
the structure of the queue. Don't put "now" in comments to refer to git
history: the reader will be thinking "now" means during execution.

Samuel

[Junling Ma]

Yes the separated patch does not contain it.

>> + __disable_irq (irqtab.irq[id]);
>
> I'd say add a struct irqdev * in the user_intr_t, so you don't have to
> hardcode irqtab. Also, no need to keep it inside splhigh.

Sure we can keep a pointer inside user_intr_t. But IO don’t think we need to protect __disable_irq, as __disable_irq itself does a splhigh/splx.

>> -int
>> -deliver_user_intr (struct irqdev *dev, int id, user_intr_t *e)
>> -{
>> - /* The reference of the port was increased
>> - * when the port was installed.
>> - * If the reference is 1, it means the port should
>> - * have been destroyed and I destroy it now. */
>> - if (e->dst_port
>> - && e->dst_port->ip_references == 1)
>> - {
>> - printf ("irq handler [%d]: release a dead delivery port %p entry %p\n", id, e->dst_port, e);
>> - ipc_port_release (e->dst_port);
>> - e->dst_port = MACH_PORT_NULL;
>> - thread_wakeup ((event_t) &intr_thread);
>> - return 0;
>
> Did you actually test it? I don't remember exactly why we wanted to
> release the port at this point. One of the reasons why we prefer patches
> to be split is to be able to test each change separately, to be able to
> determine which one breaks something.

Yes I tested. S I figured out in the other email, the dsp_port may become dead while we are in the delivery loop. So one way to solve it is to mark the user_intr_t with a dead_port free-able, and free after the loop. Another is to simply combine the delete loop and the delivery loop together: if the port is dead free, otherwise deliver. Then there would be no hairy racing conditions.

>> user_intr_t *
>> insert_intr_entry (struct irqdev *dev, int id, ipc_port_t dst_port)
>> {
> [...]
>> + /* we do not need to disable irq here, because the irq handler does not touch the queue now. */
> ??? How so?

You answered in your next email. Yes. The irq handler does not touch the queue.

>> + PROTECT(dev->lock, queue_enter (&dev->intr_queue, e, user_intr_t *, chain));
>> + printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, e);
>> + return e;
>
>
>> @@ -178,8 +156,11 @@ intr_thread (void)
>> simple_lock(&irqtab.lock);
>> queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
>> {
>> - if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
>> + /* e cannot be NULL, as we check against it when installing the handler */
>> + assert(e != NULL);
>
> Errr, it can't be null because it's a member of the queue.

It is gone in the separated patch. In the original code there was a test against NULL, which I believe wouldn’t happen, so I changed it to alert. Then I removed it in the separated patch.

>> + while (e->dst_port->ip_references == 1)
>
> while?? belowe you ipc_port_release(), so the reference will necesarily
> fall down to 0 anyway.

The pointer e moves to the next item in the queue, see e = next, below. So we remove all consecutive dead ports in the queue. Again, I propose to combine the dead port loop with the delivery loop, it would be more readable.

>> {
>> + next = (user_intr_t *)queue_next(&e->chain);
>
> See my comment in my other mail. While doing free_irq() and kfree() you
> have no idea what could happen. Don't bother to try to optimize, it's a
> rare case, just restart iterating from the beginning of the queue.
>
>> printf ("irq handler [%d]: release dead delivery %d unacked irqs port %p entry %p\n", e->id, e->n_unacked, e->dst_port, e);
>> /* The reference of the port was increased
>> * when the port was installed.
>> @@ -193,25 +174,22 @@ intr_thread (void)
>> __enable_irq (irqtab.irq[e->id]);
>> e->n_unacked--;
>> }
>> + ipc_port_release(e->dst_port);
>> + assert (!queue_empty (&main_intr_queue));
>> + queue_remove (&main_intr_queue, e, user_intr_t *, chain);
>> + printf("irq handler [%d]: removed entry %p\n", e->id, e);
>> + /* if e is the last handler registered for irq ID, then remove the linux irq handler */
>
> ? We register a handler for each user of an irq, so we just always free
> the irq, we don't need to care being "last handler".

It is gone in the separated patch.

>> diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
>> index 1e911f33..5879165f 100644
>> --- a/linux/dev/arch/i386/kernel/irq.c
>> +++ b/linux/dev/arch/i386/kernel/irq.c
>>
>> - if (!irq_action[irq])
>> - {
>> - /* No handler any more, disable interrupt */
>> - mask_irq (irq);
>> - ivect[irq] = intnull;
>> - iunit[irq] = irq;
>> - }
>> -
>
> I'd say we want to keep it?

This is already done in free_irq. If we do it in two places, we might have a race condition?

Junling

[Samuel Thibault]

Junling Ma, le mar. 04 août 2020 18:21:21 -0700, a ecrit:

> >> + __disable_irq (irqtab.irq[id]);
> >
> > I'd say add a struct irqdev * in the user_intr_t, so you don't have to
> > hardcode irqtab. Also, no need to keep it inside splhigh.
>
> Sure we can keep a pointer inside user_intr_t.

> But IO don’t think we need to protect __disable_irq, as __disable_irq itself does a splhigh/splx.

That's what I wrote above, yes.

> >> + PROTECT(dev->lock, queue_enter (&dev->intr_queue, e, user_intr_t *, chain));
> >> + printf("irq handler [%d]: new delivery port %p entry %p\n", id, dst_port, e);
> >> + return e;
> >
> >
> >> @@ -178,8 +156,11 @@ intr_thread (void)
> >> simple_lock(&irqtab.lock);
> >> queue_iterate (&main_intr_queue, e, user_intr_t *, chain)
> >> {
> >> - if ((!e->dst_port || e->dst_port->ip_references == 1) && e->n_unacked)
> >> + /* e cannot be NULL, as we check against it when installing the handler */
> >> + assert(e != NULL);
> >
> > Errr, it can't be null because it's a member of the queue.
>
> It is gone in the separated patch. In the original code there was a test against NULL,

? No there wasn't.

> >> + while (e->dst_port->ip_references == 1)
> >
> > while?? belowe you ipc_port_release(), so the reference will necesarily
> > fall down to 0 anyway.
>
> The pointer e moves to the next item in the queue, see e = next, below.

Ah ok. That would definitely deserves a comment.

> >> diff --git a/linux/dev/arch/i386/kernel/irq.c b/linux/dev/arch/i386/kernel/irq.c
> >> index 1e911f33..5879165f 100644
> >> --- a/linux/dev/arch/i386/kernel/irq.c
> >> +++ b/linux/dev/arch/i386/kernel/irq.c
> >>
> >> - if (!irq_action[irq])
> >> - {
> >> - /* No handler any more, disable interrupt */
> >> - mask_irq (irq);
> >> - ivect[irq] = intnull;
> >> - iunit[irq] = irq;
> >> - }
> >> -
> >
> > I'd say we want to keep it?
>
> This is already done in free_irq.

Ah, right, ok.

> If we do it in two places, we might have a race condition?

? No, since the free_irq is under cli.

Samuel