LinuxKPI: enhance the irq KPI for managed and threaded operations.

[bz]

Summary

Move request_irq() to an internal function which serves request_irq(),
and the newly added request_threaded_irq() and devm_request_threaded_irq().
Likewise factor out parts of free_irq() to also be used with
devm_free_irq().
Add a linux_irq_worker() implementation in case the irq handler in
linux_irq_handler() returns IRQ_WAKE_THREAD to defer further work [1].

Sponsored by: The FreeBSD Foundation
Inspired by: Similar code in a Chelsio driver [1]
MFC after: 10 days

[• hselasky]

Hi Bjoern,

This patch should not be needed. In Linux the "handler" is equivalent to the fast IRQ in FreeBSD, but the LinuxKPI lowers it to be the software interrupt. Likewise spinlocks are mutexes under FreeBSD.

You can add the "thread_handler" function pointer, but there is no need for a separate thread for that!

Just implement it like this:

diff --git a/sys/compat/linuxkpi/common/src/linux_compat.c b/sys/compat/linuxkpi/common/src/linux_compat.c
index a8f090ed..f76ba385 100644
--- a/sys/compat/linuxkpi/common/src/linux_compat.c
+++ b/sys/compat/linuxkpi/common/src/linux_compat.c
@@ -2465,7 +2465,8 @@ linux_irq_handler(void *ent)
                return;
 
        irqe = ent;
-       irqe->handler(irqe->irq, irqe->arg);
+       if (irqe->handler(irqe->irq, irqe->arg) == IRQ_WAKE_THREAD)
+               irqe->thread_handler(irqe->irq, irqe->arg);
 }
 
 #if defined(__i386__) || defined(__amd64__)

[• hselasky]

"fast interrupt" or also called "interrupt filter"

[bz]

In D30549#686444, @hselasky wrote:

Hi Bjoern,

This patch should not be needed. In Linux the "handler" is equivalent to the fast IRQ in FreeBSD, but the LinuxKPI lowers it to be the software interrupt.

Sorry, I am not sure I follow.

We do the normal bus_alloc_resource_any(..,SYS_RES_IRQ,..); bus_setup_intr(..,ithread,.); there on the BSD device_t.
Where would this be any different from any other FreeBSD device driver?

Whether Linux needs a separate thread there or not is not the question, the question is does our FreeBSD compat code need it?

Driver code does acquire the spinlock (so mtx in our case) in the thread_handler code; and if we do initialize them in linux_spin_lock_init() with MTX_DEF | MTX_NOWITNESS it still means we can sleep, right? So running this code in the ithread is not a good idea ("You can do whatever you want in an ithread routine, except sleep." quoting BUS_SETUP_INTR(9)).

I probably stared at this too long to not see the obvious part I am missing?

[bz]

In D30549#686692, @bz wrote:

In D30549#686444, @hselasky wrote:

Hi Bjoern,

This patch should not be needed. In Linux the "handler" is equivalent to the fast IRQ in FreeBSD, but the LinuxKPI lowers it to be the software interrupt.

Sorry, I am not sure I follow.

We do the normal bus_alloc_resource_any(..,SYS_RES_IRQ,..); bus_setup_intr(..,ithread,.); there on the BSD device_t.
Where would this be any different from any other FreeBSD device driver?

Whether Linux needs a separate thread there or not is not the question, the question is does our FreeBSD compat code need it?

Driver code does acquire the spinlock (so mtx in our case) in the thread_handler code; and if we do initialize them in linux_spin_lock_init() with MTX_DEF | MTX_NOWITNESS it still means we can sleep, right? So running this code in the ithread is not a good idea ("You can do whatever you want in an ithread routine, except sleep." quoting BUS_SETUP_INTR(9)).

I probably stared at this too long to not see the obvious part I am missing?

Ok that's an as silly statement;... let me re-phrase: can we be sure Linux interrupt handlers/threads never call any code which sleeps? Is that by definition of their "rules"?

[• hselasky]

Hi Bjoern,

Ok that's an as silly statement;... let me re-phrase: can we be sure Linux interrupt handlers/threads never call any code which sleeps? Is that by definition of their "rules"?

Yes, it can sleep, so simply wrap the callback in the WAKE case with:

THREAD_SLEEPING_OK();
callback goes here;
THREAD_NO_SLEEPING();

And don't create another thread for this stuff!

There is an issue you don't see, and that is that the IRQ itself is masked until the FreeBSD IRQ handler returns. When you create a simple task, the IRQ gets unmasked too early. Linux has special code to do the exact same thing for the task, but this is not possible in FreeBSD.

Software IRQ threads in FreeBSD are simply like other threads. Good practice is however that they don't sleep, but you can make an exception for the WAKE case in the LinuxKPI!

Was that clear, or do you still have questions?

--HPS

[bz]

Thanks for the explanations!
I'll update it with your suggestions + the additional NULL check for the thread_handler.

[• hselasky]

FYI: You can optimise the NULL check by installing a dummy handler when NULL is configured there.

[bz]

Remove any thread creation and only leave the call to irqe->thread_handler
in case of "WAKE" and wrap that into THREAD_SLEEPING_OK()/THREAD_NO_SLEEPING()
as suggested by @hselasky. Hope I got this right now?

[• hselasky]

Yes, you got it right.

sys/compat/linuxkpi/common/include/linux/interrupt.h

123

Please just use the default naming here. I.E. no call to bus_describe_intr(). The describe field for IRQ's is very limited in length, and breaks mlx5's IRQ naming.

[bz]

sys/compat/linuxkpi/common/include/linux/interrupt.h

123

That was a pain for other drivers but I am game changing this for now. Update coming in a minute.

[bz]

Also remove bus_describe_intr() based on request.

[• hselasky]

Looks good!

[bz]

sys/compat/linuxkpi/common/src/linux_compat.c

2501

This does not work; I kept running into panics with it and switched back locally to actually running it in a thread. Could do other ways than starting a thread as well but it seems we will need a deferred context.

[• hselasky]

sys/compat/linuxkpi/common/src/linux_compat.c

2501

I see.

How will you disable the device interrupts during the execution of that worker thread? Right now while the linux_irq_handler() function is executing the device IRQ is masked. When this function returns the device IRQ is unmasked.

Are you sure those panics can't be fixed?