bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Move user_intr handling to mach side


From: Junling Ma
Subject: Re: [PATCH] Move user_intr handling to mach side
Date: Tue, 4 Aug 2020 18:21:21 -0700


> On Aug 4, 2020, at 5:24 PM, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> 
> 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.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]