Skip to contentSkip to conversation

specific do_timer_cpu value for nohz off mode

PATCH In review Dimitri Sivanich 2 participants 7 messages Aug 17, 2011 – Sep 2, 2011

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 specific do_timer_cpu value for nohz off mode

Reposting this, as this was posted 2 weeks ago with no replies.

Jiffies updates are currently done by the tick_do_timer_cpu. This has a non-deterministic value that can be any running cpu on the system. It changes dynamically in nohz mode. When nohz mode is off, it gets set to a more static, but still non-deterministic value.

While the nohz behavior is necessary, is there a reason why the nohz off case can't have a specific value, say 0 as it was on earlier kernels? If the cpu is offlined, let the value change at that time (note that the x86 arch disallows offlining cpu 0).

There are certain cases where this would be advantageous, especially where timely jiffies updates may not necessarily occur on specific processors.

The following sample patch presents one way that this could be done. Processors wait for the selected cpu to enter high resolution mode before they do so.

Note that this patch is not hotplug aware (however, should the tick_static_do_timer_cpu be offlined, the tick_do_timer_cpu simply becomes another cpu anyway).

Comments on this idea, or the sample patch?

---
 Documentation/kernel-parameters.txt |    8 +++++
 kernel/time/tick-sched.c            |   46 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)
kernel/time/tick-sched.c +46 -0
Index: linux/kernel/time/tick-sched.c
===================================================================
@@ -30,11 +30,54 @@
  */
 static DEFINE_PER_CPU(struct tick_sched, tick_cpu_sched);
 
+DECLARE_PER_CPU(struct hrtimer_cpu_base, hrtimer_bases);
+
 /*
  * The time, when the last jiffy update happened. Protected by xtime_lock.
  */
 static ktime_t last_jiffies_update;
 
+/*
+ * Static tick_do_timer_cpu
+ */
+static int tick_static_do_timer_cpu __read_mostly = -1;
+
+/*
+ * Set tick_do_timer_cpu to a static cpu.
+ *
+ * Supported on systems w/nohz=off only.
+ * Selected cpu can be offlined.  If this is done, another cpu will be chosen,
+ * but will not revert back if selected cpu is onlined again.
+ */
+static int __init setup_static_do_timer_cpu(char *s)
+{
+	if (kstrtoint(s, 0, &tick_static_do_timer_cpu) < 0) {
+		printk(KERN_WARNING "Invalid tick_static_do_timer_cpu specified\n");
+		return 0;
+	}
+	printk(KERN_INFO "Set tick_static_do_timer_cpu %d\n",
+		tick_static_do_timer_cpu);
+
+	return 1;
+}
+__setup("do_timer_cpu=", setup_static_do_timer_cpu);
+
+/*
+ * Check to see if we need to wait for the selected static do_timer_cpu
+ * to enter high res mode first.
+ */
+static int wait_for_static_do_timer_cpu(void)
+{
+	if (tick_static_do_timer_cpu == -1)
+		return 0;
+
+	if (smp_processor_id() != tick_static_do_timer_cpu &&
+	    !*per_cpu_ptr(&hrtimer_bases.hres_active, tick_static_do_timer_cpu))
+		return 1;
+
+	return 0;
+}
+
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
 	return &per_cpu(tick_cpu_sched, cpu);
