Re: [PATCH] softirq: WARN_ON !preemptible() not check softirq cnt in bh disable on RT

[Sebastian Andrzej Siewior]

On 2026-03-11 23:34:02 [+0800], Xin Zhao wrote:
> hi, Sebastian
Hi,

> On 2026-03-11 14:51 UTC, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
> 
> > > I just think that using (system_state != SYSTEM_BOOTING) for conditional checks
> > > is more reasonable than using (softirq_ctrl.cnt).
> > 
> > We had users which which did
> > 	local_irq_disable();
> > 	local_bh_disable();
> > 
> > and we decided not to bother if there is no reason to.
> 
> If there are users have such usage, it will probabilistically hit the print statement
> for DEBUG_LOCKS_WARN_ON(softirq_ctrl.cnt). This is because the task running this code
> may preempt a task that has already entered the local_bh_disable critical section.
> Isn't that right?

Correct. And that is how got rid of the offenders. If I remember
correctly a few remained during kernel init which were considered not an
issue. But I just booted a few boxes double checking and I don't see the
warning meaning it went away.

> > > I should use the situation where CONFIG_PREEMPT_RT_NEEDS_BH_LOCK is enabled to
> > > illustrate the example above. People would expect that soft interrupts on this
> > > CPU would not execute after calling local_bh_disable(), but as shown in the
> > > example, this cannot actually be guaranteed.
> > 
> > If CONFIG_PREEMPT_RT_NEEDS_BH_LOCK is enabled then the scenario is
> > possible but the DEBUG_LOCKS_WARN_ON will trigger a warning.
> 
> Indeed, it will trigger this warning; it just reduces the probability of it being
> reported.
Yes

> > Hmm. Is there actually anything wrong in tree?
> > Longterm I would intend to make !CONFIG_PREEMPT_RT_NEEDS_BH_LOCK
> > default.
> 
> I am tracing the soft interrupt code interface and found a _local_bh_enable interface
> that only exists in non-RT Linux. This _local_bh_enable is used in sclp.c as follows:

Funny story: I did a grep for the pattern you described and this s390
driver was the only thing that popped up.

>     local_irq_disable();
>     local_ctl_load(0, &cr0);
>     if (!irq_context)
>         _local_bh_enable();
>     local_tick_enable(old_tick);
>     local_irq_restore(flags);
> 
> I did not further investigate why the above code in sclp.c is not used in RT-Linux. As
> for why _local_bh_enable does not exist in RT-Linux, it may be due to the consideration
> that local_bh_disable is "ineffective" in the !preemptible() state in RT-Linux, but I'm
> not sure if my understanding is correct.
> 
> Since you also mentioned that later CONFIG_PREEMPT_RT_NEEDS_BH_LOCK will no longer be
> enabled, at that point, local_bh_disable almost loses its significance. I think it
> should either be removed or implemented as a no-op, as it no longer achieves our
> expected effect, and it would be better to save some instruction execution time.

We can't nop it entirely. local_bh_disable() needs remain a RCU read
section and it needs to ensure that the context does not wonder off to
another CPU. Also we need to count the disable/enable because once we go
back to zero, we need to run callbacks which may have queued up.
And if we queue the softirq on per-task basis rather then per-CPU then
we don't have the problem that one task completes softirqs queued by
another one.

> Xin Zhao

Sebastian