sched/trivial: Remove cfs bandwidth period check in tg_set_cfs_period()

PATCH In review Kamalesh Babulal 3 participants 5 messages Dec 10, 2011 – Dec 21, 2011 Scheduler

lore

Share & tools

lore

Kamalesh Babulal

author

2011-12-10 13:59 UTC #0 lore raw

PATCH 1/1 sched/trivial: Remove cfs bandwidth period check in tg_set_cfs_period()

sched: Remove cfs bandwidth period check in tg_set_cfs_period()

Remove cfs bandwidth period check from tg_set_cfs_period. Invalid bandwidth period's lower/upper limits are denoted by min_cfs_quota_period/max_cfs_quota_period repsectively, and are checked against valid period in tg_set_cfs_bandwidth().

Signed-off-by Kamalesh Babulal <[email protected]>

Signature · 2 lines

 kernel/sched/core.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

kernel/sched/core.c +0 -3

index 3c5b21e..57cf3ab 100644
@@ -7689,9 +7689,6 @@ int tg_set_cfs_period(struct task_group *tg, long cfs_period_us)
 	period = (u64)cfs_period_us * NSEC_PER_USEC;
 	quota = tg->cfs_bandwidth.quota;
 
-	if (period <= 0)
-		return -EINVAL;
-
 	return tg_set_cfs_bandwidth(tg, period, quota);
 }

Peter Zijlstra

2011-12-12 13:06 UTC #1 lore raw

Quoted 17 lines · Kamalesh Babulal

sched: Remove cfs bandwidth period check in tg_set_cfs_period()
Remove cfs bandwidth period check from tg_set_cfs_period. Invalid bandwidth period's lower/upper limits are denoted by min_cfs_quota_period/max_cfs_quota_period repsectively, and are checked against valid period in tg_set_cfs_bandwidth().
Signed-off-by Kamalesh Babulal <[email protected]>
Signature · 2 lines
 kernel/sched/core.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
kernel/sched/core.c +0 -0
index 3c5b21e..57cf3ab 100644

kernel/sched/core.c int tg_set_cfs_period(struct task_group *tg, long cfs_period_us) · line 7693

Quoted 8 lines

kernel/sched/core.c +0 -3

@@ -7689,9 +7689,6 @@ int tg_set_cfs_period(struct task_group *tg, long cfs_period_us)
        period = (u64)cfs_period_us * NSEC_PER_USEC;
        quota = tg->cfs_bandwidth.quota;
 
-       if (period <= 0)
-               return -EINVAL;
-
        return tg_set_cfs_bandwidth(tg, period, quota);
 }

There's a number of funnies here... it checks an unsigned value for <= 0, which suggests it wanted to check cfs_period_us, which is a signed value.

tg_set_cfs_bandwidth() has the same problem, at that point everything is unsigned and all below zero checks will fail.

Please reconsider things and see if this patch is still the right one.

Paul Turner

Acked-by

2011-12-13 12:40 UTC #2 lore raw

Quoted 21 lines · Peter Zijlstra

Quoted 20 lines · Kamalesh Babulal
sched: Remove cfs bandwidth period check in tg_set_cfs_period()
Remove cfs bandwidth period check from tg_set_cfs_period. Invalid bandwidth period's lower/upper limits are denoted by min_cfs_quota_period/max_cfs_quota_period repsectively, and are checked against valid period in tg_set_cfs_bandwidth().
Signed-off-by Kamalesh Babulal <[email protected]>
--
kernel/sched/core.c | 3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
kernel/sched/core.c +0 -0
index 3c5b21e..57cf3ab 100644
@@ -7689,9 +7689,6 @@ int tg_set_cfs_period(struct task_group *tg, long cfs_period_us)
         period = (u64)cfs_period_us * NSEC_PER_USEC;
         quota = tg->cfs_bandwidth.quota;
 

Quoted 9 lines

kernel/sched/core.c int tg_set_cfs_period(struct task_group *tg, long cfs_period_us) · line 7693
Quoted 5 lines
kernel/sched/core.c +0 -3
-       if (period <= 0)
-               return -EINVAL;
-
         return tg_set_cfs_bandwidth(tg, period, quota);
  }
There's a number of funnies here... it checks an unsigned value for <= 0, which suggests it wanted to check cfs_period_us, which is a signed value.

Yup it wanted to check cfs_period0us -- although as Kamelesh points out, it can be omitted.

Quoted 5 lines

tg_set_cfs_bandwidth() has the same problem, at that point everything is unsigned and all below zero checks will fail.
Please reconsider things and see if this patch is still the right one.

It's OK though since period is always strictly unsigned so any -ve becomes larger than the max allowed period and it spits out -EINVAL as appropriate.

So Kamelesh's patch above is good as is.

Acked-by Paul Turner <[email protected]>

Peter Zijlstra

2011-12-13 12:54 UTC #3 lore raw

Quoted 8 lines · Paul Turner

It's OK though since period is always strictly unsigned so any -ve becomes larger than the max allowed period and it spits out -EINVAL as appropriate.
So Kamelesh's patch above is good as is.
Acked-by Paul Turner <[email protected]>

Right you are, thanks!

tip-bot for Kamalesh Babulal

author

2011-12-21 11:43 UTC #4 lore raw

[tip:sched/core] sched: Remove cfs bandwidth period check in tg_set_cfs_period()

In reply to Kamalesh Babulal

Commit-ID:  11534ec5b6cea13ae38d31799d2a5290c5d724af
Gitweb:     http://git.kernel.org/tip/11534ec5b6cea13ae38d31799d2a5290c5d724af
Author:     Kamalesh Babulal <[email protected]>
AuthorDate: Sat, 10 Dec 2011 19:29:25 +0530
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 21 Dec 2011 10:34:48 +0100

sched: Remove cfs bandwidth period check in tg_set_cfs_period()

As pjt pointed out, negative input will result in very large unsigned numbers and will be caught by the max allowed period test.

Signed-off-by Kamalesh Babulal <[email protected]>

Acked-by Paul Turner <[email protected]>

[ammended changelog to mention negative values]

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

 kernel/sched/core.c |    3 ---
 1 file changed, 3 deletions(-)

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/core.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

kernel/sched/core.c +0 -3

index dba878c..081ece2 100644
@@ -7714,9 +7714,6 @@ int tg_set_cfs_period(struct task_group *tg, long cfs_period_us)
 	period = (u64)cfs_period_us * NSEC_PER_USEC;
 	quota = tg->cfs_bandwidth.quota;
 
-	if (period <= 0)
-		return -EINVAL;
-
 	return tg_set_cfs_bandwidth(tg, period, quota);
 }