Messages in this thread | | | From | Joel Fernandes <> | Date | Wed, 16 Nov 2022 02:10:11 +0000 | 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@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
| |