[PATCH] kernel:irq:manage: request threaded irq with a specified priority

[PATCH] kernel:irq:manage: request threaded irq with a specified priority

[Song Chen]

In general, irq handler thread will be assigned a default priority which
is MAX_RT_PRIO/2, as a result, no one can preempt others.

Here is the case I found in a real project, an interrupt int_a is
coming, wakes up its handler handler_a and handler_a wakes up a
userspace RT process task_a.

However, if another irq handler handler_b which has nothing to do
with any RT tasks is running when int_a is coming, handler_a can't
preempt handler_b, as a result, task_a can't be waken up immediately
as expected until handler_b gives up cpu voluntarily. In this case,
determinism breaks.

Therefore, this patch introduce a new api to give drivers a chance to
assign expected priorities to their irq handler thread.

Signed-off-by: Song Chen <chensong_2000@189.cn>
---
 include/linux/interrupt.h  |  7 +++++
 include/linux/sched.h      |  1 +
 include/linux/sched/prio.h |  1 +
 kernel/irq/manage.c        | 64 +++++++++++++++++++++++++++++++++++++++++++---
 kernel/sched/core.c        | 11 ++++++++
 5 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 967e257..5ab9169 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -121,6 +121,7 @@ struct irqaction {
 	unsigned long		thread_mask;
 	const char		*name;
 	struct proc_dir_entry	*dir;
+	int prio;
 } ____cacheline_internodealigned_in_smp;
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
@@ -136,6 +137,12 @@ extern irqreturn_t no_action(int cpl, void *dev_id);
 #define IRQ_NOTCONNECTED	(1U << 31)
 
 extern int __must_check
+request_threaded_irq_with_prio(unsigned int irq, irq_handler_t handler,
+		     irq_handler_t thread_fn,
+		     unsigned long flags, const char *name, void *dev,
+			 int prio);
+
+extern int __must_check
 request_threaded_irq(unsigned int irq, irq_handler_t handler,
 		     irq_handler_t thread_fn,
 		     unsigned long flags, const char *name, void *dev);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ef00bb2..50edae9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1711,6 +1711,7 @@ extern int sched_setscheduler(struct task_struct *, int, const struct sched_para
 extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sched_param *);
 extern void sched_set_fifo(struct task_struct *p);
 extern void sched_set_fifo_low(struct task_struct *p);
+extern void sched_set_fifo_with_prio(struct task_struct *p, int prio);
 extern void sched_set_normal(struct task_struct *p, int nice);
 extern int sched_setattr(struct task_struct *, const struct sched_attr *);
 extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
diff --git a/include/linux/sched/prio.h b/include/linux/sched/prio.h
index ab83d85..1e1186e 100644
--- a/include/linux/sched/prio.h
+++ b/include/linux/sched/prio.h
@@ -15,6 +15,7 @@
 
 #define MAX_RT_PRIO		100
 
+#define DEFAULT_RT_PRIO	(MAX_RT_PRIO / 2)
 #define MAX_PRIO		(MAX_RT_PRIO + NICE_WIDTH)
 #define DEFAULT_PRIO		(MAX_RT_PRIO + NICE_WIDTH / 2)
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 21ea370..111b8ce 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1394,7 +1394,7 @@ setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary)
 	if (IS_ERR(t))
 		return PTR_ERR(t);
 
-	sched_set_fifo(t);
+	sched_set_fifo_with_prio(t, new->prio);
 
 	/*
 	 * We keep the reference to the task struct even if
@@ -2032,7 +2032,7 @@ const void *free_nmi(unsigned int irq, void *dev_id)
 }
 
 /**
- *	request_threaded_irq - allocate an interrupt line
+ *	request_threaded_irq_with_prio - allocate an interrupt line
  *	@irq: Interrupt line to allocate
  *	@handler: Function to be called when the IRQ occurs.
  *		  Primary handler for threaded interrupts
@@ -2043,6 +2043,7 @@ const void *free_nmi(unsigned int irq, void *dev_id)
  *	@irqflags: Interrupt type flags
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A cookie passed back to the handler function
+ *	@prio: priority of the irq handler thread
  *
  *	This call allocates interrupt resources and enables the
  *	interrupt line and IRQ handling. From the point this
@@ -2067,15 +2068,18 @@ const void *free_nmi(unsigned int irq, void *dev_id)
  *	If your interrupt is shared you must pass a non NULL dev_id
  *	as this is required when freeing the interrupt.
  *
+ *	If you want to assign a priority for your irq handler thread
+ *	instead of default value, you need to supply @prio.
+ *
  *	Flags:
  *
  *	IRQF_SHARED		Interrupt is shared
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
  *
  */
