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 <[email protected]>
---
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
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.
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
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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