Messages in this thread | | | From | Paul Turner <> | Date | Mon, 4 Apr 2011 16:10:52 -0700 | Subject | Re: [patch 02/15] sched: validate CFS quota hierarchies |
| |
On Mon, Mar 28, 2011 at 11:57 PM, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> 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@jp.fujitsu.com> > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |