Messages in this thread Patch in this message | | | Date | Thu, 16 Feb 2023 20:55:29 +0800 | Subject | Re: [PATCH v5 2/2] sched/fair: Introduce SIS_SHORT to wake up short task on current CPU | From | Abel Wu <> |
| |
Hi Chen,
I've tested this patchset (with modification) on our Redis proxy servers, and the results seems promising.
On 2/3/23 1:18 PM, Chen Yu wrote: > ... > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index aa16611c7263..d50097e5fcc1 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -6489,6 +6489,20 @@ static int wake_wide(struct task_struct *p) > return 1; > } > > +/* > + * If a task switches in and then voluntarily relinquishes the > + * CPU quickly, it is regarded as a short duration task. > + * > + * SIS_SHORT tries to wake up the short wakee on current CPU. This > + * aims to avoid race condition among CPUs due to frequent context > + * switch. > + */ > +static inline int is_short_task(struct task_struct *p) > +{ > + return sched_feat(SIS_SHORT) && p->se.dur_avg && > + ((p->se.dur_avg * 8) < sysctl_sched_min_granularity); > +}
I changed the factor to fit into the shape of tasks in question.
static inline int is_short_task(struct task_struct *p) { u64 dur = sysctl_sched_min_granularity / 8;
if (!sched_feat(SIS_SHORT) || !p->se.dur_avg) return false;
/* * Bare tracepoint to allow dynamically changing * the threshold. */ trace_sched_short_task_tp(p, &dur);
return p->se.dur_avg < dur; }
I'm not sure it is the right way to provide such flexibility, but definition of 'short' can be workload specific.
> + > /* > * The purpose of wake_affine() is to quickly determine on which CPU we can run > * soonest. For the purpose of speed we only consider the waking and previous > @@ -6525,6 +6539,11 @@ wake_affine_idle(int this_cpu, int prev_cpu, int sync) > if (available_idle_cpu(prev_cpu)) > return prev_cpu; > > + /* The only running task is a short duration one. */ > + if (cpu_rq(this_cpu)->nr_running == 1 && > + is_short_task(rcu_dereference(cpu_curr(this_cpu)))) > + return this_cpu;
Since proxy server handles simple data delivery, the tasks are generally short running ones and hate task stacking which may introduce scheduling latency (even there are only 2 short tasks competing each other). So this part brings slight regression on the proxy case. But I still think this is good for most cases.
Speaking of task stacking, I found wake_affine_weight() can be much more dangerous. It chooses the less loaded one between the prev & this cpu as a candidate, so 'small' tasks can be easily stacked on this cpu when wake up several tasks at one time if this cpu is unloaded. This really hurts if the 'small' tasks are latency-sensitive, although wake_affine_weight() does the right thing from the point of view of 'load'.
The following change greatly reduced the p99lat of Redis service from 150ms to 0.9ms, at exactly the same throughput (QPS).
@@ -5763,6 +5787,9 @@ wake_affine_weight(struct sched_domain *sd, struct task_struct *p, s64 this_eff_load, prev_eff_load; unsigned long task_load;
+ if (is_short_task(p)) + return nr_cpumask_bits; + this_eff_load = cpu_load(cpu_rq(this_cpu));
if (sync) {
I know that 'short' tasks are not necessarily 'small' tasks, e.g. sleeping duration is small or have large weights, but this works really well for this case. This is partly because delivering data is memory bandwidth intensive hence prefer cache hot cpus. And I think this is also applicable to the general purposes: do NOT let the short running tasks suffering from cache misses caused by migration.
Best regards, Abel
| |