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:   Mon, 5 Dec 2022 22:38:44 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Vincent Guittot <vincent.guittot@linaro.org>
CC:     Joel Fernandes <joel@joelfernandes.org>,
        Honglei Wang <wanghonglei@didichuxing.com>,
        Peter Zijlstra <peterz@infradead.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>,
        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: <Y44CdKrQoFKGH+1j@chenyu5-mobl1>
References: <Y4r/iuqyLG4MOK4d@chenyu5-mobl1>
 <3A5DA66F-4330-4FC4-9229-998CF98F663E@joelfernandes.org>
 <Y42uH5fglluWYOm7@chenyu5-mobl1>
 <CAKfTPtA61DDk-VbNDvAycp8Et5fTwaOS=Q3559rBF7QDHDpJfQ@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAKfTPtA61DDk-VbNDvAycp8Et5fTwaOS=Q3559rBF7QDHDpJfQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?S3hrekFwd3NGVmFpY2FLancvRzc3STJkSTNSamc2SjNqOWUrUTZYdk5RL213?=
 =?utf-8?B?VU1lTnlNNHBqMEgxaERZNk9aYkQ3dWt2cFBPMUtsSllhS1EzVzJWWWVJTGtG?=
 =?utf-8?B?QmRkM3ZNL3YxQXQyS0ZIdHpRWStKR29nQTVnRXZoSzhSdzUxQlNxNlg0ZkRX?=
 =?utf-8?B?VVhWcjVwMjRqMXQ4QkgvRXg5WjVDRmtSaWl4Y29FVWdNRjVIbDZuSTJyRmNk?=
 =?utf-8?B?bTEyenphbmZ5UElYaFhyNlJGUm9Rc1FpTG5PRm96b283dlZjd0liR04raGRk?=
 =?utf-8?B?cHlJMllUb29zMlhvZWhqdUZwSVlxTlJJSzYvdTh6MmxEYi9xVlp1MVB1VFVz?=
 =?utf-8?B?ZXRGcWtEWjhPelVxbCtYQ3VVaXFrM3hPUllpNUhNbXJuZ3VtM0ZRblNxeVhw?=
 =?utf-8?B?K1VwbzB5eTVLSkYxdG9abDVPK01GWHRFVDlIYjUxTlAvRGNxUWp0NVRQTi9Z?=
 =?utf-8?B?dVZFYkJmTC95b3hoYmp5RmI5L0pqMTVoUzFuQ2VYZkgwV3JBTU5oMy9CdEpS?=
 =?utf-8?B?cWZFY2ZQdzJUdnBMNm1IbUVNYkVzQjkvZnVZeWFuS3I1QXZRRkJkMG5ZUE85?=
 =?utf-8?B?NjZwK1FWMDFodFNDTGpUWEEyU0VQVktGR1dJTEpkYVJoVXA4bW1YMmZJRUxw?=
 =?utf-8?B?YzVyR0IyN3VrS1U0Uy9oNnJOdjlzTWphVDZnVytjeG9pbjhKZGpkV0VsdFFD?=
 =?utf-8?B?aldYa3hFRDFvaFA2L0xZekwwY2kxaTZpYTBJRmVINE5GYzYxNFJmRnZteEQ4?=
 =?utf-8?B?WHF3aWhlUitkeC9ROE1EbGJGa2htNUtoOGg2WGRkOFpKNnlDdDVOS1Z0SU9w?=
 =?utf-8?B?SFpzNFJkcFRlcm9JYjJjTHoycktkalVudmVtSDBwemhtNFhiYm0xMEVxQ09x?=
 =?utf-8?B?NjBmSXZTQWU1TncrVnl5eE1oeC9RVzBYN0NodWJaYy9helVYWlQza3FpS0ho?=
 =?utf-8?B?Ny83MnpPMjN0TlhKaTcxODlWNWRORWxWazhJQzh6K2lSSW5nczBwV09saDgr?=
 =?utf-8?B?Tjl0cXNVdytHeVE3WHduMWJocXJFMXQwTmhucVRJMGJzV1ZvdmtSZ0YrQ1dF?=
 =?utf-8?B?dlBpc1NpNU9yd1FYdGpJTHpOY1c1cVZxb3lDY2FuTXpXSm01MjZOdVF6U2xH?=
 =?utf-8?B?VGVDNFZ2eVpyN2NVbnhVRldKUlRNdGxYZE5wcGxZUWdBTUlwQkQ0azdZSDhH?=
 =?utf-8?B?cmxhSlQ3aUNhZndqZVZSY3dTNVNkWFVTWk9BbWVTaVJvQjBNSkV2T2hlcXlB?=
 =?utf-8?B?NE1vK0RBRXU0M0VYYXdPVnNJbUZrWHJHcEh0N0Y5elVLSmVsUTQzek1DUE5m?=
 =?utf-8?B?eGtGb3Q3NkIxNU1jTjJDaEFQWVVYejhHeGtweVpqWUxNMGxEQkdkaUZaU2k2?=
 =?utf-8?B?WC8zVHh0UGRFa1ZTNFpSY0owakdxY2N1YngzR1BhVUwzNk1JSHZybURZOFRZ?=
 =?utf-8?B?SEFJOXc1MnUyV21sMkE3ZXROUHpwMEdkQVRwTE9LOERYdXVrcytpZnRjTmZN?=
 =?utf-8?B?WEUrT09CM0cxOTA0a3lKd1RtbW4xeHE1SU5PTGRHZjVDT1NXWmJGa1l4bEVQ?=
 =?utf-8?B?MFUveFJlZEx3RFNhZkZuL3VydC9raGJzVUhPQ202U2F4VWtMSnVKL0Vxd3ZW?=
 =?utf-8?B?WVM2dU5OcGx5ZFpTMldURGRQa3d6cStnamN1OUJLQk5vRXRlWWJBa1NqOVda?=
 =?utf-8?B?OHVHK0dqUTByYithcXR4OEZpMXZmMlB2Ymh4d0ZCTmJaWmlvaGRhTE9qUzhD?=
 =?utf-8?B?c0IyR1FNMlZpMjAxaEttYzFaK0ZIOTFEUWhCc3lIaForYk5TUEM3RTcyTFpo?=
 =?utf-8?B?ZEdFM1pQcmxZZC9zUjFONlMvbWkxQnc0MWQ3T0N5bS9GQW9kR0V6bTByK2Zj?=
 =?utf-8?B?YWJ3NFZabndlVkd2UjBvM0xTbFlJWEp0UTJ4eFcxS3hLdjZsMUFwaTliNHI5?=
 =?utf-8?B?Y1RLc3c5TWhnc1dDa2dabVRMRmZ5aUhvTGJrQm4rbVJSZ1hMdFdYZkpJZnlL?=
 =?utf-8?B?SWVUd0Myc3lNNmNZVGtTYU1XR3psSERuckwxcmZPUXpML2cyVlBYWU5CS0sw?=
 =?utf-8?B?N2lRSSthT29JbDlzNUFTbUE3U0FwWUthdUxyMnNlSEQ4UFJUVy9oTWZacEFS?=
 =?utf-8?Q?xPeEcNKHLNpaDCf+3f0A08J3e?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e3d84b9-d65d-4602-d53b-08dad6ce7481
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Dec 2022 14:39:03.5587
 (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: cP0fX49ZAsw6M2Exmo3k0OtkfMnUL+d0tzeneuYK6R3fQjkyw3qiKckFtc94BE96fq7+JxaCBQKBlw9xIsy2LQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5611
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

On 2022-12-05 at 10:59:06 +0100, Vincent Guittot wrote:
> On Mon, 5 Dec 2022 at 09:39, Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > Hi Joel,
> > On 2022-12-03 at 10:35:46 -0500, Joel Fernandes wrote:
> > >
> > >
> > > > On Dec 3, 2022, at 2:50 AM, Chen Yu <yu.c.chen@intel.com> wrote:
> > > >
> > > > ﻿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.
> > >
> > > There are already ways to misconfigure sched sysctl to make bad/weird things happen.
> > >
> > > >> 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.
> > >
> > > It would be nice to not have to introduce a new knob for this. IMO, min_granularity is reasonable.
> > >
> > OK, got it, thanks for the suggestion.
> 
> Sorry for the late answer.
> We don't want to add more dedicated knobs. So using
> sysctl_sched_min_granularity as you are doing in this patch looks ok
>
I see, thanks Vincent.

thanks,
Chenyu 
> >
> > thanks,
> > Chenyu