OpenVZ Forum - RDF feed
type: channel
items: [PATCH v3 0/6] per cgroup /proc/stat statistics
items: [PATCH v3 1/6] measure exec_clock for rt sched entities
items: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
items: [PATCH v3 4/6] add a new scheduler hook for context switch
items: [PATCH v3 5/6] Also record sleep start for a task group
items: [PATCH v3 6/6] expose per-taskgroup schedstats in cgroup
items: Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
items: Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
items: Re: [PATCH v3 4/6] add a new scheduler hook for context switch
items: Re: [PATCH v3 6/6] expose per-taskgroup schedstats in cgroup
items: Re: [PATCH v3 5/6] Also record sleep start for a task group

[PATCH v3 0/6] per cgroup /proc/stat statistics
type: item
description: Hi This is a rework of my v2 series taking into account Paul Turner's comments. All the series is the same except for the patch that collets nr_switches that is now completely reworked. It is out of schedule(), relegated to a scheduler hook. Only difference from Paul's suggestion is that I added a new one, instead of messing with put_prev_task + pick_next_task pairs. In the patchset that adds it, I'll try to argue for my solution. Hope this is acceptable, and please let me know of any other concerns. v2: * completely reworked nr_switches gathering * separated per-se sleep_start to be more clear about it Glauber Costa (6): measure exec_clock for rt sched entities account guest time per-cgroup as well. expose fine-grained per-cpu data for cpuacct stats add a new scheduler hook for context switch Also record sleep start for a task group expose per-taskgroup schedstats in cgroup include/linux/sched.h | 1 + kernel/sched/core.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++-- kernel/sched/fair.c | 42 ++++++++++++- kernel/sched/rt.c | 20 ++++++ kernel/sched/sched.h | 6 ++ 5 files changed, 228 insertions(+), 7 deletions(-) -- 1.7.10.2
creator: Glauber Costa
date: 2012-05-30T09:48:31-00:00

[PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: For simetry with the cfq tasks, measure exec_clock for the rt sched entities (rt_se). This can be used in a number of fashions. For instance, to compute total cpu usage in a cgroup that is generated by rt tasks. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Peter Zijlstra <a.p.zijlstra@chello.nl> CC: Paul Turner <pjt@google.com> --- kernel/sched/rt.c | 5 +++++ kernel/sched/sched.h | 1 + 2 files changed, 6 insertions(+) diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c index c5565c3..30ee4e2 100644 --- a/kernel/sched/rt.c +++ b/kernel/sched/rt.c @@ -919,6 +919,11 @@ static void update_curr_rt(struct rq *rq) sched_rt_avg_update(rq, delta_exec); + for_each_sched_rt_entity(rt_se) { + rt_rq = rt_rq_of_se(rt_se); + schedstat_add(rt_rq, exec_clock, delta_exec); + } + if (!rt_bandwidth_enabled()) return; diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index ba9dccf..cd2f1e1 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -295,6 +295,7 @@ struct rt_rq { struct plist_head pushable_tasks; #endif int rt_throttled; + u64 exec_clock; u64 rt_time; u64 rt_runtime; /* Nests inside the rq lock: */ -- 1.7.10.2
creator: Glauber Costa
date: 2012-05-30T09:48:32-00:00

[PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: The cpuacct cgroup already exposes user and system numbers in a per-cgroup fashion. But they are a summation along the whole group, not a per-cpu figure. Also, they are coarse-grained version of the stats usually shown at places like /proc/stat. I want to have enough cgroup data to emulate the /proc/stat interface. To achieve that, I am creating a new file "stat_percpu" that displays the fine-grained per-cpu data. The original data is left alone. The format of this file resembles the one found in the usual cgroup's stat files. But of course, the fields will be repeated, one per cpu, and prefixed with the cpu number. Therefore, we'll have something like: cpu0.user X cpu0.system Y ... cpu1.user X1 cpu1.system Y1 ... Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Peter Zijlstra <a.p.zijlstra@chello.nl> CC: Paul Turner <pjt@google.com> --- kernel/sched/core.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 220d416..4c1d7e9 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8178,6 +8178,35 @@ static int cpuacct_stats_show(struct cgroup *cgrp, struct cftype *cft, return 0; } +static inline void do_fill_cb(struct cgroup_map_cb *cb, struct cpuacct *ca, + char *str, int cpu, int index) +{ + char name[24]; + struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu); + + snprintf(name, sizeof(name), "cpu%d.%s", cpu, str); + cb->fill(cb, name, cputime64_to_clock_t(kcpustat->cpustat[index])); +} + +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, + struct cgroup_map_cb *cb) +{ + struct cpuacct *ca = cgroup_ca(cgrp); + int cpu; + + for_each_online_cpu(cpu) { + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); + do_fill_cb(cb, ca, "guest", cpu,…
creator: Glauber Costa
date: 2012-05-30T09:48:34-00:00

[PATCH v3 4/6] add a new scheduler hook for context switch
type: item
description: To be able to count the number of switches per-cgroup, and merging Paul's hint that it would be better to do it in fair.c and rt.c, I am introducing a new write-side walk through a new scheduler hook, called at every context switch away from a task (prev). Read-side is greatly simplified, and as I'll show, the performance impact does not seem huge. First, aside from the function call, this walk is O(depth), which is not likely to be huge (if it is, the performance impact is indeed bad - but one can argue this is a good punishment) Also, this walk is likely to be cache-hot, since it is at most the very same loop done by put_prev_task, except it loops depth - 1 instead of depth times. This is specially important not to hurt tasks in the root cgroup, that will pay just a branch. I am introducing a new hook, because the existing ones didn't seem appropriate. The main possibilities would be put_prev_task and pick_next_task. With pick_next_task, there are two main problems: 1) first, the loop is only cache hot in pick_next_task if tasks actually belong to the same class and group as prev. Depending on the workload, this is possibly unlikely. 2) This is vulnerable to wrongdoings in accountings when exiting to idle. Consider two groups A and B with a following pattern: A exits to idle, but after that, B is scheduled. B, on the other hand, always get back to itself when it yields to idle. This means that the to-idle transition in A is never noted. So because of that, I do believe that prev is the right point of that. However, put_prev_task is called many times from multiple places, and the logic to differentiate a context switch from another kind of put would make a mess out of it. On a 4-way x86_64, hackbench -pipe 1 process 4000 shows the following results: - units are seconds to complete the whole benchmark - percentual stdev for easier assesment Task sitting in the root cgroup: Without patchset: 4.857700 (0.69 %) With patchset: 4.863700 (0.63 %) Difference : 0.12 % Task …
creator: Glauber Costa
date: 2012-05-30T09:48:35-00:00

[PATCH v3 5/6] Also record sleep start for a task group
type: item
description: When we're dealing with a task group, instead of a task, also record the start of its sleep time. Since the test agains TASK_UNINTERRUPTIBLE does not really make sense and lack an obvious analogous, we always record it as sleep_start, never block_start. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Peter Zijlstra <a.p.zijlstra@chello.nl> CC: Paul Turner <pjt@google.com> --- kernel/sched/fair.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index c26fe38..d932559 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -1182,7 +1182,8 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags) se->statistics.sleep_start = rq_of(cfs_rq)->clock; if (tsk->state & TASK_UNINTERRUPTIBLE) se->statistics.block_start = rq_of(cfs_rq)->clock; - } + } else + se->statistics.sleep_start = rq_of(cfs_rq)->clock; #endif } -- 1.7.10.2
creator: Glauber Costa
date: 2012-05-30T09:48:36-00:00

[PATCH v3 6/6] expose per-taskgroup schedstats in cgroup
type: item
description: This patch aims at exposing stat information per-cgroup, such as: * idle time, * iowait time, * steal time, * # context switches and friends. The ultimate goal is to be able to present a per-container view of /proc/stat inside a container. With this patch, everything that is needed to do that is in place, except for number of tasks. For most of the data, I achieve that by hooking into the schedstats framework, so although the overhead of that is prone to discussion, I am not adding anything, but reusing what's already there instead. The exception being that the data is now computed and stored in non-task se's as well, instead of entity_is_task() branches. However, I expect this to be minimum comparing to the alternative of adding new hierarchy walks. Those are kept intact. The format of the new file added is the same as the one recently introduced for cpuacct: cpu0.idle X cpu0.steal Y ... cpu1.idle X1 cpu1.steal Y1 ... Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Peter Zijlstra <a.p.zijlstra@chello.nl> CC: Paul Turner <pjt@google.com> --- kernel/sched/core.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++ kernel/sched/fair.c | 24 +++++++++++ kernel/sched/sched.h | 2 + 3 files changed, 140 insertions(+) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index db4f2c3..9c344d3 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -7969,6 +7969,107 @@ static u64 cpu_rt_period_read_uint(struct cgroup *cgrp, struct cftype *cft) } #endif /* CONFIG_RT_GROUP_SCHED */ +#ifdef CONFIG_SCHEDSTATS + +#ifdef CONFIG_FAIR_GROUP_SCHED +#define fair_rq(field, tg, i) tg->cfs_rq[i]->field +#else +#define fair_rq(field, tg, i) 0 +#endif + +#ifdef CONFIG_RT_GROUP_SCHED +#define rt_rq(field, tg, i) tg->rt_rq[i]->field +#else +#define rt_rq(field, tg, i) 0 +#endif + +static u64 tg_nr_switches(struct task_group *tg, int cpu) +{ + if (tg != &root_task_group) + return rt_rq(rt_nr_switches, tg, cpu) + fair_rq(nr_switches, tg, cpu); + + return cpu_rq(cpu)->nr_sw…
creator: Glauber Costa
date: 2012-05-30T09:48:37-00:00

Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: > For simetry with the cfq tasks, measure exec_clock for the rt > sched entities (rt_se). Symmetry methinks.. anyway, where is the symmetry?, fair.c:update_curr() doesn't do the for_each_sched_entity() thing. > This can be used in a number of fashions. For instance, to > compute total cpu usage in a cgroup that is generated by > rt tasks. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > CC: Paul Turner <pjt@google.com> > --- > kernel/sched/rt.c | 5 +++++ > kernel/sched/sched.h | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c > index c5565c3..30ee4e2 100644 > --- a/kernel/sched/rt.c > +++ b/kernel/sched/rt.c > @@ -919,6 +919,11 @@ static void update_curr_rt(struct rq *rq) > > sched_rt_avg_update(rq, delta_exec); > > + for_each_sched_rt_entity(rt_se) { > + rt_rq = rt_rq_of_se(rt_se); > + schedstat_add(rt_rq, exec_clock, delta_exec); > + } > + > if (!rt_bandwidth_enabled()) > return; See, this just makes me sad.. you now have a double for_each_sched_rt_entity() loop.
creator: Peter Zijlstra
date: 2012-05-30T10:29:19-00:00

Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: On 05/30/2012 02:29 PM, Peter Zijlstra wrote: > On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: >> For simetry with the cfq tasks, measure exec_clock for the rt >> sched entities (rt_se). > > Symmetry methinks.. =p bad me > anyway, where is the symmetry?, fair.c:update_curr() > doesn't do the for_each_sched_entity() thing. It does implicitly, because fair.c:update_curr() is called from within enqueue_task(), that is called for_each_sched_entity in enqueue_task_fair(). > >> This can be used in a number of fashions. For instance, to >> compute total cpu usage in a cgroup that is generated by >> rt tasks. >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> >> CC: Peter Zijlstra<a.p.zijlstra@chello.nl> >> CC: Paul Turner<pjt@google.com> >> --- >> kernel/sched/rt.c | 5 +++++ >> kernel/sched/sched.h | 1 + >> 2 files changed, 6 insertions(+) >> >> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c >> index c5565c3..30ee4e2 100644 >> --- a/kernel/sched/rt.c >> +++ b/kernel/sched/rt.c >> @@ -919,6 +919,11 @@ static void update_curr_rt(struct rq *rq) >> >> sched_rt_avg_update(rq, delta_exec); >> >> + for_each_sched_rt_entity(rt_se) { >> + rt_rq = rt_rq_of_se(rt_se); >> + schedstat_add(rt_rq, exec_clock, delta_exec); >> + } >> + >> if (!rt_bandwidth_enabled()) >> return; > > See, this just makes me sad.. you now have a double > for_each_sched_rt_entity() loop. The way I read the rt.c code, it it is called from enqueue_task_rt only once.
creator: Glauber Costa
date: 2012-05-30T10:32:07-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: > +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, > + struct cgroup_map_cb *cb) > +{ > + struct cpuacct *ca = cgroup_ca(cgrp); > + int cpu; > + > + for_each_online_cpu(cpu) { > + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); > + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); > + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); > + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); > + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); > + do_fill_cb(cb, ca, "guest", cpu, CPUTIME_GUEST); > + do_fill_cb(cb, ca, "guest_nice", cpu, CPUTIME_GUEST_NICE); > + } > + > + return 0; > +} Uhm, hotplug anyone?
creator: Peter Zijlstra
date: 2012-05-30T10:34:01-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On 05/30/2012 02:34 PM, Peter Zijlstra wrote: > On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: > >> +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, >> + struct cgroup_map_cb *cb) >> +{ >> + struct cpuacct *ca = cgroup_ca(cgrp); >> + int cpu; >> + >> + for_each_online_cpu(cpu) { >> + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); >> + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); >> + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); >> + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); >> + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); >> + do_fill_cb(cb, ca, "guest", cpu, CPUTIME_GUEST); >> + do_fill_cb(cb, ca, "guest_nice", cpu, CPUTIME_GUEST_NICE); >> + } >> + >> + return 0; >> +} > > Uhm, hotplug anyone? What's with hotplug ? If you mean we should accumulate that on hotplug, I don't see why. We certainly don't do that for the tick-based counters. Or do you mean I am missing a get_online_cpus() lock ? humm, I don't see it being taken on other loops like that
creator: Glauber Costa
date: 2012-05-30T10:34:08-00:00

Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: On Wed, 2012-05-30 at 14:32 +0400, Glauber Costa wrote: > >> + for_each_sched_rt_entity(rt_se) { > >> + rt_rq = rt_rq_of_se(rt_se); > >> + schedstat_add(rt_rq, exec_clock, delta_exec); > >> + } > >> + > >> if (!rt_bandwidth_enabled()) > >> return; > > > > See, this just makes me sad.. you now have a double > > for_each_sched_rt_entity() loop. > > The way I read the rt.c code, it it is called from enqueue_task_rt only > once. Ah, what I meant was, right after that !rt_bandwidth_enabled() muck we do another for_each_sched_rt_entity() walk.
creator: Peter Zijlstra
date: 2012-05-30T10:42:35-00:00

Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: On 05/30/2012 02:42 PM, Peter Zijlstra wrote: > On Wed, 2012-05-30 at 14:32 +0400, Glauber Costa wrote: >>>> + for_each_sched_rt_entity(rt_se) { >>>> + rt_rq = rt_rq_of_se(rt_se); >>>> + schedstat_add(rt_rq, exec_clock, delta_exec); >>>> + } >>>> + >>>> if (!rt_bandwidth_enabled()) >>>> return; >>> >>> See, this just makes me sad.. you now have a double >>> for_each_sched_rt_entity() loop. >> >> The way I read the rt.c code, it it is called from enqueue_task_rt only >> once. > > Ah, what I meant was, right after that !rt_bandwidth_enabled() muck we > do another for_each_sched_rt_entity() walk. I guess I can fold it there...
creator: Glauber Costa
date: 2012-05-30T10:42:35-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On Wed, 2012-05-30 at 14:34 +0400, Glauber Costa wrote: > On 05/30/2012 02:34 PM, Peter Zijlstra wrote: > > On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: > > > >> +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, > >> + struct cgroup_map_cb *cb) > >> +{ > >> + struct cpuacct *ca = cgroup_ca(cgrp); > >> + int cpu; > >> + > >> + for_each_online_cpu(cpu) { > >> + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); > >> + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); > >> + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); > >> + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); > >> + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); > >> + do_fill_cb(cb, ca, "guest", cpu, CPUTIME_GUEST); > >> + do_fill_cb(cb, ca, "guest_nice", cpu, CPUTIME_GUEST_NICE); > >> + } > >> + > >> + return 0; > >> +} > > > > Uhm, hotplug anyone? > What's with hotplug ? Who's to say all cpus are online at this point? If you don't want to make the files come and go with hotplug, one should use for_each_possible_cpu().
creator: Peter Zijlstra
date: 2012-05-30T10:43:48-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On 05/30/2012 02:43 PM, Peter Zijlstra wrote: > On Wed, 2012-05-30 at 14:34 +0400, Glauber Costa wrote: >> On 05/30/2012 02:34 PM, Peter Zijlstra wrote: >>> On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: >>> >>>> +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, >>>> + struct cgroup_map_cb *cb) >>>> +{ >>>> + struct cpuacct *ca = cgroup_ca(cgrp); >>>> + int cpu; >>>> + >>>> + for_each_online_cpu(cpu) { >>>> + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); >>>> + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); >>>> + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); >>>> + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); >>>> + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); >>>> + do_fill_cb(cb, ca, "guest", cpu, CPUTIME_GUEST); >>>> + do_fill_cb(cb, ca, "guest_nice", cpu, CPUTIME_GUEST_NICE); >>>> + } >>>> + >>>> + return 0; >>>> +} >>> >>> Uhm, hotplug anyone? >> What's with hotplug ? > > > Who's to say all cpus are online at this point? If you don't want to > make the files come and go with hotplug, one should use > for_each_possible_cpu(). > I don't oppose that. But I don't really parse what you mean by "make the files go away". We have just one file, with multiple entries. The entries are in key:value form, so I don't see a huge problem in having some entries disappearing. As a matter of fact, this is exactly what happens in /proc/stat when you offline a cpu: the correspondent line will stop showing up.
creator: Glauber Costa
date: 2012-05-30T10:44:48-00:00

Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: On Wed, May 30, 2012 at 3:42 AM, Glauber Costa <glommer@parallels.com> wrote: > On 05/30/2012 02:42 PM, Peter Zijlstra wrote: >> >> On Wed, 2012-05-30 at 14:32 +0400, Glauber Costa wrote: >>>>> >>>>> + for_each_sched_rt_entity(rt_se) { >>>>> + rt_rq = rt_rq_of_se(rt_se); >>>>> + schedstat_add(rt_rq, exec_clock, delta_exec); >>>>> + } >>>>> + >>>>> if (!rt_bandwidth_enabled()) >>>>> return; >>>> >>>> >>>> See, this just makes me sad.. you now have a double >>>> for_each_sched_rt_entity() loop. >>> >>> >>> The way I read the rt.c code, it it is called from enqueue_task_rt only >>> once. >> >> >> Ah, what I meant was, right after that !rt_bandwidth_enabled() muck we >> do another for_each_sched_rt_entity() walk. > > I guess I can fold it there... > Does this even need to be hierarchical? While it's natural for it to be in the CFS case, it feels forced here. You could instead make this rt_rq->local_exec_clock charging only to the parenting rt_rq and post-aggregate when you want to report. The only thing you'd need to be careful of is also accounting children somewhere on the parent on destruction (reaped_exec_clock?). Harking back to symmetry, local_exec_clock is also a potentially useful stat on the CFS side of things since it allows you to usefully disambiguate versus your children (common case where this is useful is calculating usage of threads in the root cgroup); so it wouldn't need to be unique to rt_rq.
creator: Paul Turner
date: 2012-05-30T11:00:37-00:00

