[PATCH, v10 0/3] Introduce timer slack controller

[PATCH, v10 0/3] Introduce timer slack controller

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

Changelog:

v10 [7 month later, thanks to A Plumber's Wish List]:
 - rebased to linux-next-20111011
v9:
 - update documentation
 - minor cleanup
v8:
 - change hiearchy rules
 - introduce timer_slack.effective_slack_ns
 - get_task_timer_slack() -> task_get_effective_timer_slack()
 - task_get_effective_timer_slack() splited in separate patch
 - implement PR_GET_EFFECTIVE_TIMERSLACK
v7:
 - totally reworked interface and rewritten from scratch
   (See Documentation/cgroups/timer_slack.txt for more information)
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):
  hrtimer: introduce effective timer slack
  hrtimer: implement PR_GET_EFFECTIVE_TIMERSLACK
  cgroups: introduce timer slack controller

 Documentation/cgroups/timer_slack.txt |   72 +++++++++++++++++++
 fs/select.c                           |    7 +--
 include/linux/cgroup_subsys.h         |    7 ++
 include/linux/prctl.h                 |    6 ++
 include/linux/sched.h                 |   10 +++
 init/Kconfig                          |    8 ++
 kernel/Makefile                       |    1 +
 kernel/cgroup_timer_slack.c           |  124 +++++++++++++++++++++++++++++++++
 kernel/fork.c                         |    4 +
 kernel/futex.c                        |    4 +-
 kernel/hrtimer.c                      |    2 +-
 kernel/sys.c                          |    3 +
 12 files changed, 240 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/cgroups/timer_slack.txt
 create mode 100644 kernel/cgroup_timer_slack.c

-- 
1.7.6.3

[PATCH, v10 1/3] hrtimer: introduce effective timer slack

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

task_get_effective_timer_slack() returns timer slack value to be used
to configure per-task timers. It can be equal or higher than task's
timer slack value.

For now task_get_effective_timer_slack() returns timer_slack_ns of the
task. Timer slack cgroup controller will implement a bit more
sophisticated logic.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 fs/select.c           |    7 ++-----
 include/linux/sched.h |    6 ++++++
 kernel/fork.c         |    4 ++++
 kernel/futex.c        |    4 ++--
 kernel/hrtimer.c      |    2 +-
 5 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index b6765cf..c6eb8fa 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -69,7 +69,6 @@ static long __estimate_accuracy(struct timespec *tv)
 
 long select_estimate_accuracy(struct timespec *tv)
 {
-	unsigned long ret;
 	struct timespec now;
 
 	/*
@@ -81,10 +80,8 @@ long select_estimate_accuracy(struct timespec *tv)
 
 	ktime_get_ts(&now);
 	now = timespec_sub(*tv, now);
-	ret = __estimate_accuracy(&now);
-	if (ret < current->timer_slack_ns)
-		return current->timer_slack_ns;
-	return ret;
+	return min_t(long, __estimate_accuracy(&now),
+			task_get_effective_timer_slack(current));
 }
 
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 28edc96..d79e300 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2719,6 +2719,12 @@ static inline unsigned long rlimit_max(unsigned int limit)
 	return task_rlimit_max(current, limit);
 }
 
+static inline unsigned long task_get_effective_timer_slack(
+		struct task_struct *tsk)
+{
+	return tsk->timer_slack_ns;
+}
+
 #endif /* __KERNEL__ */
 
 #endif
diff --git a/kernel/fork.c b/kernel/fork.c
index 0675082..4a9fd53 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1180,6 +1180,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	memset(&p->rss_stat, 0, sizeof(p->rss_stat));
 #endif
 
+	/*
+	 * Save current task's (not effective) timer slack value as default
+	 * timer slack value for new task.
+	 */
 	p->default_timer_slack_ns = current->timer_slack_ns;
 
 	task_io_accounting_init(&p->ioac);
diff --git a/kernel/futex.c b/kernel/futex.c
index ea87f4d..14f04b9 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1875,7 +1875,7 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 				      HRTIMER_MODE_ABS);
 		hrtimer_init_sleeper(to, current);
 		hrtimer_set_expires_range_ns(&to->timer, *abs_time,
-					     current->timer_slack_ns);
+				     task_get_effective_timer_slack(current));
 	}
 
 retry:
@@ -2269,7 +2269,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 				      HRTIMER_MODE_ABS);
 		hrtimer_init_sleeper(to, current);
 		hrtimer_set_expires_range_ns(&to->timer, *abs_time,
-					     current->timer_slack_ns);
+				     task_get_effective_timer_slack(current));
 	}
 
 	/*
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 422e567..379460b 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1562,7 +1562,7 @@ long hrtimer_nanosleep(struct timespec *rqtp, struct timespec __user *rmtp,
 	int ret = 0;
 	unsigned long slack;
 
-	slack = current->timer_slack_ns;
+	slack = task_get_effective_timer_slack(current);
 	if (rt_task(current))
 		slack = 0;
 
-- 
1.7.6.3

[PATCH, v10 2/3] hrtimer: implement PR_GET_EFFECTIVE_TIMERSLACK

[Kirill A. Shutemov]

From: "Kirill A. Shutemov" <kirill@shutemov.name>

PR_GET_EFFECTIVE_TIMERSLACK allows process to know its effective timer
slack value.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 include/linux/prctl.h |    6 ++++++
 kernel/sys.c          |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index a3baeb2..3199458 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -102,4 +102,10 @@
 
 #define PR_MCE_KILL_GET 34
 
+/*
+ * Get effective timerslack value for the process.
+ * It can be higher than PR_GET_TIMERSLACK.
+ */
+#define PR_GET_EFFECTIVE_TIMERSLACK 35
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 6fecef0..4b86e41 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1798,6 +1798,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		case PR_GET_TIMERSLACK:
 			error = current->timer_slack_ns;
 			break;
+		case PR_GET_EFFECTIVE_TIMERSLACK:
+			error = task_get_effective_timer_slack(current);
+			break;
 		case PR_SET_TIMERSLACK:
 			if (arg2 <= 0)
 				current->timer_slack_ns =
-- 
1.7.6.3

[PATCH, v10 3/3] cgroups: introduce timer slack controller

[Kirill A. Shutemov]

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.

Originally, prctl() was the only way to change timer slack value of
a process. So you was not able change timer slack value of another
process.

cgroup subsys "timer_slack" implements timer slack controller. It
provides a way to set minimal timer slack value for a group of tasks.
If a task belongs to a cgroup with minimal timer slack value higher than
task's value, cgroup's value will be applied.

Timer slack controller allows to implement setting timer slack value of
a process based on a policy. For example, you can create foreground and
background cgroups and move tasks between them based on system state.