@@ -848,6 +891,9 @@ int tick_check_oneshot_change(int allow_
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
 
+	if (!allow_nohz && !tick_nohz_enabled && wait_for_static_do_timer_cpu())
+		return 0;
+
 	if (!test_and_clear_bit(0, &ts->check_clocks))
 		return 0;
 
Documentation/kernel-parameters.txt +8 -0
Index: linux/Documentation/kernel-parameters.txt
===================================================================
@@ -665,6 +665,14 @@ bytes respectively. Such letter suffixes
 			The filter can be disabled or changed to another
 			driver later using sysfs.
 
+	do_timer_cpu=<cpu number>
+			This selects the cpu to process jiffies updates
+			when nohz mode is disabled.
+			Works only with high resolution mode enabled.
+			The selected cpu can be offlined.  If this is done,
+			another cpu will be chosen, but will not revert back
+			if the selected cpu is onlined again.
+
 	dscc4.setup=	[NET]
 
 	earlycon=	[KNL] Output early console device and options.
#1 lore raw
Quoted 1 line · Dimitri Sivanich

Reposting this, as this was posted 2 weeks ago with no replies.

It's still in my vacation backlog :)

Quoted 7 lines

Jiffies updates are currently done by the tick_do_timer_cpu. This has a non-deterministic value that can be any running cpu on the system. It changes dynamically in nohz mode. When nohz mode is off, it gets set to a more static, but still non-deterministic value.

While the nohz behavior is necessary, is there a reason why the nohz off case can't have a specific value, say 0 as it was on earlier kernels?

Yes, we had troubles when switching over to highres/oneshot mode when the first cpu which did the switch did not take the do_timer duty. See changelogs.

Quoted 5 lines

If the cpu is offlined, let the value change at that time (note that the x86 arch disallows offlining cpu 0).

There are certain cases where this would be advantageous, especially where timely jiffies updates may not necessarily occur on specific processors.

Huch? How about fixing those long interrupt disabled regions instead? And honestly jiffies update being delayed for a bit is not really a problem.

Quoted 3 lines

The following sample patch presents one way that this could be done. Processors wait for the selected cpu to enter high resolution mode before they do so.

That's a horrible hack.

Quoted 5 lines

Note that this patch is not hotplug aware (however, should the tick_static_do_timer_cpu be offlined, the tick_do_timer_cpu simply becomes another cpu anyway).

Comments on this idea, or the sample patch?

I still have no idea why a random assignment is so harmful. Also if there is really a reason to make that assignment static, what about using a sysfs file, which lets you read out the assignment and update it in the NOHZ off case ?

Thanks,

tglx

#2 lore raw
Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Dimitri Sivanich

Reposting this, as this was posted 2 weeks ago with no replies.

It's still in my vacation backlog :)

Thanks for the prompt reply to this last one.

Quoted 23 lines
Quoted 8 lines

Jiffies updates are currently done by the tick_do_timer_cpu. This has a non-deterministic value that can be any running cpu on the system. It changes dynamically in nohz mode. When nohz mode is off, it gets set to a more static, but still non-deterministic value.

While the nohz behavior is necessary, is there a reason why the nohz off case can't have a specific value, say 0 as it was on earlier kernels?

Yes, we had troubles when switching over to highres/oneshot mode when the first cpu which did the switch did not take the do_timer duty. See changelogs.

Quoted 5 lines

If the cpu is offlined, let the value change at that time (note that the x86 arch disallows offlining cpu 0).

There are certain cases where this would be advantageous, especially where timely jiffies updates may not necessarily occur on specific processors.

Huch? How about fixing those long interrupt disabled regions instead? And honestly jiffies update being delayed for a bit is not really a problem.

Quoted 5 lines
Quoted 3 lines

The following sample patch presents one way that this could be done. Processors wait for the selected cpu to enter high resolution mode before they do so.

That's a horrible hack.

Hopefully the patch below is more tolerable.

Quoted 11 lines
Quoted 6 lines

Note that this patch is not hotplug aware (however, should the tick_static_do_timer_cpu be offlined, the tick_do_timer_cpu simply becomes another cpu anyway).

Comments on this idea, or the sample patch?

I still have no idea why a random assignment is so harmful. Also if there is really a reason to make that assignment static, what about using a sysfs file, which lets you read out the assignment and update it in the NOHZ off case ?

While not necessarily harmful, doing jiffies updates on an application cpu does cause some extra overhead that HPC benchmarking people notice. They prefer to have OS activity isolated to certain cpus. They like reproducibility of results, and having jiffies updates bouncing around introduces variability.

Maybe this is useful for other folks as well? It does give an indication of which cpu is currently doing jiffies updates.

The patch below puts the file in /proc/sys/kernel, but if you think it should be in /sys, please let me know where you'd like to see it.

---
 include/linux/tick.h        |    5 +++++
 kernel/sysctl.c             |    9 +++++++++
 kernel/time/tick-internal.h |    1 -
 kernel/time/tick-sched.c    |   23 +++++++++++++++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)
kernel/sysctl.c +9 -0
Index: linux/kernel/sysctl.c
===================================================================
@@ -57,6 +57,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
 #include <linux/kmod.h>
+#include <linux/tick.h>
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
@@ -368,6 +369,14 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= sched_rt_handler,
 	},