Re: [PATCH v3 4/6] add a new scheduler hook for context switch
type: item
description: On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: > To be able to count the number of switches per-cgroup, and merging > Paul's hint that it would be better to do it in fair.c and rt.c, > I am introducing a new write-side walk through a new scheduler hook, > called at every context switch away from a task (prev). > > Read-side is greatly simplified, and as I'll show, the performance impact > does not seem huge. First, aside from the function call, this walk is > O(depth), which is not likely to be huge (if it is, the performance > impact is indeed bad - but one can argue this is a good punishment) > > Also, this walk is likely to be cache-hot, since it is at most the very > same loop done by put_prev_task, except it loops depth - 1 instead of depth > times. This is specially important not to hurt tasks in the root cgroup, > that will pay just a branch. /me cries a little.. I was hoping to fix put_prev_task.. see: https://lkml.org/lkml/2012/2/16/487 (I've actually got a 4 patch split out of that if anybody cares) Its just one of those things stuck behind the -ENOTIME tree :/ The plan is to 'merge' put_prev_task and pick_next_task into one and avoid a lot of the up-down walking. You just added a constraint for always having to walk the entire thing up -- cgroups is too damn expensive already, we should be trimming this nonsense not bloating it. > However, put_prev_task is called many times from multiple places, > and the logic to differentiate a context switch from another kind of > put would make a mess out of it. I'm hoping the fold of put_prev in pick_next as per that patch I referenced could help some, but the cross class switch makes that messy still :/ Reducing the indirect calls is good, adding them is bad.. which makes me the worst offender I'm afraid. > On a 4-way x86_64, hackbench -pipe 1 process 4000 shows the following results: > - units are seconds to complete the whole benchmark > - percentual stdev for easier assesment > > Task sitting in the root…
creator: Peter Zijlstra
date: 2012-05-30T11:20:51-00:00

