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

From: "Kirill A. Shutemov" <redacted>

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

From: "Kirill A. Shutemov" <redacted>

PR_GET_EFFECTIVE_TIMERSLACK allows process to know its effective timer
slack value.

Signed-off-by: Kirill A. Shutemov <redacted>
---
 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

From: "Kirill A. Shutemov" <redacted>

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 [off-list ref]
Signed-off-by: Kirill A. Shutemov <redacted>
---
 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

From: "Kirill A. Shutemov" <redacted>

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 <redacted>
---
 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

On Tue, Oct 11, 2011 at 07:15:29PM +0300, Kirill A. Shutemov wrote:

Sorry, I've missed:

#include <linux/err.h>

-- 
 Kirill A. Shutemov

On Tue, 11 Oct 2011 19:15:29 +0300
"Kirill A. Shutemov" [off-list ref] 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?

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" [off-list ref] wrote:

quoted

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

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

quoted

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.

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:

quoted

quoted

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?

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

quoted

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.

quoted

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.

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:

quoted

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

On 10/16/2011 8:22 PM, Matthew Garrett wrote:

On Mon, Oct 17, 2011 at 03:39:21AM +0200, Lennart Poettering wrote:

quoted

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

quoted

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.

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.

quoted

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.

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?

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.

On Mon, 17 Oct 2011, Peter Zijlstra wrote:

On Sun, 2011-10-16 at 22:21 -0700, Arjan van de Ven wrote:

quoted

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

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

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:

quoted

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.

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:

quoted

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.

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:

quoted

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

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:

quoted

On Mon, 2011-10-17 at 13:46 +0100, Matthew Garrett wrote:

quoted

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?

quoted

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.

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.

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:

quoted

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?

quoted

 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

On Mon, 2011-10-17 at 07:35 -0700, Arjan van de Ven wrote:

On 10/17/2011 7:28 AM, Peter Zijlstra wrote:

quoted

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.

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:

quoted

On Mon, 2011-10-17 at 15:11 +0100, Matthew Garrett wrote:

quoted

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.

quoted

quoted

 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?

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:

quoted

On Mon, Oct 17, 2011 at 04:28:27PM +0200, Peter Zijlstra wrote:

quoted

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.

quoted

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

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:

quoted

On Mon, 2011-10-17 at 15:40 +0100, Matthew Garrett wrote:

quoted

On Mon, Oct 17, 2011 at 04:28:27PM +0200, Peter Zijlstra wrote:

quoted

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.

quoted

quoted

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.

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.

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:

quoted

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

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:

quoted

On Mon, 2011-10-17 at 15:59 +0100, Matthew Garrett wrote:

quoted

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.

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

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.

On Mon, 2011-10-17 at 16:48 +0100, Alan Cox wrote:

quoted

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.

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:

quoted

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?

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:

quoted

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

quoted

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