+	{
+		.procname	= "sched_jiffies_cpu",
+		.data		= &tick_do_timer_cpu,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= tick_do_timer_cpu_handler,
+		.extra1		= &zero,
+	},
 #ifdef CONFIG_SCHED_AUTOGROUP
 	{
 		.procname	= "sched_autogroup_enabled",
include/linux/tick.h +5 -0
Index: linux/include/linux/tick.h
===================================================================
@@ -72,6 +72,11 @@ struct tick_sched {
 extern void __init tick_init(void);
 extern int tick_is_oneshot_available(void);
 extern struct tick_device *tick_get_device(int cpu);
+extern int tick_do_timer_cpu_handler(struct ctl_table *table, int write,
+				 void __user *buffer, size_t *lenp,
+				 loff_t *ppos);
+
+extern int tick_do_timer_cpu __read_mostly;
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 extern int tick_init_highres(void);
kernel/time/tick-sched.c +23 -0
Index: linux/kernel/time/tick-sched.c
===================================================================
@@ -815,6 +815,29 @@ void tick_cancel_sched_timer(int cpu)
 }
 #endif
 
+int tick_do_timer_cpu_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int ret, old_val;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (write && tick_nohz_enabled)
+		return -EINVAL;
+#endif
+
+	old_val = tick_do_timer_cpu;
+
+	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+
+	if (!ret && write && !cpu_online(tick_do_timer_cpu)) {
+		tick_do_timer_cpu = old_val;
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 /**
  * Async notification about clocksource changes
  */
kernel/time/tick-internal.h +0 -1
Index: linux/kernel/time/tick-internal.h
===================================================================
@@ -12,7 +12,6 @@
 DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
 extern ktime_t tick_next_period;
 extern ktime_t tick_period;
-extern int tick_do_timer_cpu __read_mostly;
 
 extern void tick_setup_periodic(struct clock_event_device *dev, int broadcast);
 extern void tick_handle_periodic(struct clock_event_device *dev);
#3 lore raw
Quoted 7 lines · Dimitri Sivanich
Quoted 1 line · Thomas Gleixner

On Wed, 17 Aug 2011, Dimitri Sivanich wrote:

While not necessarily harmful, doing jiffies updates on an application cpu does cause some extra overhead that HPC benchmarking people notice. They prefer to have OS activity isolated to certain cpus. They like reproducibility of results, and having jiffies updates bouncing around introduces variability.

Quoted 5 lines

Maybe this is useful for other folks as well? It does give an indication of which cpu is currently doing jiffies updates.

The patch below puts the file in /proc/sys/kernel, but if you think it should be in /sys, please let me know where you'd like to see it.

/sys/devices/system/timekeeping/timekeeping0/....

Please

Quoted 99 lines
---
 include/linux/tick.h        |    5 +++++
 kernel/sysctl.c             |    9 +++++++++
 kernel/time/tick-internal.h |    1 -
 kernel/time/tick-sched.c    |   23 +++++++++++++++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)

Index: linux/kernel/sysctl.c
===================================================================

kernel/sysctl.c +9 -0
@@ -57,6 +57,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
 #include <linux/kmod.h>
+#include <linux/tick.h>
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
@@ -368,6 +369,14 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= sched_rt_handler,
 	},
+	{
+		.procname	= "sched_jiffies_cpu",
+		.data		= &tick_do_timer_cpu,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= tick_do_timer_cpu_handler,
+		.extra1		= &zero,
+	},
 #ifdef CONFIG_SCHED_AUTOGROUP
 	{
 		.procname	= "sched_autogroup_enabled",

Index: linux/include/linux/tick.h
===================================================================

include/linux/tick.h +5 -0
@@ -72,6 +72,11 @@ struct tick_sched {
 extern void __init tick_init(void);
 extern int tick_is_oneshot_available(void);
 extern struct tick_device *tick_get_device(int cpu);
+extern int tick_do_timer_cpu_handler(struct ctl_table *table, int write,
+				 void __user *buffer, size_t *lenp,
+				 loff_t *ppos);
+
+extern int tick_do_timer_cpu __read_mostly;
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 extern int tick_init_highres(void);

Index: linux/kernel/time/tick-sched.c
===================================================================

kernel/time/tick-sched.c +23 -0
@@ -815,6 +815,29 @@ void tick_cancel_sched_timer(int cpu)
 }
 #endif
 
+int tick_do_timer_cpu_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int ret, old_val;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (write && tick_nohz_enabled)
+		return -EINVAL;
+#endif
+
+	old_val = tick_do_timer_cpu;
+
+	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+
+	if (!ret && write && !cpu_online(tick_do_timer_cpu)) {
+		tick_do_timer_cpu = old_val;
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 /**
  * Async notification about clocksource changes
  */

Index: linux/kernel/time/tick-internal.h
===================================================================

kernel/time/tick-internal.h +0 -1
@@ -12,7 +12,6 @@
 DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
 extern ktime_t tick_next_period;
 extern ktime_t tick_period;
-extern int tick_do_timer_cpu __read_mostly;
 
 extern void tick_setup_periodic(struct clock_event_device *dev, int broadcast);
 extern void tick_handle_periodic(struct clock_event_device *dev);
#4 lore raw
Quoted 11 lines · Thomas Gleixner
Quoted 10 lines · Dimitri Sivanich
Quoted 1 line · Thomas Gleixner

On Wed, 17 Aug 2011, Dimitri Sivanich wrote:

While not necessarily harmful, doing jiffies updates on an application cpu does cause some extra overhead that HPC benchmarking people notice. They prefer to have OS activity isolated to certain cpus. They like reproducibility of results, and having jiffies updates bouncing around introduces variability.

Maybe this is useful for other folks as well? It does give an indication of which cpu is currently doing jiffies updates.

Quoted 7 lines
Quoted 2 lines

The patch below puts the file in /proc/sys/kernel, but if you think it should be in /sys, please let me know where you'd like to see it.

/sys/devices/system/timekeeping/timekeeping0/....

Please

/sys/devices/system/timekeeping/timekeeping0/jiffies_cpu

Allow manual override of the tick_do_timer_cpu.

---
 kernel/time/tick-sched.c |   98 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)
kernel/time/tick-sched.c +98 -0
Index: linux/kernel/time/tick-sched.c
===================================================================
@@ -815,6 +815,104 @@ void tick_cancel_sched_timer(int cpu)
 }
 #endif
 
