Re: [PATCH v2 29/35] sched: handle preempt=voluntary under PREEMPT_AUTO

[Tianchen Ding]

On 2024/6/22 02:58, Ankur Arora wrote:
> Tianchen Ding <dtcccc@xxxxxxxxxxxxxxxxx> writes:
>
> > On 2024/5/28 08:35, Ankur Arora wrote:
> > > The default preemption policy for voluntary preemption under
> > > PREEMPT_AUTO is to schedule eagerly for tasks of higher scheduling
> > > class, and lazily for well-behaved, non-idle tasks.
> > > This is the same policy as preempt=none, with an eager handling of
> > > higher priority scheduling classes.
> > > Comparing a cyclictest workload with a background kernel load of
> > > 'stress-ng --mmap', shows that both the average and the maximum
> > > latencies improve:
> > >    # stress-ng --mmap 0 &
> > >    # cyclictest --mlockall --smp --priority=80 --interval=200 --distance=0 -q -D 300
> > >                                        Min     (  %stdev )    Act     (  %stdev
> > > )   Avg     (  %stdev )   Max      (  %stdev )
> > >     PREEMPT_AUTO, preempt=voluntary    1.73  ( +-  25.43% )   62.16 ( +-
> > > 303.39% )  14.92 ( +-  17.96% )  2778.22 ( +-  15.04% )
> > >     PREEMPT_DYNAMIC, preempt=voluntary 1.83  ( +-  20.76% )  253.45 ( +- 233.21% )  18.70 ( +-  15.88% )  2992.45 ( +-  15.95% )
> > > The table above shows the aggregated latencies across all CPUs.
> > > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > > Cc: Peter Ziljstra <peterz@xxxxxxxxxxxxx>
> > > Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> > > Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> > > Originally-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
> > > Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> > > ---
> > >    kernel/sched/core.c  | 12 ++++++++----
> > >    kernel/sched/sched.h |  6 ++++++
> > >    2 files changed, 14 insertions(+), 4 deletions(-)
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index c25cccc09b65..2bc3ae21a9d0 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -1052,6 +1052,9 @@ static resched_t resched_opt_translate(struct task_struct *curr,
> > >    	if (preempt_model_preemptible())
> > >    		return RESCHED_NOW;
> > >    +	if (preempt_model_voluntary() && opt == RESCHED_PRIORITY)
> > > +		return RESCHED_NOW;
> > > +
> > >    	if (is_idle_task(curr))
> > >    		return RESCHED_NOW;
> > >    @@ -2289,7 +2292,7 @@ void wakeup_preempt(struct rq *rq, struct task_struct
> > > *p, int flags)
> > >    	if (p->sched_class == rq->curr->sched_class)
> > >    		rq->curr->sched_class->wakeup_preempt(rq, p, flags);
> > >    	else if (sched_class_above(p->sched_class, rq->curr->sched_class))
> > > -		resched_curr(rq);
> > > +		resched_curr_priority(rq);
> > Besides the conditions about higher class, can we do resched_curr_priority() in the same class?
> > For example, in fair class, we can do it when SCHED_NORMAL vs SCHED_IDLE.
>
> So, I agree about the specific case of SCHED_NORMAL vs SCHED_IDLE.
> (And, that case is already handled by resched_opt_translate() explicitly
> promoting idle tasks to TIF_NEED_RESCHED.)
>
> But, on the general question of doing resched_curr_priority() in the
> same class: I did consider it. But, it seemed to me that we want to
> keep run to completion semantics for lazy scheduling, and so not
> enforcing priority in a scheduling class was a good line.

OK, on general question, this is just a suggestion :-)

Actually, my key point is about SCHED_IDLE. It's not a real idle task, but a 
normal task with lowest priority. So is_idle_task() in resched_opt_translate() 
does not fit it. Should add task_has_idle_policy().

However, even using task_has_idle_policy() may be still not enough. Because 
SCHED_IDLE policy:
  1. It is the lowest priority, but still belongs to fair_sched_class, which is 
the same as SCHED_NORMAL.
  2. Not only tasks, *se of cgroup* can be SCHED_IDLE, too. (introduced by 
commit 304000390f88d)

So in the special case about SCHED_NORMAL vs SCHED_IDLE, I suggest still do some 
work in fair.c.

Thanks.