Re: [PATCH v3 6/6] expose per-taskgroup schedstats in cgroup
type: item
description: On Wed, 2012-05-30 at 13:48 +0400, Glauber Costa wrote: > +static u64 tg_idle(struct task_group *tg, int cpu) > +{ > + u64 val; > + > + if (tg != &root_task_group) { > + val = cfs_read_sleep(tg->se[cpu]); > + /* If we have rt tasks running, we're not really idle */ > + val -= rt_rq(exec_clock, tg, cpu); > + } else > + /* > + * There are many errors here that we are accumulating. > + * However, we only provide this in the interest of having > + * a consistent interface for all cgroups. Everybody > + * probing the root cgroup should be getting its figures > + * from system-wide files as /proc/stat. That would be faster > + * to begin with... > + * > + * Ditto for steal. > + */ > + val = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE] * TICK_NSEC; You just violated 2 coding style rules in one go :-) If one side of the if-else has braces, the other side should have too. If a block is multi-line (regardless of multi-stmt) it should have braces. /me hands you a bucket full of {}. > + return val; > +}
creator: Peter Zijlstra
date: 2012-05-30T11:22:47-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On Wed, May 30, 2012 at 2:48 AM, Glauber Costa <glommer@parallels.com> wrote: > The cpuacct cgroup already exposes user and system numbers in a per-cgroup > fashion. But they are a summation along the whole group, not a per-cpu figure. > Also, they are coarse-grained version of the stats usually shown at places > like /proc/stat. > > I want to have enough cgroup data to emulate the /proc/stat interface. To > achieve that, I am creating a new file "stat_percpu" that displays the > fine-grained per-cpu data. The original data is left alone. > > The format of this file resembles the one found in the usual cgroup's stat > files. But of course, the fields will be repeated, one per cpu, and prefixed > with the cpu number. > > Therefore, we'll have something like: > > cpu0.user X > cpu0.system Y > ... > cpu1.user X1 > cpu1.system Y1 > ... > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > CC: Paul Turner <pjt@google.com> > --- > kernel/sched/core.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 220d416..4c1d7e9 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -8178,6 +8178,35 @@ static int cpuacct_stats_show(struct cgroup *cgrp, struct cftype *cft, > return 0; > } > > +static inline void do_fill_cb(struct cgroup_map_cb *cb, struct cpuacct *ca, > + char *str, int cpu, int index) > +{ > + char name[24]; > + struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu); > + > + snprintf(name, sizeof(name), "cpu%d.%s", cpu, str); > + cb->fill(cb, name, cputime64_to_clock_t(kcpustat->cpustat[index])); > +} > + > +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, > + struct cgroup_map_cb *cb) > +{ > + struct cpuacct *ca = cgroup_ca(cgrp); > + int cpu; > + > + for_each_online_cpu(cpu) { > + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); > + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE);…
creator: Paul Turner
date: 2012-05-30T11:24:29-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On Wed, 2012-05-30 at 14:44 +0400, Glauber Costa wrote: > But I don't really parse what you mean by "make the files go away". We > have just one file, with multiple entries. The entries are in key:value > form, so I don't see a huge problem in having some entries disappearing. > > As a matter of fact, this is exactly what happens in /proc/stat when you > offline a cpu: the correspondent line will stop showing up. Oh, I thought you had a file per cpu. If you have content per cpu, this all might become a problem with 4096 cpus, that's bound to overflow the page of output space?
creator: Peter Zijlstra
date: 2012-05-30T11:24:32-00:00

