[PATCH] RFC: Set irq thread to RT priority on creation

[PATCH] RFC: Set irq thread to RT priority on creation

[Ivo Sieben]

When a threaded irq handler is installed the irq thread is initially created
on normal scheduling priority. Only after the the irq thread is woken up it
sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.

This means that interrupts that occur directly after the irq handler is
installed will be handled on a normal scheduling priority instead of the
realtime priority that you would expect. Fixed this by setting the RT
priority on creation of the irq_thread.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---

RFC:
Whas there a specific reason for the irq_thread to be created on normal
scheduling and only set to RT priority when woken up?
This patch solves an issue for me where a device driver is expected to handle an
interrupt immediatly after irq handlers are installed and interrupts enabled.

(If this does make sense: I guess the sched_setscheduler() call in the irq_thread
function can be removed?)

 kernel/irq/manage.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..0ffe37b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -950,6 +950,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 */
 	if (new->thread_fn && !nested) {
 		struct task_struct *t;
+		static const struct sched_param param = {
+			.sched_priority = MAX_USER_RT_PRIO/2,
+		};
 
 		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
 				   new->name);
@@ -957,6 +960,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			ret = PTR_ERR(t);
 			goto out_mput;
 		}
+
+		sched_setscheduler(t, SCHED_FIFO, &param);
+
 		/*
 		 * We keep the reference to the task struct even if
 		 * the thread dies to avoid that the interrupt code
-- 
1.7.9.5

Re: [PATCH] RFC: Set irq thread to RT priority on creation

[Thomas Gleixner]

On Thu, 30 May 2013, Ivo Sieben wrote:

> When a threaded irq handler is installed the irq thread is initially created
> on normal scheduling priority. Only after the the irq thread is woken up it
> sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.
> 
> This means that interrupts that occur directly after the irq handler is
> installed will be handled on a normal scheduling priority instead of the
> realtime priority that you would expect. Fixed this by setting the RT
> priority on creation of the irq_thread.
> 
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
> ---
> 
> RFC:
> Whas there a specific reason for the irq_thread to be created on normal
> scheduling and only set to RT priority when woken up?

No.

> This patch solves an issue for me where a device driver is expected to handle an
> interrupt immediatly after irq handlers are installed and interrupts enabled.

You miss to explain what kind of issue that is.

Thanks,

	tglx

Re: [PATCH] RFC: Set irq thread to RT priority on creation

[Steven Rostedt]

On Thu, 2013-05-30 at 16:07 +0200, Thomas Gleixner wrote:

> > This patch solves an issue for me where a device driver is expected to handle an
> > interrupt immediatly after irq handlers are installed and interrupts enabled.
> 
> You miss to explain what kind of issue that is.

I could envision the case where the interrupt is initialized but doesn't
go off until much later. If it never ran, then it would still be in
SCHED_OTHER(), and that first interrupt could have a large delay.

-- Steve

Re: [PATCH] RFC: Set irq thread to RT priority on creation

[Thomas Gleixner]

On Thu, 30 May 2013, Steven Rostedt wrote:

> On Thu, 2013-05-30 at 16:07 +0200, Thomas Gleixner wrote:
> 
> > > This patch solves an issue for me where a device driver is expected to handle an
> > > interrupt immediatly after irq handlers are installed and interrupts enabled.
> > 
> > You miss to explain what kind of issue that is.
> 
> I could envision the case where the interrupt is initialized but doesn't
> go off until much later. If it never ran, then it would still be in
> SCHED_OTHER(), and that first interrupt could have a large delay.

Nope. As Ivo explained it's about an interrupt coming in right away,
i.e. before __setup_irq() reaches:

        if (new->thread)
	   wake_up_process(new->thread);

The ones which come much later do not have that issue as the thread
code already sits in the waiting loop and already adjusted the
priority.

Thanks,

	tglx

[PATCH-v2] Set irq thread to RT priority on creation

[Ivo Sieben]

When a threaded irq handler is installed the irq thread is initially created
on normal scheduling priority. Only after the the irq thread is woken up it
immediately sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.

This means that interrupts that occur directly after the irq handler is
installed will be handled on a normal scheduling priority instead of the
realtime priority that you would expect. Fixed this by setting the RT
priority on creation of the irq_thread.

This solves the following issue with a UART device driver:
On start of the application there is already data present in the uart RX
fifo buffer. On opening the uart device the hard & threaded interrupt
handlers are installed and the interrupts are enabled. Immediately a hard
irq occurs because there is still data in the RX fifo. Because the threaded
irq handler is still on normal scheduling, my application is not immediatly
interrupted by the threaded handler and continues to run: it tries to flush
the uart input buffer and writes data to the uart device. After this the
threaded handler finally gets scheduled in and fills the buffer with the
"old" received data. When my application reads data from the uart port it
receives the "old" data and gives an error.

Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---

v2:
 * Removed the sched_setscheduler() call in irq_thread() function
 * Updated commit log why this solves an issue for me with a UART driver

 kernel/irq/manage.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..e16caa8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -840,9 +840,6 @@ static void irq_thread_dtor(struct callback_head *unused)
 static int irq_thread(void *data)
 {
 	struct callback_head on_exit_work;
-	static const struct sched_param param = {
-		.sched_priority = MAX_USER_RT_PRIO/2,
-	};
 	struct irqaction *action = data;
 	struct irq_desc *desc = irq_to_desc(action->irq);
 	irqreturn_t (*handler_fn)(struct irq_desc *desc,
@@ -854,8 +851,6 @@ static int irq_thread(void *data)
 	else
 		handler_fn = irq_thread_fn;
 
-	sched_setscheduler(current, SCHED_FIFO, &param);
-
 	init_task_work(&on_exit_work, irq_thread_dtor);
 	task_work_add(current, &on_exit_work, false);
 
@@ -950,6 +945,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 */
 	if (new->thread_fn && !nested) {
 		struct task_struct *t;
+		static const struct sched_param param = {
+			.sched_priority = MAX_USER_RT_PRIO/2,
+		};
 
 		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
 				   new->name);