Idea-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 Documentation/cgroups/timer_slack.txt |   72 +++++++++++++++++++
 include/linux/cgroup_subsys.h         |    7 ++
 include/linux/sched.h                 |    4 +
 init/Kconfig                          |    8 ++
 kernel/Makefile                       |    1 +
 kernel/cgroup_timer_slack.c           |  124 +++++++++++++++++++++++++++++++++
 6 files changed, 216 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..6e9cb6a
--- /dev/null
+++ b/Documentation/cgroups/timer_slack.txt
@@ -0,0 +1,72 @@
+Timer Slack Controller
+======================
+
+Overview
+--------
+
+Every task_struct has timer_slack_ns value. This value is used to round
+up poll() and select() timeout values. This feature can be useful in
+mobile environment where combined wakeups are desired.
+
+Originally, prctl() was the only way to change timer slack value of
+a process. So you was not able change timer slack value of another
+process.
+
+cgroup subsys "timer_slack" implements timer slack controller. It
+provides a way to set minimal timer slack value for a group of tasks.
+If a task belongs to a cgroup with minimal timer slack value higher than
+task's value, cgroup's value will be applied.
+
+Timer slack controller allows to implement setting timer slack value of
+a process based on a policy. For example, you can create foreground and
+background cgroups and move tasks between them based on system state.
+
+User interface
+--------------
+
+To get timer slack controller functionality you need to enable it in
+kernel configuration:
+
+CONFIG_CGROUP_TIMER_SLACK=y
+
+The controller provides two files:
+
+# mount -t cgroup -o timer_slack none /sys/fs/cgroup
+# ls /sys/fs/cgroup/timer_slack.*
+/sys/fs/cgroup/timer_slack.effective_slack_ns
+/sys/fs/cgroup/timer_slack.min_slack_ns
+
+By default timer_slack.min_slack_ns is 0:
+
+# cat /sys/fs/cgroup/timer_slack.min_slack_ns
+0
+
+You can set it to some value:
+
+# echo 50000 > /sys/fs/cgroup/timer_slack.min_slack_ns
+# cat /sys/fs/cgroup/timer_slack.min_slack_ns
+50000
+
+Tasks still can set task's value below 50000 using prctl(), but in this
+case cgroup's value will be applied.
+
+Timer slack controller supports hierarchical groups.
+
+# mkdir /sys/fs/cgroup/a
+# cat /sys/fs/cgroup/a/timer_slack.min_slack_ns
+50000
+# echo 70000 > /sys/fs/cgroup/a/timer_slack.min_slack_ns
+# cat /sys/fs/cgroup/a/timer_slack.min_slack_ns
+70000
+
+You can set any value you want, but effective value will the highest value
+up by hierarchy. You can see effective timer slack value for the cgroup from
+timer_slack.effective_slack_ns file:
+
+# cat /sys/fs/cgroup/a/timer_slack.effective_slack_ns
+70000
+# echo 100000 > /sys/fs/cgroup/timer_slack.min_slack_ns
+# cat /sys/fs/cgroup/a/timer_slack.min_slack_ns
+70000
+# cat /sys/fs/cgroup/a/timer_slack.effective_slack_ns
+100000
diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
index 5425822..2c8148f 100644
--- a/include/linux/cgroup_subsys.h
+++ b/include/linux/cgroup_subsys.h
@@ -72,3 +72,10 @@ SUBSYS(tasks)
 #endif
 
 /* */
+
+#ifdef CONFIG_CGROUP_TIMER_SLACK
+SUBSYS(timer_slack)
+#endif
+
+/* */
+
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d79e300..37c19ec 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2719,11 +2719,15 @@ static inline unsigned long rlimit_max(unsigned int limit)
 	return task_rlimit_max(current, limit);
 }
 
+#ifdef CONFIG_CGROUP_TIMER_SLACK
+extern unsigned long task_get_effective_timer_slack(struct task_struct *tsk);
+#else
 static inline unsigned long task_get_effective_timer_slack(
 		struct task_struct *tsk)
 {
 	return tsk->timer_slack_ns;
 }
+#endif
 
 #endif /* __KERNEL__ */
 
diff --git a/init/Kconfig b/init/Kconfig
index 6dfc8c3..71ea5dd 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -603,6 +603,14 @@ config CGROUP_FREEZER
 	  Provides a way to freeze and unfreeze all tasks in a
 	  cgroup.
 
+config CGROUP_TIMER_SLACK
+	bool "Timer slack cgroup controller"
+	help
+	  Provides a way to set minimal timer slack value for tasks in
+	  a cgroup.
+	  It's useful in mobile devices where certain background apps
+	  are attached to a cgroup and combined wakeups are desired.
+
 config CGROUP_DEVICE
 	bool "Device controller for cgroups"
 	help
diff --git a/kernel/Makefile b/kernel/Makefile
index 7469ecc..93672f5 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_COMPAT) += compat.o
 obj-$(CONFIG_CGROUPS) += cgroup.o
 obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
 obj-$(CONFIG_CGROUP_TASK_COUNTER) += cgroup_task_counter.o
+obj-$(CONFIG_CGROUP_TIMER_SLACK) += cgroup_timer_slack.o
 obj-$(CONFIG_CPUSETS) += cpuset.o
 obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_USER_NS) += user_namespace.o
