Re: [patch 02/15] sched: validate CFS quota hierarchies
From: Paul Turner
Date: Mon Apr 04 2011 - 19:11:31 EST
On Mon, Mar 28, 2011 at 11:57 PM, Hidetoshi Seto
<seto.hidetoshi@xxxxxxxxxxxxxx> wrote:
> (2011/03/23 12:03), Paul Turner wrote:
>> @@ -9251,7 +9255,13 @@ static int tg_set_cfs_bandwidth(struct t
>> if (period > max_cfs_quota_period)
>> return -EINVAL;
>>
>> - mutex_lock(&mutex);
>> + mutex_lock(&cfs_constraints_mutex);
>> + if (sysctl_sched_cfs_bandwidth_consistent) {
>> + ret = __cfs_schedulable(tg, period, quota);
>
> At this point:
> period => scale in ns unit
> quota => scale in ns unit, or RUNTIME_INF
>
> And both are unsigned. But...
Ack.. I had accounted for this at one point but I obviously churned it out.
Good catch, thanks!
>
>> @@ -9339,6 +9350,108 @@ static int cpu_cfs_period_write_u64(stru
>> return tg_set_cfs_period(cgroup_tg(cgrp), cfs_period_us);
>> }
>>
>> +
>> +struct cfs_schedulable_data {
>> + struct task_group *tg;
>> + u64 period, quota;
>> +};
>> +
>> +/*
>> + * normalize group quota/period to be quota/max_period
>> + * note: units are usecs
>> + */
>> +static u64 normalize_cfs_quota(struct task_group *tg,
>> + struct cfs_schedulable_data *d)
>> +{
>> + u64 quota, period;
>> + struct load_weight lw;
>> +
>> + if (tg == d->tg) {
>> + period = d->period;
>> + quota = d->quota;
>> + } else {
>> + period = tg_get_cfs_period(tg);
>> + quota = tg_get_cfs_quota(tg);
>> + }
>
> ... at this point:
> period => scale in us unit
> quota => scale in us unit, or -1
> Moreover:
> d->period => (scale in ns unit) / NSEC_PER_USEC
> d->quota => (scale in ns unit, or RUNTIME_INF) / NSEC_PER_USEC
>
> Therefore, ...
>
>> +
>> + if (quota == RUNTIME_INF)
>> + return RUNTIME_INF;
>
> This check doesn't work properly.
Right. Fixed, sorry for the delayed response -- was out last week.
>
> I found this problem because I could not get child group back to be
> unconstrained:
>
> [root@localhost group0]# cat cpu.cfs_*
> 500000
> 500000
> [root@localhost group0]# cat sub0/cpu.cfs_*
> 500000
> 100000
> [root@localhost group0]# cat sub1/cpu.cfs_*
> 500000
> 100000
> [root@localhost group0]# echo -1 > sub1/cpu.cfs_quota_us
> bash: echo: write error: Invalid argument
>
> I confirmed that this write error is removed by the following
> change. I'm looking forward to seeing your V6 soon.
>
> Reviewed-by: Hidetoshi Seto <seto.hidetoshi@xxxxxxxxxxxxxx>
>
>
> Thanks,
> H.Seto
>
> ---
> kernel/sched.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 6d764b5..c8f9820 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -9467,8 +9467,8 @@ static u64 normalize_cfs_quota(struct task_group *tg,
> period = d->period;
> quota = d->quota;
> } else {
> - period = tg_get_cfs_period(tg);
> - quota = tg_get_cfs_quota(tg);
> + period = ktime_to_ns(tg_cfs_bandwidth(tg)->period);
> + quota = tg_cfs_bandwidth(tg)->quota;
> }
>
> if (quota == RUNTIME_INF)
> @@ -9515,8 +9515,8 @@ static int __cfs_schedulable(struct task_group *tg, u64 period, u64 quota)
> int ret;
> struct cfs_schedulable_data data = {
> .tg = tg,
> - .period = period / NSEC_PER_USEC,
> - .quota = quota / NSEC_PER_USEC,
> + .period = period,
> + .quota = quota,
> };
>
> if (!sysctl_sched_cfs_bandwidth_consistent)
> --
> 1.7.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/