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

Authored by bz on May 30 2021, 2:12 PM.

Details

Reviewers

Group Reviewers

Commits

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

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bz created this revision.May 30 2021, 2:12 PM

Herald added subscribers: emaste, imp. · View Herald TranscriptMay 30 2021, 2:12 PM

bz requested review of this revision.May 30 2021, 2:12 PM

Harbormaster completed remote builds in B39569: Diff 90124.May 30 2021, 2:12 PM

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__)

"fast interrupt" or also called "interrupt filter"

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?

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"?

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

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

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

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?

Harbormaster completed remote builds in B39703: Diff 90416.Jun 4 2021, 4:14 PM

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 marked an inline comment as done.Jun 4 2021, 4:57 PM

bz added inline comments.

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.

Also remove bus_describe_intr() based on request.

Harbormaster completed remote builds in B39707: Diff 90422.Jun 4 2021, 5:00 PM

Looks good!

This revision is now accepted and ready to land.Jun 4 2021, 9:03 PM

Closed by commit rGd16b6cb17837: LinuxKPI: enhance the irq KPI for managed and threaded operations. (authored by bz). · Explain WhyJun 18 2021, 9:49 PM

This revision was automatically updated to reflect the committed changes.

bz added a commit: rGd16b6cb17837: LinuxKPI: enhance the irq KPI for managed and threaded operations..

bz added a commit: rG1eaaada457d6: LinuxKPI: enhance the irq KPI for managed and threaded operations..Jul 18 2021, 12:39 AM

bz reopened this revision.Oct 22 2021, 11:26 AM

bz added inline comments.

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 added inline comments.Oct 22 2021, 9:02 PM

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?