Message-ID: <36ba3b68-5b73-9db0-2247-061627b0d95a@bytedance.com>
Date:   Thu, 16 Feb 2023 20:55:29 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.7.2
Subject: Re: [PATCH v5 2/2] sched/fair: Introduce SIS_SHORT to wake up short
 task on current CPU
Content-Language: en-US
To:     Chen Yu <yu.c.chen@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>
Cc:     Mel Gorman <mgorman@techsingularity.net>,
        Tim Chen <tim.c.chen@intel.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Honglei Wang <wanghonglei@didichuxing.com>,
        Len Brown <len.brown@intel.com>,
        Chen Yu <yu.chen.surf@gmail.com>,
        Tianchen Ding <dtcccc@linux.alibaba.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Josh Don <joshdon@google.com>, Hillf Danton <hdanton@sina.com>,
        linux-kernel@vger.kernel.org,
        kernel test robot <yujie.liu@intel.com>
References: <cover.1675361144.git.yu.c.chen@intel.com>
 <1b8af8d99da99a20449288ab4fbba64dc05057ce.1675361144.git.yu.c.chen@intel.com>
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <1b8af8d99da99a20449288ab4fbba64dc05057ce.1675361144.git.yu.c.chen@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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