Message-ID: <CAEXW_YTaG_TzY9r7Q297R1C7S4FzOopzQA-xRKU_JFKOVRA6hA@mail.gmail.com>
Date:   Wed, 16 Nov 2022 02:10:11 +0000
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, vschneid@...hat.com,
        linux-kernel@...r.kernel.org, parth@...ux.ibm.com,
        qyousef@...alina.io, chris.hyser@...cle.com,
        patrick.bellasi@...bug.net, David.Laight@...lab.com,
        pjt@...gle.com, pavel@....cz, tj@...nel.org, qperret@...gle.com,
        tim.c.chen@...ux.intel.com, joshdon@...gle.com, timj@....org,
        kprateek.nayak@....com, yu.c.chen@...el.com,
        youssefesmat@...omium.org, riel@...hat.com
Subject: Re: [PATCH v8 1/9] sched/fair: fix unfairness at wakeup

Hi Vincent,

On Mon, Nov 14, 2022 at 11:05 AM Vincent Guittot
<vincent.guittot@...aro.org> wrote:
[...]
> >
> > On Thu, Nov 10, 2022 at 06:50:01PM +0100, Vincent Guittot wrote:
> > > At wake up, the vruntime of a task is updated to not be more older than
> > > a sched_latency period behind the min_vruntime. This prevents long sleeping
> > > task to get unlimited credit at wakeup.
> > > Such waking task should preempt current one to use its CPU bandwidth but
> > > wakeup_gran() can be larger than sched_latency, filter out the
> > > wakeup preemption and as a results steals some CPU bandwidth to
> > > the waking task.
> >
> > Just a thought: one can argue that this also hurts the running task because
> > wakeup_gran() is expected to not preempt the running task for a certain
> > minimum amount of time right?
>
> No because you should not make wakeup_gran() higher than sched_latency.
>
> >
> > So for example, if I set sysctl_sched_wakeup_granularity to a high value, I
> > expect the current task to not be preempted for that long, even if the
> > sched_latency cap in place_entity() makes the delta smaller than
> > wakeup_gran(). The place_entity() in current code is used to cap the sleep
> > credit, it does not really talk about preemption.
>
> But one should never set such nonsense values.

It is not about the user setting nonsense sysctl value. Even if you do
not change sysctl_sched_wakeup_granularity, wakeup_gran() can be large
due to NICE scaling.
wakeup_gran() scales the sysctl by the ratio of the nice-load of the
se, with the NICE_0_LOAD.

On my system, by default sysctl_sched_wakeup_granularity is 3ms, and
sysctl_sched_latency is 18ms.

However, if you set the task to nice +10, the wakeup_gran() scaling
can easily make the gran exceed sysctl_sched_latency. And also, just
to note (per my experience) sysctl_sched_latency does not really hold
anyway when nice values are not default. IOW, all tasks are not
guaranteed to run within the sched_latency window always.

Again, like I said I don't mind this change (and I think it is OK to
do) but I was just preparing you/us for someone who might say they
don't much like the aggressive preemption.

> > I don't mind this change, but it does change the meaning a bit of
> > sysctl_sched_wakeup_granularity I think.
> >
> > > Make sure that a task, which vruntime has been capped, will preempt current
> > > task and use its CPU bandwidth even if wakeup_gran() is in the same range
> > > as sched_latency.
> >
> > nit: I would prefer we say, instead of "is in the same range", "is greater
> > than". Because it got confusing a bit for me.
>
> I prefer keeping current description because the sentence below gives
> the reason why it's not strictly greater than

Honestly saying "is in the same range" is ambiguous and confusing. I
prefer the commit messages to be clear, but I leave it up to you.

> > Just a few more comments below:
[...]
> > > +
> > > +     /*
> > > +      * At wake up, the vruntime of a task is capped to not be older than
> > > +      * a sched_latency period compared to min_vruntime. This prevents long
> > > +      * sleeping task to get unlimited credit at wakeup. Such waking up task
> > > +      * has to preempt current in order to not lose its share of CPU
> > > +      * bandwidth but wakeup_gran() can become higher than scheduling period
> > > +      * for low priority task. Make sure that long sleeping task will get a
> > > +      * chance to preempt current.
> > > +      */
> > > +     gran = min_t(s64, gran, get_latency_max());
> > > +
> >
> > Can we move this to wakeup_gran(se)? IMO, it belongs there because you are
> > adjusting the wakeup_gran().
>
> I prefer keep current code because patch 8 adds offset in the equation

Ack.

> > >       if (vdiff > gran)
> > >               return 1;
> > >
> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > index 1fc198be1ffd..14879d429919 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -2432,9 +2432,9 @@ extern void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags);
> > >  extern const_debug unsigned int sysctl_sched_nr_migrate;
> > >  extern const_debug unsigned int sysctl_sched_migration_cost;
> > >
> > > -#ifdef CONFIG_SCHED_DEBUG
> > >  extern unsigned int sysctl_sched_latency;
> > >  extern unsigned int sysctl_sched_min_granularity;
> > > +#ifdef CONFIG_SCHED_DEBUG
> > >  extern unsigned int sysctl_sched_idle_min_granularity;
> > >  extern unsigned int sysctl_sched_wakeup_granularity;
> > >  extern int sysctl_resched_latency_warn_ms;
> > > @@ -2448,6 +2448,34 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
> > >  extern unsigned int sysctl_numa_balancing_scan_size;
> > >  #endif
> > >
> > > +static inline unsigned long  get_sched_latency(bool idle)
> > > +{
> >
> > IMO, since there are other users of sysctl_sched_latency, it would be better
> > to call this get_max_sleep_credit() or something.
>
> get_sleep_latency()

Ack.

> >
> > > +     unsigned long thresh;
> > > +
> > > +     if (idle)
> > > +             thresh = sysctl_sched_min_granularity;
> > > +     else
> > > +             thresh = sysctl_sched_latency;
> > > +
> > > +     /*
> > > +      * Halve their sleep time's effect, to allow
> > > +      * for a gentler effect of sleepers:
> > > +      */
> > > +     if (sched_feat(GENTLE_FAIR_SLEEPERS))
> > > +             thresh >>= 1;
> > > +
> > > +     return thresh;
> > > +}
> > > +
> > > +static inline unsigned long  get_latency_max(void)
> > > +{
> > > +     unsigned long thresh = get_sched_latency(false);
> > > +
> > > +     thresh -= sysctl_sched_min_granularity;
> >
> > Could you clarify, why are you subtracting sched_min_granularity here? Could
> > you add some comments here to make it clear?
>
> If the waking task failed to preempt current it could to wait up to
> sysctl_sched_min_granularity before preempting it during next tick.

Ok, makes sense, thanks.

Reviewed-by: Joel Fernandes (Google) <joel@...lfernandes.org>

 - Joel

Powered by blists - more mailing lists