Re: [PATCH v8 1/9] sched/fair: fix unfairness at wakeup

[Vincent Guittot]

On Mon, 14 Nov 2022 at 04:06, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Vincent,
>
> 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.

>
> 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

>
> > If the waking task failed to preempt current it could to wait up to
> > sysctl_sched_min_granularity before preempting it during next tick.
> >
> > Strictly speaking, we should use cfs->min_vruntime instead of
> > curr->vruntime but it doesn't worth the additional overhead and complexity
> > as the vruntime of current should be close to min_vruntime if not equal.
>
> Could we add here,
> Reported-by: Youssef Esmat <youssefesmat@xxxxxxxxxxxx>

yes

>
> > Signed-off-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
>
> Just a few more comments below:
>
> > ---
> >  kernel/sched/fair.c  | 46 ++++++++++++++++++++------------------------
> >  kernel/sched/sched.h | 30 ++++++++++++++++++++++++++++-
> >  2 files changed, 50 insertions(+), 26 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 5ffec4370602..eb04c83112a0 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4345,33 +4345,17 @@ place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int initial)
> >  {
> >       u64 vruntime = cfs_rq->min_vruntime;
> >
> > -     /*
> > -      * The 'current' period is already promised to the current tasks,
> > -      * however the extra weight of the new task will slow them down a
> > -      * little, place the new task so that it fits in the slot that
> > -      * stays open at the end.
> > -      */
> > -     if (initial && sched_feat(START_DEBIT))
> > -             vruntime += sched_vslice(cfs_rq, se);
> > -
> > -     /* sleeps up to a single latency don't count. */
> > -     if (!initial) {
> > -             unsigned long thresh;
> > -
> > -             if (se_is_idle(se))
> > -                     thresh = sysctl_sched_min_granularity;
> > -             else
> > -                     thresh = sysctl_sched_latency;
> > -
> > +     if (!initial)
> > +             /* sleeps up to a single latency don't count. */
> > +             vruntime -= get_sched_latency(se_is_idle(se));
> > +     else if (sched_feat(START_DEBIT))
> >               /*
> > -              * Halve their sleep time's effect, to allow
> > -              * for a gentler effect of sleepers:
> > +              * The 'current' period is already promised to the current tasks,
> > +              * however the extra weight of the new task will slow them down a
> > +              * little, place the new task so that it fits in the slot that
> > +              * stays open at the end.
> >                */
> > -             if (sched_feat(GENTLE_FAIR_SLEEPERS))
> > -                     thresh >>= 1;
> > -
> > -             vruntime -= thresh;
> > -     }
> > +             vruntime += sched_vslice(cfs_rq, se);
> >
> >       /* ensure we never gain time by being placed backwards. */
> >       se->vruntime = max_vruntime(se->vruntime, vruntime);
> > @@ -7187,6 +7171,18 @@ wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se)
> >               return -1;
> >
> >       gran = wakeup_gran(se);
> > +
> > +     /*
> > +      * 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

>
> >       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()

>
> > +     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.

>
> thanks,
>
>  - Joel
>
>
> > +
> > +     return thresh;
> > +}
> > +
> >  #ifdef CONFIG_SCHED_HRTICK
> >
> >  /*
> > --
> > 2.17.1
> >