Re: [tip: core/urgent] panic: Reenable preemption in WARN slowpath

[Ingo Molnar]

* Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> > > panic: Reenable preemption in WARN slowpath
> > > 
> > > Commit:
> > > 
> > >   5a5d7e9badd2 ("cpuidle: lib/bug: Disable rcu_is_watching() during WARN/BUG")
> > > 
> > > amended warn_slowpath_fmt() to disable preemption until the WARN splat
> > > has been emitted.
> > > 
> > > However the commit neglected to reenable preemption in the !fmt codepath,
> > > i.e. when a WARN splat is emitted without additional format string.
> > > 
> > > One consequence is that users may see more splats than intended.  E.g. a
> > > WARN splat emitted in a work item results in at least two extra splats:
> > > 
> > >   BUG: workqueue leaked lock or atomic
> > >   (emitted by process_one_work())
> > > 
> > >   BUG: scheduling while atomic
> > >   (emitted by worker_thread() -> schedule())
> > > 
> > > Ironically the point of the commit was to *avoid* extra splats. ;)
> > > 
> > > Fix it.
> > 
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index 07239d4..ffa037f 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -697,6 +697,7 @@ void warn_slowpath_fmt(const char *file, int line, unsigned taint,
> > >  	if (!fmt) {
> > >  		__warn(file, line, __builtin_return_address(0), taint,
> > >  		       NULL, NULL);
> > > +		warn_rcu_exit(rcu);
> > >  		return;
> > 
> > BTW., one more thing we might want to consider here is to re-enable 
> > preemption in warn_rcu_exit() a bit more gently, without forcing a
> > pending reschedule, ie. preempt_enable_no_resched() or so?
> 
> nah, it's a warn, if that triggers you get to keep the pieces.

But but ... my overall point is that since we just WARN()ed, we are facing 
some sort of kernel bug, and scheduling policies are only a secondary 
concern, debuggability & getting the bug fixed is the primary concern.

So the scheduler should switch to a debugging-friendlier behavior:

  'Schedule tasks around as little as possible, to keep the debug output 
   tidy & to keep things working a bit better even if it's all broken 
   already'.

... or so. My suggestion was a small subset of that principle.

> [...] Also preempt_enable_no_resched() isn't exported because its a 
> horribly dangerous function.

Special exception for RCU debugging only, or so - it's a core kernel 
facility after all.

No strong feelings either way though.

Thanks,

	Ingo