Re: [PATCH v3 5/6] Also record sleep start for a task group
type: item
description: On Wed, May 30, 2012 at 2:48 AM, Glauber Costa <glommer@parallels.com> wrote: > When we're dealing with a task group, instead of a task, also record > the start of its sleep time. Since the test agains TASK_UNINTERRUPTIBLE > does not really make sense and lack an obvious analogous, we always > record it as sleep_start, never block_start. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > CC: Paul Turner <pjt@google.com> > --- > kernel/sched/fair.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index c26fe38..d932559 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -1182,7 +1182,8 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags) > se->statistics.sleep_start = rq_of(cfs_rq)->clock; > if (tsk->state & TASK_UNINTERRUPTIBLE) > se->statistics.block_start = rq_of(cfs_rq)->clock; > - } > + } else > + se->statistics.sleep_start = rq_of(cfs_rq)->clock; You can't sanely account sleep on a group entity. Suppose you have 2 sleepers on 1 cpu: you account 1s/s of idle Suppose you have 2 sleepers now on 2 cpus: you account 2s/s of idle Furthermore, in the latter case when one wakes up you still continue to accrue sleep time whereas in the former you don't. Just don't report/collect this. > #endif > } > > -- > 1.7.10.2 >
creator: Paul Turner
date: 2012-05-30T11:35:13-00:00

