Messages in this thread | | | From | Vincent Guittot <> | Date | Mon, 14 Nov 2022 12:05:18 +0100 | Subject | Re: [PATCH v8 1/9] sched/fair: fix unfairness at wakeup |
| |
On Mon, 14 Nov 2022 at 04:06, Joel Fernandes <joel@joelfernandes.org> 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@chromium.org>
yes
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > 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 > >
| |