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

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

[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 <kamalesh@linux.vnet.ibm.com>
-- 
 kernel/sched/core.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3c5b21e..57cf3ab 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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);
 }
 

Re: [PATCH tip] sched/trivial: Remove cfs bandwidth period check in tg_set_cfs_period()

[Peter Zijlstra]

On Sat, 2011-12-10 at 19:29 +0530, Kamalesh Babulal wrote:
> 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 <kamalesh@linux.vnet.ibm.com>
> -- 
>  kernel/sched/core.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3c5b21e..57cf3ab 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -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.

Re: [PATCH tip] sched/trivial: Remove cfs bandwidth period check in tg_set_cfs_period()

[Paul Turner]

On Mon, Dec 12, 2011 at 5:05 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Sat, 2011-12-10 at 19:29 +0530, Kamalesh Babulal wrote:
>> 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 <kamalesh@linux.vnet.ibm.com>
>> --
>>  kernel/sched/core.c |    3 ---
>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 3c5b21e..57cf3ab 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -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.

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

>
> 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 <pjt@google.com>

Re: [PATCH tip] sched/trivial: Remove cfs bandwidth period check in tg_set_cfs_period()

[Peter Zijlstra]

On Tue, 2011-12-13 at 04:40 -0800, Paul Turner wrote:
> 
> 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 <pjt@google.com> 

Right you are, thanks!

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

[tip-bot for Kamalesh Babulal]

Commit-ID:  11534ec5b6cea13ae38d31799d2a5290c5d724af
Gitweb:     http://git.kernel.org/tip/11534ec5b6cea13ae38d31799d2a5290c5d724af
Author:     Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
AuthorDate: Sat, 10 Dec 2011 19:29:25 +0530
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 21 Dec 2011 10:34:48 +0100

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().

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 <kamalesh@linux.vnet.ibm.com>
Acked-by: Paul Turner <pjt@google.com>
[ammended changelog to mention negative values]
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20111210135925.GA14593@linux.vnet.ibm.com
--
 kernel/sched/core.c |    3 ---
 1 file changed, 3 deletions(-)

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched/core.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index dba878c..081ece2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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);
 }