Re: [PATCH v3 4/6] add a new scheduler hook for context switch
type: item
description: On Wed, 2012-05-30 at 13:20 +0200, Peter Zijlstra wrote: > > $ perf stat -e cycles --repeat 100 perf bench sched messaging -p -g 100 > > 48,826,146,710 cycles # 2.470 GHz ( +- 0.17% ) > 2.149005270 seconds time elapsed ( +- 0.12% ) A repeat count of 250 dropped it down to: 2.144582157 seconds time elapsed ( +- 0.08% ) Also, if you're poking at the context switch path, something like: $ taskset 1 perf stat -e cycles --repeat 10 perf bench sched pipe -l 100000 gives a good number and is usually more stable than hackbench.
creator: Peter Zijlstra
date: 2012-05-30T11:40:51-00:00

Re: [PATCH v3 4/6] add a new scheduler hook for context switch
type: item
description: On 05/30/2012 03:20 PM, Peter Zijlstra wrote: > /me cries a little.. I was hoping to fix put_prev_task.. see: > > https://lkml.org/lkml/2012/2/16/487 > > (I've actually got a 4 patch split out of that if anybody cares) > > Its just one of those things stuck behind the -ENOTIME tree :/ > > The plan is to 'merge' put_prev_task and pick_next_task into one and > avoid a lot of the up-down walking. > > You just added a constraint for always having to walk the entire thing > up -- cgroups is too damn expensive already, we should be trimming this > nonsense not bloating it. if they are merged, then I don't need a new hook. Do you have plans to do it? I'd be happy to continue the work if you lack the time, since I am directly interested in the functionality.
creator: Glauber Costa
date: 2012-05-30T12:07:57-00:00

Re: [PATCH v3 4/6] add a new scheduler hook for context switch
type: item
description: On 05/30/2012 03:40 PM, Peter Zijlstra wrote: > On Wed, 2012-05-30 at 13:20 +0200, Peter Zijlstra wrote: >> >> $ perf stat -e cycles --repeat 100 perf bench sched messaging -p -g 100 >> >> 48,826,146,710 cycles # 2.470 GHz ( +- 0.17% ) >> 2.149005270 seconds time elapsed ( +- 0.12% ) > > A repeat count of 250 dropped it down to: > > 2.144582157 seconds time elapsed ( +- 0.08% ) > > Also, if you're poking at the context switch path, something like: > > $ taskset 1 perf stat -e cycles --repeat 10 perf bench sched pipe -l 100000 > > gives a good number and is usually more stable than hackbench. thanks for the pointers
creator: Glauber Costa
date: 2012-05-30T12:08:24-00:00

