[PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[Chen Yu]

When no eligible entity is found in pick_eevdf(), it has to pick
the entity with smallest vruntime. This indicates a potential issue
and scheduler will print this error.

However this printk could introduce possible circular locking issue
because when the code path reaches here with the rq lock held, the
printk could trigger further scheduling which loops back to the
scheduler.

Use printk_deferred() to defer the console write from current context
to the irq work in the next tick.

Fixes: 147f3efaa241 ("sched/fair: Implement an EEVDF-like scheduling policy")
Suggested-by: Phil Auld <pauld@redhat.com>
Reported-by: Biju Das <biju.das.jz@bp.renesas.com>
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 061a30a8925a..70f38e54b6ce 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -973,7 +973,7 @@ static struct sched_entity *pick_eevdf(struct cfs_rq *cfs_rq)
 	if (!se) {
 		struct sched_entity *left = __pick_first_entity(cfs_rq);
 		if (left) {
-			pr_err("EEVDF scheduling fail, picking leftmost\n");
+			printk_deferred(KERN_ERR "EEVDF scheduling fail, picking leftmost\n");
 			return left;
 		}
 	}
-- 
2.25.1

Re: [PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[Peter Zijlstra]

On Tue, Oct 10, 2023 at 11:25:41AM +0800, Chen Yu wrote:
> When no eligible entity is found in pick_eevdf(), it has to pick
> the entity with smallest vruntime. This indicates a potential issue
> and scheduler will print this error.
> 
> However this printk could introduce possible circular locking issue
> because when the code path reaches here with the rq lock held, the
> printk could trigger further scheduling which loops back to the
> scheduler.
> 
> Use printk_deferred() to defer the console write from current context
> to the irq work in the next tick.

No.. I detest printk_deferred with a passion. This is effectively a WARN
and we don't do silly buggers for them either.

Re: [PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[Phil Auld]

On Tue, Oct 10, 2023 at 09:59:28AM +0200 Peter Zijlstra wrote:
> On Tue, Oct 10, 2023 at 11:25:41AM +0800, Chen Yu wrote:
> > When no eligible entity is found in pick_eevdf(), it has to pick
> > the entity with smallest vruntime. This indicates a potential issue
> > and scheduler will print this error.
> > 
> > However this printk could introduce possible circular locking issue
> > because when the code path reaches here with the rq lock held, the
> > printk could trigger further scheduling which loops back to the
> > scheduler.
> > 
> > Use printk_deferred() to defer the console write from current context
> > to the irq work in the next tick.

Chen: I was not actually suggesting you make this change, just answering your
question about printk/rq lock.  You don't need to put that line in there.

> 
> No.. I detest printk_deferred with a passion. This is effectively a WARN
> and we don't do silly buggers for them either.
>

Sure, printk_deferred is not ideal, but is getting this message in the right
order worth locking up people's machines?  Not sure you get the message at
all when that happens.  I have to dig the code location out of the crash
dump to find which sched warning fired and took down the (usually virtual)
machine.


Cheers,
Phil

-- 

Re: [PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[Peter Zijlstra]

On Tue, Oct 10, 2023 at 08:26:00AM -0400, Phil Auld wrote:

> > No.. I detest printk_deferred with a passion. This is effectively a WARN
> > and we don't do silly buggers for them either.
> >
> 
> Sure, printk_deferred is not ideal, but is getting this message in the right
> order worth locking up people's machines?  Not sure you get the message at
> all when that happens.  I have to dig the code location out of the crash
> dump to find which sched warning fired and took down the (usually virtual)
> machine.

Same thing with WARN(), we don't have a silly bugger version of that
either. Just use a sane printk() / console or whatever.

Virt stuff has perfectly functional serial consoles that works just fine
and don't lock up the machine -- mostly. Use my early_printk hacks if
you want something reliable:

  https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=debug/experimental

Boot with: "earlyprintk=serial,ttyS0,115200 force_early_printk". And all
will be well.

The fact that crashdump is more reliable than printk is a *BIG* problem
and the only solution is fixing printk() (people are sorta working on
that). We should not try and work around this problem.

I fundamentally despise the delayed stuff, I've had countless insteances
where delaying output means you have no output because the machine is
dead.

Re: [PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[Chen Yu]

On 2023-10-10 at 16:12:44 +0200, Peter Zijlstra wrote:
> On Tue, Oct 10, 2023 at 08:26:00AM -0400, Phil Auld wrote:
> 
> > > No.. I detest printk_deferred with a passion. This is effectively a WARN
> > > and we don't do silly buggers for them either.
> > >
> > 
> > Sure, printk_deferred is not ideal, but is getting this message in the right
> > order worth locking up people's machines?  Not sure you get the message at
> > all when that happens.  I have to dig the code location out of the crash
> > dump to find which sched warning fired and took down the (usually virtual)
> > machine.
> 
> Same thing with WARN(), we don't have a silly bugger version of that
> either. Just use a sane printk() / console or whatever.
> 
> Virt stuff has perfectly functional serial consoles that works just fine
> and don't lock up the machine -- mostly. Use my early_printk hacks if
> you want something reliable:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=debug/experimental
> 
> Boot with: "earlyprintk=serial,ttyS0,115200 force_early_printk". And all
> will be well.
> 
> The fact that crashdump is more reliable than printk is a *BIG* problem
> and the only solution is fixing printk() (people are sorta working on
> that). We should not try and work around this problem.
> 
> I fundamentally despise the delayed stuff, I've had countless insteances
> where delaying output means you have no output because the machine is
> dead.

I see, thanks for this information.

thanks,
Chenyu

Re: [PATCH] sched/fair: Use printk_deferred instead of printk in pick_eevdf()

[Chen Yu]

On 2023-10-10 at 08:26:00 -0400, Phil Auld wrote:
> On Tue, Oct 10, 2023 at 09:59:28AM +0200 Peter Zijlstra wrote:
> > On Tue, Oct 10, 2023 at 11:25:41AM +0800, Chen Yu wrote:
> > > When no eligible entity is found in pick_eevdf(), it has to pick
> > > the entity with smallest vruntime. This indicates a potential issue
> > > and scheduler will print this error.
> > > 
> > > However this printk could introduce possible circular locking issue
> > > because when the code path reaches here with the rq lock held, the
> > > printk could trigger further scheduling which loops back to the
> > > scheduler.
> > > 
> > > Use printk_deferred() to defer the console write from current context
> > > to the irq work in the next tick.
> 
> Chen: I was not actually suggesting you make this change, just answering your
> question about printk/rq lock.  You don't need to put that line in there.
>

Phil,

Sorry for misunderstanding that, and thanks again for your guidance.

thanks,
Chenyu