Re: [PATCH] Move user_intr handling to mach side

[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