Re: [PATCH v3 1/6] measure exec_clock for rt sched entities
type: item
description: On 05/30/2012 03:00 PM, Paul Turner wrote: > Does this even need to be hierarchical? While it's natural for it to > be in the CFS case, it feels forced here. > > You could instead make this rt_rq->local_exec_clock charging only to > the parenting rt_rq and post-aggregate when you want to report. The > only thing you'd need to be careful of is also accounting children > somewhere on the parent on destruction (reaped_exec_clock?). > > Harking back to symmetry, local_exec_clock is also a potentially > useful stat on the CFS side of things since it allows you to usefully > disambiguate versus your children (common case where this is useful is > calculating usage of threads in the root cgroup); so it wouldn't need > to be unique to rt_rq. I can try this approach.
creator: Glauber Costa
date: 2012-05-30T12:09:54-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On 05/30/2012 03:24 PM, Paul Turner wrote: >> +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype *cft, >> > + struct cgroup_map_cb *cb) >> > +{ >> > + struct cpuacct *ca = cgroup_ca(cgrp); >> > + int cpu; >> > + >> > + for_each_online_cpu(cpu) { >> > + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); >> > + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); >> > + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); >> > + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); >> > + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); >> > + do_fill_cb(cb, ca, "guest", cpu, CPUTIME_GUEST); >> > + do_fill_cb(cb, ca, "guest_nice", cpu, CPUTIME_GUEST_NICE); >> > + } >> > + > I don't know if there's much that can be trivially done about it but I > suspect these are a bit of a memory allocation time-bomb on a many-CPU > machine. The cgroup:seq_file mating (via read_map) treats everything > as/one/ record. This means that seq_printf is going to end up > eventually allocating a buffer that can fit_everything_ (as well as > every power-of-2 on the way there). Adding insult to injury is that > that the backing buffer is kmalloc() not vmalloc(). > > 200+ bytes per-cpu above really is not unreasonable (46 bytes just for > the text, plus a byte per base 10 digit we end up reporting), but that > then leaves us looking at order-12/13 allocations just to print this > thing when there are O(many) cpus. > And how's /proc/stat different ? It will suffer from the very same problems, since it also have this very same information (actually more, since I am skipping some), per-cpu. Now, if you guys are okay with a file per-cpu, I can do it as well. It pollutes the filesystem, but at least protects against the fact that this is kmalloc-backed.
creator: Glauber Costa
date: 2012-05-30T12:20:15-00:00

Re: [PATCH v3 5/6] Also record sleep start for a task group
type: item
description: On 05/30/2012 03:35 PM, Paul Turner wrote: > On Wed, May 30, 2012 at 2:48 AM, Glauber Costa<glommer@parallels.com> wrote: >> When we're dealing with a task group, instead of a task, also record >> the start of its sleep time. Since the test agains TASK_UNINTERRUPTIBLE >> does not really make sense and lack an obvious analogous, we always >> record it as sleep_start, never block_start. >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> >> CC: Peter Zijlstra<a.p.zijlstra@chello.nl> >> CC: Paul Turner<pjt@google.com> >> --- >> kernel/sched/fair.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index c26fe38..d932559 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -1182,7 +1182,8 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags) >> se->statistics.sleep_start = rq_of(cfs_rq)->clock; >> if (tsk->state& TASK_UNINTERRUPTIBLE) >> se->statistics.block_start = rq_of(cfs_rq)->clock; >> - } >> + } else >> + se->statistics.sleep_start = rq_of(cfs_rq)->clock; > > You can't sanely account sleep on a group entity. > > Suppose you have 2 sleepers on 1 cpu: you account 1s/s of idle > Suppose you have 2 sleepers now on 2 cpus: you account 2s/s of idle > > Furthermore, in the latter case when one wakes up you still continue > to accrue sleep time whereas in the former you don't. > > Just don't report/collect this. sleep_start is not for iowait. This is for idle. And I know no other way to collect idle time per cgroup, other than the time during which it was out of the runqueue. Now what you say about the sleepers don't make that much sense for idle because this information is per-cpu as well. When the se is being dequeued, it means none of its children is running on that runqueue. That's idle. >> #endif >> } >> >> -- >> 1.7.10.2 >>
creator: Glauber Costa
date: 2012-05-30T12:24:40-00:00

Re: [PATCH v3 5/6] Also record sleep start for a task group
type: item
description: On 05/30/2012 04:44 PM, Peter Zijlstra wrote: > On Wed, 2012-05-30 at 16:24 +0400, Glauber Costa wrote: >> sleep_start is not for iowait. This is for idle. And I know no other way >> to collect idle time per cgroup, other than the time during which it was >> out of the runqueue. >> >> Now what you say about the sleepers don't make that much sense for idle >> because this information is per-cpu as well. >> >> When the se is being dequeued, it means none of its children is running >> on that runqueue. That's idle. > > But does that mean the cgroup is idle? Its impossible to re-construct > the machine state from this per-cpu data if your definition of > cgroup-idle is the time when _all_ cpus are idle. > It is idle for that runqueue, aka cpu. The cgroup itself is idle when all cpus are idle. And yes, then you have 2s per sec of idle in a 2-way system. That's pretty much how a physical box works as well.
creator: Glauber Costa
date: 2012-05-30T12:44:09-00:00

