[PATCH] sched,fair: Remove > u32 weight handling for delta

[PATCH] sched,fair: Remove > u32 weight handling for delta

[Afzal Mohammed]

scaled down weight 'fact' would not be > u32 rather than unlikely as the
values being passed for delta is either NICE_O_LOAD or the weight of the
'se' which would be a value that can be accomodated in a u32. Remove the
initial > u32 handling on 'fact'.

9dbdb15553239 ("sched/fair: Rework sched_fair time accounting") in
addition to fixing the original issue of time moving backwards elsewhere
in the code, handled delta > u32 case (due to NO_HZ_FULL) which brought
in as it's part the change changed here. The hunk being removed here
would not make a difference to it as this is on scaled weight > u32.
And pre-"9dbdb15553239" doesn't seem to have logical equivalent of hunk
removed here either.

Signed-off-by: Afzal Mohammed <afzal.mohd.ma@gmail.com>
---
    
System here (that mainly does kernel build) has been running with this
change for last couple of weeks without any issue observed.
    
 kernel/sched/fair.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 3d57cc0ca0a6..0cc58a7bdd16 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -218,13 +218,6 @@ static u64 __calc_delta(u64 delta_exec, unsigned long weight, struct load_weight
 
 	__update_inv_weight(lw);
 
-	if (unlikely(fact >> 32)) {
-		while (fact >> 32) {
-			fact >>= 1;
-			shift--;
-		}
-	}
-
 	/* hint to use a 32x32->64 mul */
 	fact = (u64)(u32)fact * lw->inv_weight;
 
-- 
2.1.4

Re: [PATCH] sched,fair: Remove > u32 weight handling for delta

[Peter Zijlstra]

On Mon, Jul 06, 2015 at 08:14:37AM +0530, Afzal Mohammed wrote:
> scaled down weight 'fact' would not be > u32 rather than unlikely as the
> values being passed for delta is either NICE_O_LOAD or the weight of the
> 'se' which would be a value that can be accomodated in a u32.

This needs a bit more on why se->load.weight must fit u32 (its true, but
not evident from this text).

> Remove the initial > u32 handling on 'fact'.
> 
> 9dbdb15553239 ("sched/fair: Rework sched_fair time accounting") in
> addition to fixing the original issue of time moving backwards elsewhere
> in the code, handled delta > u32 case (due to NO_HZ_FULL) which brought
> in as it's part the change changed here.

Because the unsigned long weight is a u64 on 64bit..

Now as long as we never call __calc_delta() on a rq weight -- which is a
sum of weights and can indeed be larger than u32, we can indeed remove
this.

And I think we already assume such, see this story on why shift will
remain positive.

> The hunk being removed here
> would not make a difference to it as this is on scaled weight > u32.
> And pre-"9dbdb15553239" doesn't seem to have logical equivalent of hunk
> removed here either.

-ENOPARSE.

Re: [PATCH] sched,fair: Remove > u32 weight handling for delta

[Afzal Mohammed]

Hi,

On Mon, Jul 06, 2015 at 01:44:30PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 06, 2015 at 08:14:37AM +0530, Afzal Mohammed wrote:

> > scaled down weight 'fact' would not be > u32 rather than unlikely as the
> > values being passed for delta is either NICE_O_LOAD or the weight of the
> > 'se' which would be a value that can be accomodated in a u32.
> 
> This needs a bit more on why se->load.weight must fit u32 (its true, but
> not evident from this text).

Okay, I will add an equivalent of the below to the log,

"se->load.weight can have either the values in prio_to_weight[] for
cases where 'se' is a task or capped to MAX_SHARES (1 << 18) when it
is a group. And these values can be accomodated in a u32.",

and send the patch, unless a negative opinion on the above.

> Now as long as we never call __calc_delta() on a rq weight -- which is a
> sum of weights and can indeed be larger than u32, we can indeed remove
> this.

My understanding is that we do not call __calc_delta() on rq weight.

> And I think we already assume such, see this story on why shift will
> remain positive.

ok

> > The hunk being removed here
> > would not make a difference to it as this is on scaled weight > u32.
> > And pre-"9dbdb15553239" doesn't seem to have logical equivalent of hunk
> > removed here either.
> 
> -ENOPARSE.

Reading 9dbdb15553239 ("sched/fair: Rework sched_fair time
accounting") again, realized that I am wrong on this, that was
referring to the below statement removed in that commit,

        if (likely(weight > (1UL << SCHED_LOAD_RESOLUTION)))
                tmp = (u64)delta_exec * scale_load_down(weight);

earlier came to a reasoning that as scale_load_down(weight) was not
separately typecasted, value above u32 would be discarded, that non
parsable statement meant that weight > u32 was not considered. Since
cast has precedence over multiply, that statement of mine was wrong.

Regards
Afzal

Re: [PATCH] sched,fair: Remove > u32 weight handling for delta

[Afzal Mohammed]

Hi,

On Mon, Jul 06, 2015 at 07:04:07PM +0530, Afzal Mohammed wrote:
> On Mon, Jul 06, 2015 at 01:44:30PM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 06, 2015 at 08:14:37AM +0530, Afzal Mohammed wrote:

> > > The hunk being removed here
> > > would not make a difference to it as this is on scaled weight > u32.
> > > And pre-"9dbdb15553239" doesn't seem to have logical equivalent of hunk
> > > removed here either.
> > 
> > -ENOPARSE.
> 
> Reading 9dbdb15553239 ("sched/fair: Rework sched_fair time
> accounting") again, realized that I am wrong on this, that was
> referring to the below statement removed in that commit,
> 
>         if (likely(weight > (1UL << SCHED_LOAD_RESOLUTION)))
>                 tmp = (u64)delta_exec * scale_load_down(weight);
> 
> earlier came to a reasoning that as scale_load_down(weight) was not
> separately typecasted, value above u32 would be discarded, that non
> parsable statement meant that weight > u32 was not considered. Since
> cast has precedence over multiply, that statement of mine was wrong.

I take that back, consider as though I have not mentioned anything
above :)

Regards
Afzal

[PATCH v2] sched,fair: Remove > u32 weight handling for delta

[Afzal Mohammed]

scaled down weight 'fact' would not be > u32 rather than unlikely as the
weight being passed in __calc_delta() is either 'se->load.weight' or
NICE_O_LOAD. 'se->load.weight' would be one of the values of
prio_to_weight[] in the case of task, while in the case of task group it
would be capped to MAX_SHARES. All those possible values can fit in u32.
Hence remove the initial > u32 handling on 'fact'.

Signed-off-by: Afzal Mohammed <afzal.mohd.ma@gmail.com>
---

v2: expand change log on why weight can fit in u32, remove incorrect
    comments.

 kernel/sched/fair.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 65c8f3ebdc3c..22cf3958bbac 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -218,13 +218,6 @@ static u64 __calc_delta(u64 delta_exec, unsigned long weight, struct load_weight
 
 	__update_inv_weight(lw);
 
-	if (unlikely(fact >> 32)) {
-		while (fact >> 32) {
-			fact >>= 1;
-			shift--;
-		}
-	}
-
 	/* hint to use a 32x32->64 mul */
 	fact = (u64)(u32)fact * lw->inv_weight;
 
-- 
2.1.4

Re: [PATCH v2] sched,fair: Remove > u32 weight handling for delta

[Afzal Mohammed]

Hi Peter,

On Mon, Jul 06, 2015 at 11:49:15PM +0530, Afzal Mohammed wrote:
> scaled down weight 'fact' would not be > u32 rather than unlikely as the
> weight being passed in __calc_delta() is either 'se->load.weight' or
> NICE_O_LOAD. 'se->load.weight' would be one of the values of
> prio_to_weight[] in the case of task, while in the case of task group it
> would be capped to MAX_SHARES. All those possible values can fit in u32.
> Hence remove the initial > u32 handling on 'fact'.
> 
> Signed-off-by: Afzal Mohammed <afzal.mohd.ma@gmail.com>
> ---
> 
> v2: expand change log on why weight can fit in u32, remove incorrect
>     comments.
> 
>  kernel/sched/fair.c | 7 -------
>  1 file changed, 7 deletions(-)

bool __weak patch_adds_some_value(void)
{
        return (~3 asm instruction less for calc_delta) && (less LOC);
}

if (patch_adds_some_value() && revised_message_okay())
        please help $subject reach mainline;

Regards
Afzal