Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 3 Dec 2022 15:49:30 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Honglei Wang <wanghonglei@didichuxing.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Tim Chen <tim.c.chen@intel.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Juri Lelli <juri.lelli@redhat.com>,
        "Rik van Riel" <riel@surriel.com>, Aaron Lu <aaron.lu@intel.com>,
        Abel Wu <wuyun.abel@bytedance.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        "Yicong Yang" <yangyicong@hisilicon.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Ingo Molnar <mingo@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        Hillf Danton <hdanton@sina.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>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] sched/fair: Introduce short duration task check
Message-ID: <Y4r/iuqyLG4MOK4d@chenyu5-mobl1>
References: <7621888af45a89c3963b39e062695602a3b9499f.1669862147.git.yu.c.chen@intel.com>
 <8d976d4a-ecb6-41af-85b0-65e6022a0d82@didichuxing.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <8d976d4a-ecb6-41af-85b0-65e6022a0d82@didichuxing.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?TYop7csf20Dzw0JD0mBOTbsKaiDssyREyUbXJqsZc6jWn3o6jGcp3Aw2kbsJ?=
 =?us-ascii?Q?kjwN7vNL5wgCCOdOOoqSbnFaGrfHqC2ylZ5nWably5HII5LknoV/A+Lr1bKh?=
 =?us-ascii?Q?78ThyOx41LZBkKfVZJgwgT+KcE6/lCTULGqsLgDJN9rRiwK9N0GKTHuQhZ6G?=
 =?us-ascii?Q?h8bSsM7YW34HXq57XZdw+gE1oVRsPyhfbVcTa3j9K9diERxQCaz9iAj2pLbZ?=
 =?us-ascii?Q?xi7ycjObrGpcmY3W1G442ajVjaKdkkQ2Xfpk57Dpub8zU3FuF1oWZ4HVh5RG?=
 =?us-ascii?Q?gS67LJ9PHVfLKHK5zHTZODDk4vOWRfjzNKnWUOTal8pV37i3z2o8PVEpiguO?=
 =?us-ascii?Q?bL7UaSZM6vlnOt7DVeHYCPGFjPPe1KMz4Dqv52ZiNq2xVNpo7yX4OrcUyMZs?=
 =?us-ascii?Q?Kme1vXtidsu3InU7MYoKC/EO3czu5aITE/vb1DOk3718J/XaWECxbFK5jlib?=
 =?us-ascii?Q?c7R7kAhtKr4UQy33VJwuHH2o5Ispdbwnl/Lfm3wQImtdSM4D8prIBUDfLvGF?=
 =?us-ascii?Q?ncGL3iYTh6mNBFQnf9guFneiDIkXHlkboWE46AZK/IbUy1M5vLsvaZxJ2o9W?=
 =?us-ascii?Q?pKSoqKEidA3ItPdfSsvl1onE1cdIglgmAa59RH+1t3s5Ir56daJ4NIbqxeP3?=
 =?us-ascii?Q?mxqCCRge4dBt314Z3w2wpPDXMaCgZWMdFOCsI4TjzaS8rJwHgRxlroLVtltJ?=
 =?us-ascii?Q?gG82Svo4W1VqdehxJGR7+FCooIcYodjc6AflbTJ1B9vedusCcpy7FUt6czCO?=
 =?us-ascii?Q?53NGYaDWOQacUEiQz6jXV57iEZJ/XszLGb/WdUA+sPZDNoHrYzKwIfjRr9pW?=
 =?us-ascii?Q?yrwnRP45EjoferaRpAP8xsrEORGbJUmLp2CqBd3XFWSNzLQhZ3/3vzYC6P2Q?=
 =?us-ascii?Q?1qG25sXbz7VBh7EBo6VgIkmQE44WfkRw1I2AfyCyqTgddcySUcRm2j6BiSJe?=
 =?us-ascii?Q?8eAywPI02hY/eko7NSuiFgdrQ8J6xh7RxFcTitZ7F6e84YfhGjyOIBuaAzQ2?=
 =?us-ascii?Q?DYgLWmqm0dO66dEwFDGxDYH4RHBn9CykdPoPTN8K8GqaRRLsc/R35qid6C1X?=
 =?us-ascii?Q?117NpfD1n3EegzsPWz67Y1muu1MxsL86XbWI0msL4jrlsglQzTLOJCYo97DF?=
 =?us-ascii?Q?5VoLTxsPwRUppUCSuOkohgin+1G71WdDyphVyTpeiPMj6OVcHZbwyLwemcYp?=
 =?us-ascii?Q?qZDkYOdRCzE7hHV9NRyDYx4KFlJr/GCX6N/PCM0Olnq+1co4mBMrmjvoPFmD?=
 =?us-ascii?Q?9Ih7bRmEEVCOr20Gh7w1l0mdsJFZAaWfb1xK/VKWEo8adbWWx03d6lX8juQN?=
 =?us-ascii?Q?VlytUiYe7eKzGEb+jHodnHA7XKuxwszmgjvv6qQxRp3hmU0AsTzjIRvkU5fo?=
 =?us-ascii?Q?8BPK5z4FD9vT2V0u8WCZqLENgwovqBkTv2gVYNKiPcK87kPNoVx6UF9vr4Yy?=
 =?us-ascii?Q?WXJwD+Tvmv6WOlkS4qC9CM4mxwyD0m9Fj4yif0VFP3zMzw1/nzzO/u2FCXv1?=
 =?us-ascii?Q?rV/tbJOOSoVkCk+ClSj6WDEcxdKtCMdS6LVlNnCF2MBxsqNa51Az2c7CII+U?=
 =?us-ascii?Q?if6fEbbVlZOoQTsm5FJY7vmlwEjZq3aaivms0tfZ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0b3c7487-87ac-4333-5922-08dad502f6c8
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Dec 2022 07:49:53.5353
 (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: V9lg6plM5jORA+vqTVxGPB+DmJRH/HBkNiidjUJcmGhgIzQXVx3tW9Jj5i8uqgChjN4ANJw+zOi5UxSkAUzrpQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5315
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Honglei,
On 2022-12-02 at 15:44:18 +0800, Honglei Wang wrote:
> 
> 
> On 2022/12/1 16:44, Chen Yu wrote:
> > Introduce short-duration task checks, as there is requirement
> > to leverage this attribute for better task placement.
> > 
> > There are several choices of metrics that could be used to
> > indicate if a task is a short-duration task.
> > 
> > At first thought the (p->se.sum_exec_runtime / p->nvcsw)
> > could be used to measure the task duration. However, the
> > history long past was factored too heavily in such a formula.
> > Ideally, the old activity should decay and not affect
> > the current status too much.
> > 
> > Although something based on PELT could be used, se.util_avg might
> > not be appropriate to describe the task duration:
> > 1. Task p1 and task p2 are doing frequent ping-pong scheduling on
> >     one CPU, both p1 and p2 have a short duration, but the util_avg
> >     can be up to 50%.
> > 2. Suppose a task lasting less than 4ms is regarded as a short task.
> >     If task p3 runs for 6ms and sleeps for 32ms, p3 should not be a
> >     short-duration task. However, PELT would decay p3's accumulated
> >     running time from 6ms to 3ms, because 32ms is the half-life in PELT.
> >     As a result, p3 would be incorrectly treated as a short task.
> > 
> > It was found that there was once a similar feature to track the
> > duration of a task, which is in Commit ad4b78bbcbab ("sched: Add
> > new wakeup preemption mode: WAKEUP_RUNNING"). Unfortunately, it
> > was reverted because it was an experiment. So pick the patch up
> > again, by recording the average duration when a task voluntarily
> > switches out. Introduce SIS_SHORT to control this strategy.
> > 
> > The threshold of short duration reuses sysctl_sched_min_granularity,
> > so it can be tuned by the user. Ideally there should be a dedicated
> > parameter for the threshold, but that might introduce complexity.
> > 
> > Suggested-by: Tim Chen <tim.c.chen@intel.com>
> > Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >   include/linux/sched.h   |  4 ++++
> >   kernel/sched/core.c     |  2 ++
> >   kernel/sched/fair.c     | 17 +++++++++++++++++
> >   kernel/sched/features.h |  1 +
> >   4 files changed, 24 insertions(+)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index ffb6eb55cd13..64b7acb77a11 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -558,6 +558,10 @@ struct sched_entity {
> >   	u64				nr_migrations;
> > +	u64				prev_sum_exec_runtime_vol;
> > +	/* average duration of a task */
> > +	u64				dur_avg;
> > +
> >   #ifdef CONFIG_FAIR_GROUP_SCHED
> >   	int				depth;
> >   	struct sched_entity		*parent;
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index daff72f00385..c5202f1be3f7 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4348,6 +4348,8 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
> >   	p->se.prev_sum_exec_runtime	= 0;
> >   	p->se.nr_migrations		= 0;
> >   	p->se.vruntime			= 0;
> > +	p->se.dur_avg			= 0;
> > +	p->se.prev_sum_exec_runtime_vol	= 0;
> >   	INIT_LIST_HEAD(&p->se.group_node);
> >   #ifdef CONFIG_FAIR_GROUP_SCHED
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index e4a0b8bd941c..a4b314b664f8 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6200,6 +6200,16 @@ 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.
> > + */
> > +static inline int is_short_task(struct task_struct *p)
> > +{
> > +	return sched_feat(SIS_SHORT) &&
> > +		(p->se.dur_avg <= sysctl_sched_min_granularity);
> > +}
> > +
> 
> Hi Yu,
> 
> I still have a bit concern about the sysctl_sched_min_granularity stuff..
> This grab can be set to different value which will impact the action of this
> patch and make things not totally under control.
> 
> Not sure if we can add a new grab for this.. The test result shows good
> improvement for short task, and with this grab, admins will be able to
> custom the system base on their own 'short task' view.
>
It would be ideal to have a dedicated parameter to tweak this. For example,
something under /sys/kernel/debug/sched/, and initilized to sysctl_sched_min_granularity
by default. 
Hi Peter, Vincent,
may I have your opinion if this is applicable?

thanks,
Chenyu