-int request_threaded_irq(unsigned int irq, irq_handler_t handler,
+int request_threaded_irq_with_prio(unsigned int irq, irq_handler_t handler,
 			 irq_handler_t thread_fn, unsigned long irqflags,
-			 const char *devname, void *dev_id)
+			 const char *devname, void *dev_id, int prio)
 {
 	struct irqaction *action;
 	struct irq_desc *desc;
@@ -2121,6 +2125,7 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	action->flags = irqflags;
 	action->name = devname;
 	action->dev_id = dev_id;
+	action->prio = prio;
 
 	retval = irq_chip_pm_get(&desc->irq_data);
 	if (retval < 0) {
@@ -2157,6 +2162,57 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 #endif
 	return retval;
 }
+EXPORT_SYMBOL(request_threaded_irq_with_prio);
+
+/**
+ *	request_threaded_irq - allocate an interrupt line
+ *	@irq: Interrupt line to allocate
+ *	@handler: Function to be called when the IRQ occurs.
+ *		  Primary handler for threaded interrupts
+ *		  If NULL and thread_fn != NULL the default
+ *		  primary handler is installed
+ *	@thread_fn: Function called from the irq handler thread
+ *		    If NULL, no irq thread is created
+ *	@irqflags: Interrupt type flags
+ *	@devname: An ascii name for the claiming device
+ *	@dev_id: A cookie passed back to the handler function
+ *
+ *	This call allocates interrupt resources and enables the
+ *	interrupt line and IRQ handling. From the point this
+ *	call is made your handler function may be invoked. Since
+ *	your handler function must clear any interrupt the board
+ *	raises, you must take care both to initialise your hardware
+ *	and to set up the interrupt handler in the right order.
+ *
+ *	If you want to set up a threaded irq handler for your device
+ *	then you need to supply @handler and @thread_fn. @handler is
+ *	still called in hard interrupt context and has to check
+ *	whether the interrupt originates from the device. If yes it
+ *	needs to disable the interrupt on the device and return
+ *	IRQ_WAKE_THREAD which will wake up the handler thread and run
+ *	@thread_fn. This split handler design is necessary to support
+ *	shared interrupts.
+ *
+ *	Dev_id must be globally unique. Normally the address of the
+ *	device data structure is used as the cookie. Since the handler
+ *	receives this value it makes sense to use it.
+ *
+ *	If your interrupt is shared you must pass a non NULL dev_id
+ *	as this is required when freeing the interrupt.
+ *
+ *	Flags:
+ *
+ *	IRQF_SHARED		Interrupt is shared
+ *	IRQF_TRIGGER_*		Specify active edge(s) or level
+ *
+ */
+int request_threaded_irq(unsigned int irq, irq_handler_t handler,
+			 irq_handler_t thread_fn, unsigned long irqflags,
+			 const char *devname, void *dev_id)
+{
+	return request_threaded_irq_with_prio(irq, handler, thread_fn,
+				irqflags, devname, dev_id, DEFAULT_RT_PRIO);
+}
 EXPORT_SYMBOL(request_threaded_irq);
 
 /**
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9819121..7941595 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6439,6 +6439,17 @@ void sched_set_fifo_low(struct task_struct *p)
 }
 EXPORT_SYMBOL_GPL(sched_set_fifo_low);
 
+/*
+ * For when you want a specific priority.
+ */
+void sched_set_fifo_with_prio(struct task_struct *p, int prio)
+{
+	struct sched_param sp = { .sched_priority =
+		(prio > 0 && prio < MAX_RT_PRIO) ? prio : DEFAULT_RT_PRIO };
+	WARN_ON_ONCE(sched_setscheduler_nocheck(p, SCHED_FIFO, &sp) != 0);
+}
+EXPORT_SYMBOL_GPL(sched_set_fifo_with_prio);
+
 void sched_set_normal(struct task_struct *p, int nice)
 {
 	struct sched_attr attr = {
-- 
2.7.4

Re: [PATCH] kernel:irq:manage: request threaded irq with a specified priority

[Thomas Gleixner]

On Tue, Apr 13 2021 at 14:19, Song Chen wrote:
> In general, irq handler thread will be assigned a default priority which
> is MAX_RT_PRIO/2, as a result, no one can preempt others.
>
> Here is the case I found in a real project, an interrupt int_a is
> coming, wakes up its handler handler_a and handler_a wakes up a
> userspace RT process task_a.
>
> However, if another irq handler handler_b which has nothing to do
> with any RT tasks is running when int_a is coming, handler_a can't
> preempt handler_b, as a result, task_a can't be waken up immediately
> as expected until handler_b gives up cpu voluntarily. In this case,
> determinism breaks.

It breaks because the system designer failed to assign proper priorities
to the irq threads int_a, int_b and to the user space process task_a.

That's not solvable at the kernel level.

Thanks,

        tglx

Re: [PATCH] kernel:irq:manage: request threaded irq with a specified priority

[chensong]

On 2021/4/13 下午4:39, Thomas Gleixner wrote:
> On Tue, Apr 13 2021 at 14:19, Song Chen wrote:
>> In general, irq handler thread will be assigned a default priority which
>> is MAX_RT_PRIO/2, as a result, no one can preempt others.
>>
>> Here is the case I found in a real project, an interrupt int_a is
>> coming, wakes up its handler handler_a and handler_a wakes up a
>> userspace RT process task_a.
>>
>> However, if another irq handler handler_b which has nothing to do
>> with any RT tasks is running when int_a is coming, handler_a can't
>> preempt handler_b, as a result, task_a can't be waken up immediately
>> as expected until handler_b gives up cpu voluntarily. In this case,
>> determinism breaks.
> 
> It breaks because the system designer failed to assign proper priorities
> to the irq threads int_a, int_b and to the user space process task_a.

yes, it's designers' responsibility to assign proper priorities, but 
kernel is also obliged to provide interfaces for those designers.

chrt can help designers in this case, however, the truth is lot of 
customers are not familiar with it. what's more, chrt can also apply to 
userspace rt task, but userspace also has sched_setscheduler to assgin 
proper priority inside code like cyclictest, why can't driver writers 
have another choice?

Further, what if irq handlear thread has to run on the expected priority 
at the very beginning? This patch helps.

BR

Song

> 
> That's not solvable at the kernel level.
> 
> Thanks,
> 
>          tglx
> 
> 

Re: [PATCH] kernel:irq:manage: request threaded irq with a specified priority

[Daniel Bristot de Oliveira]

On 4/16/21 6:57 AM, chensong wrote:
> 
> 
> On 2021/4/13 下午4:39, Thomas Gleixner wrote:
>> On Tue, Apr 13 2021 at 14:19, Song Chen wrote:
>>> In general, irq handler thread will be assigned a default priority which
>>> is MAX_RT_PRIO/2, as a result, no one can preempt others.
>>>
>>> Here is the case I found in a real project, an interrupt int_a is
>>> coming, wakes up its handler handler_a and handler_a wakes up a
>>> userspace RT process task_a.
>>>
>>> However, if another irq handler handler_b which has nothing to do
>>> with any RT tasks is running when int_a is coming, handler_a can't
>>> preempt handler_b, as a result, task_a can't be waken up immediately
>>> as expected until handler_b gives up cpu voluntarily. In this case,
>>> determinism breaks.
>>
>> It breaks because the system designer failed to assign proper priorities
>> to the irq threads int_a, int_b and to the user space process task_a.
> 
> yes, it's designers' responsibility to assign proper priorities, but
> kernel is also obliged to provide interfaces for those designers.

There is no optimal priority assignment for fixed priority schedulers without a
prior knowledge of all tasks (and their timing behavior, e.g., exec time,
activation pattern...). So, the developer will never know what is the best
priority. Such fine tune should be done by the user.

> 
> chrt can help designers in this case, however, the truth is lot of customers are
> not familiar with it.

And making this change in C in kernel is just turning it even more complex.

what's more, chrt can also apply to userspace rt task, but
> userspace also has sched_setscheduler to assgin proper priority inside code like
> cyclictest, why can't driver writers have another choice?

The developer of task_a can also use sched_setscheduler() to adjust the priority
of the handler_a - or even better, decrease the priority of the handler_b as it
is not that important. The developer is supposed to know how to change priority
because task_a is RT too.

Note that the user sets the priority on cyclictest (-p....).

-- Daniel

Re: [PATCH] kernel:irq:manage: request threaded irq with a specified priority

[Thomas Gleixner]

On Fri, Apr 16 2021 at 12:57, chensong wrote:
> On 2021/4/13 下午4:39, Thomas Gleixner wrote:
>> It breaks because the system designer failed to assign proper priorities
>> to the irq threads int_a, int_b and to the user space process task_a.
>
> yes, it's designers' responsibility to assign proper priorities, but 
> kernel is also obliged to provide interfaces for those designers.

The interface exists. sched_setscheduler(2)

> chrt can help designers in this case, however, the truth is lot of 
> customers are not familiar with it.

The truth is that real-time systems need to be carefully designed and
parametrized. And that's only possible when _all_ of the system
components and their constraints are known. Trying to solve it at the
device driver level of a single device is impossible and a guarantee for
fail.

If the customer does not know how to do it, then I really have to ask
why he's trying to use a real-time system at all. There is no real-time
system which magically tunes itself by pulling the overall system
constraints out of thin air.
 
> what's more, chrt can also apply to userspace rt task, but userspace
> also has sched_setscheduler to assgin proper priority inside code like
> cyclictest, why can't driver writers have another choice?

There is a very simple reason: The driver writer cannot know about the
requirements of the complete system which is composed of kernel, drivers
and user space applications, unless the driver writer is fully aware of
the overall system design and constraints.

How is that supposed to work on a general purpose kernel which is
utilized for a gazillion of different use cases which all have different
expectations?

It simply cannot work because default A will only work for usecase A and
be completely wrong for all others.

> Further, what if irq handlear thread has to run on the expected priority 
> at the very beginning? This patch helps.

There is no such thing as the expected priority of an interrupt thread
which can be applied upfront.

There are ~5400 instances of request*irq() in the kernel source and
there is no way to make priority decisions for them which work for every
RT system out there.

The kernel sets a default and the system designer, admin, user has to
take care of tuning it to match the expectations and constraints of his
particular application scenario.

The kernel provides an userspace interface to do that. That interface
might be a bit awkward to use, but there are tools out there which help
with that, and if at all we can think about providing a better and
easier to use interface for this.

Trying to solve that at the kernel level is putting the cart before the
horse.

Thanks,

        tglx

Re: [PATCH] kernel:irq:manage: request threaded irq with a specified priority

[chensong]

Daniel & tglx,

Points taken and thanks a lot for such detailed explanations.

BR

Song

On 2021/4/16 下午5:09, Thomas Gleixner wrote:
> On Fri, Apr 16 2021 at 12:57, chensong wrote:
>> On 2021/4/13 下午4:39, Thomas Gleixner wrote:
>>> It breaks because the system designer failed to assign proper priorities
>>> to the irq threads int_a, int_b and to the user space process task_a.
>>
>> yes, it's designers' responsibility to assign proper priorities, but
>> kernel is also obliged to provide interfaces for those designers.
> 
> The interface exists. sched_setscheduler(2)
> 
>> chrt can help designers in this case, however, the truth is lot of
>> customers are not familiar with it.
> 
> The truth is that real-time systems need to be carefully designed and
> parametrized. And that's only possible when _all_ of the system
> components and their constraints are known. Trying to solve it at the
> device driver level of a single device is impossible and a guarantee for
> fail.
> 
> If the customer does not know how to do it, then I really have to ask
> why he's trying to use a real-time system at all. There is no real-time
> system which magically tunes itself by pulling the overall system
> constraints out of thin air.
>   
>> what's more, chrt can also apply to userspace rt task, but userspace
>> also has sched_setscheduler to assgin proper priority inside code like
>> cyclictest, why can't driver writers have another choice?
> 
> There is a very simple reason: The driver writer cannot know about the
> requirements of the complete system which is composed of kernel, drivers
> and user space applications, unless the driver writer is fully aware of
> the overall system design and constraints.
> 
> How is that supposed to work on a general purpose kernel which is
> utilized for a gazillion of different use cases which all have different
> expectations?
> 
> It simply cannot work because default A will only work for usecase A and
> be completely wrong for all others.
> 
>> Further, what if irq handlear thread has to run on the expected priority
>> at the very beginning? This patch helps.
> 
> There is no such thing as the expected priority of an interrupt thread
> which can be applied upfront.
> 
> There are ~5400 instances of request*irq() in the kernel source and
> there is no way to make priority decisions for them which work for every
> RT system out there.
> 
> The kernel sets a default and the system designer, admin, user has to
> take care of tuning it to match the expectations and constraints of his
> particular application scenario.
> 
> The kernel provides an userspace interface to do that. That interface
> might be a bit awkward to use, but there are tools out there which help
> with that, and if at all we can think about providing a better and
> easier to use interface for this.
> 
> Trying to solve that at the kernel level is putting the cart before the
> horse.
> 
> Thanks,
> 
>          tglx
>