[PATCH] 4.9.263-rt177: Balance local_irq_{disable,enable} in irq_forced_thread_fn

[PATCH] 4.9.263-rt177: Balance local_irq_{disable,enable} in irq_forced_thread_fn

[Joe Korty]

Balance local_irq_{disable,enable} usage in irq_forced_thread_fn

Re: 0152-genirq-Allow-disabling-of-softirq-processing-in-irq-.patch

In 4.9.263-rt177, irq_forced_thread_fn has potentially unbalanced calls to
local_irq_disable ... local_irq_enable.  This is probably not intentional.

I am not absolutely sure what the proper fix is.  Attached is an example
of what that might look like.

[ Issue detected via compiler warning, using a sufficiently advanced gcc ]

Signed-off-by: Joe Korty <joe.korty@concurrent-rt.com>

Index: b/kernel/irq/manage.c
===================================================================
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1035,6 +1035,9 @@ irq_forced_thread_fn(struct irq_desc *de
 		atomic_inc(&desc->threads_handled);
 
 	irq_finalize_oneshot(desc, action);
+
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
+		local_irq_enable();
 	/*
 	 * Interrupts which have real time requirements can be set up
 	 * to avoid softirq processing in the thread handler. This is
@@ -1043,8 +1046,6 @@ irq_forced_thread_fn(struct irq_desc *de
 	if (irq_settings_no_softirq_call(desc))
 		_local_bh_enable();
 	else
-		if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
-			local_irq_enable();
 		local_bh_enable();
 	return ret;
 }

Re: [PATCH] 4.9.263-rt177: Balance local_irq_{disable,enable} in irq_forced_thread_fn

[Luis Claudio R. Goncalves]

On Tue, Apr 27, 2021 at 06:10:09PM -0400, Joe Korty wrote:
> Balance local_irq_{disable,enable} usage in irq_forced_thread_fn
> 
> Re: 0152-genirq-Allow-disabling-of-softirq-processing-in-irq-.patch
> 
> In 4.9.263-rt177, irq_forced_thread_fn has potentially unbalanced calls to
> local_irq_disable ... local_irq_enable.  This is probably not intentional.
> 
> I am not absolutely sure what the proper fix is.  Attached is an example
> of what that might look like.
> 
> [ Issue detected via compiler warning, using a sufficiently advanced gcc ]
> 
> Signed-off-by: Joe Korty <joe.korty@concurrent-rt.com>

Joe, I fell a bit behind on v4.9-rt and intend to get up to date this
weekend. I am now satisfied with my current backport of the futex changes
from v4.9.264 and my 96h of pi_stress testing.

Your patch makes sense, taking v4.19-rt as reference, but I will wait
for comments from the wise people you listed on the Cc: before taking
action.

Best regards,
Luis

> 
> Index: b/kernel/irq/manage.c
> ===================================================================
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1035,6 +1035,9 @@ irq_forced_thread_fn(struct irq_desc *de
>  		atomic_inc(&desc->threads_handled);
>  
>  	irq_finalize_oneshot(desc, action);
> +
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
> +		local_irq_enable();
>  	/*
>  	 * Interrupts which have real time requirements can be set up
>  	 * to avoid softirq processing in the thread handler. This is
> @@ -1043,8 +1046,6 @@ irq_forced_thread_fn(struct irq_desc *de
>  	if (irq_settings_no_softirq_call(desc))
>  		_local_bh_enable();
>  	else
> -		if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
> -			local_irq_enable();
>  		local_bh_enable();
>  	return ret;
>  }
---end quoted text---

Re: [PATCH] 4.9.263-rt177: Balance local_irq_{disable,enable} in irq_forced_thread_fn

[Daniel Wagner]

Hi,

On Tue, Apr 27, 2021 at 07:47:41PM -0300, Luis Claudio R. Goncalves wrote:
> On Tue, Apr 27, 2021 at 06:10:09PM -0400, Joe Korty wrote:
> > Balance local_irq_{disable,enable} usage in irq_forced_thread_fn
> > 
> > Re: 0152-genirq-Allow-disabling-of-softirq-processing-in-irq-.patch
> > 
> > In 4.9.263-rt177, irq_forced_thread_fn has potentially unbalanced calls to
> > local_irq_disable ... local_irq_enable.  This is probably not intentional.
> > 
> > I am not absolutely sure what the proper fix is.  Attached is an example
> > of what that might look like.
> > 
> > [ Issue detected via compiler warning, using a sufficiently advanced gcc ]
> > 
> > Signed-off-by: Joe Korty <joe.korty@concurrent-rt.com>
> 
> Joe, I fell a bit behind on v4.9-rt and intend to get up to date this
> weekend. I am now satisfied with my current backport of the futex changes
> from v4.9.264 and my 96h of pi_stress testing.
> 
> Your patch makes sense, taking v4.19-rt as reference, but I will wait
> for comments from the wise people you listed on the Cc: before taking
> action.

I had to resolve the same merge conflict in v4.4-rt. The fix from Joe
resolve to the same solution I've done in v4.4-rt

> 
> Best regards,
> Luis
> 
> > 
> > Index: b/kernel/irq/manage.c
> > ===================================================================
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -1035,6 +1035,9 @@ irq_forced_thread_fn(struct irq_desc *de
> >  		atomic_inc(&desc->threads_handled);
> >  
> >  	irq_finalize_oneshot(desc, action);
> > +

Nit there is no new line here in upstream, dd652c6ab49b ("genirq:
Disable interrupts for force threaded handlers")

> > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
> > +		local_irq_enable();
> >  	/*
> >  	 * Interrupts which have real time requirements can be set up
> >  	 * to avoid softirq processing in the thread handler. This is
> > @@ -1043,8 +1046,6 @@ irq_forced_thread_fn(struct irq_desc *de
> >  	if (irq_settings_no_softirq_call(desc))
> >  		_local_bh_enable();
> >  	else
> > -		if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
> > -			local_irq_enable();
> >  		local_bh_enable();
> >  	return ret;
> >  }
> ---end quoted text---

Thanks,
Daniel

Re: [PATCH] 4.9.263-rt177: Balance local_irq_{disable,enable} in irq_forced_thread_fn

[Luis Claudio R. Goncalves]

On Wed, Apr 28, 2021 at 09:11:53AM +0200, Daniel Wagner wrote:
> Hi,
> 
> On Tue, Apr 27, 2021 at 07:47:41PM -0300, Luis Claudio R. Goncalves wrote:
> > On Tue, Apr 27, 2021 at 06:10:09PM -0400, Joe Korty wrote:
> > > Balance local_irq_{disable,enable} usage in irq_forced_thread_fn
> > > 
> > > Re: 0152-genirq-Allow-disabling-of-softirq-processing-in-irq-.patch
> > > 
> > > In 4.9.263-rt177, irq_forced_thread_fn has potentially unbalanced calls to
> > > local_irq_disable ... local_irq_enable.  This is probably not intentional.
> > > 
> > > I am not absolutely sure what the proper fix is.  Attached is an example
> > > of what that might look like.
> > > 
> > > [ Issue detected via compiler warning, using a sufficiently advanced gcc ]
> > > 
> > > Signed-off-by: Joe Korty <joe.korty@concurrent-rt.com>
> > 
> > Joe, I fell a bit behind on v4.9-rt and intend to get up to date this
> > weekend. I am now satisfied with my current backport of the futex changes
> > from v4.9.264 and my 96h of pi_stress testing.
> > 
> > Your patch makes sense, taking v4.19-rt as reference, but I will wait
> > for comments from the wise people you listed on the Cc: before taking
> > action.
> 
> I had to resolve the same merge conflict in v4.4-rt. The fix from Joe
> resolve to the same solution I've done in v4.4-rt

I will add this patch to the next v4.9-rt without the extra empty line.

Thank you both,
Luis
 
> > 
> > Best regards,
> > Luis
> > 
> > > 
> > > Index: b/kernel/irq/manage.c
> > > ===================================================================
> > > --- a/kernel/irq/manage.c
> > > +++ b/kernel/irq/manage.c
> > > @@ -1035,6 +1035,9 @@ irq_forced_thread_fn(struct irq_desc *de
> > >  		atomic_inc(&desc->threads_handled);
> > >  
> > >  	irq_finalize_oneshot(desc, action);
> > > +
> 
> Nit there is no new line here in upstream, dd652c6ab49b ("genirq:
> Disable interrupts for force threaded handlers")
> 
> > > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
> > > +		local_irq_enable();
> > >  	/*
> > >  	 * Interrupts which have real time requirements can be set up
> > >  	 * to avoid softirq processing in the thread handler. This is
> > > @@ -1043,8 +1046,6 @@ irq_forced_thread_fn(struct irq_desc *de
> > >  	if (irq_settings_no_softirq_call(desc))
> > >  		_local_bh_enable();
> > >  	else
> > > -		if (!IS_ENABLED(CONFIG_PREEMPT_RT_BASE))
> > > -			local_irq_enable();
> > >  		local_bh_enable();
> > >  	return ret;
> > >  }
> > ---end quoted text---
> 
> Thanks,
> Daniel
---end quoted text---