Re: [PATCH-1] A new irq device interface

[Samuel Thibault]

Junling Ma, le dim. 02 août 2020 15:32:01 -0700, a ecrit:
> >> 1. Change devicer_user_intr to be a linux interrupt handler. 
> > 
> > Right, this makes sense, but did you actually try it? Check the existing
> > code twice to realize why it's done that way. Here linux_intr is looking
> > through the list of actions for an irq. You are making action->handler()
> > call free_irq itself when the handler should be removed. But then the
> > loop of linux_intr will have its action pointer undefined since freed by
> > free_irq.
> 
> Right I realized that, and moved the free_irq to intr_thread. Will clean up 
> the patch.
> 
> > I agree that we should probably remove some of the "user_intr" pieces of
> > linux_intr(). But we cannot afford removing the part that checks the
> > value returned by the handler, otherwise it'll break.
> 
> If we move the cleaning-up-dead-notification code to intr_thread, then the 
> code should not break.

Ok.

> >> 2. Make user_intr_t represent a specific interrupt line, so that 
> >> notifications queue on each line.
> >> 3. Make struct irqdev store a vector user_intr_t, indexed by interrupt 
> >> line.
> > 
> > ? But then we can't have several userland translators using the same
> > irq? That'll break.
> 
> Each user_intr_t has a queue of notifications. Originally, each notification 
> was queued up on the linux side. In this patch we just move them back to the 
> mach side and sort them by interrupt line (i.e., attach to user_intr_t). I am 
> not sure why it would break. 

To be honest, I don't know. Because your patch changes so many thing
that I can't even take the time to try to track what is being changed
how. What I can see easily, however, is:

+typedef struct {
+       queue_head_t notification_queue;
+  unsigned interrupts; /* Number of interrupts occurred since last run of 
intr_thread */
+  unsigned n_unacked;  /* Number of times irqs were disabled for this */
+  decl_simple_lock_data(, lock); /* a lock to protect the queues */
 } user_intr_t;

So the port is gone indeed, so it tends to say that now it's per intr
instead of per-user. But then why calling it user_intr_t? Changing the
role of an existing structure is really not the way for changes to be
reviewable.

Now, in that struct there is n_unacked. But how can this be per-irq? As
I mentioned elsewhere and it is kept here, it's the number of times irq
where disabled for this. But that's for a given *user*, not for a given
irq. If it's not per-user, when a user dies you don't know how many
times you need to call __enable_irq.

> IN fact, I was pondering on the idea that each irq* device should be opened 
> exclusively,

Then things won't work. Shared IRQ is a thing you can't circumvent with
the hardware that people have atm.

> I am afraid that an interrupt going through two indirect calls would be too 
> time costly to hurt driver performance.

Interrupts *are* inherently costly. A couple software layers are really
not much compared to that. Drivers are already used to have to gather
notifications so as to spare interrupts.

> >> 4. The n_unacked counter only increases when successfully delivered an 
> >> interrupt.
> > 
> > ? It is meant to know how many times __disable_irq was called, and thus
> > how many times we should call __enable_irq if the port dies.
> 
> The __disable_irq uses its own counter.

Yes, and the way you do it just duplicates it, that's nonsense. Either
it's the same, or it's not. As mentioned above, it's not.µ

> The original way of counting means that the interrupt will be unfortunately 
> blocked forever if a message somehow failed to deliver.

There is no such thing. Either the message gets delivered, or it does
not. It can take some time to get delivered. Then other translators
using the same IRQ will suffer from the situation. But that's what we
*have* to do. Until the userland translator either acknowledges the irq
or dies, we *have* to keep the irq disabled (and know that we have done
so for the death case).  We can't go around it anyway, so there is no
point in trying to fix it.

> This patch increases n_unacked only if a notification is delivered,

That would mean that the responsibility of ack-ing interrupts belongs to
userland?

> I am of course happy to produce a new version of the patches. But I
> also feel that whether we want to introduce this new scheme still
> needs more discussion.

So far (except for the first part of your first patch) what you
have proposed really does not convince me at all as being an actual
improvement over what we currently have, on the contrary.

Samuel