diff --git a/kernel/cgroup_timer_slack.c b/kernel/cgroup_timer_slack.c
new file mode 100644
index 0000000..9824807
--- /dev/null
+++ b/kernel/cgroup_timer_slack.c
@@ -0,0 +1,124 @@
+/*
+ * cgroup_timer_slack.c - control group timer slack subsystem
+ *
+ * Copyright Nokia Corparation, 2011
+ * Author: Kirill A. Shutemov
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/cgroup.h>
+#include <linux/slab.h>
+
+struct cgroup_subsys timer_slack_subsys;
+struct tslack_cgroup {
+	struct cgroup_subsys_state css;
+	unsigned long min_slack_ns;
+};
+
+static struct tslack_cgroup *cgroup_to_tslack(struct cgroup *cgroup)
+{
+	struct cgroup_subsys_state *css;
+
+	css = cgroup_subsys_state(cgroup, timer_slack_subsys.subsys_id);
+	return container_of(css, struct tslack_cgroup, css);
+}
+
+static struct cgroup_subsys_state *tslack_create(struct cgroup_subsys *subsys,
+		struct cgroup *cgroup)
+{
+	struct tslack_cgroup *tslack_cgroup;
+
+	tslack_cgroup = kmalloc(sizeof(*tslack_cgroup), GFP_KERNEL);
+	if (!tslack_cgroup)
+		return ERR_PTR(-ENOMEM);
+
+	if (cgroup->parent) {
+		struct tslack_cgroup *parent;
+
+		parent = cgroup_to_tslack(cgroup->parent);
+		tslack_cgroup->min_slack_ns = parent->min_slack_ns;
+	} else
+		tslack_cgroup->min_slack_ns = 0UL;
+
+	return &tslack_cgroup->css;
+}
+
+static void tslack_destroy(struct cgroup_subsys *tslack_cgroup,
+		struct cgroup *cgroup)
+{
+	kfree(cgroup_to_tslack(cgroup));
+}
+
+static u64 tslack_read_min(struct cgroup *cgroup, struct cftype *cft)
+{
+	return cgroup_to_tslack(cgroup)->min_slack_ns;
+}
+
+static int tslack_write_min(struct cgroup *cgroup, struct cftype *cft, u64 val)
+{
+	if (val > ULONG_MAX)
+		return -EINVAL;
+
+	cgroup_to_tslack(cgroup)->min_slack_ns = val;
+
+	return 0;
+}
+
+static u64 tslack_read_effective(struct cgroup *cgroup, struct cftype *cft)
+{
+	unsigned long min;
+
+	min = cgroup_to_tslack(cgroup)->min_slack_ns;
+	while (cgroup->parent) {
+		cgroup = cgroup->parent;
+		min = max(cgroup_to_tslack(cgroup)->min_slack_ns, min);
+	}
+
+	return min;
+}
+
+static struct cftype files[] = {
+	{
+		.name = "min_slack_ns",
+		.read_u64 = tslack_read_min,
+		.write_u64 = tslack_write_min,
+	},
+	{
+		.name = "effective_slack_ns",
+		.read_u64 = tslack_read_effective,
+	},
+};
+
+static int tslack_populate(struct cgroup_subsys *subsys, struct cgroup *cgroup)
+{
+	return cgroup_add_files(cgroup, subsys, files, ARRAY_SIZE(files));
+}
+
+struct cgroup_subsys timer_slack_subsys = {
+	.name		= "timer_slack",
+	.subsys_id	= timer_slack_subsys_id,
+	.create		= tslack_create,
+	.destroy	= tslack_destroy,
+	.populate	= tslack_populate,
+};
+
+unsigned long task_get_effective_timer_slack(struct task_struct *tsk)
+{
+	struct cgroup *cgroup;
+	unsigned long slack;
+
+	rcu_read_lock();
+	cgroup = task_cgroup(tsk, timer_slack_subsys.subsys_id);
+	slack = tslack_read_effective(cgroup, NULL);
+	rcu_read_unlock();
+
+	return max(tsk->timer_slack_ns, slack);
+}
-- 
1.7.6.3

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Kirill A. Shutemov]

On Tue, Oct 11, 2011 at 07:15:29PM +0300, Kirill A. Shutemov wrote:
> diff --git a/kernel/cgroup_timer_slack.c b/kernel/cgroup_timer_slack.c
> new file mode 100644
> index 0000000..9824807
> --- /dev/null
> +++ b/kernel/cgroup_timer_slack.c
> @@ -0,0 +1,124 @@
> +/*
> + * cgroup_timer_slack.c - control group timer slack subsystem
> + *
> + * Copyright Nokia Corparation, 2011
> + * Author: Kirill A. Shutemov
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/cgroup.h>
> +#include <linux/slab.h>

Sorry, I've missed:

#include <linux/err.h>

-- 
 Kirill A. Shutemov

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Andrew Morton]

On Tue, 11 Oct 2011 19:15:29 +0300
"Kirill A. Shutemov" <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org> wrote:

> 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.
> 
> Originally, prctl() was the only way to change timer slack value of
> a process. So you was not able change timer slack value of another
> process.
> 
> cgroup subsys "timer_slack" implements timer slack controller. It
> provides a way to set minimal timer slack value for a group of tasks.
> If a task belongs to a cgroup with minimal timer slack value higher than
> task's value, cgroup's value will be applied.
> 
> Timer slack controller allows to implement setting timer slack value of
> a process based on a policy. For example, you can create foreground and
> background cgroups and move tasks between them based on system state.

I'm having trouble understanding the value of this feature.  Users can
presently control the timer-slack of a group of processes via
inherit-over-fork.

Perhaps there's a case for providing a way for process A to set process
B's slack.  And perhaps B's children.  That would be a simpler patch
and would have the considerable advantage that it doesn't require
cgroups.

So.... why should we merge this?

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Kirill A. Shutemov]

On Fri, Oct 14, 2011 at 03:43:48PM -0700, Andrew Morton wrote:
> On Tue, 11 Oct 2011 19:15:29 +0300
> "Kirill A. Shutemov" <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org> wrote:
> 
> > 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.
> > 
> > Originally, prctl() was the only way to change timer slack value of
> > a process. So you was not able change timer slack value of another
> > process.
> > 
> > cgroup subsys "timer_slack" implements timer slack controller. It
> > provides a way to set minimal timer slack value for a group of tasks.
> > If a task belongs to a cgroup with minimal timer slack value higher than
> > task's value, cgroup's value will be applied.
> > 
> > Timer slack controller allows to implement setting timer slack value of
> > a process based on a policy. For example, you can create foreground and
> > background cgroups and move tasks between them based on system state.
> 
> I'm having trouble understanding the value of this feature.  Users can
> presently control the timer-slack of a group of processes via
> inherit-over-fork.
> 
> Perhaps there's a case for providing a way for process A to set process
> B's slack.  And perhaps B's children.  That would be a simpler patch
> and would have the considerable advantage that it doesn't require
> cgroups.
> 
> So.... why should we merge this?

Putting a task to a cgroup isn't change task's timer slack, it may affect
"effective timer slack", if min timer slack for the cgroup > task's timer
slack.

Since we don't touch task's slack value we can drag tasks between cgroups
and always get the most relaxed slack value for a task without
saving/restoring it in userspace.

-- 
 Kirill A. Shutemov

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Lennart Poettering]

On Fri, 14.10.11 15:43, Andrew Morton (akpm@linux-foundation.org) wrote:

> > cgroup subsys "timer_slack" implements timer slack controller. It
> > provides a way to set minimal timer slack value for a group of tasks.
> > If a task belongs to a cgroup with minimal timer slack value higher than
> > task's value, cgroup's value will be applied.
> > 
> > Timer slack controller allows to implement setting timer slack value of
> > a process based on a policy. For example, you can create foreground and
> > background cgroups and move tasks between them based on system state.
> 
> I'm having trouble understanding the value of this feature.  Users can
> presently control the timer-slack of a group of processes via
> inherit-over-fork.
> 
> Perhaps there's a case for providing a way for process A to set process
> B's slack.  And perhaps B's children.  That would be a simpler patch
> and would have the considerable advantage that it doesn't require
> cgroups.
> 
> So.... why should we merge this?

Our usecase is basically this:

consider you have one or more desktop user sessions logged in, each one
in a timer slack cgroup. Now, userspace already tracks when sessions
become idle (i.e. currently desktop userspace then starts a screensaver,
or turns off the screen, or similar), and we'd like to increase the timer slack
for the session cgroups individually as the individual session becomes
idle, and decrease it again if the session stops being idle.

Matthew Garrett experimented with a logic like this based on existing
patches, and it showed quite positive effects on power consumption. If
you need hard data I am sure Matthew can fill you in (added him to CC).

What matters for us here is that the timer slack cgroup controller
provides us with a very nice way to change the timerslack for the whole
set of session processes in one go and from the outside. i.e. the idle
logic in userspace would be trivially easy to implement, since we
already track per-session idle states and with the timerlsack cgroup
controller we'd have to touch only a single kernel file to make our
changes.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Sat, 2011-10-15 at 13:20 +0200, Lennart Poettering wrote:
> On Fri, 14.10.11 15:43, Andrew Morton (akpm@linux-foundation.org) wrote:
> 
> > > cgroup subsys "timer_slack" implements timer slack controller. It
> > > provides a way to set minimal timer slack value for a group of tasks.
> > > If a task belongs to a cgroup with minimal timer slack value higher than
> > > task's value, cgroup's value will be applied.
> > > 
> > > Timer slack controller allows to implement setting timer slack value of
> > > a process based on a policy. For example, you can create foreground and
> > > background cgroups and move tasks between them based on system state.
> > 
> > I'm having trouble understanding the value of this feature.  Users can
> > presently control the timer-slack of a group of processes via
> > inherit-over-fork.
> > 
> > Perhaps there's a case for providing a way for process A to set process
> > B's slack.  And perhaps B's children.  That would be a simpler patch
> > and would have the considerable advantage that it doesn't require
> > cgroups.
> > 
> > So.... why should we merge this?
> 
> Our usecase is basically this:
> 
> consider you have one or more desktop user sessions logged in, each one
> in a timer slack cgroup. Now, userspace already tracks when sessions
> become idle (i.e. currently desktop userspace then starts a screensaver,
> or turns off the screen, or similar), and we'd like to increase the timer slack
> for the session cgroups individually as the individual session becomes
> idle, and decrease it again if the session stops being idle.

> What matters for us here is that the timer slack cgroup controller
> provides us with a very nice way to change the timerslack for the whole
> set of session processes in one go and from the outside. i.e. the idle
> logic in userspace would be trivially easy to implement, since we
> already track per-session idle states and with the timerlsack cgroup
> controller we'd have to touch only a single kernel file to make our
> changes.

I would argue this is an excellent reason not to merge this. This really
is in the camp of lets paper over shitty userspace instead of fix it.

Such a scheme takes away the immediate need to fix crap and therefore
crap will not get fixed. Why not focus on creating tools (I think
powertop really already does everything you need) and track down WTF
apps are firing so many timers when the screen if off.

Also, the downside of your approach is that you treat all applications
the same, what if there is a genuine need for reasonable timer response
and your 'policy' wrecks things?

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Lennart Poettering]

On Sat, 15.10.11 21:11, Peter Zijlstra (peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org) wrote:

> > consider you have one or more desktop user sessions logged in, each one
> > in a timer slack cgroup. Now, userspace already tracks when sessions
> > become idle (i.e. currently desktop userspace then starts a screensaver,
> > or turns off the screen, or similar), and we'd like to increase the timer slack
> > for the session cgroups individually as the individual session becomes
> > idle, and decrease it again if the session stops being idle.
> 
> > What matters for us here is that the timer slack cgroup controller
> > provides us with a very nice way to change the timerslack for the whole
> > set of session processes in one go and from the outside. i.e. the idle
> > logic in userspace would be trivially easy to implement, since we
> > already track per-session idle states and with the timerlsack cgroup
> > controller we'd have to touch only a single kernel file to make our
> > changes.
> 
> I would argue this is an excellent reason not to merge this. This really
> is in the camp of lets paper over shitty userspace instead of fix it.
> 
> Such a scheme takes away the immediate need to fix crap and therefore
> crap will not get fixed. Why not focus on creating tools (I think
> powertop really already does everything you need) and track down WTF
> apps are firing so many timers when the screen if off.

Well, maybe that works in fantasy land. In real life however there's
stuff like closed source crap, and all kinds of other things you cannot
fix. In a world where everybody wants "apps" (i.e. 3rd party code
running with limited privileges) your chance to fix every bit of code is
gone.

Despite that: even if all software was open source, and even if we had
the manpower to fix every single project: do you honestly believe it is
such a good idea to build the idle state tracking into each program so
that it can change the timer slack from the inside, if this could be
done in a much much simpler way from the outside?

So yeah, because there'll always be closed source userspace, and because
I believe hacking userspace software should be easy and accessible to
everybody, and because we also should make it difficult to fuck up
runtime behaviour of the machine (such as power usage) I believe
enforcing logic the way we can do it with timerslack cgroups is a good
thing, not a bad thing.

> Also, the downside of your approach is that you treat all applications
> the same, what if there is a genuine need for reasonable timer response
> and your 'policy' wrecks things?

Well, if some thread needs reasonable timer response, then they probably
should be enabling RT anyway and IIRC the patch excludes RT threads when
it mucks with the timer slack. But I guess Kirill can comment on this
much better than I could.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 03:39:21AM +0200, Lennart Poettering wrote:
> On Sat, 15.10.11 21:11, Peter Zijlstra (peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org) wrote:
> > I would argue this is an excellent reason not to merge this. This really
> > is in the camp of lets paper over shitty userspace instead of fix it.
> > 
> > Such a scheme takes away the immediate need to fix crap and therefore
> > crap will not get fixed. Why not focus on creating tools (I think
> > powertop really already does everything you need) and track down WTF
> > apps are firing so many timers when the screen if off.
> 
> Well, maybe that works in fantasy land. In real life however there's
> stuff like closed source crap, and all kinds of other things you cannot
> fix. In a world where everybody wants "apps" (i.e. 3rd party code
> running with limited privileges) your chance to fix every bit of code is
> gone.

I agree. We can't assume perfect software, and I don't think *requiring* 
the duplication of this functionality in every single session 
application is sensible. We want to be able to shut down timer based 
code when the session is idle. We could do this by exposing the slack in 
/proc. We could do this by modifying every piece of code to listen for 
session idle events and (independently) enact session-wide policy. Or we 
could just accept that it's a problem that maps onto what people are 
already using cgroups for and implement it in the same way.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Arjan van de Ven]

On 10/16/2011 8:22 PM, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 03:39:21AM +0200, Lennart Poettering wrote:
>> On Sat, 15.10.11 21:11, Peter Zijlstra (peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org) wrote:
>>> I would argue this is an excellent reason not to merge this. This really
>>> is in the camp of lets paper over shitty userspace instead of fix it.
>>>
>>> Such a scheme takes away the immediate need to fix crap and therefore
>>> crap will not get fixed. Why not focus on creating tools (I think
>>> powertop really already does everything you need) and track down WTF
>>> apps are firing so many timers when the screen if off.
>> Well, maybe that works in fantasy land. In real life however there's
>> stuff like closed source crap, and all kinds of other things you cannot
>> fix. In a world where everybody wants "apps" (i.e. 3rd party code
>> running with limited privileges) your chance to fix every bit of code is
>> gone.
> I agree. We can't assume perfect software, and I don't think *requiring*
> the duplication of this functionality in every single session
> application is sensible. We want to be able to shut down timer based
> code when the session is idle. We could do this by exposing the slack in
> /proc. We could do this by modifying every piece of code to listen for
> session idle events and (independently) enact session-wide policy. Or we
> could just accept that it's a problem that maps onto what people are
> already using cgroups for and implement it in the same way.

and..it's not like we let the bad guys go free.
We actually seriously hurt their behavior while we throttle their timers 
here.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Sun, 2011-10-16 at 22:21 -0700, Arjan van de Ven wrote:
> and..it's not like we let the bad guys go free.
> We actually seriously hurt their behavior while we throttle their timers 
> here. 

How are they hurt? The app appears to behave properly (there are no
repercussions, the app doesn't crash, the developer doesn't get punched
in the face). This doesn't make the developer improve his app, nor make
him take greater care for the next app.

Such behaviour will in fact encourage crap since he seems to be getting
away with it.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Thomas Gleixner]

On Mon, 17 Oct 2011, Peter Zijlstra wrote:

> On Sun, 2011-10-16 at 22:21 -0700, Arjan van de Ven wrote:
> > and..it's not like we let the bad guys go free.
> > We actually seriously hurt their behavior while we throttle their timers 
> > here. 
> 
> How are they hurt? The app appears to behave properly (there are no
> repercussions, the app doesn't crash, the developer doesn't get punched
> in the face). This doesn't make the developer improve his app, nor make
> him take greater care for the next app.
> 
> Such behaviour will in fact encourage crap since he seems to be getting
> away with it.

Unfortunately this is a full speed rolling trainwreck already. The
whole app space is made to encourage shitty applications. Android is
designed exactly in that way and it seems that all other desktop folks
have fully adopted that adsurdity.

Back when NOHZ was merged in the kernel, powertop made people fix crap
left and right in not time. Why should we add something which actively
fosters crappy applications today?

Thanks,

	tglx

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 11:38:03AM +0200, Thomas Gleixner wrote:

> Unfortunately this is a full speed rolling trainwreck already. The
> whole app space is made to encourage shitty applications. Android is
> designed exactly in that way and it seems that all other desktop folks
> have fully adopted that adsurdity.
> 
> Back when NOHZ was merged in the kernel, powertop made people fix crap
> left and right in not time. Why should we add something which actively
> fosters crappy applications today?

They're orthogonal. Sometimes you want timer-driven behaviour (how else 
are you going to draw animations?) and that's obviously going to 
generate wakeups. This is for the idle case, where the user is no longer 
sitting in front of the machine but you don't want to trigger a full 
system suspend. We either need every application that ever uses 
timer-driven behaviour to support setting its own timer slack on some 
sort of external policy decision, or we need a way to let the kernel 
force them to.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 13:46 +0100, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 11:38:03AM +0200, Thomas Gleixner wrote:
> 
> > Unfortunately this is a full speed rolling trainwreck already. The
> > whole app space is made to encourage shitty applications. Android is
> > designed exactly in that way and it seems that all other desktop folks
> > have fully adopted that adsurdity.
> > 
> > Back when NOHZ was merged in the kernel, powertop made people fix crap
> > left and right in not time. Why should we add something which actively
> > fosters crappy applications today?
> 
> They're orthogonal. Sometimes you want timer-driven behaviour (how else 
> are you going to draw animations?) and that's obviously going to 
> generate wakeups. This is for the idle case, where the user is no longer 
> sitting in front of the machine but you don't want to trigger a full 
> system suspend. We either need every application that ever uses 
> timer-driven behaviour to support setting its own timer slack on some 
> sort of external policy decision, or we need a way to let the kernel 
> force them to.

No!! you want that application to stop drawing stuff that is invisible.
Setting your own timerslack for something you know is pointless is worse
than doing something pointless, its down right stupid.

Please, work with the X folks and make it an error to draw to invisible
surfaces. And yes, I know that composition makes that a non-trivial
problem. But at least the blank screen case should be trivial, and I
suspect there's more tractable cases as well. Mostly the desktop is
still a very static place and a few extra timer ticks for when you're
spinning your desktop cube around isn't going to be a problem.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 03:06:26PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-10-17 at 13:46 +0100, Matthew Garrett wrote:
> > They're orthogonal. Sometimes you want timer-driven behaviour (how else 
> > are you going to draw animations?) and that's obviously going to 
> > generate wakeups. This is for the idle case, where the user is no longer 
> > sitting in front of the machine but you don't want to trigger a full 
> > system suspend. We either need every application that ever uses 
> > timer-driven behaviour to support setting its own timer slack on some 
> > sort of external policy decision, or we need a way to let the kernel 
> > force them to.
> 
> No!! you want that application to stop drawing stuff that is invisible.
> Setting your own timerslack for something you know is pointless is worse
> than doing something pointless, its down right stupid.

Whether or not you want the animation to carry on animating is policy, 
and you need something to be the policy agent. Let's say firefox is 
invisible. I now grab a copy of its window contents. What do I get?

> Please, work with the X folks and make it an error to draw to invisible
> surfaces. And yes, I know that composition makes that a non-trivial
> problem. But at least the blank screen case should be trivial, and I
> suspect there's more tractable cases as well. Mostly the desktop is
> still a very static place and a few extra timer ticks for when you're
> spinning your desktop cube around isn't going to be a problem.

So, just to be clear on this, you want to change the semantics of X and 
modify every piece of userspace that currently uses X (because it'll now 
otherwise crash when the screen blanks) in preference to merging a piece 
of code that's functionally consistent with the rest of the cgroups 
infrastructure?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 15:11 +0100, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 03:06:26PM +0200, Peter Zijlstra wrote:
> > On Mon, 2011-10-17 at 13:46 +0100, Matthew Garrett wrote:
> > > They're orthogonal. Sometimes you want timer-driven behaviour (how else 
> > > are you going to draw animations?) and that's obviously going to 
> > > generate wakeups. This is for the idle case, where the user is no longer 
> > > sitting in front of the machine but you don't want to trigger a full 
> > > system suspend. We either need every application that ever uses 
> > > timer-driven behaviour to support setting its own timer slack on some 
> > > sort of external policy decision, or we need a way to let the kernel 
> > > force them to.
> > 
> > No!! you want that application to stop drawing stuff that is invisible.
> > Setting your own timerslack for something you know is pointless is worse
> > than doing something pointless, its down right stupid.
> 
> Whether or not you want the animation to carry on animating is policy, 
> and you need something to be the policy agent. Let's say firefox is 
> invisible. I now grab a copy of its window contents. What do I get?

An XDamage and repaint from the X client, after which your copy will
complete and you get what you asked for?

> > Please, work with the X folks and make it an error to draw to invisible
> > surfaces. And yes, I know that composition makes that a non-trivial
> > problem. But at least the blank screen case should be trivial, and I
> > suspect there's more tractable cases as well. Mostly the desktop is
> > still a very static place and a few extra timer ticks for when you're
> > spinning your desktop cube around isn't going to be a problem.
> 
> So, just to be clear on this, you want to change the semantics of X and 
> modify every piece of userspace that currently uses X (because it'll now 
> otherwise crash when the screen blanks)

Or even when I minimize firefox. That said, ff will probably crash as
soon as I open a second tab because the retarded thing will very likely
continue animating everything on the invisible tab anyway.

You could start by making the X lib of the day, is that XCB these days?,
issue an error print (you get plenty of those anyway) and progress to
full on crashing later.

This gives developers a migration window and incentive to fix up their
apps.

>  in preference to merging a piece 
> of code that's functionally consistent with the rest of the cgroups 
> infrastructure?

Yep.. because as of yet there isn't a sane use-case to warrant adding
the maintenance burden. Any cgroup controller is functionally
consistent, per definition, that doesn't make it useful or even sane.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Arjan van de Ven]

On 10/17/2011 7:28 AM, Peter Zijlstra wrote:
> Or even when I minimize firefox. That said, ff will probably crash as 
> soon as I open a second tab because the retarded thing will very 
> likely continue animating everything on the invisible tab anyway. You 
> could start by making the X lib of the day, is that XCB these days?, 
> issue an error print (you get plenty of those anyway) and progress to 
> full on crashing later. This gives developers a migration window and 
> incentive to fix up their apps.

so back in the time that worked on a project that used Qt as their 
toolkit (geez is it that long ago already ;-) )... we fixed Qt to stop 
doing this.
The right level for this sort of thing is the toolkit level (which by 
and large also does the animations), not Xlib.
The toolkit level also will then need to provide the right notifications 
to the app for things the toolkit does not do
(eg "we're at least partially visible" versus "now none of our pixels 
are on the screen").

It ended up being a thing for the toolkit and a minor tweak in the 
compositor... and it worked quite ok, the app guys actually
asked for the API (because they knew I would beat them up for getting it 
wrong)...

doing things on the xlib level (or even X level) means you take away the 
chance for the App(tm) to do things the right way;
make it easy for the app, not hard.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 07:35 -0700, Arjan van de Ven wrote:
> On 10/17/2011 7:28 AM, Peter Zijlstra wrote:
> > Or even when I minimize firefox. That said, ff will probably crash as 
> > soon as I open a second tab because the retarded thing will very 
> > likely continue animating everything on the invisible tab anyway. You 
> > could start by making the X lib of the day, is that XCB these days?, 
> > issue an error print (you get plenty of those anyway) and progress to 
> > full on crashing later. This gives developers a migration window and 
> > incentive to fix up their apps.
> 
> so back in the time that worked on a project that used Qt as their 
> toolkit (geez is it that long ago already ;-) )... we fixed Qt to stop 
> doing this.
> The right level for this sort of thing is the toolkit level (which by 
> and large also does the animations), not Xlib.
> The toolkit level also will then need to provide the right notifications 
> to the app for things the toolkit does not do
> (eg "we're at least partially visible" versus "now none of our pixels 
> are on the screen").
> 
> It ended up being a thing for the toolkit and a minor tweak in the 
> compositor... and it worked quite ok, the app guys actually
> asked for the API (because they knew I would beat them up for getting it 
> wrong)...
> 
> doing things on the xlib level (or even X level) means you take away the 
> chance for the App(tm) to do things the right way;
> make it easy for the app, not hard.

I'm not immediately seeing how enforcing these semantics through X makes
it harder for the app, or would in fact get in the way of the toolkit
providing similar features.

But they key point to my argument is that at whatever level you're going
to do this, it needs to be unforgivingly enforced. Otherwise its useless
because people either don't know or don't care.

Esp. when you then go do crap like proposed and fudge the timers so that
the impact of doing it wrong is dampened.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 04:28:27PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-10-17 at 15:11 +0100, Matthew Garrett wrote:
> > Whether or not you want the animation to carry on animating is policy, 
> > and you need something to be the policy agent. Let's say firefox is 
> > invisible. I now grab a copy of its window contents. What do I get?
> 
> An XDamage and repaint from the X client, after which your copy will
> complete and you get what you asked for?

An XDamage and then an asynchronous RPC call to the remote server to 
identify the contents of the next frame before drawing them, plus some 
sort of new synchronisation mechanism for blocking the X query until 
that point?

> >  in preference to merging a piece 
> > of code that's functionally consistent with the rest of the cgroups 
> > infrastructure?
> 
> Yep.. because as of yet there isn't a sane use-case to warrant adding
> the maintenance burden. Any cgroup controller is functionally
> consistent, per definition, that doesn't make it useful or even sane.

Timers are a resource. People want to manage that resource. cgroups are 
a convenient mechanism for managing resources.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 15:40 +0100, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 04:28:27PM +0200, Peter Zijlstra wrote:
> > On Mon, 2011-10-17 at 15:11 +0100, Matthew Garrett wrote:
> > > Whether or not you want the animation to carry on animating is policy, 
> > > and you need something to be the policy agent. Let's say firefox is 
> > > invisible. I now grab a copy of its window contents. What do I get?
> > 
> > An XDamage and repaint from the X client, after which your copy will
> > complete and you get what you asked for?
> 
> An XDamage and then an asynchronous RPC call to the remote server to 
> identify the contents of the next frame before drawing them, plus some 
> sort of new synchronisation mechanism for blocking the X query until 
> that point?

Why would this be a problem? 

I mean, why would getting a copy of an otherwise invisible surface be a
performance sensitive path? If the compositor needs the surface it would
make it visible and send the XDamage once and keep it visible henceforth
until the time it again becomes invisible, at which point you have to
stop updates again.

The normal case is where things are visible and things are kept
up-to-date, that doesn't change.

> > >  in preference to merging a piece 
> > > of code that's functionally consistent with the rest of the cgroups 
> > > infrastructure?
> > 
> > Yep.. because as of yet there isn't a sane use-case to warrant adding
> > the maintenance burden. Any cgroup controller is functionally
> > consistent, per definition, that doesn't make it useful or even sane.
> 
> Timers are a resource. People want to manage that resource. cgroups are 
> a convenient mechanism for managing resources.

Yes, and a ball is round (unless you're a USA-ian, in which case they're
ovoid), what's your point?

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 04:49:29PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-10-17 at 15:40 +0100, Matthew Garrett wrote:
> > On Mon, Oct 17, 2011 at 04:28:27PM +0200, Peter Zijlstra wrote:
> > > An XDamage and repaint from the X client, after which your copy will
> > > complete and you get what you asked for?
> > 
> > An XDamage and then an asynchronous RPC call to the remote server to 
> > identify the contents of the next frame before drawing them, plus some 
> > sort of new synchronisation mechanism for blocking the X query until 
> > that point?
> 
> Why would this be a problem? 
> 
> I mean, why would getting a copy of an otherwise invisible surface be a
> performance sensitive path? If the compositor needs the surface it would
> make it visible and send the XDamage once and keep it visible henceforth
> until the time it again becomes invisible, at which point you have to
> stop updates again.

I'm not saying that it's a problem. I'm saying that your approach 
changes behavioural semantics in a way that may violate application 
expectations just as surely as changing the timer behaviour does. 
There's no free approach.

> > Timers are a resource. People want to manage that resource. cgroups are 
> > a convenient mechanism for managing resources.
> 
> Yes, and a ball is round (unless you're a USA-ian, in which case they're
> ovoid), what's your point?

If there's no reason to want to manage that resource, why do we support 
timer slack at all?

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 15:59 +0100, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 04:49:29PM +0200, Peter Zijlstra wrote:
> > On Mon, 2011-10-17 at 15:40 +0100, Matthew Garrett wrote:
> > > On Mon, Oct 17, 2011 at 04:28:27PM +0200, Peter Zijlstra wrote:
> > > > An XDamage and repaint from the X client, after which your copy will
> > > > complete and you get what you asked for?
> > > 
> > > An XDamage and then an asynchronous RPC call to the remote server to 
> > > identify the contents of the next frame before drawing them, plus some 
> > > sort of new synchronisation mechanism for blocking the X query until 
> > > that point?
> > 
> > Why would this be a problem? 
> > 
> > I mean, why would getting a copy of an otherwise invisible surface be a
> > performance sensitive path? If the compositor needs the surface it would
> > make it visible and send the XDamage once and keep it visible henceforth
> > until the time it again becomes invisible, at which point you have to
> > stop updates again.
> 
> I'm not saying that it's a problem. I'm saying that your approach 
> changes behavioural semantics in a way that may violate application 
> expectations just as surely as changing the timer behaviour does. 
> There's no free approach.

I'm not saying its free, I'm saying its a much better approach since it
gets rid of the entire problem instead of papering over the worst of it.

> > > Timers are a resource. People want to manage that resource. cgroups are 
> > > a convenient mechanism for managing resources.
> > 
> > Yes, and a ball is round (unless you're a USA-ian, in which case they're
> > ovoid), what's your point?
> 
> If there's no reason to want to manage that resource, why do we support 
> timer slack at all?

So that individual applications can provide their timing tolerance and
the OS can group these timers in order to optimize idle time or whatnot.

Indiscriminately changing the slack for a whole group of otherwise
unrelated applications will violate their timing tolerances and break
them in non-obvious ways.


  
What I'm saying is that its much better to attack the primary source of
evil in a manner that is unforgiving instead of trying to avoid the
worst excesses and cause non-obvious breakage.

For a while people were promoting the idea that its good to be lenient
in what you accept as input and strict in what you send out. I think
people are starting to realize that was a horrid mistake since now
they're getting utter crap and people don't even know what right is
anymore.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 05:11:00PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-10-17 at 15:59 +0100, Matthew Garrett wrote:
> > I'm not saying that it's a problem. I'm saying that your approach 
> > changes behavioural semantics in a way that may violate application 
> > expectations just as surely as changing the timer behaviour does. 
> > There's no free approach.
> 
> I'm not saying its free, I'm saying its a much better approach since it
> gets rid of the entire problem instead of papering over the worst of it.

It solves it for a specific case, ie animations. Any other timer driven 
behaviour continues. It really does need to be tied to session idle, not 
application visibility, and enforcement at the X level does nothing to 
help that.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 16:19 +0100, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 05:11:00PM +0200, Peter Zijlstra wrote:
> > On Mon, 2011-10-17 at 15:59 +0100, Matthew Garrett wrote:
> > > I'm not saying that it's a problem. I'm saying that your approach 
> > > changes behavioural semantics in a way that may violate application 
> > > expectations just as surely as changing the timer behaviour does. 
> > > There's no free approach.
> > 
> > I'm not saying its free, I'm saying its a much better approach since it
> > gets rid of the entire problem instead of papering over the worst of it.
> 
> It solves it for a specific case, ie animations. Any other timer driven 
> behaviour continues. It really does need to be tied to session idle, not 
> application visibility, and enforcement at the X level does nothing to 
> help that.

Well what other cases are there? Can we enumerate them and come up with
similar solutions?

Hard enforcement is very much better than papering over because it makes
the individual developer instantly aware that he's got a problem.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 05:21:10PM +0200, Peter Zijlstra wrote:

> Well what other cases are there? Can we enumerate them and come up 
> with similar solutions?

Javascript has timers. We execute rather a lot of javascript from 
sources we can't influence at all.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 16:31 +0100, Matthew Garrett wrote:
> On Mon, Oct 17, 2011 at 05:21:10PM +0200, Peter Zijlstra wrote:
> 
> > Well what other cases are there? Can we enumerate them and come up 
> > with similar solutions?
> 
> Javascript has timers. We execute rather a lot of javascript from 
> sources we can't influence at all.

Which brings me to yet another gripe I've got with firefox, _WHY_ does
it run javascript for tabs I can't see? Simply freeze the interpreter.

Then again, what does JavaScript do with these timers? I don't really
see how javascript/random-native-binary are really different, something
needs doing. What something?

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Matthew Garrett]

On Mon, Oct 17, 2011 at 06:36:44PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-10-17 at 16:31 +0100, Matthew Garrett wrote:
> > On Mon, Oct 17, 2011 at 05:21:10PM +0200, Peter Zijlstra wrote:
> > 
> > > Well what other cases are there? Can we enumerate them and come up 
> > > with similar solutions?
> > 
> > Javascript has timers. We execute rather a lot of javascript from 
> > sources we can't influence at all.
> 
> Which brings me to yet another gripe I've got with firefox, _WHY_ does
> it run javascript for tabs I can't see? Simply freeze the interpreter.

Because people expect gmail to provide them with status updates even if 
it's not the foreground tab.

> Then again, what does JavaScript do with these timers? I don't really
> see how javascript/random-native-binary are really different, something
> needs doing. What something?

I agree that the thing that needs doing probably involves web developers 
and threats of implied violence, but I suspect that web developers are 
being created faster than we can reeducate them.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Alan Cox]

> Hard enforcement is very much better than papering over because it makes
> the individual developer instantly aware that he's got a problem.

Not always

If you do weird container crap with timers than app authors will simply
point out that the weird container crap is buggy as it works properly in
a normal environment.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 16:48 +0100, Alan Cox wrote:
> > Hard enforcement is very much better than papering over because it makes
> > the individual developer instantly aware that he's got a problem.
> 
> Not always
> 
> If you do weird container crap with timers than app authors will simply
> point out that the weird container crap is buggy as it works properly in
> a normal environment.

Oh agreed. I never said the hard enforcement would/should rely on
container crap. I'm very much in favour of a machine that's fully
functional with a CGROUP=n kernel, thank you very much.

Sadly a number of (userspace) wankers (some included on this thread)
seem hell bent on making that impossible.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 17:11 +0200, Peter Zijlstra wrote:
> For a while people were promoting the idea that its good to be lenient
> in what you accept as input and strict in what you send out. I think
> people are starting to realize that was a horrid mistake since now
> they're getting utter crap and people don't even know what right is
> anymore. 

Since different apps might have treated similar non-conformant input
differently and now no single behaviour is correct.

I think either dhowells or dwmw2 had a good blog post on that a while
back.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 04:22 +0100, Matthew Garrett wrote:
> 
> I agree. We can't assume perfect software, and I don't think *requiring* 
> the duplication of this functionality in every single session 
> application is sensible. We want to be able to shut down timer based 
> code when the session is idle. We could do this by exposing the slack in 
> /proc. We could do this by modifying every piece of code to listen for 
> session idle events and (independently) enact session-wide policy. Or we 
> could just accept that it's a problem that maps onto what people are 
> already using cgroups for and implement it in the same way. 

So what about the app that is both screen oriented and has a server
part, say some multi-player game thing. Suppose you run one as a server
but leave the client idle (you go have dinner with a pretty lady instead
of playing quake, but leave your machine up to serve for your nerd
friends).

Should your friends get punished with a horrible gaming experience just
because you're having a good time IRL and your screen went blank?

Sure pushing up timer slack will save you power, but it will also hurt
people, what will you do for them? The same that you do for those that
want to run a CGROUP=n kernel?

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Lennart Poettering]

On Mon, 17.10.11 09:34, Peter Zijlstra (peterz@infradead.org) wrote:

> 
> On Mon, 2011-10-17 at 04:22 +0100, Matthew Garrett wrote:
> > 
> > I agree. We can't assume perfect software, and I don't think *requiring* 
> > the duplication of this functionality in every single session 
> > application is sensible. We want to be able to shut down timer based 
> > code when the session is idle. We could do this by exposing the slack in 
> > /proc. We could do this by modifying every piece of code to listen for 
> > session idle events and (independently) enact session-wide policy. Or we 
> > could just accept that it's a problem that maps onto what people are 
> > already using cgroups for and implement it in the same way. 
> 
> So what about the app that is both screen oriented and has a server
> part, say some multi-player game thing. Suppose you run one as a server
> but leave the client idle (you go have dinner with a pretty lady instead
> of playing quake, but leave your machine up to serve for your nerd
> friends).
> 
> Should your friends get punished with a horrible gaming experience just
> because you're having a good time IRL and your screen went blank?

Well, there's already an API to ensure that sessions are not considered
idle, which is used by movie players and such to disable screen
savers.

> Sure pushing up timer slack will save you power, but it will also hurt
> people, what will you do for them? The same that you do for those that
> want to run a CGROUP=n kernel?

As you might have noticed already I am not overly concerned about
those. And anyway, if you don't enable the timer slack cgroup controller
we won't make use of it, it's that easy.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Peter Zijlstra]

On Mon, 2011-10-17 at 03:39 +0200, Lennart Poettering wrote:

> Well, maybe that works in fantasy land. In real life however there's
> stuff like closed source crap, and all kinds of other things you cannot
> fix. In a world where everybody wants "apps" (i.e. 3rd party code
> running with limited privileges) your chance to fix every bit of code is
> gone.

I don't care for closed source crap, and I'm the proof your stmt is
false since I don't want no fucking apps. That's the same kind of
'everybody' that makes GNOME the utter head-up-arse crap it is.

> Despite that: even if all software was open source, and even if we had
> the manpower to fix every single project: do you honestly believe it is
> such a good idea to build the idle state tracking into each program so
> that it can change the timer slack from the inside, if this could be
> done in a much much simpler way from the outside?

That would be doing it wrong. You want your runtime enforcing this
stuff. Simply let X kill whomever issues a drawing request for a
nonvisible surface.

Also, no apps shouldn't change their timer slack, if you think that you
really just don't get it, go away! They shouldn't be using those timers
to begin with. Increasing the slack is a bandaid trying to sort of
compensate for wrong timer usage.

There's absolutely no point in firefox trying to animate a GIF in a
non-visible tab (all are non-visible when the screen if off). Yet it
does. Go and fix that.

As to the proprietary plugins, let firefox spawn one per tab and simply
SIGSTOP the thing when the tab becomes invisible or so, dunno.

> So yeah, because there'll always be closed source userspace, and because
> I believe hacking userspace software should be easy and accessible to
> everybody, and because we also should make it difficult to fuck up
> runtime behaviour of the machine (such as power usage) I believe
> enforcing logic the way we can do it with timerslack cgroups is a good
> thing, not a bad thing.

I'm of the absolute opposite camp, make the runtime unforgiving of
fuckups, like we are for memory protection violations. Apps that crash a
lot aren't popular like you can see in various app markets.

So improve the runtime to detect power unfriendly behaviour and simply
kill.

> > Also, the downside of your approach is that you treat all applications
> > the same, what if there is a genuine need for reasonable timer response
> > and your 'policy' wrecks things?
> 
> Well, if some thread needs reasonable timer response, then they probably
> should be enabling RT anyway and IIRC the patch excludes RT threads when
> it mucks with the timer slack. But I guess Kirill can comment on this
> much better than I could.

That's nonsense, you shouldn't need RT privs in order to get a timer to
fire more than once every few seconds just because the screen if off.

Re: [PATCH, v10 3/3] cgroups: introduce timer slack controller

[Valdis.Kletnieks-PjAqaU27lzQ]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Mon, 17 Oct 2011 09:28:49 +0200, Peter Zijlstra said:
> As to the proprietary plugins, let firefox spawn one per tab and simply
> SIGSTOP the thing when the tab becomes invisible or so, dunno.

Which really sucks if you're trying to listen to streaming audio from the tab
while trying to do other work.  Having to keep the tab visible is a pain.

(And yes, I know the *right* answer is to find a URI suitable to feed to amarok
or xine or something - but have you ever *looked* at the maze of indirections
and Javascript and Flash and who knows what other horrors some radio stations
go through to keep you from doing that? ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]