OpenVZ Forum - RDF feed
type: channel
items: [PATCH, v6 0/3] Introduce timer slack controller
items: [PATCH, v6 2/3] Implement timer slack notifier chain
items: [PATCH, v6 3/3] cgroups: introduce timer slack controller
items: Re: [PATCH, v6 0/3] Introduce timer slack controller
items: Re: [PATCH, v6 2/3] Implement timer slack notifier chain
items: Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller

[PATCH, v6 0/3] Introduce timer slack controller
type: item
description: From: Kirill A. Shutemov <kirill@shutemov.name> Changelog: v6: - add documentation - use notifier_call_chain() instead of check hook - fix validate_change() - cleanup v5: - -EBUSY on writing to timer_slack.min_slack_ns/max_slack_ns if a child has wider min-max range v4: - hierarchy support - drop dummy_timer_slack_check() - workaround lockdep false (?) positive - allow 0 as timer slack value v3: - rework interface - s/EXPORT_SYMBOL/EXPORT_SYMBOL_GPL/ v2: - fixed with CONFIG_CGROUP_TIMER_SLACK=y v1: - initial revision Kirill A. Shutemov (3): cgroups: export cgroup_iter_{start,next,end} Implement timer slack notifier chain cgroups: introduce timer slack controller Documentation/cgroups/timer_slack.txt | 93 +++++++++++ include/linux/cgroup_subsys.h | 6 + include/linux/sched.h | 5 + init/Kconfig | 10 ++ kernel/Makefile | 3 +- kernel/cgroup.c | 3 + kernel/cgroup_timer_slack.c | 285 +++++++++++++++++++++++++++++++++ kernel/sys.c | 9 +- kernel/timer_slack.c | 57 +++++++ 9 files changed, 463 insertions(+), 8 deletions(-) create mode 100644 Documentation/cgroups/timer_slack.txt create mode 100644 kernel/cgroup_timer_slack.c create mode 100644 kernel/timer_slack.c -- 1.7.4 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Kirill A. Shutsemov
date: 2011-02-14T13:06:24-00:00

[PATCH, v6 2/3] Implement timer slack notifier chain
type: item
description: From: Kirill A. Shutemov <kirill@shutemov.name> Process can change its timer slack using prctl(). Timer slack notifier call chain allows to react on such change or forbid it. Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> --- include/linux/sched.h | 5 ++++ kernel/Makefile | 2 +- kernel/sys.c | 9 +------ kernel/timer_slack.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 kernel/timer_slack.c diff --git a/include/linux/sched.h b/include/linux/sched.h index d747f94..f1a4ec2 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2620,6 +2620,11 @@ static inline unsigned long rlimit_max(unsigned int limit) return task_rlimit_max(current, limit); } +extern int register_timer_slack_notifier(struct notifier_block *nb); +extern void unregister_timer_slack_notifier(struct notifier_block *nb); +long prctl_get_timer_slack(void); +extern long prctl_set_timer_slack(long timer_slack_ns); + #endif /* __KERNEL__ */ #endif diff --git a/kernel/Makefile b/kernel/Makefile index 353d3fe..aa43e63 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -10,7 +10,7 @@ obj-y = sched.o fork.o exec_domain.o panic.o printk.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \ notifier.o ksysfs.o pm_qos_params.o sched_clock.o cred.o \ - async.o range.o jump_label.o + async.o range.o jump_label.o timer_slack.o obj-y += groups.o ifdef CONFIG_FUNCTION_TRACER diff --git a/kernel/sys.c b/kernel/sys.c index 18da702..a33e4b8 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1691,15 +1691,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = perf_event_task_enable(); break; case PR_GET_TIMERSLACK: - error = current->timer_slack_ns; + error = prctl_get_timer_slack(); break; case PR_SET_TIMERSLACK: - if (arg2 <= 0) - current->timer_slack_ns = - current->default_timer_slack_ns; - else - current->timer_slack_ns = a…
creator: Kirill A. Shutsemov
date: 2011-02-14T13:06:26-00:00

[PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: From: Kirill A. Shutemov <kirill@shutemov.name> Every task_struct has timer_slack_ns value. This value uses to round up poll() and select() timeout values. This feature can be useful in mobile environment where combined wakeups are desired. cgroup subsys "timer_slack" implement timer slack controller. It provides a way to group tasks by timer slack value and manage the value of group's tasks. Idea-by: Jacob Pan <jacob.jun.pan@linux.intel.com> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> --- Documentation/cgroups/timer_slack.txt | 93 +++++++++++ include/linux/cgroup_subsys.h | 6 + init/Kconfig | 10 ++ kernel/Makefile | 1 + kernel/cgroup_timer_slack.c | 285 +++++++++++++++++++++++++++++++++ 5 files changed, 395 insertions(+), 0 deletions(-) create mode 100644 Documentation/cgroups/timer_slack.txt create mode 100644 kernel/cgroup_timer_slack.c diff --git a/Documentation/cgroups/timer_slack.txt b/Documentation/cgroups/timer_slack.txt new file mode 100644 index 0000000..e7ec2f3 --- /dev/null +++ b/Documentation/cgroups/timer_slack.txt @@ -0,0 +1,93 @@ +Timer Slack Controller +===================== + +Overview +-------- + +Every task_struct has timer_slack_ns value. This value uses to round up +poll() and select() timeout values. This feature can be useful in +mobile environment where combined wakeups are desired. + +cgroup subsys "timer_slack" implement timer slack controller. It +provides a way to group tasks by timer slack value and manage the +value of group's tasks. + + +User interface +-------------- + +To get timer slack controller functionality you need to enable it in +kernel configuration: + +CONFIG_CGROUP_TIMER_SLACK=y + +or if you want to compile it as module: + +CONFIG_CGROUP_TIMER_SLACK=m + +The controller provides three files in cgroup directory: + +# mount -t cgroup -o timer_slack none /sys/fs/cgroup +# ls /sys/fs/cgroup/timer_slack.* +/sys/fs/cgroup/timer_slack.max_slack_ns +/sys/fs/cgroup/timer_slack.min_slack_ns +/sys/fs/cgroup/timer_slack.s…
creator: Kirill A. Shutsemov
date: 2011-02-14T13:06:27-00:00

Re: [PATCH, v6 0/3] Introduce timer slack controller
type: item
description: On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: This 0/0 explains nothing nada. What is the whole muck about ? Thanks, tglx _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Thomas Gleixner
date: 2011-02-14T13:26:42-00:00

Re: [PATCH, v6 2/3] Implement timer slack notifier chain
type: item
description: On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > From: Kirill A. Shutemov <kirill@shutemov.name> > > Process can change its timer slack using prctl(). Timer slack notifier > call chain allows to react on such change or forbid it. So we add a notifier call chain and more exports to allow what ? > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1691,15 +1691,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > error = perf_event_task_enable(); > break; > case PR_GET_TIMERSLACK: > - error = current->timer_slack_ns; > + error = prctl_get_timer_slack(); What's the point of replacing current->timer_slack_ns with a function which does exactly the same ? > +long prctl_set_timer_slack(long timer_slack_ns) > +{ > + int err; > + > + /* Reset timer slack to default value */ > + if (timer_slack_ns <= 0) { > + current->timer_slack_ns = current->default_timer_slack_ns; > + return 0; That does not make any sense at all. Why is setting default_timer_slack_ns not subject to validation ? Why is it treaded seperately ? > + } > + > + err = blocking_notifier_call_chain(&timer_slack_notify_list, > + timer_slack_ns, NULL); > + if (err == NOTIFY_DONE) > + current->timer_slack_ns = timer_slack_ns; > + > + return notifier_to_errno(err); Thanks, tglx _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Thomas Gleixner
date: 2011-02-14T13:32:23-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Mon, Feb 14, 2011 at 03:06:27PM +0200, Kirill A. Shutsemov wrote: > From: Kirill A. Shutemov <kirill@shutemov.name> > > Every task_struct has timer_slack_ns value. This value uses to round up > poll() and select() timeout values. This feature can be useful in > mobile environment where combined wakeups are desired. > > cgroup subsys "timer_slack" implement timer slack controller. It > provides a way to group tasks by timer slack value and manage the > value of group's tasks. > > Idea-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> > --- > Documentation/cgroups/timer_slack.txt | 93 +++++++++++ > include/linux/cgroup_subsys.h | 6 + > init/Kconfig | 10 ++ > kernel/Makefile | 1 + > kernel/cgroup_timer_slack.c | 285 +++++++++++++++++++++++++++++++++ > 5 files changed, 395 insertions(+), 0 deletions(-) > create mode 100644 Documentation/cgroups/timer_slack.txt > create mode 100644 kernel/cgroup_timer_slack.c > > diff --git a/Documentation/cgroups/timer_slack.txt b/Documentation/cgroups/timer_slack.txt > new file mode 100644 > index 0000000..e7ec2f3 > --- /dev/null > +++ b/Documentation/cgroups/timer_slack.txt > @@ -0,0 +1,93 @@ > +Timer Slack Controller > +===================== > + > +Overview > +-------- > + > +Every task_struct has timer_slack_ns value. This value uses to round up > +poll() and select() timeout values. This feature can be useful in > +mobile environment where combined wakeups are desired. > + > +cgroup subsys "timer_slack" implement timer slack controller. It > +provides a way to group tasks by timer slack value and manage the > +value of group's tasks. > + > + > +User interface > +-------------- > + > +To get timer slack controller functionality you need to enable it in > +kernel configuration: > + > +CONFIG_CGROUP_TIMER_SLACK=y > + > +or if you want to compile it as module: > + > +CONFIG_CGROUP_TIMER_SLACK=m > + > +The controller provides three files in cgroup directory: > + > +# mount -t cgr…
creator: Matt Helsley
date: 2011-02-14T13:59:26-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > From: Kirill A. Shutemov <kirill@shutemov.name> > > Every task_struct has timer_slack_ns value. This value uses to round up > poll() and select() timeout values. This feature can be useful in > mobile environment where combined wakeups are desired. > > cgroup subsys "timer_slack" implement timer slack controller. It > provides a way to group tasks by timer slack value and manage the > value of group's tasks. I have no objections against the whole thing in general, but why do we need a module for this? Why can't we add this to the cgroups muck and compile it in? > +struct cgroup_subsys timer_slack_subsys; > +struct timer_slack_cgroup { > + struct cgroup_subsys_state css; > + unsigned long min_slack_ns; > + unsigned long max_slack_ns; > +}; > + > +enum { > + TIMER_SLACK_MIN, > + TIMER_SLACK_MAX, > +}; > + > +static struct timer_slack_cgroup *cgroup_to_tslack_cgroup(struct cgroup *cgroup) > +{ > + struct cgroup_subsys_state *css; > + > + css = cgroup_subsys_state(cgroup, timer_slack_subsys.subsys_id); > + return container_of(css, struct timer_slack_cgroup, css); > +} > + > +static int is_timer_slack_allowed(struct timer_slack_cgroup *tslack_cgroup, bool perhaps ? > + unsigned long slack_ns) > +{ > + if (slack_ns < tslack_cgroup->min_slack_ns || > + slack_ns > tslack_cgroup->max_slack_ns) > + return false; > + return true; > +} > + > +static int cgroup_timer_slack_check(struct notifier_block *nb, > + unsigned long slack_ns, void *data) > +{ > + struct cgroup_subsys_state *css; > + struct timer_slack_cgroup *tslack_cgroup; > + > + /* XXX: lockdep false positive? */ What? Either this has a reason or not. If it's a false positive then it needs to be fixed in lockdep. If not, .... > + rcu_read_lock(); > + css = task_subsys_state(current, timer_slack_subsys.subsys_id); > + tslack_cgroup = container_of(css, struct timer_slack_cgroup, css); > + rcu_read_unlock(); > + > + if (!is_timer_slack_allowed(tslack_cgroup, slack_ns)) > +…
creator: Thomas Gleixner
date: 2011-02-14T14:00:03-00:00

Re: [PATCH, v6 2/3] Implement timer slack notifier chain
type: item
description: On Mon, Feb 14, 2011 at 02:32:23PM +0100, Thomas Gleixner wrote: > On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > > > From: Kirill A. Shutemov <kirill@shutemov.name> > > > > Process can change its timer slack using prctl(). Timer slack notifier > > call chain allows to react on such change or forbid it. > > So we add a notifier call chain and more exports to allow what ? To allow the cgroup contoller validate the value. > > --- a/kernel/sys.c > > +++ b/kernel/sys.c > > @@ -1691,15 +1691,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > > error = perf_event_task_enable(); > > break; > > case PR_GET_TIMERSLACK: > > - error = current->timer_slack_ns; > > + error = prctl_get_timer_slack(); > > What's the point of replacing current->timer_slack_ns with a > function which does exactly the same ? To keep it consistent. BTW, prctl_get_seccomp() does the same. > > > +long prctl_set_timer_slack(long timer_slack_ns) > > +{ > > + int err; > > + > > + /* Reset timer slack to default value */ > > + if (timer_slack_ns <= 0) { > > + current->timer_slack_ns = current->default_timer_slack_ns; > > + return 0; > > That does not make any sense at all. Why is setting > default_timer_slack_ns not subject to validation ? Hm.. In case of cgroup_timer_slack it's always valid. But, yes, in general, we should validate it. > Why is it treaded seperately ? What do you mean? > > + } > > + > > + err = blocking_notifier_call_chain(&timer_slack_notify_list, > > + timer_slack_ns, NULL); > > + if (err == NOTIFY_DONE) > > + current->timer_slack_ns = timer_slack_ns; > > + > > + return notifier_to_errno(err); > > Thanks, > > tglx -- Kirill A. Shutemov _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Kirill A. Shutsemov
date: 2011-02-14T14:52:44-00:00

Re: [PATCH, v6 2/3] Implement timer slack notifier chain
type: item
description: On Mon, 14 Feb 2011, Kirill A. Shutemov wrote: > On Mon, Feb 14, 2011 at 02:32:23PM +0100, Thomas Gleixner wrote: > > On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > > > > > From: Kirill A. Shutemov <kirill@shutemov.name> > > > > > > Process can change its timer slack using prctl(). Timer slack notifier > > > call chain allows to react on such change or forbid it. > > > > So we add a notifier call chain and more exports to allow what ? > > To allow the cgroup contoller validate the value. So we add 5 exports and a notifier chain to have a module? Errm, I mean there is not really a high probability that we'll add 5 more of those validation thingies, right? So instead of having #ifdef CONFIG_CGROUP_MUCK int cgroup_set_slack(....); #else static inline int cgroup_set_slack(...) { return .... } #endif We add all that stuff ? > > > --- a/kernel/sys.c > > > +++ b/kernel/sys.c > > > @@ -1691,15 +1691,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > > > error = perf_event_task_enable(); > > > break; > > > case PR_GET_TIMERSLACK: > > > - error = current->timer_slack_ns; > > > + error = prctl_get_timer_slack(); > > > > What's the point of replacing current->timer_slack_ns with a > > function which does exactly the same ? > > To keep it consistent. BTW, prctl_get_seccomp() does the same. That does not make it less bloat. > > > > > +long prctl_set_timer_slack(long timer_slack_ns) > > > +{ > > > + int err; > > > + > > > + /* Reset timer slack to default value */ > > > + if (timer_slack_ns <= 0) { > > > + current->timer_slack_ns = current->default_timer_slack_ns; > > > + return 0; > > > > That does not make any sense at all. Why is setting > > default_timer_slack_ns not subject to validation ? > > Hm.. In case of cgroup_timer_slack it's always valid. > But, yes, in general, we should validate it. > > > Why is it treaded seperately ? > > What do you mean? Should have read: Why is it treated seperately from the other settings? So setting the de…
creator: Thomas Gleixner
date: 2011-02-14T15:16:28-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Mon, Feb 14, 2011 at 03:00:03PM +0100, Thomas Gleixner wrote: > On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > > From: Kirill A. Shutemov <kirill@shutemov.name> > > > > Every task_struct has timer_slack_ns value. This value uses to round up > > poll() and select() timeout values. This feature can be useful in > > mobile environment where combined wakeups are desired. > > > > cgroup subsys "timer_slack" implement timer slack controller. It > > provides a way to group tasks by timer slack value and manage the > > value of group's tasks. > > I have no objections against the whole thing in general, but why do we > need a module for this? Why can't we add this to the cgroups muck and > compile it in? It was easier to test and debug with module. What is wrong with module? Do you worry about number of exports? > > +struct cgroup_subsys timer_slack_subsys; > > +struct timer_slack_cgroup { > > + struct cgroup_subsys_state css; > > + unsigned long min_slack_ns; > > + unsigned long max_slack_ns; > > +}; > > + > > +enum { > > + TIMER_SLACK_MIN, > > + TIMER_SLACK_MAX, > > +}; > > + > > +static struct timer_slack_cgroup *cgroup_to_tslack_cgroup(struct cgroup *cgroup) > > +{ > > + struct cgroup_subsys_state *css; > > + > > + css = cgroup_subsys_state(cgroup, timer_slack_subsys.subsys_id); > > + return container_of(css, struct timer_slack_cgroup, css); > > +} > > + > > +static int is_timer_slack_allowed(struct timer_slack_cgroup *tslack_cgroup, > > bool perhaps ? Right. > > + unsigned long slack_ns) > > +{ > > + if (slack_ns < tslack_cgroup->min_slack_ns || > > + slack_ns > tslack_cgroup->max_slack_ns) > > + return false; > > + return true; > > +} > > + > > +static int cgroup_timer_slack_check(struct notifier_block *nb, > > + unsigned long slack_ns, void *data) > > +{ > > + struct cgroup_subsys_state *css; > > + struct timer_slack_cgroup *tslack_cgroup; > > + > > + /* XXX: lockdep false positive? */ > > What? Either this has a reason or not. If it's a false positive then > it…
creator: Kirill A. Shutsemov
date: 2011-02-14T15:19:14-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: B1;2401;0cOn Mon, 14 Feb 2011, Kirill A. Shutemov wrote: > On Mon, Feb 14, 2011 at 03:00:03PM +0100, Thomas Gleixner wrote: > > On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > > > From: Kirill A. Shutemov <kirill@shutemov.name> > > > > > > Every task_struct has timer_slack_ns value. This value uses to round up > > > poll() and select() timeout values. This feature can be useful in > > > mobile environment where combined wakeups are desired. > > > > > > cgroup subsys "timer_slack" implement timer slack controller. It > > > provides a way to group tasks by timer slack value and manage the > > > value of group's tasks. > > > > I have no objections against the whole thing in general, but why do we > > need a module for this? Why can't we add this to the cgroups muck and > > compile it in? > > It was easier to test and debug with module. > What is wrong with module? Do you worry about number of exports? Not only about the number. We don't want exports when they are not techically necessary, i.e. for driver stuff. > > > +static int cgroup_timer_slack_check(struct notifier_block *nb, > > > + unsigned long slack_ns, void *data) > > > +{ > > > + struct cgroup_subsys_state *css; > > > + struct timer_slack_cgroup *tslack_cgroup; > > > + > > > + /* XXX: lockdep false positive? */ > > > > What? Either this has a reason or not. If it's a false positive then > > it needs to be fixed in lockdep. If not, .... > > I was not sure about it. There is similar workaround in freezer_fork(). I don't care about workarounds in freezer_work() at all. The above question remains and this is new code and therefor it either needs to hold rcu_read_lock() or it does not. > > > + rcu_read_lock(); > > > + css = task_subsys_state(current, timer_slack_subsys.subsys_id); > > > + tslack_cgroup = container_of(css, struct timer_slack_cgroup, css); > > > + rcu_read_unlock(); > > > + > > > + if (!is_timer_slack_allowed(tslack_cgroup, slack_ns)) > > > + return notifier_from_errno(-EPERM); > > > > If the abov…
creator: Thomas Gleixner
date: 2011-02-14T17:01:06-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Mon, Feb 14, 2011 at 06:01:06PM +0100, Thomas Gleixner wrote: > B1;2401;0cOn Mon, 14 Feb 2011, Kirill A. Shutemov wrote: > > > On Mon, Feb 14, 2011 at 03:00:03PM +0100, Thomas Gleixner wrote: > > > On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > > > > From: Kirill A. Shutemov <kirill@shutemov.name> > > > > > > > > Every task_struct has timer_slack_ns value. This value uses to round up > > > > poll() and select() timeout values. This feature can be useful in > > > > mobile environment where combined wakeups are desired. > > > > > > > > cgroup subsys "timer_slack" implement timer slack controller. It > > > > provides a way to group tasks by timer slack value and manage the > > > > value of group's tasks. > > > > > > I have no objections against the whole thing in general, but why do we > > > need a module for this? Why can't we add this to the cgroups muck and > > > compile it in? > > > > It was easier to test and debug with module. > > What is wrong with module? Do you worry about number of exports? > > Not only about the number. We don't want exports when they are not > techically necessary, i.e. for driver stuff. Ok, I'll drop module support. > > > > +static int cgroup_timer_slack_check(struct notifier_block *nb, > > > > + unsigned long slack_ns, void *data) > > > > +{ > > > > + struct cgroup_subsys_state *css; > > > > + struct timer_slack_cgroup *tslack_cgroup; > > > > + > > > > + /* XXX: lockdep false positive? */ > > > > > > What? Either this has a reason or not. If it's a false positive then > > > it needs to be fixed in lockdep. If not, .... > > > > I was not sure about it. There is similar workaround in freezer_fork(). > > I don't care about workarounds in freezer_work() at all. The above > question remains and this is new code and therefor it either needs to > hold rcu_read_lock() or it does not. I'll recheck everything once again. > > > > + rcu_read_lock(); > > > > + css = task_subsys_state(current, timer_slack_subsys.subsys_id); > > > > + tslack_cgrou…
creator: Kirill A. Shutsemov
date: 2011-02-14T22:39:39-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Mon, Feb 14, 2011 at 05:59:26AM -0800, Matt Helsley wrote: > On Mon, Feb 14, 2011 at 03:06:27PM +0200, Kirill A. Shutsemov wrote: > > From: Kirill A. Shutemov <kirill@shutemov.name> > > > > Every task_struct has timer_slack_ns value. This value uses to round up > > poll() and select() timeout values. This feature can be useful in > > mobile environment where combined wakeups are desired. > > > > cgroup subsys "timer_slack" implement timer slack controller. It > > provides a way to group tasks by timer slack value and manage the > > value of group's tasks. > > > > Idea-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> > > --- > > Documentation/cgroups/timer_slack.txt | 93 +++++++++++ > > include/linux/cgroup_subsys.h | 6 + > > init/Kconfig | 10 ++ > > kernel/Makefile | 1 + > > kernel/cgroup_timer_slack.c | 285 +++++++++++++++++++++++++++++++++ > > 5 files changed, 395 insertions(+), 0 deletions(-) > > create mode 100644 Documentation/cgroups/timer_slack.txt > > create mode 100644 kernel/cgroup_timer_slack.c > > > > diff --git a/Documentation/cgroups/timer_slack.txt b/Documentation/cgroups/timer_slack.txt > > new file mode 100644 > > index 0000000..e7ec2f3 > > --- /dev/null > > +++ b/Documentation/cgroups/timer_slack.txt > > @@ -0,0 +1,93 @@ > > +Timer Slack Controller > > +===================== > > + > > +Overview > > +-------- > > + > > +Every task_struct has timer_slack_ns value. This value uses to round up > > +poll() and select() timeout values. This feature can be useful in > > +mobile environment where combined wakeups are desired. > > + > > +cgroup subsys "timer_slack" implement timer slack controller. It > > +provides a way to group tasks by timer slack value and manage the > > +value of group's tasks. > > + > > + > > +User interface > > +-------------- > > + > > +To get timer slack controller functionality you need to enable it in > > +kernel configuration: > > + > > +CONFIG_CGROUP_TIMER_SLACK=y > >…
creator: Kirill A. Shutsemov
date: 2011-02-14T22:59:40-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Tue, Feb 15, 2011 at 12:39:39AM +0200, Kirill A. Shutemov wrote: > On Mon, Feb 14, 2011 at 06:01:06PM +0100, Thomas Gleixner wrote: > > B1;2401;0cOn Mon, 14 Feb 2011, Kirill A. Shutemov wrote: > > > > > On Mon, Feb 14, 2011 at 03:00:03PM +0100, Thomas Gleixner wrote: > > > > On Mon, 14 Feb 2011, Kirill A. Shutsemov wrote: > > > > > From: Kirill A. Shutemov <kirill@shutemov.name> <snip> > > > > > + list_for_each_entry(cur, &cgroup->children, sibling) { > > > > > + child = cgroup_to_tslack_cgroup(cur); > > > > > + if (type == TIMER_SLACK_MIN && val > child->min_slack_ns) > > > > > + return -EBUSY; > > > > > > > > I thought the whole point is to propagate values through the group. > > > > > > I think silent change here is wrong. cpuset returns -EBUSY in similar > > > case. > > > > And how is cpuset relevant for this ? Not at all. This is about I agree with Thomas here -- cpusets aren't relevant. > > timer_slack and we better have a well defined scheme for all of this > > and not some cobbled together thing with tons of exceptions and corner > > cases. Of course undocumented as far the code goes. > > I don't like silent cascade changes. Userspace can implement it if It need not be totally silent. memcg has a "use_hierarchy" flag file. Alternately, you could punt for now and disable hierarchy somewhat like blkio does. > needed. -EBUSY is appropriate. Hmm, I haven't thought about that method of cascading enough. The important question to consider is how will the parent cgroup be constrained if the owner/group of the children is different and thus disallows userspace from implementing this cascade. I suppose it's consistent with the owner/group ids but it hardly seems consistent with the "spirit" of using cgroups to enable things like containers. Cheers, -Matt Helsley _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Matt Helsley
date: 2011-02-14T23:39:36-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Tue, Feb 15, 2011 at 12:59:40AM +0200, Kirill A. Shutemov wrote: > On Mon, Feb 14, 2011 at 05:59:26AM -0800, Matt Helsley wrote: > > On Mon, Feb 14, 2011 at 03:06:27PM +0200, Kirill A. Shutsemov wrote: > > > From: Kirill A. Shutemov <kirill@shutemov.name> <snip> > > > + list_for_each_entry(cur, &cgroup->children, sibling) { > > > + child = cgroup_to_tslack_cgroup(cur); > > > + if (type == TIMER_SLACK_MIN && val > child->min_slack_ns) > > > + return -EBUSY; > > > + if (type == TIMER_SLACK_MAX && val < child->max_slack_ns) > > > + return -EBUSY; > > > + } > > > > This doesn't look right. Child cgroups should not constrain their > > parents. Instead you should allow the change and propagate the > > constraint to the children. > > See discussion with Thomas. <OK, shifting this topic to that thread> <snip> > > > +static struct cftype files[] = { > > > + { > > > + .name = "set_slack_ns", > > > + .write_u64 = tslack_write_set_slack_ns, > > > + }, > > > + { > > > + .name = "min_slack_ns", > > > + .private = TIMER_SLACK_MIN, > > > + .read_u64 = tslack_read_range, > > > + .write_u64 = tslack_write_range, > > > + }, > > > + { > > > + .name = "max_slack_ns", > > > + .private = TIMER_SLACK_MAX, > > > + .read_u64 = tslack_read_range, > > > + .write_u64 = tslack_write_range, > > > + }, > > > > I didn't get a reply on how a max_slack_ns is useful. It seems > > prudent to add as little interface as possible and only when > > we clearly see the utility of it. > > For example, you can create two groups (excluding root cgroup): > > default - timer slack range 50000-50000 > relaxed - timer slack range 500000-unlimited. > > Now you can drag tasks between these group without need to reset value on > relaxed -> default transition. Perhaps you misunderstood my point. Yes, I can see that a maximum allows you to do counter-productive/pointless little tricks like "setting" the timer slack when you move the task. I just don't get the point of it. Why is setting a maximum timer slack useful? …
creator: Matt Helsley
date: 2011-02-15T00:00:55-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Mon, Feb 14, 2011 at 04:00:55PM -0800, Matt Helsley wrote: > On Tue, Feb 15, 2011 at 12:59:40AM +0200, Kirill A. Shutemov wrote: > > On Mon, Feb 14, 2011 at 05:59:26AM -0800, Matt Helsley wrote: > > > On Mon, Feb 14, 2011 at 03:06:27PM +0200, Kirill A. Shutsemov wrote: > > > > From: Kirill A. Shutemov <kirill@shutemov.name> > > <snip> > > > > > + list_for_each_entry(cur, &cgroup->children, sibling) { > > > > + child = cgroup_to_tslack_cgroup(cur); > > > > + if (type == TIMER_SLACK_MIN && val > child->min_slack_ns) > > > > + return -EBUSY; > > > > + if (type == TIMER_SLACK_MAX && val < child->max_slack_ns) > > > > + return -EBUSY; > > > > + } > > > > > > This doesn't look right. Child cgroups should not constrain their > > > parents. Instead you should allow the change and propagate the > > > constraint to the children. > > > > See discussion with Thomas. > > <OK, shifting this topic to that thread> > <snip> > > > > > +static struct cftype files[] = { > > > > + { > > > > + .name = "set_slack_ns", > > > > + .write_u64 = tslack_write_set_slack_ns, > > > > + }, > > > > + { > > > > + .name = "min_slack_ns", > > > > + .private = TIMER_SLACK_MIN, > > > > + .read_u64 = tslack_read_range, > > > > + .write_u64 = tslack_write_range, > > > > + }, > > > > + { > > > > + .name = "max_slack_ns", > > > > + .private = TIMER_SLACK_MAX, > > > > + .read_u64 = tslack_read_range, > > > > + .write_u64 = tslack_write_range, > > > > + }, > > > > > > I didn't get a reply on how a max_slack_ns is useful. It seems > > > prudent to add as little interface as possible and only when > > > we clearly see the utility of it. > > > > For example, you can create two groups (excluding root cgroup): > > > > default - timer slack range 50000-50000 > > relaxed - timer slack range 500000-unlimited. > > > > Now you can drag tasks between these group without need to reset value on > > relaxed -> default transition. > > Perhaps you misunderstood my point. > > Yes, I can see that a maximum allows you to do c…
creator: Kirill A. Shutsemov
date: 2011-02-15T00:10:04-00:00

Re: [PATCH, v6 3/3] cgroups: introduce timer slack controller
type: item
description: On Tue, 15 Feb 2011, Kirill A. Shutemov wrote: > On Mon, Feb 14, 2011 at 06:01:06PM +0100, Thomas Gleixner wrote: > > > I think silent change here is wrong. cpuset returns -EBUSY in similar > > > case. > > > > And how is cpuset relevant for this ? Not at all. This is about > > timer_slack and we better have a well defined scheme for all of this > > and not some cobbled together thing with tons of exceptions and corner > > cases. Of course undocumented as far the code goes. > > I don't like silent cascade changes. Userspace can implement it if > needed. -EBUSY is appropriate. And I don't like totally uncommented code which follows come cobbled together completely non obvious rules. It's not about what you like. It's about getting useful functionality when we add a new infrastructure like this. So could you please explain what the rules of updating are, so that a reviewer has a chance to understand the rationale of all this. Thanks, tglx _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Thomas Gleixner
date: 2011-02-15T06:04:00-00:00