+
+#ifdef CONFIG_SYSFS
+/**
+ * sysfs_show_do_timer_cpu - sysfs interface for tick_do_timer_cpu
+ * @dev:        unused
+ * @buf:        char buffer where value of tick_do_timer_cpu is copied
+ *
+ * Provides sysfs interface for showing the current tick_do_timer_cpu.
+ */
+static ssize_t
+sysfs_show_do_timer_cpu(struct sys_device *dev,
+				struct sysdev_attribute *attr, char *buf)
+{
+	ssize_t count = 0;
+
+	count = snprintf(buf, PAGE_SIZE, "%d\n", tick_do_timer_cpu);
+
+	return count;
+}
+
+/**
+ * sysfs_override_do_timer_cpu - manually override tick_do_timer_cpu
+ * @dev:        unused
+ * @buf:        cpu number of desired tick_do_timer_cpu
+ * @count:      length of buffer
+ *
+ * Takes input from sysfs interface for manually overriding the selected
+ * tick_do_timer_cpu.  Only applicable when not running in nohz mode.
+ */
+static ssize_t
+sysfs_override_do_timer_cpu(struct sys_device *dev,
+					struct sysdev_attribute *attr,
+					const char *buf, size_t count)
+{
+	char b[16];
+	size_t ret = count;
+	int c;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+	/* strings from sysfs write are not 0 terminated! */
+	if (count >= sizeof(b))
+		return -EINVAL;
+
+	/* strip off \n: */
+	if (buf[count-1] == '\n')
+		count--;
+	if (count < 1)
+		return -EINVAL;
+
+	memcpy(b, buf, count);
+	b[count] = 0;
+
+	if (sscanf(b, "%d", &c) != 1)
+		return -EINVAL;
+
+	if (!cpu_online(c))
+		return -EINVAL;
+
+	tick_do_timer_cpu = c;
+
+	return ret;
+}
+
+/*
+ * Sysfs setup bits:
+ */
+static SYSDEV_ATTR(jiffies_cpu, 0644, sysfs_show_do_timer_cpu,
+		   sysfs_override_do_timer_cpu);
+
+static struct sysdev_class timekeeping_sysclass = {
+	.name = "timekeeping",
+};
+
+static struct sys_device device_timekeeping = {
+	.id     = 0,
+	.cls    = &timekeeping_sysclass,
+};
+
+static int __init init_timekeeping_sysfs(void)
+{
+	int error = sysdev_class_register(&timekeeping_sysclass);
+
+	if (!error)
+		error = sysdev_register(&device_timekeeping);
+	if (!error)
+		error = sysdev_create_file(
+				&device_timekeeping,
+				&attr_jiffies_cpu);
+	return error;
+}
+
+device_initcall(init_timekeeping_sysfs);
+#endif /* SYSFS */
+
 /**
  * Async notification about clocksource changes
  */
#5 lore raw
Quoted 48 lines · Dimitri Sivanich

On Fri, Sep 02, 2011 at 10:19:36AM +0200, Thomas Gleixner wrote:

