1. The code below is duplicated in two for loops and need to be
consolidated
2. Fix the bug where a se's on_rq is true but its parent is not
```c
cfs_rq->h_nr_running++;
cfs_rq->idle_h_nr_running += idle_h_nr_running;
if (cfs_rq_is_idle(cfs_rq))
idle_h_nr_running = 1;
/* end evaluation on encountering a throttled cfs_rq */
if (cfs_rq_throttled(cfs_rq))
goto enqueue_throttle;
```
Signed-off-by: WangJinchao <[email protected]>
---
kernel/sched/fair.c | 31 ++++++++-----------------------
1 file changed, 8 insertions(+), 23 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d7a3c63a2171..e1373bfd4f2e 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6681,30 +6681,15 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
cpufreq_update_util(rq, SCHED_CPUFREQ_IOWAIT);
for_each_sched_entity(se) {
- if (se->on_rq)
- break;
cfs_rq = cfs_rq_of(se);
- enqueue_entity(cfs_rq, se, flags);
-
- cfs_rq->h_nr_running++;
- cfs_rq->idle_h_nr_running += idle_h_nr_running;
-
- if (cfs_rq_is_idle(cfs_rq))
- idle_h_nr_running = 1;
-
- /* end evaluation on encountering a throttled cfs_rq */
- if (cfs_rq_throttled(cfs_rq))
- goto enqueue_throttle;
-
- flags = ENQUEUE_WAKEUP;
- }
-
- for_each_sched_entity(se) {
- cfs_rq = cfs_rq_of(se);
-
- update_load_avg(cfs_rq, se, UPDATE_TG);
- se_update_runnable(se);
- update_cfs_group(se);
+ if (se->on_rq) {
+ update_load_avg(cfs_rq, se, UPDATE_TG);
+ se_update_runnable(se);
+ update_cfs_group(se);
+ } else {
+ enqueue_entity(cfs_rq, se, flags);
+ flags = ENQUEUE_WAKEUP;
+ }
cfs_rq->h_nr_running++;
cfs_rq->idle_h_nr_running += idle_h_nr_running;
--
2.40.0
On Sun, 10 Dec 2023 at 10:22, WangJinchao <[email protected]> wrote:
>
> 1. The code below is duplicated in two for loops and need to be
> consolidated
> 2. Fix the bug where a se's on_rq is true but its parent is not
Could you clarify which bug you want to fix ?
>
> ```c
> cfs_rq->h_nr_running++;
> cfs_rq->idle_h_nr_running += idle_h_nr_running;
>
> if (cfs_rq_is_idle(cfs_rq))
> idle_h_nr_running = 1;
>
> /* end evaluation on encountering a throttled cfs_rq */
> if (cfs_rq_throttled(cfs_rq))
> goto enqueue_throttle;
> ```
>
> Signed-off-by: WangJinchao <[email protected]>
> ---
> kernel/sched/fair.c | 31 ++++++++-----------------------
> 1 file changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d7a3c63a2171..e1373bfd4f2e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6681,30 +6681,15 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> cpufreq_update_util(rq, SCHED_CPUFREQ_IOWAIT);
>
> for_each_sched_entity(se) {
> - if (se->on_rq)
> - break;
> cfs_rq = cfs_rq_of(se);
> - enqueue_entity(cfs_rq, se, flags);
> -
> - cfs_rq->h_nr_running++;
> - cfs_rq->idle_h_nr_running += idle_h_nr_running;
> -
> - if (cfs_rq_is_idle(cfs_rq))
> - idle_h_nr_running = 1;
> -
> - /* end evaluation on encountering a throttled cfs_rq */
> - if (cfs_rq_throttled(cfs_rq))
> - goto enqueue_throttle;
> -
> - flags = ENQUEUE_WAKEUP;
> - }
> -
> - for_each_sched_entity(se) {
> - cfs_rq = cfs_rq_of(se);
> -
> - update_load_avg(cfs_rq, se, UPDATE_TG);
> - se_update_runnable(se);
> - update_cfs_group(se);
> + if (se->on_rq) {
> + update_load_avg(cfs_rq, se, UPDATE_TG);
> + se_update_runnable(se);
> + update_cfs_group(se);
> + } else {
> + enqueue_entity(cfs_rq, se, flags);
> + flags = ENQUEUE_WAKEUP;
> + }
>
> cfs_rq->h_nr_running++;
> cfs_rq->idle_h_nr_running += idle_h_nr_running;
> --
> 2.40.0
>
On Sun, 2023-12-10 at 17:21 +0800, WangJinchao wrote:
> 1. The code below is duplicated in two for loops and need to be
> consolidated
> 2. Fix the bug where a se's on_rq is true but its parent is not
In the current code, If a se is already on a rq, all the parents would have already been
on rq too. I don't think there's a case where se's on_rq and parent
is not for the current code before your patch. Otherwise the child
would never get scheduled. I don't think we have seen such bug being
reported.
>
> ```c
> cfs_rq->h_nr_running++;
> cfs_rq->idle_h_nr_running += idle_h_nr_running;
>
> if (cfs_rq_is_idle(cfs_rq))
> idle_h_nr_running = 1;
>
> /* end evaluation on encountering a throttled cfs_rq */
> if (cfs_rq_throttled(cfs_rq))
> goto enqueue_throttle;
> ```
>
> Signed-off-by: WangJinchao <[email protected]>
> ---
> kernel/sched/fair.c | 31 ++++++++-----------------------
> 1 file changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d7a3c63a2171..e1373bfd4f2e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6681,30 +6681,15 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> cpufreq_update_util(rq, SCHED_CPUFREQ_IOWAIT);
>
> for_each_sched_entity(se) {
> - if (se->on_rq)
> - break;
> cfs_rq = cfs_rq_of(se);
> - enqueue_entity(cfs_rq, se, flags);
> -
> - cfs_rq->h_nr_running++;
> - cfs_rq->idle_h_nr_running += idle_h_nr_running;
> -
> - if (cfs_rq_is_idle(cfs_rq))
> - idle_h_nr_running = 1;
> -
> - /* end evaluation on encountering a throttled cfs_rq */
> - if (cfs_rq_throttled(cfs_rq))
> - goto enqueue_throttle;
> -
> - flags = ENQUEUE_WAKEUP;
> - }
> -
> - for_each_sched_entity(se) {
> - cfs_rq = cfs_rq_of(se);
> -
> - update_load_avg(cfs_rq, se, UPDATE_TG);
> - se_update_runnable(se);
> - update_cfs_group(se);
> + if (se->on_rq) {
> + update_load_avg(cfs_rq, se, UPDATE_TG);
> + se_update_runnable(se);
> + update_cfs_group(se);
> + } else {
> + enqueue_entity(cfs_rq, se, flags);
> + flags = ENQUEUE_WAKEUP;
> + }
The code change looks like a reasonable simplification. Logic
is the same as the old code, assuming that once a se's on_rq flag
is true, all its parents are too.
Thanks.
Tim
>
> cfs_rq->h_nr_running++;
> cfs_rq->idle_h_nr_running += idle_h_nr_running;
On Mon, Dec 11, 2023 at 04:23:52PM +0100, Vincent Guittot wrote:
> On Sun, 10 Dec 2023 at 10:22, WangJinchao <[email protected]> wrote:
> >
> > 1. The code below is duplicated in two for loops and need to be
> > consolidated
> > 2. Fix the bug where a se's on_rq is true but its parent is not
>
> Could you clarify which bug you want to fix ?
Taking into account the additional information provided by Tim,
this is not a bug. Therefore, this patch is merely a logical
simplification.
I will send out a v2 and update the description in it.
On Wed, 13 Dec 2023 at 08:04, Wang Jinchao <[email protected]> wrote:
>
> On Mon, Dec 11, 2023 at 04:23:52PM +0100, Vincent Guittot wrote:
> > On Sun, 10 Dec 2023 at 10:22, WangJinchao <[email protected]> wrote:
> > >
> > > 1. The code below is duplicated in two for loops and need to be
> > > consolidated
> > > 2. Fix the bug where a se's on_rq is true but its parent is not
> >
> > Could you clarify which bug you want to fix ?
> Taking into account the additional information provided by Tim,
> this is not a bug. Therefore, this patch is merely a logical
> simplification.
If there is no bug why changing it ?
The duplication is done in order to have the same pattern in :
enqueue_task_fair
dequeue_task_fair
throttle_cfs_rq
unthrottle_cfs_rq
so there is no need to change it
>
> I will send out a v2 and update the description in it.
On Wed, Dec 13, 2023 at 09:23:46AM +0100, Vincent Guittot wrote:
> On Wed, 13 Dec 2023 at 08:04, Wang Jinchao <[email protected]> wrote:
> >
> > On Mon, Dec 11, 2023 at 04:23:52PM +0100, Vincent Guittot wrote:
> > > On Sun, 10 Dec 2023 at 10:22, WangJinchao <[email protected]> wrote:
> > > >
> > > > 1. The code below is duplicated in two for loops and need to be
> > > > consolidated
> > > > 2. Fix the bug where a se's on_rq is true but its parent is not
> > >
> > > Could you clarify which bug you want to fix ?
> > Taking into account the additional information provided by Tim,
> > this is not a bug. Therefore, this patch is merely a logical
> > simplification.
>
> If there is no bug why changing it ?
For two reasons:
1. (from Abel Wu)
It doesn't need to, but it can actually bring some benefit from
the point of view of text size, especially in warehouse-scale
computers where icache is extremely contended.
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-56 (-56)
Function old new delta
enqueue_task_fair 936 880 -56
Total: Before=64899, After=64843, chg -0.09%
2. For better code comprehension
I became curious when I reached this part, wondering why there is a lot of
repetition inside these two for-loops. Then I thought about 'do not repeat yourself,'
and I feel that merging them would lead to a clearer understanding. Of course,
it might be because I am just starting to read scheduler-related code and am not
yet familiar with the entire logic.
>
> The duplication is done in order to have the same pattern in :
> enqueue_task_fair
> dequeue_task_fair
> throttle_cfs_rq
> unthrottle_cfs_rq
Due to the two points mentioned above, do we need to adjust all four functions?
>
> so there is no need to change it
I plan to get familiar with the scheduler-related code first and then consider this.
Thanks
>
On 12/14/23 5:47 PM, Wang Jinchao Wrote:
> On Wed, Dec 13, 2023 at 09:23:46AM +0100, Vincent Guittot wrote:
>> On Wed, 13 Dec 2023 at 08:04, Wang Jinchao <[email protected]> wrote:
>>>
>>> On Mon, Dec 11, 2023 at 04:23:52PM +0100, Vincent Guittot wrote:
>>>> On Sun, 10 Dec 2023 at 10:22, WangJinchao <[email protected]> wrote:
>>>>>
>>>>> 1. The code below is duplicated in two for loops and need to be
>>>>> consolidated
>>>>> 2. Fix the bug where a se's on_rq is true but its parent is not
>>>>
>>>> Could you clarify which bug you want to fix ?
>>> Taking into account the additional information provided by Tim,
>>> this is not a bug. Therefore, this patch is merely a logical
>>> simplification.
>>
>> If there is no bug why changing it ?
> For two reasons:
> 1. (from Abel Wu)
> It doesn't need to, but it can actually bring some benefit from
> the point of view of text size, especially in warehouse-scale
> computers where icache is extremely contended.
>
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-56 (-56)
> Function old new delta
> enqueue_task_fair 936 880 -56
> Total: Before=64899, After=64843, chg -0.09%
But TBH this benefit is kind of weak to argue about, given that you
don't have any data supporting it.
>
> 2. For better code comprehension
> I became curious when I reached this part, wondering why there is a lot of
> repetition inside these two for-loops. Then I thought about 'do not repeat yourself,'
> and I feel that merging them would lead to a clearer understanding. Of course,
> it might be because I am just starting to read scheduler-related code and am not
> yet familiar with the entire logic.
>>
>> The duplication is done in order to have the same pattern in :
>> enqueue_task_fair
>> dequeue_task_fair
>> throttle_cfs_rq
>> unthrottle_cfs_rq
> Due to the two points mentioned above, do we need to adjust all four functions?
>>
>> so there is no need to change it
> I plan to get familiar with the scheduler-related code first and then consider this.
>
> Thanks
>>
On Thu, Dec 14, 2023 at 08:10:57PM +0800, Abel Wu wrote:
> On 12/14/23 5:47 PM, Wang Jinchao Wrote:
> > On Wed, Dec 13, 2023 at 09:23:46AM +0100, Vincent Guittot wrote:
> > > On Wed, 13 Dec 2023 at 08:04, Wang Jinchao <[email protected]> wrote:
> > > >
> > > > On Mon, Dec 11, 2023 at 04:23:52PM +0100, Vincent Guittot wrote:
> > > > > On Sun, 10 Dec 2023 at 10:22, WangJinchao <[email protected]> wrote:
> > > > > >
> > > > > > 1. The code below is duplicated in two for loops and need to be
> > > > > > consolidated
> > > > > > 2. Fix the bug where a se's on_rq is true but its parent is not
> > > > >
> > > > > Could you clarify which bug you want to fix ?
> > > > Taking into account the additional information provided by Tim,
> > > > this is not a bug. Therefore, this patch is merely a logical
> > > > simplification.
> > >
> > > If there is no bug why changing it ?
> > For two reasons:
> > 1. (from Abel Wu)
> > It doesn't need to, but it can actually bring some benefit from
> > the point of view of text size, especially in warehouse-scale
> > computers where icache is extremely contended.
> >
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-56 (-56)
> > Function old new delta
> > enqueue_task_fair 936 880 -56
> > Total: Before=64899, After=64843, chg -0.09%
>
> But TBH this benefit is kind of weak to argue about, given that you
> don't have any data supporting it.
Agree. My initinal target is clear comprehension.
And thanks for your numbers.
>
> >
> > 2. For better code comprehension
> > I became curious when I reached this part, wondering why there is a lot of
> > repetition inside these two for-loops. Then I thought about 'do not repeat yourself,'
> > and I feel that merging them would lead to a clearer understanding. Of course,
> > it might be because I am just starting to read scheduler-related code and am not
> > yet familiar with the entire logic.
> > >
> > > The duplication is done in order to have the same pattern in :
> > > enqueue_task_fair
> > > dequeue_task_fair
> > > throttle_cfs_rq
> > > unthrottle_cfs_rq
> > Due to the two points mentioned above, do we need to adjust all four functions?
> > >
> > > so there is no need to change it
> > I plan to get familiar with the scheduler-related code first and then consider this.
> >
> > Thanks
> > >