sched/pelt: simplify load_sum assignment code in attach_entity_load_avg()

[PATCH] sched/pelt: simplify load_sum assignment code in attach_entity_load_avg()

Posted by Zhaoyu Liu 3 years, 10 months ago

In commit 40f5aa4c5eae ("sched/pelt: Fix attach_entity_load_avg() corner case"),
these code was committed:
	if (se_weight(se) < se->avg.load_sum)
		se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
	else
		se->avg.load_sum = 1;

they could be replace with:
	se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se)) ?: 1;

to make the code cleaner.

Signed-off-by: Zhaoyu Liu <zackary.liu.pro@gmail.com>
---
 kernel/sched/fair.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 78795a997d9c..ed32f66bbd3d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3881,10 +3881,7 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
 	se->avg.runnable_sum = se->avg.runnable_avg * divider;
 
 	se->avg.load_sum = se->avg.load_avg * divider;
-	if (se_weight(se) < se->avg.load_sum)
-		se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
-	else
-		se->avg.load_sum = 1;
+	se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se)) ?: 1;
 
 	enqueue_load_avg(cfs_rq, se);
 	cfs_rq->avg.util_avg += se->avg.util_avg;
-- 
2.17.1

Re: [PATCH] sched/pelt: simplify load_sum assignment code in attach_entity_load_avg()

Posted by Vincent Guittot 3 years, 10 months ago

On Tue, 21 Jun 2022 at 17:45, Zhaoyu Liu <zackary.liu.pro@gmail.com> wrote:
>
> In commit 40f5aa4c5eae ("sched/pelt: Fix attach_entity_load_avg() corner case"),
> these code was committed:
>         if (se_weight(se) < se->avg.load_sum)
>                 se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
>         else
>                 se->avg.load_sum = 1;
>
> they could be replace with:
>         se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se)) ?: 1;
>
> to make the code cleaner.

This quite subjective as I consider current version cleaner than your proposal

>
> Signed-off-by: Zhaoyu Liu <zackary.liu.pro@gmail.com>
> ---
>  kernel/sched/fair.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 78795a997d9c..ed32f66bbd3d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3881,10 +3881,7 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
>         se->avg.runnable_sum = se->avg.runnable_avg * divider;
>
>         se->avg.load_sum = se->avg.load_avg * divider;
> -       if (se_weight(se) < se->avg.load_sum)
> -               se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
> -       else
> -               se->avg.load_sum = 1;
> +       se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se)) ?: 1;
>
>         enqueue_load_avg(cfs_rq, se);
>         cfs_rq->avg.util_avg += se->avg.util_avg;
> --
> 2.17.1
>

Re: [PATCH] sched/pelt: simplify load_sum assignment code in attach_entity_load_avg()

Posted by Zackary Liu 3 years, 10 months ago

On Jun 22 2022, at 11:20 pm, Vincent Guittot
<vincent.guittot@linaro.org> wrote:

> On Tue, 21 Jun 2022 at 17:45, Zhaoyu Liu <zackary.liu.pro@gmail.com> wrote:
>> 
>> In commit 40f5aa4c5eae ("sched/pelt: Fix attach_entity_load_avg()
>> corner case"),
>> these code was committed:
>>         if (se_weight(se) < se->avg.load_sum)
>>                 se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
>>         else
>>                 se->avg.load_sum = 1;
>> 
>> they could be replace with:
>>         se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se))
>> ?: 1;
>> 
>> to make the code cleaner.
> 
> This quite subjective as I consider current version cleaner than your proposal

Thanks for your reply, vincent

Perhaps, this code is more concise, and this form can exist in many
places in the kernel, and can be searched with 'grep "?: 1;" -nR kernel'

--
zackary

Re: [PATCH] sched/pelt: simplify load_sum assignment code in attach_entity_load_avg()

Posted by Zackary Liu 3 years, 9 months ago


On Jun 22 2022, at 11:49 pm, Zackary Liu <zackary.liu.pro@gmail.com> wrote:

> On Jun 22 2022, at 11:20 pm, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> 
>> On Tue, 21 Jun 2022 at 17:45, Zhaoyu Liu <zackary.liu.pro@gmail.com> wrote:
>>> 
>>> In commit 40f5aa4c5eae ("sched/pelt: Fix attach_entity_load_avg()
>>> corner case"),
>>> these code was committed:
>>>         if (se_weight(se) < se->avg.load_sum)
>>>                 se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
>>>         else
>>>                 se->avg.load_sum = 1;
>>> 
>>> they could be replace with:
>>>         se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se))
>>> ?: 1;
>>> 
>>> to make the code cleaner.
>> 
>> This quite subjective as I consider current version cleaner than your proposal
> 
> Thanks for your reply, vincent
> 
> Perhaps, this code is more concise, and this form can exist in many
> places in the kernel, and can be searched with 'grep "?: 1;" -nR kernel'
> 
> --
> zackary

I have sent a patch couple days ago but still i don't get the reply,
I am looking forward to your reply,
thank you

--
zackary

Re: [PATCH] sched/pelt: simplify load_sum assignment code in attach_entity_load_avg()

Posted by Vincent Guittot 3 years, 9 months ago

On Fri, 15 Jul 2022 at 16:33, Zackary Liu <zackary.liu.pro@gmail.com> wrote:
>
>
>
> On Jun 22 2022, at 11:49 pm, Zackary Liu <zackary.liu.pro@gmail.com> wrote:
>
> > On Jun 22 2022, at 11:20 pm, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> >
> >> On Tue, 21 Jun 2022 at 17:45, Zhaoyu Liu <zackary.liu.pro@gmail.com> wrote:
> >>>
> >>> In commit 40f5aa4c5eae ("sched/pelt: Fix attach_entity_load_avg()
> >>> corner case"),
> >>> these code was committed:
> >>>         if (se_weight(se) < se->avg.load_sum)
> >>>                 se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se));
> >>>         else
> >>>                 se->avg.load_sum = 1;
> >>>
> >>> they could be replace with:
> >>>         se->avg.load_sum = div_u64(se->avg.load_sum, se_weight(se))
> >>> ?: 1;
> >>>
> >>> to make the code cleaner.
> >>
> >> This quite subjective as I consider current version cleaner than your proposal
> >
> > Thanks for your reply, vincent
> >
> > Perhaps, this code is more concise, and this form can exist in many
> > places in the kernel, and can be searched with 'grep "?: 1;" -nR kernel'

As said, the current code is cleaner and easier to read and I don't
see any benefit from this patch.
So it's a NAK


> >
> > --
> > zackary
>
> I have sent a patch couple days ago but still i don't get the reply,
> I am looking forward to your reply,
> thank you
>
> --
> zackary