Date:   Mon, 20 Feb 2023 12:58:24 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Honglei Wang <wanghonglei@didichuxing.com>
CC:     Abel Wu <wuyun.abel@bytedance.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        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>,
        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>
Subject: Re: [PATCH v5 2/2] sched/fair: Introduce SIS_SHORT to wake up short
 task on current CPU
Message-ID: <Y/L98AVIqkBPVkWG@chenyu5-mobl1>
References: <36ba3b68-5b73-9db0-2247-061627b0d95a@bytedance.com>
 <f88838ff-2024-ca32-069e-f7a4c0465961@didichuxing.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f88838ff-2024-ca32-069e-f7a4c0465961@didichuxing.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?osWNXR6klyrfGGf+hlo8zqn1HlbQ8NvUu+D4apE0vqW5ktH9+y0qR8k9Pf?=
 =?iso-8859-1?Q?uH3sGjI/KRlxxCfWpbFBsIRRALEeKjZtFkhYXdz15ARsycR0TpXTjrAMdd?=
 =?iso-8859-1?Q?GATgeuLhC735wDqPk8CBf0juvbVJzBrQx0keCplv1n1atylpdbXXY59D7g?=
 =?iso-8859-1?Q?4SZ/BMaI2gbonstY6U333EqgZr74oIEPKjfa2lGPG2JHIrxnDd5QXqGL4/?=
 =?iso-8859-1?Q?mUEFWc9NWRpYSBUna+7Flsl/TsZ/Xx7lo8OlmT4XcNdOYD4WtQ2K/eAiIV?=
 =?iso-8859-1?Q?K3jgsgn1lPMItAFPZyhj/yrbI12AF/fWc/BLLXSJEMVTXKBHIooLQt7YVT?=
 =?iso-8859-1?Q?NxN7TW7LXxbFRizMklACkGhDqkEjHBnBWkcvMX+BoscFKViAEC2N5hzVaw?=
 =?iso-8859-1?Q?bG2FfQRZk0TxBILyIg6LH0f/BqT1Q0yKUONckLX9SqgLj0MarJb17uiYNT?=
 =?iso-8859-1?Q?QRbT/vL6Wk89tyG5lnTSX0ZONTcnvQ9idSdHlXhaPFlA4/FnpYcmw+XQVb?=
 =?iso-8859-1?Q?bUXxSnKnsjJwlA+xHzKbvPN6Q+Z7OS/Wrl38dOVPnd6PphKfmZ6FRPm+nn?=
 =?iso-8859-1?Q?ClioOpL9R05LUWvpRp0bKNVtOMFFFfe5ZI0D1bgbTok8TDIdBnGrxupuq/?=
 =?iso-8859-1?Q?d+s7UWiLsOs7pFYBy9LAf3FEV5RQ9l+TQlrnJb8YxFCLQDo6UpFzgTYhO9?=
 =?iso-8859-1?Q?mVSrrZKUX+42Jjq5wCODLNRkqYsIT3dImZVbC2aNWK0j1h/TSAFDeunthy?=
 =?iso-8859-1?Q?ajiqZyIO/C9pEufBCmHJZIGMcuf8ELAyTVEq53nEych3OHVRwy90VsD5YS?=
 =?iso-8859-1?Q?naB980KsVLectZwrhz6iM9fga7ZzJeHkV46zDGtS+wSbh9fR26Q39BGify?=
 =?iso-8859-1?Q?K48J/FwLWWtv9TGGYvtOACvwbw0QND0IKzXslhi6Eas68n6SY+DknIu346?=
 =?iso-8859-1?Q?oXbWjHQNv0orxnEyW7Pn4p3EfJ25nTbW4FiRNbgGi5unIIjG0iW/x+HCx6?=
 =?iso-8859-1?Q?h4mfnFVaD6el8V3vfZUWHkdy677F17nbZIeagl/G0a22fBDIE2uesaiFcG?=
 =?iso-8859-1?Q?GQ9AeTAXLKlMOvOKqLltXroUnWwbh2TMVKKmmNGIA5SsHCsXyfn3A9fr54?=
 =?iso-8859-1?Q?v5Oxy+VLyPOI35AlitbgbqfEmN7aup+WxCYrE+yknwk+Nd0sB+rgbo3bt7?=
 =?iso-8859-1?Q?lVRXO6JNvfzcf+O/sdz3FWR5gEz5mfs+0EZoTRoTTUo0WDws/PVM3MoF+d?=
 =?iso-8859-1?Q?oCHSwcIpmrXYSjRBuiflbIi9vEcSFvWqnEMRaUTRXhRDjuFa1WXGtwlsjG?=
 =?iso-8859-1?Q?Pd1sYcf7BfSaDgweApUrAL94Gg2u/J30eNJo/dJZDNB0pBP4XqCI1A04Fq?=
 =?iso-8859-1?Q?dxUFHaQzcaxpYUjN1AltbtvArQ8xVCxaAlgQlhisTb+8DuTAAyoGg9hinO?=
 =?iso-8859-1?Q?Ucj5vEIy0c5dnvUNeXMRomnqI8VA9X51qK+VNEIxCgsPl/9PaLRQ/NAyBI?=
 =?iso-8859-1?Q?qcIPwyXHWziPxhCRLrv3u/TGUPJZtGM029Br8dPLLq33zXA6mpXrIhfWe6?=
 =?iso-8859-1?Q?TK0WiyGEMbfKdqY3xrSTIdIukJVZ2kgd6YW2Pw6I6aQ4rQwsebFAJHtHvy?=
 =?iso-8859-1?Q?SUN7g3R6CqbSnOuLQJntb3Wg95FsinQigd?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b300d59a-0c43-41cb-ffe9-08db12ff2347
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2023 04:58:42.2985
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 38d8Ti64ljBps7f7mp3sQNAQzeCqgtapg2nUKDFCmgzvq3jHNLuN+4DpX3r1PjrZkneIx1mpuJaCxG4Ck/o61g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5512
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2023-02-17 at 16:35:24 +0800, Honglei Wang wrote:
> 
> 
> On 2023/2/16 20:55, Abel Wu wrote:
> > 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.
> > 
> 
> Redis is a bit special. It runs quick and really sensitive on schedule
> latency. The purpose of this 'short task' feature from Yu is to mitigate the
> migration and tend to place the waking task on local cpu, this is somehow on
> the opposite side of workload such as Redis. The changes you did remind me
> of the latency-prio stuff. Maybe we can do something base on both the 'short
> task' and 'latency-prio' to make your changes more general. thoughts?
>
Looks reasonable, I suppose you were refering to 'latency nice' proposed by
Vincent. For now I'd like to keep this patch simple enough, later we can
extend it.

thanks,
Chenyu 
> Thanks,
> Honglei
> 
> > Best regards,
> >  ????Abel