kernel/time/tick-sched.c +47 -0
+#ifdef CONFIG_SYSFS
+/**
+ * sysfs_show_do_timer_cpu - sysfs interface for tick_do_timer_cpu
+ * @dev:        unused
+ * @buf:        char buffer where value of tick_do_timer_cpu is copied
+ *
+ * Provides sysfs interface for showing the current tick_do_timer_cpu.
+ */
+static ssize_t
+sysfs_show_do_timer_cpu(struct sys_device *dev,
+				struct sysdev_attribute *attr, char *buf)
+{
+	ssize_t count = 0;
+
+	count = snprintf(buf, PAGE_SIZE, "%d\n", tick_do_timer_cpu);
+
+	return count;
+}
+
+/**
+ * sysfs_override_do_timer_cpu - manually override tick_do_timer_cpu
+ * @dev:        unused
+ * @buf:        cpu number of desired tick_do_timer_cpu
+ * @count:      length of buffer
+ *
+ * Takes input from sysfs interface for manually overriding the selected
+ * tick_do_timer_cpu.  Only applicable when not running in nohz mode.
+ */
+static ssize_t
+sysfs_override_do_timer_cpu(struct sys_device *dev,
+					struct sysdev_attribute *attr,
+					const char *buf, size_t count)
+{
+	char b[16];
+	size_t ret = count;
+	int c;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+	/* strings from sysfs write are not 0 terminated! */
+	if (count >= sizeof(b))
+		return -EINVAL;
+
+	/* strip off \n: */

kernel/time/tick-sched.c void tick_cancel_sched_timer(int cpu) · line 875

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	if (buf[count-1] == '\n')
+		count--;
+	if (count < 1)
+		return -EINVAL;
+
+	memcpy(b, buf, count);
+	b[count] = 0;
+
+	if (sscanf(b, "%d", &c) != 1)
+		return -EINVAL;

Isn't there a function which parses sysfs writes into an integer ?

Thanks,

tglx

#6 lore raw
Quoted 53 lines · Thomas Gleixner
Quoted 52 lines · Dimitri Sivanich

On Fri, Sep 02, 2011 at 10:19:36AM +0200, Thomas Gleixner wrote:

kernel/time/tick-sched.c +51 -0
+#ifdef CONFIG_SYSFS
+/**
+ * sysfs_show_do_timer_cpu - sysfs interface for tick_do_timer_cpu
+ * @dev:        unused
+ * @buf:        char buffer where value of tick_do_timer_cpu is copied
+ *
+ * Provides sysfs interface for showing the current tick_do_timer_cpu.
+ */
+static ssize_t
+sysfs_show_do_timer_cpu(struct sys_device *dev,
+				struct sysdev_attribute *attr, char *buf)
+{
+	ssize_t count = 0;
+
+	count = snprintf(buf, PAGE_SIZE, "%d\n", tick_do_timer_cpu);
+
+	return count;
+}
+
+/**
+ * sysfs_override_do_timer_cpu - manually override tick_do_timer_cpu
+ * @dev:        unused
+ * @buf:        cpu number of desired tick_do_timer_cpu
+ * @count:      length of buffer
+ *
+ * Takes input from sysfs interface for manually overriding the selected
+ * tick_do_timer_cpu.  Only applicable when not running in nohz mode.
+ */
+static ssize_t
+sysfs_override_do_timer_cpu(struct sys_device *dev,
+					struct sysdev_attribute *attr,
+					const char *buf, size_t count)
+{
+	char b[16];
+	size_t ret = count;
+	int c;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+	/* strings from sysfs write are not 0 terminated! */
+	if (count >= sizeof(b))
+		return -EINVAL;
+
+	/* strip off \n: */
+	if (buf[count-1] == '\n')
+		count--;
+	if (count < 1)
+		return -EINVAL;
Quoted 9 lines

kernel/time/tick-sched.c void tick_cancel_sched_timer(int cpu) · line 875

Quoted 6 lines
kernel/time/tick-sched.c +6 -0
+
+	memcpy(b, buf, count);
+	b[count] = 0;
+
+	if (sscanf(b, "%d", &c) != 1)
+		return -EINVAL;

Isn't there a function which parses sysfs writes into an integer ?

There is sysdev_store_int(), but one would need to define a sysdev_ext_attribute specifying a global int (as 'var') to accept the value. We would not want that value to be tick_do_timer_cpu, however.

Maybe there's another function that I'm missing?

Given the value restrictions for tick_do_timer_cpu, would you agree that it seems simpler to leave it this way (or use some form of strtol) rather than using sysdev_store_int?