lkml.org 
[lkml]   [2022]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@linaro.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@joelfernandes.org>

- Joel

\
 
 \ /
  Last update: 2022-11-16 03:11    [W:0.078 / U:2.964 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site