[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <3A5DA66F-4330-4FC4-9229-998CF98F663E@joelfernandes.org>
Date: Sat, 3 Dec 2022 10:35:46 -0500
From: Joel Fernandes <joel@...lfernandes.org>
To: Chen Yu <yu.c.chen@...el.com>
Cc: Honglei Wang <wanghonglei@...ichuxing.com>,
Peter Zijlstra <peterz@...radead.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
Tim Chen <tim.c.chen@...el.com>,
Mel Gorman <mgorman@...hsingularity.net>,
Juri Lelli <juri.lelli@...hat.com>,
Rik van Riel <riel@...riel.com>,
Aaron Lu <aaron.lu@...el.com>,
Abel Wu <wuyun.abel@...edance.com>,
K Prateek Nayak <kprateek.nayak@....com>,
Yicong Yang <yangyicong@...ilicon.com>,
"Gautham R . Shenoy" <gautham.shenoy@....com>,
Ingo Molnar <mingo@...hat.com>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Steven Rostedt <rostedt@...dmis.org>,
Ben Segall <bsegall@...gle.com>,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Valentin Schneider <vschneid@...hat.com>,
Hillf Danton <hdanton@...a.com>,
Len Brown <len.brown@...el.com>,
Chen Yu <yu.chen.surf@...il.com>,
Tianchen Ding <dtcccc@...ux.alibaba.com>,
Josh Don <joshdon@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] sched/fair: Introduce short duration task check
> On Dec 3, 2022, at 2:50 AM, Chen Yu <yu.c.chen@...el.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@...el.com>
>>> Suggested-by: Vincent Guittot <vincent.guittot@...aro.org>
>>> Signed-off-by: Chen Yu <yu.c.chen@...el.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.
Thanks.
>
> thanks,
> Chenyu
Powered by blists - more mailing lists