@@ -957,6 +955,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			ret = PTR_ERR(t);
 			goto out_mput;
 		}
+
+		sched_setscheduler(t, SCHED_FIFO, &param);
+
 		/*
 		 * We keep the reference to the task struct even if
 		 * the thread dies to avoid that the interrupt code
-- 
1.7.9.5

Re: [PATCH-v2] Set irq thread to RT priority on creation

[Thomas Gleixner]

On Mon, 3 Jun 2013, Ivo Sieben wrote:

> When a threaded irq handler is installed the irq thread is initially created
> on normal scheduling priority. Only after the the irq thread is woken up it
> immediately sets its priority to RT_FIFO MAX_USER_RT_PRIO/2.
> 
> This means that interrupts that occur directly after the irq handler is
> installed will be handled on a normal scheduling priority instead of the
> realtime priority that you would expect. Fixed this by setting the RT
> priority on creation of the irq_thread.
> 
> This solves the following issue with a UART device driver:
> On start of the application there is already data present in the uart RX
> fifo buffer. On opening the uart device the hard & threaded interrupt
> handlers are installed and the interrupts are enabled. Immediately a hard
> irq occurs because there is still data in the RX fifo. Because the threaded
> irq handler is still on normal scheduling, my application is not immediatly
> interrupted by the threaded handler and continues to run: it tries to flush
> the uart input buffer and writes data to the uart device. After this the
> threaded handler finally gets scheduled in and fills the buffer with the
> "old" received data. When my application reads data from the uart port it
> receives the "old" data and gives an error.

While I in principle agree with the patch, the issue you are
describing is just solved accidentaly.

The question is why there is data present in the UART when the UART
driver has not initialized the UART. Up to the point where the UART is
opened and the interrupt handler is installed the receiver should be
disabled. Also there is the question why a flush does not kill
everything including the pending data in the UART itself.

And I don't think, that your issue is solved completely. Assume the
following:

Open UART
Flush Buffers (including UART fifo)

---> UART receives data
---> Interrupt
	Data is available in tty buffer

Write data to UART

Receive data from UART

You still get data which got into the buffer before you sent stuff
out. So your application should not be surpriced at all by random data
on the receive line when it starts up.

Thanks,

	tglx

Re: [PATCH-v2] Set irq thread to RT priority on creation

[Ivo Sieben]

Hi Thomas,

2013/6/3 Thomas Gleixner <tglx@linutronix.de>:
>
> The question is why there is data present in the UART when the UART
> driver has not initialized the UART. Up to the point where the UART is
> opened and the interrupt handler is installed the receiver should be
> disabled. Also there is the question why a flush does not kill
> everything including the pending data in the UART itself.
>
> And I don't think, that your issue is solved completely. Assume the
> following:
>
> Open UART
> Flush Buffers (including UART fifo)
>
> ---> UART receives data
> ---> Interrupt
>         Data is available in tty buffer
>
> Write data to UART
>
> Receive data from UART
>
> You still get data which got into the buffer before you sent stuff
> out. So your application should not be surpriced at all by random data
> on the receive line when it starts up.
>
> Thanks,
>
>         tglx

You are absolutely right, the real issue was that my UART device still
received data while the device was closed. And yes, the protocol that
we use should be robust to unexpected data. I solved & will solve
these problems now. So you indeed the uart explanation in my commit
log should be removed.

The point is that we while we were debugging & tracing this problem,
we didn't expect this behavior: in the trace we saw a threaded handler
scheduled in on 'normal' priority which surprised us. Also I think
there are situations thinkable (altough I cannot come up with a proper
example) where it is normal behavior that a IRQ directly kicks in
after enabling the interrupts.

Regards,
Ivo Sieben

Re: [PATCH-v2] Set irq thread to RT priority on creation

[Sebastian Andrzej Siewior]

* Ivo Sieben | 2013-06-03 12:12:02 [+0200]:

this is in -tip so I take this for v3.8-rt

Sebastian

Re: [PATCH] RFC: Set irq thread to RT priority on creation

[Steven Rostedt]

On Thu, 2013-05-30 at 14:12 +0200, Ivo Sieben wrote:

>  kernel/irq/manage.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index fa17855..0ffe37b 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -950,6 +950,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	 */
>  	if (new->thread_fn && !nested) {
>  		struct task_struct *t;
> +		static const struct sched_param param = {
> +			.sched_priority = MAX_USER_RT_PRIO/2,
> +		};
>  
>  		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
>  				   new->name);
> @@ -957,6 +960,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  			ret = PTR_ERR(t);
>  			goto out_mput;
>  		}
> +
> +		sched_setscheduler(t, SCHED_FIFO, &param);
> +

If you are adding this here, might as well remove the
sched_set_scheduler() from irq_thread() as well. No need to do it twice.

-- Steve

>  		/*
>  		 * We keep the reference to the task struct even if
>  		 * the thread dies to avoid that the interrupt code