Re: [PATCH v3 5/6] Also record sleep start for a task group
type: item
description: On Wed, 2012-05-30 at 16:24 +0400, Glauber Costa wrote: > sleep_start is not for iowait. This is for idle. And I know no other way > to collect idle time per cgroup, other than the time during which it was > out of the runqueue. > > Now what you say about the sleepers don't make that much sense for idle > because this information is per-cpu as well. > > When the se is being dequeued, it means none of its children is running > on that runqueue. That's idle. But does that mean the cgroup is idle? Its impossible to re-construct the machine state from this per-cpu data if your definition of cgroup-idle is the time when _all_ cpus are idle.
creator: Peter Zijlstra
date: 2012-05-30T12:44:37-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On Wed, May 30, 2012 at 5:20 AM, Glauber Costa <glommer@parallels.com> wrote: > On 05/30/2012 03:24 PM, Paul Turner wrote: >>> >>> +static int cpuacct_stats_percpu_show(struct cgroup *cgrp, struct cftype >>> *cft, >>> > + struct cgroup_map_cb *cb) >>> > +{ >>> > + struct cpuacct *ca = cgroup_ca(cgrp); >>> > + int cpu; >>> > + >>> > + for_each_online_cpu(cpu) { >>> > + do_fill_cb(cb, ca, "user", cpu, CPUTIME_USER); >>> > + do_fill_cb(cb, ca, "nice", cpu, CPUTIME_NICE); >>> > + do_fill_cb(cb, ca, "system", cpu, CPUTIME_SYSTEM); >>> > + do_fill_cb(cb, ca, "irq", cpu, CPUTIME_IRQ); >>> > + do_fill_cb(cb, ca, "softirq", cpu, CPUTIME_SOFTIRQ); >>> > + do_fill_cb(cb, ca, "guest", cpu, CPUTIME_GUEST); >>> > + do_fill_cb(cb, ca, "guest_nice", cpu, >>> > CPUTIME_GUEST_NICE); >>> > + } >>> > + >> >> I don't know if there's much that can be trivially done about it but I >> suspect these are a bit of a memory allocation time-bomb on a many-CPU >> machine. The cgroup:seq_file mating (via read_map) treats everything >> as/one/ record. This means that seq_printf is going to end up >> eventually allocating a buffer that can fit_everything_ (as well as >> >> every power-of-2 on the way there). Adding insult to injury is that >> that the backing buffer is kmalloc() not vmalloc(). >> >> 200+ bytes per-cpu above really is not unreasonable (46 bytes just for >> the text, plus a byte per base 10 digit we end up reporting), but that >> then leaves us looking at order-12/13 allocations just to print this >> thing when there are O(many) cpus. >> > > And how's /proc/stat different ? > It will suffer from the very same problems, since it also have this very > same information (actually more, since I am skipping some), per-cpu. So, a) the information in /proc/stat is actually much denser since it's "cpu VAL VAL VAL VAL" as opposed to "cpuX.FIELD VAL" b) If it became a problem the /proc/stat case is actually fairly trivially fixable by defining each cpu as a record and "everything else" as a mag…
creator: Paul Turner
date: 2012-05-30T12:48:40-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On 05/30/2012 04:48 PM, Paul Turner wrote: > a) the information in /proc/stat is actually much denser since it's > "cpu VAL VAL VAL VAL" as opposed to "cpuX.FIELD VAL" easily fixable here. Less descriptive, but we can use a header line with the description much like how /proc/slabinfo does, and we still have an extensible interface that is dense, at the same time. > b) If it became a problem the /proc/stat case is actually fairly > trivially fixable by defining each cpu as a record and "everything > else" as a magic im-out-of-cpus value. > >> > >> > Now, if you guys are okay with a file per-cpu, I can do it as well. >> > It pollutes the filesystem, but at least protects against the fact that this >> > is kmalloc-backed. >> > > As I prefaced, I'm not sure there's much that can be trivially done > about it. This is really a fundamental limitation of how read_map() > works. > > What we really need is a proper seq_file exposed through cftypes. That can be done.
creator: Glauber Costa
date: 2012-05-30T12:52:31-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On 05/30/2012 04:48 PM, Paul Turner wrote: >> > Now, if you guys are okay with a file per-cpu, I can do it as well. >> > It pollutes the filesystem, but at least protects against the fact that this >> > is kmalloc-backed. >> > > As I prefaced, I'm not sure there's much that can be trivially done > about it. This is really a fundamental limitation of how read_map() > works. > > What we really need is a proper seq_file exposed through cftypes. Tejun, would you be okay with an interface that exports somehow the raw seq_file in cgroups ? This way we could call s_show for each cpu and get away with the memory usage problem, I presume
creator: Glauber Costa
date: 2012-05-30T13:26:12-00:00

Re: [PATCH v3 3/6] expose fine-grained per-cpu data for cpuacct stats
type: item
description: On 05/30/2012 04:48 PM, Paul Turner wrote: >> > Now, if you guys are okay with a file per-cpu, I can do it as well. >> > It pollutes the filesystem, but at least protects against the fact that this >> > is kmalloc-backed. >> > > As I prefaced, I'm not sure there's much that can be trivially done > about it. This is really a fundamental limitation of how read_map() > works. > > What we really need is a proper seq_file exposed through cftypes. Tejun, would you be okay with an interface that exports somehow the raw seq_file in cgroups ? This way we could call s_show for each cpu and get away with the memory usage problem, I presume
creator: Glauber Costa
date: 2012-05-30T13:26:24-00:00