Skip to contentSkip to conversation

specific do_timer_cpu value for nohz off mode

PATCH In review Dimitri Sivanich 6 participants 27 messages Nov 8, 2011 – Mar 19, 2013

Subsystems (5)
x86 Scheduler Driver core Nohz, dynticks support +1 more subsystems
lore
Share & tools
lore

Dimitri Sivanich

author
#0 lore raw

PATCH 1/1 specific do_timer_cpu value for nohz off mode

Resending this.

Allow manual override of the tick_do_timer_cpu.

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.

Signed-off-by Dimitri Sivanich <[email protected]>
---
 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
===================================================================
@@ -834,6 +834,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
  */
#1 lore raw
Quoted 1 line · On Tue, 8 Nov 2011 13:11:49 -0600 Dimitri Sivanich <[email protected]> wrote:

Resending this.

Not enough times apparently :(

Quoted 7 lines

Allow manual override of the tick_do_timer_cpu.

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.

This doesn't really explain what the patch does. "override" with what? What effects can the user expect to see from doing the-action-which-you-didn't-describe?

Quoted 15 lines

===================================================================

kernel/time/tick-sched.c +8 -0
@@ -834,6 +834,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.

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+ */
+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;

Save some trees:

	return snprintf(buf, PAGE_SIZE, "%d\n", tick_do_timer_cpu);
Quoted 16 lines
kernel/time/tick-sched.c +16 -0
+}
+
+/**
+ * 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)
+{

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	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! */

hm. Aren't they?

Quoted 4 lines
kernel/time/tick-sched.c +4 -0
+	if (count >= sizeof(b))
+		return -EINVAL;
+
+	/* strip off \n: */

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

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;

You should be able to use strim() in here, and eliminate b[]. Not that the \n needed to be removed anyway! I think it's OK to modify the incoming memory for sysfs write handlers.

Also, kstrto*() should be used to detect and reject input of the form "42foo".

But surely we have a helper function somewhere to read an integer out of a sysfs-written buffer. If not, we should!

Quoted 7 lines
kernel/time/tick-sched.c +7 -0
+	if (!cpu_online(c))
+		return -EINVAL;
+
+	tick_do_timer_cpu = c;
+
+	return ret;
+}

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+
+/*
+ * 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",
+};

The patch shouod add some user-facing documentation for the user-facing feature.

Quoted 20 lines
kernel/time/tick-sched.c +20 -0
+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 */
author
#2 lore raw
Quoted 4 lines · Andrew Morton
Quoted 2 lines · On Tue, 8 Nov 2011 13:11:49 -0600 Dimitri Sivanich <[email protected]> wrote:

Allow manual override of the tick_do_timer_cpu.

Quoted 8 lines
Quoted 4 lines

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.

This doesn't really explain what the patch does. "override" with what? What effects can the user expect to see from doing the-action-which-you-didn't-describe?

Hopefully my new description and documentation are acceptable.

Quoted 11 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+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;

Save some trees:

Quoted 1 line
	return snprintf(buf, PAGE_SIZE, "%d\n", tick_do_timer_cpu);

The new patch below should address this by using the already existing 'sysdev_show_int' function.

Quoted 10 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+
+	if (sscanf(b, "%d", &c) != 1)
+		return -EINVAL;

You should be able to use strim() in here, and eliminate b[]. Not that the \n needed to be removed anyway! I think it's OK to modify the incoming memory for sysfs write handlers.

Also, kstrto*() should be used to detect and reject input of the form "42foo".

Using kstrtouint, see patch below.

Quoted 3 lines

But surely we have a helper function somewhere to read an integer out of a sysfs-written buffer. If not, we should!

Here I need more strict input checking for online cpu values.

Quoted 13 lines
Quoted 13 lines
(quoted diff) +12 -0
+	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,
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+		   sysfs_override_do_timer_cpu);
+
+static struct sysdev_class timekeeping_sysclass = {
+	.name = "timekeeping",
+};

The patch shouod add some user-facing documentation for the user-facing feature.

Hopefully documenting this in Documentation/ABI/testing is acceptable at this point(?). See patch below.

Show and modify the tick_do_timer_cpu via sysfs. This determines the cpu on which global time (jiffies) updates occur. Modification can only be done on systems with nohz mode turned off.

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.

Signed-off-by Dimitri Sivanich <[email protected]>
---
 Documentation/ABI/testing/sysfs-devices-system-timekeeping |   16 ++
 drivers/base/sys.c                                         |   10 -
 include/linux/sysdev.h                                     |    2 
 kernel/time/tick-sched.c                                   |   59 ++++++++++
 4 files changed, 81 insertions(+), 6 deletions(-)
include/linux/sysdev.h +2 -0
Index: linux/include/linux/sysdev.h
===================================================================
@@ -132,6 +132,8 @@ struct sysdev_ext_attribute {
 	void *var;
 };
 
+#define SYSDEV_TO_EXT_ATTR(x) container_of(x, struct sysdev_ext_attribute, attr)
+
 /*
  * Support for simple variable sysdev attributes.
  * The pointer to the variable is stored in a sysdev_ext_attribute
drivers/base/sys.c +4 -6
Index: linux/drivers/base/sys.c
===================================================================
@@ -339,13 +339,11 @@ int __init system_bus_init(void)
 	return 0;
 }
 
-#define to_ext_attr(x) container_of(x, struct sysdev_ext_attribute, attr)
-
 ssize_t sysdev_store_ulong(struct sys_device *sysdev,
 			   struct sysdev_attribute *attr,
 			   const char *buf, size_t size)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	char *end;
 	unsigned long new = simple_strtoul(buf, &end, 0);
 	if (end == buf)

@@ -360,7 +358,7 @@ ssize_t sysdev_show_ulong(struct sys_dev
 			  struct sysdev_attribute *attr,
 			  char *buf)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(sysdev_show_ulong);

@@ -369,7 +367,7 @@ ssize_t sysdev_store_int(struct sys_devi
 			   struct sysdev_attribute *attr,
 			   const char *buf, size_t size)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	char *end;
 	long new = simple_strtol(buf, &end, 0);
 	if (end == buf || new > INT_MAX || new < INT_MIN)

@@ -384,7 +382,7 @@ ssize_t sysdev_show_int(struct sys_devic
 			  struct sysdev_attribute *attr,
 			  char *buf)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(sysdev_show_int);
kernel/time/tick-sched.c +59 -0

65 lines not on this page · raw message

Documentation/ABI/testing/sysfs-devices-system-timekeeping new file +16 -0
Index: linux/Documentation/ABI/testing/sysfs-devices-system-timekeeping
===================================================================
@@ -0,0 +1,16 @@
+What:		/sys/devices/system/timekeeping/
+Date:		November 2011
+Contact:	Linux kernel mailing list <[email protected]>
+Description:	Timekeeping attributes
+
+
+What:		/sys/devices/system/timekeeping/timekeeping0/jiffies_cpu
+Date:		November 2011
+Contact:	Linux kernel mailing list <[email protected]>
+Description:	Show and modify the kernel's tick_do_timer_cpu.  This
+		determines the cpu on which global time (jiffies) updates
+		occur.  This can only be modified on systems running with
+		the nohz mode turned off (nohz=off).
+
+		Possible values are:
+			0 - <num online cpus>
#3 lore raw
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

#4 lore raw
Quoted 30 lines · On Wed, 30 Nov 2011 16:11:31 -0800 Andrew Morton <[email protected]>…
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

Quoted 5 lines

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

And the whole thing is racy, isn't it? The "new" CPU can go offline a nanosecond after we performed that test, so why perform it at all?

author
#5 lore raw
Quoted 33 lines · Andrew Morton
Quoted 30 lines · On Wed, 30 Nov 2011 16:11:31 -0800 Andrew Morton <[email protected]>…
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

Quoted 8 lines
Quoted 5 lines

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

And the whole thing is racy, isn't it? The "new" CPU can go offline a nanosecond after we performed that test, so why perform it at all?

See my email concerning the panic in cpu_online().

#6 lore raw
Quoted 4 lines · On Wed, 30 Nov 2011 20:07:27 -0600 Dimitri Sivanich <[email protected]> wrote:
Quoted 2 lines

And the whole thing is racy, isn't it? The "new" CPU can go offline a nanosecond after we performed that test, so why perform it at all?

See my email concerning the panic in cpu_online().

That doesn't address my question.

What's the point in checking cpu_online() when we have no locks to prevent the online map from changing?

What happens if this cpu goes offline immediately after that check has passed?

author
#7 lore raw
Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Wed, 30 Nov 2011 20:07:27 -0600 Dimitri Sivanich <[email protected]> wrote:
Quoted 2 lines

And the whole thing is racy, isn't it? The "new" CPU can go offline a nanosecond after we performed that test, so why perform it at all?

See my email concerning the panic in cpu_online().

Quoted 7 lines

That doesn't address my question.

What's the point in checking cpu_online() when we have no locks to prevent the online map from changing?

What happens if this cpu goes offline immediately after that check has passed?

Yes, there is some raciness here, and I see your point.

I've changed the patch to protect the operation. The version below should resolve this with get/put_online_cpus. It also checks input against nr_cpu_ids before checking cpu_online(), which is what for_each_present_cpu()->for_each_cpu() uses, and makes checkpatch.pl happy.

Show and modify the tick_do_timer_cpu via sysfs. This determines the cpu on which global time (jiffies) updates occur. Modification can only be done on systems with nohz mode turned off.

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.

Signed-off-by Dimitri Sivanich <[email protected]>
---
 Documentation/ABI/testing/sysfs-devices-system-timekeeping |   16 ++
 drivers/base/sys.c                                         |   10 -
 include/linux/sysdev.h                                     |    2 
 kernel/time/tick-sched.c                                   |   67 ++++++++++
 4 files changed, 89 insertions(+), 6 deletions(-)
include/linux/sysdev.h +2 -0
Index: linux/include/linux/sysdev.h
===================================================================
@@ -132,6 +132,8 @@ struct sysdev_ext_attribute {
 	void *var;
 };
 
+#define SYSDEV_TO_EXT_ATTR(x) container_of(x, struct sysdev_ext_attribute, attr)
+
 /*
  * Support for simple variable sysdev attributes.
  * The pointer to the variable is stored in a sysdev_ext_attribute
drivers/base/sys.c +4 -6
Index: linux/drivers/base/sys.c
===================================================================
@@ -339,13 +339,11 @@ int __init system_bus_init(void)
 	return 0;
 }
 
-#define to_ext_attr(x) container_of(x, struct sysdev_ext_attribute, attr)
-
 ssize_t sysdev_store_ulong(struct sys_device *sysdev,
 			   struct sysdev_attribute *attr,
 			   const char *buf, size_t size)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	char *end;
 	unsigned long new = simple_strtoul(buf, &end, 0);
 	if (end == buf)

@@ -360,7 +358,7 @@ ssize_t sysdev_show_ulong(struct sys_dev
 			  struct sysdev_attribute *attr,
 			  char *buf)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(sysdev_show_ulong);

@@ -369,7 +367,7 @@ ssize_t sysdev_store_int(struct sys_devi
 			   struct sysdev_attribute *attr,
 			   const char *buf, size_t size)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	char *end;
 	long new = simple_strtol(buf, &end, 0);
 	if (end == buf || new > INT_MAX || new < INT_MIN)

@@ -384,7 +382,7 @@ ssize_t sysdev_show_int(struct sys_devic
 			  struct sysdev_attribute *attr,
 			  char *buf)
 {
-	struct sysdev_ext_attribute *ea = to_ext_attr(attr);
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
 	return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
 }
 EXPORT_SYMBOL_GPL(sysdev_show_int);
kernel/time/tick-sched.c +67 -0

73 lines not on this page · raw message

Documentation/ABI/testing/sysfs-devices-system-timekeeping new file +16 -0
Index: linux/Documentation/ABI/testing/sysfs-devices-system-timekeeping
===================================================================
@@ -0,0 +1,16 @@
+What:		/sys/devices/system/timekeeping/
+Date:		November 2011
+Contact:	Linux kernel mailing list <[email protected]>
+Description:	Timekeeping attributes
+
+
+What:		/sys/devices/system/timekeeping/timekeeping0/jiffies_cpu
+Date:		November 2011
+Contact:	Linux kernel mailing list <[email protected]>
+Description:	Show and modify the kernel's tick_do_timer_cpu.  This
+		determines the cpu on which global time (jiffies) updates
+		occur.  This can only be modified on systems running with
+		the nohz mode turned off (nohz=off).
+
+		Possible values are:
+			0 - <num online cpus>
#8 lore raw
Quoted 22 lines · On Thu, 1 Dec 2011 10:37:40 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +22 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	/* Protect against cpu-hotplug */
+	get_online_cpus();
+
+	if (new >= nr_cpu_ids || !cpu_online(new)) {

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+		put_online_cpus();
+		return -ERANGE;
+	}
+
+	*(unsigned int *)(ea->var) = new;
+
+	put_online_cpus();
+
+	return size;
+}

OK, I think this fixes one race. We modify tick_do_timer_cpu inside get_online_cpus(). If that cpu goes offline then tick_handover_do_timer() will correctly hand the timer functions over to a new CPU, and tick_handover_do_timer() runs in the CPU hotplug handler which I assume is locked by get_online_cpus(). Please check all this.

Now, the above code can alter tick_do_timer_cpu while a timer interrupt is actually executing on another CPU. Will this disrupt aything? I think it might cause problems. If we take an interrupt on CPU 5 and that CPU enters tick_periodic() and another CPU alters tick_do_timer_cpu from 5 to 4 at exactly the correct time, tick_periodic() might fail to run do_timer(). Or it might run do_timer() on both CPUs 4 and 5 concurrently?

author
#9 lore raw
Quoted 34 lines · Andrew Morton
Quoted 31 lines · On Thu, 1 Dec 2011 10:37:40 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +31 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	/* Protect against cpu-hotplug */
+	get_online_cpus();
+
+	if (new >= nr_cpu_ids || !cpu_online(new)) {
+		put_online_cpus();
+		return -ERANGE;
+	}
+
+	*(unsigned int *)(ea->var) = new;
+
+	put_online_cpus();
+
+	return size;
Quoted 8 lines
Quoted 1 line

+}

OK, I think this fixes one race. We modify tick_do_timer_cpu inside get_online_cpus(). If that cpu goes offline then tick_handover_do_timer() will correctly hand the timer functions over to a new CPU, and tick_handover_do_timer() runs in the CPU hotplug handler which I assume is locked by get_online_cpus(). Please check all this.

Yes, _cpu_down() runs cpu_hotplug_begin(), which locks and holds the mutex that get_online_cpus() needs in order to update the refcount (cpu_hotplug_begin doesn't exit until refcount==0).

The notification that calls tick_handover_do_timer() is done in both the CPU_DYING and CPU_DYING_FROZEN (CPU_TASKS_FROZEN), but I believe this always comes from _cpu_down() in either case.

Quoted 9 lines

Now, the above code can alter tick_do_timer_cpu while a timer interrupt is actually executing on another CPU. Will this disrupt aything? I think it might cause problems. If we take an interrupt on CPU 5 and that CPU enters tick_periodic() and another CPU alters tick_do_timer_cpu from 5 to 4 at exactly the correct time, tick_periodic() might fail to run do_timer(). Or it might run do_timer() on both CPUs 4 and 5 concurrently?

Well, we do have to take the write_seqlock() in tick_periodic, so there's no danger of do_timer running exactly concurrently.

But yes, we may end up with 2 jiffies ticks occurring close together (when 5 runs do_timer while 4 waits for the seqlock), or we might end up missing a jiffies update for almost a full tick (when it changes from 5 to 4 immediately after 4 has done the 'tick_do_timer_cpu == cpu' check).

So at that time, we could be off +- almost a tick. The question is, how critical is that? When you down a cpu, the same sort of thing could happen via tick_handover_do_timer(), which itself does nothing more than change tick_do_timer_cpu.

author
#10 lore raw
Quoted 4 lines · Dimitri Sivanich

So at that time, we could be off +- almost a tick. The question is, how critical is that? When you down a cpu, the same sort of thing could happen via tick_handover_do_timer(), which itself does nothing more than change tick_do_timer_cpu.

I should clarify that by 'same sort of thing' I mean that the target cpu might've just gone through tick_periodic(), so we could miss a tick before the next update.

#11 lore raw
In reply to Dimitri Sivanich
Quoted 9 lines · Dimitri Sivanich

On Thu, Dec 01, 2011 at 02:56:23PM -0800, Andrew Morton wrote:
Well, we do have to take the write_seqlock() in tick_periodic, so there's no danger of do_timer running exactly concurrently.

But yes, we may end up with 2 jiffies ticks occurring close together (when 5 runs do_timer while 4 waits for the seqlock), or we might end up missing a jiffies update for almost a full tick (when it changes from 5 to 4 immediately after 4 has done the 'tick_do_timer_cpu == cpu' check).

Quoted 4 lines

So at that time, we could be off +- almost a tick. The question is, how critical is that? When you down a cpu, the same sort of thing could happen via tick_handover_do_timer(), which itself does nothing more than change tick_do_timer_cpu.

It's uncritical as long as you are not using clocksource=jiffies. With all other clocksources you just miss a jiffies update, which does not affect timekeeping at all. It just might expire your network timeout a jiffie earlier or later. So there is no damage to expect.

Thanks,

tglx

author
#12 lore raw
In reply to Andrew Morton
Quoted 30 lines · Andrew Morton
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

Quoted 1 line

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think a check against num_possible_cpus() should be OK.

Quoted 3 lines

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

A value > NR_CPUS and < MAX_INT caused a panic in sysfs_store_do_timer_cpu, presumably from the cpu_online() check. The check against NR_CPUS avoided the panic.

#13 lore raw
Quoted 31 lines · On Wed, 30 Nov 2011 20:06:31 -0600 Dimitri Sivanich <[email protected]> wrote:
Quoted 30 lines · Andrew Morton
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

Quoted 3 lines
Quoted 1 line

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think a check against num_possible_cpus() should be OK.

You want cpu_possible(). Or maybe cpu_present().

Quoted 7 lines
Quoted 3 lines

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

A value > NR_CPUS and < MAX_INT caused a panic in sysfs_store_do_timer_cpu, presumably from the cpu_online() check. The check against NR_CPUS avoided the panic.

OK. Well, it's not a panic:

static inline unsigned int cpumask_check(unsigned int cpu) {

#ifdef CONFIG_DEBUG_PER_CPU_MAPS
	WARN_ON_ONCE(cpu >= nr_cpumask_bits);
#endif /* CONFIG_DEBUG_PER_CPU_MAPS */
	return cpu;
}

so we can't do cpu_online(insane number)

author
#14 lore raw
Quoted 33 lines · Andrew Morton
Quoted 31 lines · On Wed, 30 Nov 2011 20:06:31 -0600 Dimitri Sivanich <[email protected]> wrote:
Quoted 30 lines · Andrew Morton
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think a check against num_possible_cpus() should be OK.

You want cpu_possible(). Or maybe cpu_present().

May be splitting hairs, but I think I like num_present_cpus even better.

Quoted 10 lines
Quoted 8 lines
Quoted 4 lines

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

A value > NR_CPUS and < MAX_INT caused a panic in sysfs_store_do_timer_cpu, presumably from the cpu_online() check. The check against NR_CPUS avoided the panic.

OK. Well, it's not a panic:

Actually, I didn't have CONFIG_DEBUG_PER_CPU_MAPS turned on.

Quoted 11 lines

static inline unsigned int cpumask_check(unsigned int cpu) {

#ifdef CONFIG_DEBUG_PER_CPU_MAPS
	WARN_ON_ONCE(cpu >= nr_cpumask_bits);
#endif /* CONFIG_DEBUG_PER_CPU_MAPS */
	return cpu;
}

so we can't do cpu_online(insane number)

#15 lore raw
Quoted 34 lines · On Wed, 30 Nov 2011 20:34:23 -0600 Dimitri Sivanich <[email protected]> wrote:
Quoted 33 lines · Andrew Morton
Quoted 31 lines · On Wed, 30 Nov 2011 20:06:31 -0600 Dimitri Sivanich <[email protected]> wrote:
Quoted 30 lines · Andrew Morton
Quoted 14 lines · On Wed, 30 Nov 2011 09:29:59 -0600 Dimitri Sivanich <[email protected]> wrote:
kernel/time/tick-sched.c +14 -0
+static ssize_t sysfs_store_do_timer_cpu(struct sys_device *dev,
+						struct sysdev_attribute *attr,
+						const char *buf, size_t size)
+{
+	struct sysdev_ext_attribute *ea = SYSDEV_TO_EXT_ATTR(attr);
+	unsigned int new;
+	int rv;
+
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif
+

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

Quoted 10 lines
kernel/time/tick-sched.c +10 -0
+	rv = kstrtouint(buf, 0, &new);
+	if (rv)
+		return rv;
+
+	if (new >= NR_CPUS || !cpu_online(new))
+		return -ERANGE;
+
+	*(unsigned int *)(ea->var) = new;
+	return size;
+}

checkpatch tells us:

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc

I think a check against num_possible_cpus() should be OK.

You want cpu_possible(). Or maybe cpu_present().

May be splitting hairs, but I think I like num_present_cpus even better.

Does that work correctly if the cpu-present map has holes?

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

I think the check can just be removed? Surely cpu_online(1000000000) will return false?

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

A value > NR_CPUS and < MAX_INT caused a panic in sysfs_store_do_timer_cpu, presumably from the cpu_online() check. The check against NR_CPUS avoided the panic.

OK. Well, it's not a panic:

Actually, I didn't have CONFIG_DEBUG_PER_CPU_MAPS turned on.

So why did it crash? test_bit() far out of range?

#16 lore raw
In reply to Dimitri Sivanich
Quoted 4 lines · Dimitri Sivanich

Resending this.

Allow manual override of the tick_do_timer_cpu.

Bigger button below.

Review

Quoted 11 lines

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 diff) +5 -0
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif

Uhuh, we have something in common, your HPC folks don't like NO_HZ because it makes loads of jitter, my RT jitter test proggy hates it to pieces for the same reason. I can't just config it out like you though.

Aside: how come your HPC folks aren't griping about (SGI monster) boxen ticking all at the same time? That makes my 64 core box jitter plenty.

Anyway, if you boot one of your monster boxen nohz=off, or in this case, just don't build with NO_HZ, the electric meter spins a lot faster than with NO_HZ, yes? So I wonder if something like the below would help. Seems to me cpusets is the right place to do this kind of tweaking, though this particular patch may be eligible for an "award" or two :)

Turning NO_HZ off is probably not as good as just building without NO_HZ, but for me, running an all pinned RT load in an isolated set and distro config, I can switch an isolated cpuset to "sched_hpc", and only the part of the box that NEEDS to kill NO_HZ does so, RT test proggy becomes happy without booting nohz=off, and the partition can resume green machine mode when done doing whatever required turning NO_HZ off, and in my case rt push/pull as well to further improve jitter, since it's not only a jitter source, but a waste of cycles for a 100% pinned and isolated load.

Poke sched_hpc, CPU0 starts ticking, becomes jiffies knave (you need), isolated partition also starts ticking (we both need). Or, poke sched_hpc_rt, it'll do that and turn rt push/pull off as well, which I don't desperately need, but it does cut remaining jitter in half.

There's space for other "isolate me harder" buttons, though one would hope the space would never be used.. these two are ugly enough. I couldn't figure out how to make it any prettier.

---
 include/linux/sched.h    |   29 ++++++
 init/Kconfig             |   11 ++
 kernel/cpuset.c          |  203 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/sched/core.c      |   95 +++++++++++++++++++++
 kernel/sched/fair.c      |    4 
 kernel/sched/rt.c        |   18 +++-
 kernel/sched/sched.h     |   17 +++
 kernel/time/tick-sched.c |    2 
 8 files changed, 372 insertions(+), 7 deletions(-)
include/linux/sched.h +29 -0
@@ -271,6 +271,35 @@ extern void init_idle_bootup_task(struct
 
 extern int runqueue_is_locked(int cpu);
 
+/* Cpuset runqueue behavior modifier bits */
+enum
+{
+	RQ_TICK=0,
+	RQ_HPC,
+	RQ_HPCRT,
+	RQ_CLEAR=~0,
+};
+
+#ifdef CONFIG_HPC_CPUSETS
+extern int runqueue_is_flagged(int cpu, int nr);
+extern int runqueue_is_isolated(int cpu);
+extern void cpuset_flags_set(int cpu, unsigned bits);
+extern void cpuset_flags_clr(int cpu, unsigned bits);
+
+#ifdef CONFIG_NO_HZ
+static inline int sched_needs_cpu(int cpu)
+{
+	return runqueue_is_flagged(cpu, RQ_TICK);
+}
+#endif
+#else /* !CONFIG_HPC_CPUSETS */
+static inline int runqueue_is_flagged(int cpu, int nr) { return 0; }
+static inline int runqueue_is_isolated(int cpu) { return 0; }
+static inline int sched_needs_cpu(int cpu) { return 0; }
+static inline void cpuset_flag_set(int cpu, unsigned bits) { }
+static inline void cpuset_flag_clr(int cpu, unsigned bits) { }
+#endif /* CONFIG_HPC_CPUSETS */
+
 #if defined(CONFIG_SMP) && defined(CONFIG_NO_HZ)
 extern void select_nohz_load_balancer(int stop_tick);
 extern void set_cpu_sd_state_idle(void);
init/Kconfig +11 -0
@@ -624,6 +624,17 @@ config PROC_PID_CPUSET
 	depends on CPUSETS
 	default y
 
+config HPC_CPUSETS
+	bool "HPC cpusets"
+	depends on CPUSETS && SMP
+	default n
+	help
+	  This option provides per CPUSET scheduler behavior control switches.
+	  This is primarily useful on large SMP systems where some partitions
+	  may be dedicated to sensitive HPC applications, while others are not.
+
+	  Say N if unsure.
+
 config CGROUP_CPUACCT
 	bool "Simple CPU accounting cgroup subsystem"
 	help
kernel/cpuset.c +203 -0

283 lines not on this page · raw message

kernel/sched/core.c +93 -2

125 lines not on this page · raw message

kernel/sched/fair.c +4 -0
@@ -4620,6 +4620,9 @@ void idle_balance(int this_cpu, struct r
 	int pulled_task = 0;
 	unsigned long next_balance = jiffies + HZ;
 
+	if (!this_rq->sd)
+		return;
+
 	this_rq->idle_stamp = this_rq->clock;
 
 	if (this_rq->avg_idle < sysctl_sched_migration_cost)

@@ -4914,6 +4917,7 @@ void select_nohz_load_balancer(int stop_
 	}
 	return;
 }
+
 #endif
 
 static DEFINE_SPINLOCK(balancing);
kernel/sched/rt.c +14 -4
@@ -917,8 +917,13 @@ inc_rt_prio_smp(struct rt_rq *rt_rq, int
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
-	if (rq->online && prio < prev_prio)
-		cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
+	if (!rq->online || prio >= prev_prio)
+		return;
+
+	if (rq_flag(rq, SCHED_HPCRT))
+		return;
+
+	cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
 }
 
 static void

@@ -926,8 +931,13 @@ dec_rt_prio_smp(struct rt_rq *rt_rq, int
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
-	if (rq->online && rt_rq->highest_prio.curr != prev_prio)
-		cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
+	if (!rq->online || rt_rq->highest_prio.curr == prev_prio)
+		return;
+
+	if (rq_flag(rq, SCHED_HPCRT))
+		return;
+
+	cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
 }
 
 #else /* CONFIG_SMP */
kernel/sched/sched.h +17 -0
@@ -424,6 +424,11 @@ struct rq {
 	int cpu;
 	int online;
 
+#ifdef CONFIG_CPUSETS
+	/* cpuset scheduler modifiers */
+	unsigned int cpuset_flags;
+#endif
+
 	u64 rt_avg;
 	u64 age_stamp;
 	u64 idle_stamp;

@@ -539,6 +544,18 @@ DECLARE_PER_CPU(int, sd_llc_id);
 
 #endif /* CONFIG_SMP */
 
+#ifdef CONFIG_HPC_CPUSETS
+static inline int rq_flag(struct rq *rq, int nr)
+{
+	return rq->cpuset_flags & (1 << nr);
+}
+#else
+static inline int rq_flag(struct rq *rq, int nr)
+{
+	return 0;
+}
+#endif
+
 #include "stats.h"
 #include "auto_group.h"
kernel/time/tick-sched.c +1 -1
@@ -327,7 +327,7 @@ static void tick_nohz_stop_sched_tick(st
 	} while (read_seqretry(&xtime_lock, seq));
 
 	if (rcu_needs_cpu(cpu) || printk_needs_cpu(cpu) ||
-	    arch_needs_cpu(cpu)) {
+	    arch_needs_cpu(cpu) || sched_needs_cpu(cpu)) {
 		next_jiffies = last_jiffies + 1;
 		delta_jiffies = 1;
 	} else {
#17 lore raw
Quoted 24 lines · Mike Galbraith
Quoted 4 lines · Dimitri Sivanich

Resending this.

Allow manual override of the tick_do_timer_cpu.

Bigger button below.

Review

Quoted 11 lines

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 diff) +5 -0
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif

Uhuh, we have something in common, your HPC folks don't like NO_HZ because it makes loads of jitter, my RT jitter test proggy hates it to pieces for the same reason. I can't just config it out like you though.

Quoted 2 lines

Aside: how come your HPC folks aren't griping about (SGI monster) boxen ticking all at the same time? That makes my 64 core box jitter plenty.

P.S.

Using SGI and RT in the same message reminded me. I have the below in my 3.0-rt tree, and 32 core UV100 box works fine. Wish I had a UV2 to try out, the old (allegedly single digit s/n) UV1 is kinda slow. Anyway, this patch is against mainline + preempt_rt FWIW.

rt,UV: rt conversion

Signed-off-by Mike Galbraith <[email protected]>
---
 arch/x86/include/asm/uv/uv_bau.h   |   12 ++++++------
 arch/x86/kernel/apic/x2apic_uv_x.c |    6 +++---
 arch/x86/platform/uv/tlb_uv.c      |   18 +++++++++---------
 arch/x86/platform/uv/uv_time.c     |   21 +++++++++++++--------
 4 files changed, 31 insertions(+), 26 deletions(-)
arch/x86/include/asm/uv/uv_bau.h +6 -6
@@ -518,8 +518,8 @@ struct bau_control {
 	unsigned short		uvhub_quiesce;
 	short			socket_acknowledge_count[DEST_Q_SIZE];
 	cycles_t		send_message;
-	spinlock_t		uvhub_lock;
-	spinlock_t		queue_lock;
+	raw_spinlock_t		uvhub_lock;
+	raw_spinlock_t		queue_lock;
 	/* tunables */
 	int			max_concurr;
 	int			max_concurr_const;

@@ -670,15 +670,15 @@ static inline int atom_asr(short i, stru
  * to be lowered below the current 'v'.  atomic_add_unless can only stop
  * on equal.
  */
-static inline int atomic_inc_unless_ge(spinlock_t *lock, atomic_t *v, int u)
+static inline int atomic_inc_unless_ge(raw_spinlock_t *lock, atomic_t *v, int u)
 {
-	spin_lock(lock);
+	raw_spin_lock(lock);
 	if (atomic_read(v) >= u) {
-		spin_unlock(lock);
+		raw_spin_unlock(lock);
 		return 0;
 	}
 	atomic_inc(v);
-	spin_unlock(lock);
+	raw_spin_unlock(lock);
 	return 1;
 }
arch/x86/kernel/apic/x2apic_uv_x.c +3 -3
@@ -56,7 +56,7 @@ int uv_min_hub_revision_id;
 EXPORT_SYMBOL_GPL(uv_min_hub_revision_id);
 unsigned int uv_apicid_hibits;
 EXPORT_SYMBOL_GPL(uv_apicid_hibits);
-static DEFINE_SPINLOCK(uv_nmi_lock);
+static DEFINE_RAW_SPINLOCK(uv_nmi_lock);
 
 static struct apic apic_x2apic_uv_x;
 

@@ -707,10 +707,10 @@ int uv_handle_nmi(unsigned int reason, s
 	 * Use a lock so only one cpu prints at a time.
 	 * This prevents intermixed output.
 	 */
-	spin_lock(&uv_nmi_lock);
+	raw_spin_lock(&uv_nmi_lock);
 	pr_info("UV NMI stack dump cpu %u:\n", smp_processor_id());
 	dump_stack();
-	spin_unlock(&uv_nmi_lock);
+	raw_spin_unlock(&uv_nmi_lock);
 
 	return NMI_HANDLED;
 }
arch/x86/platform/uv/tlb_uv.c +9 -9
@@ -39,7 +39,7 @@ static int timeout_base_ns[] = {
 static int timeout_us;
 static int nobau;
 static int baudisabled;
-static spinlock_t disable_lock;
+static raw_spinlock_t disable_lock;
 static cycles_t congested_cycles;
 
 /* tunables: */

@@ -608,9 +608,9 @@ static void destination_plugged(struct b
 
 		quiesce_local_uvhub(hmaster);
 
-		spin_lock(&hmaster->queue_lock);
+		raw_spin_lock(&hmaster->queue_lock);
 		reset_with_ipi(&bau_desc->distribution, bcp);
-		spin_unlock(&hmaster->queue_lock);
+		raw_spin_unlock(&hmaster->queue_lock);
 
 		end_uvhub_quiesce(hmaster);
 

@@ -630,9 +630,9 @@ static void destination_timeout(struct b
 
 		quiesce_local_uvhub(hmaster);
 
-		spin_lock(&hmaster->queue_lock);
+		raw_spin_lock(&hmaster->queue_lock);
 		reset_with_ipi(&bau_desc->distribution, bcp);
-		spin_unlock(&hmaster->queue_lock);
+		raw_spin_unlock(&hmaster->queue_lock);
 
 		end_uvhub_quiesce(hmaster);
 

@@ -649,7 +649,7 @@ static void disable_for_congestion(struc
 					struct ptc_stats *stat)
 {
 	/* let only one cpu do this disabling */
-	spin_lock(&disable_lock);
+	raw_spin_lock(&disable_lock);
 
 	if (!baudisabled && bcp->period_requests &&
 	    ((bcp->period_time / bcp->period_requests) > congested_cycles)) {

@@ -668,7 +668,7 @@ static void disable_for_congestion(struc
 		}
 	}
 
-	spin_unlock(&disable_lock);
+	raw_spin_unlock(&disable_lock);
 }
 
 static void count_max_concurr(int stat, struct bau_control *bcp,

@@ -717,7 +717,7 @@ static void record_send_stats(cycles_t t
  */
 static void uv1_throttle(struct bau_control *hmaster, struct ptc_stats *stat)
 {
-	spinlock_t *lock = &hmaster->uvhub_lock;
+	raw_spinlock_t *lock = &hmaster->uvhub_lock;
 	atomic_t *v;
 
 	v = &hmaster->active_descriptor_count;

@@ -1835,7 +1835,7 @@ static int __init uv_bau_init(void)
 	}
 
 	nuvhubs = uv_num_possible_blades();
-	spin_lock_init(&disable_lock);
+	raw_spin_lock_init(&disable_lock);
 	congested_cycles = usec_2_cycles(congested_respns_us);
 
 	uv_base_pnode = 0x7fffffff;
arch/x86/platform/uv/uv_time.c +13 -8
@@ -58,7 +58,7 @@ static DEFINE_PER_CPU(struct clock_event
 
 /* There is one of these allocated per node */
 struct uv_rtc_timer_head {
-	spinlock_t	lock;
+	raw_spinlock_t	lock;
 	/* next cpu waiting for timer, local node relative: */
 	int		next_cpu;
 	/* number of cpus on this node: */

@@ -178,7 +178,7 @@ static __init int uv_rtc_allocate_timers
 				uv_rtc_deallocate_timers();
 				return -ENOMEM;
 			}
-			spin_lock_init(&head->lock);
+			raw_spin_lock_init(&head->lock);
 			head->ncpus = uv_blade_nr_possible_cpus(bid);
 			head->next_cpu = -1;
 			blade_info[bid] = head;

@@ -232,7 +232,7 @@ static int uv_rtc_set_timer(int cpu, u64
 	unsigned long flags;
 	int next_cpu;
 
-	spin_lock_irqsave(&head->lock, flags);
+	raw_spin_lock_irqsave(&head->lock, flags);
 
 	next_cpu = head->next_cpu;
 	*t = expires;

@@ -244,12 +244,12 @@ static int uv_rtc_set_timer(int cpu, u64
 		if (uv_setup_intr(cpu, expires)) {
 			*t = ULLONG_MAX;
 			uv_rtc_find_next_timer(head, pnode);
-			spin_unlock_irqrestore(&head->lock, flags);
+			raw_spin_unlock_irqrestore(&head->lock, flags);
 			return -ETIME;
 		}
 	}
 
-	spin_unlock_irqrestore(&head->lock, flags);
+	raw_spin_unlock_irqrestore(&head->lock, flags);
 	return 0;
 }
 

@@ -268,7 +268,7 @@ static int uv_rtc_unset_timer(int cpu, i
 	unsigned long flags;
 	int rc = 0;
 
-	spin_lock_irqsave(&head->lock, flags);
+	raw_spin_lock_irqsave(&head->lock, flags);
 
 	if ((head->next_cpu == bcpu && uv_read_rtc(NULL) >= *t) || force)
 		rc = 1;

@@ -280,7 +280,7 @@ static int uv_rtc_unset_timer(int cpu, i
 			uv_rtc_find_next_timer(head, pnode);
 	}
 
-	spin_unlock_irqrestore(&head->lock, flags);
+	raw_spin_unlock_irqrestore(&head->lock, flags);
 
 	return rc;
 }

@@ -300,13 +300,18 @@ static int uv_rtc_unset_timer(int cpu, i
 static cycle_t uv_read_rtc(struct clocksource *cs)
 {
 	unsigned long offset;
+	cycle_t cycles;
 
+	migrate_disable();
 	if (uv_get_min_hub_revision_id() == 1)
 		offset = 0;
 	else
 		offset = (uv_blade_processor_id() * L1_CACHE_BYTES) % PAGE_SIZE;
 
-	return (cycle_t)uv_read_local_mmr(UVH_RTC | offset);
+	cycles = (cycle_t)uv_read_local_mmr(UVH_RTC | offset);
+	migrate_enable();
+
+	return cycles;
 }
 
 /*
#18 lore raw
In reply to Mike Galbraith
Quoted 22 lines · Mike Galbraith
Quoted 4 lines · Dimitri Sivanich

Resending this.

Allow manual override of the tick_do_timer_cpu.

Bigger button below.

kernel/sched/core.c +13 -0
+/* Called with cgroup_mutex held */
+void cpuset_flags_set(int cpu, unsigned bits)
+{
+	struct rq *rq = cpu_rq(cpu);
+	unsigned long flags;
+	int nr;
+
+	raw_spin_lock_irqsave(&rq->lock, flags);
+	/* Set blocker flags before taking any action */
+	rq->cpuset_flags |= bits;
+	for (nr = 0; bits; nr++) {
+		if (!(bits & (1 << nr)))
+			continue;

kernel/sched/core.c void thread_group_times(struct task_stru · line 3019

Quoted 10 lines
kernel/sched/core.c +10 -0
+		switch (nr) {
+		case RQ_TICK:
+			break;
+		case RQ_HPC:
+			/* Ensure that jiffies doesn't go stale */
+			if (!nr_hpc_cpus++) {
+				tick_do_timer_cpu = 0;
+				/* safe, CPU0 is modifier excluded */
+				cpuset_flags_set(0, 1 << RQ_TICK);
+				wake_up_idle_cpu(0);

Just in case someone was going to mention it, I'd already moved that wakeup_idle_cpu() to RQ_TICK case, but the darn thing crawled back.

-Mike

#19 lore raw
In reply to Mike Galbraith
Quoted 14 lines · Mike Galbraith
Quoted 4 lines · Dimitri Sivanich

Resending this.

Allow manual override of the tick_do_timer_cpu.

Bigger button below.

Quoted 4 lines

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 9 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#ifdef CONFIG_NO_HZ
+	/* nohz mode not supported */
+	if (tick_nohz_enabled)
+		return -EINVAL;
+#endif

Uhuh, we have something in common, your HPC folks don't like NO_HZ because it makes loads of jitter, my RT jitter test proggy hates it to pieces for the same reason. I can't just config it out like you though....

Not expecting any enthusiasm, but this is _one_ way to let nohz=off go away, and gives a little more control to users who have to provide a home for jitter intolerant applications.

It's not very pretty, but is pretty convenient.

sched, cpusets: "HPC" cpusets extension

Give the user the ability to dynamically influence scheduler behavior through "HPC" cpusets.

When enabled, the user can dynamically inform the scheduler that a cpuset cannot tolerate jitter induced by NO_HZ, jiffies update, and RT load balancing locic. A large generic machine can re-partition to service transient jitter sensitive loads without requiring the entire machine to run nohz=off continuously.

Should the user invalidate "HPC" prerequisites, modifiers are self canceling for safety reasons. Prerequisites are: the set may not contain CPU0, must be cpu exclusive (obviously), and must be fully disconnected from scheduler domains.

Signed-off-by Mike Galbraith <[email protected]>
---
 include/linux/sched.h    |   29 +++++
 init/Kconfig             |   11 ++
 kernel/cpuset.c          |  245 ++++++++++++++++++++++++++++++++++++++++++++++-
 kernel/sched/core.c      |   94 +++++++++++++++++-
 kernel/sched/rt.c        |   18 ++-
 kernel/sched/sched.h     |   15 ++
 kernel/time/tick-sched.c |    6 -
 7 files changed, 407 insertions(+), 11 deletions(-)
include/linux/sched.h +29 -0
@@ -271,6 +271,35 @@ extern void init_idle_bootup_task(struct
 
 extern int runqueue_is_locked(int cpu);
 
+/* Cpuset runqueue behavior modifier flags */
+enum
+{
+	RQ_TICK		= (1 << 0),
+	RQ_HPC		= (1 << 1),
+	RQ_HPCRT	= (1 << 2),
+	RQ_CLEAR	= ~0,
+};
+
+#ifdef CONFIG_HPC_CPUSETS
+extern int runqueue_is_flagged(int cpu, unsigned flag);
+extern int runqueue_is_isolated(int cpu);
+extern void cpuset_flags_set(int cpu, unsigned bits);
+extern void cpuset_flags_clr(int cpu, unsigned bits);
+
+#ifdef CONFIG_NO_HZ
+static inline int sched_needs_cpu(int cpu)
+{
+	return runqueue_is_flagged(cpu, RQ_TICK);
+}
+#endif
+#else /* !CONFIG_HPC_CPUSETS */
+static inline int runqueue_is_flagged(int cpu, int nr) { return 0; }
+static inline int runqueue_is_isolated(int cpu) { return 0; }
+static inline int sched_needs_cpu(int cpu) { return 0; }
+static inline void cpuset_flag_set(int cpu, unsigned bits) { }
+static inline void cpuset_flag_clr(int cpu, unsigned bits) { }
+#endif /* CONFIG_HPC_CPUSETS */
+
 #if defined(CONFIG_SMP) && defined(CONFIG_NO_HZ)
 extern void select_nohz_load_balancer(int stop_tick);
 extern void set_cpu_sd_state_idle(void);
init/Kconfig +11 -0
@@ -638,6 +638,17 @@ config PROC_PID_CPUSET
 	depends on CPUSETS
 	default y
 
+config HPC_CPUSETS
+	bool "HPC cpusets"
+	depends on CPUSETS && SMP
+	default n
+	help
+	  This option provides per CPUSET scheduler behavior control switches.
+	  This is primarily useful on large SMP systems where some partitions
+	  may be dedicated to sensitive HPC applications, while others are not.
+
+	  Say N if unsure.
+
 config CGROUP_CPUACCT
 	bool "Simple CPU accounting cgroup subsystem"
 	help
kernel/cpuset.c +243 -2

333 lines not on this page · raw message

kernel/sched/core.c +92 -2

124 lines not on this page · raw message

kernel/sched/rt.c +14 -4
@@ -917,8 +917,13 @@ inc_rt_prio_smp(struct rt_rq *rt_rq, int
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
-	if (rq->online && prio < prev_prio)
-		cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
+	if (!rq->online || prio >= prev_prio)
+		return;
+
+	if (rq_flag(rq, RQ_HPCRT))
+		return;
+
+	cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
 }
 
 static void

@@ -926,8 +931,13 @@ dec_rt_prio_smp(struct rt_rq *rt_rq, int
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
-	if (rq->online && rt_rq->highest_prio.curr != prev_prio)
-		cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
+	if (!rq->online || rt_rq->highest_prio.curr == prev_prio)
+		return;
+
+	if (rq_flag(rq, RQ_HPCRT))
+		return;
+
+	cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
 }
 
 #else /* CONFIG_SMP */
kernel/sched/sched.h +15 -0
@@ -419,6 +419,9 @@ struct rq {
 	int post_schedule;
 	int active_balance;
 	int push_cpu;
+#ifdef CONFIG_CPUSETS
+	unsigned int cpuset_flags;
+#endif
 	struct cpu_stop_work active_balance_work;
 	/* cpu of this runqueue: */
 	int cpu;

@@ -539,6 +542,18 @@ DECLARE_PER_CPU(int, sd_llc_id);
 
 #endif /* CONFIG_SMP */
 
+#ifdef CONFIG_HPC_CPUSETS
+static inline int rq_flag(struct rq *rq, unsigned flag)
+{
+	return rq->cpuset_flags & flag;
+}
+#else
+static inline int rq_flag(struct rq *rq, unsigned flag)
+{
+	return 0;
+}
+#endif
+
 #include "stats.h"
 #include "auto_group.h"
kernel/time/tick-sched.c +3 -3
@@ -303,9 +303,6 @@ static void tick_nohz_stop_sched_tick(st
 	if (unlikely(ts->nohz_mode == NOHZ_MODE_INACTIVE))
 		return;
 
-	if (need_resched())
-		return;
-
 	if (unlikely(local_softirq_pending() && cpu_online(cpu))) {
 		static int ratelimit;
 

@@ -317,6 +314,9 @@ static void tick_nohz_stop_sched_tick(st
 		return;
 	}
 
+	if (need_resched() || sched_needs_cpu(cpu))
+		return;
+
 	ts->idle_calls++;
 	/* Read jiffies and the time when jiffies were updated last */
 	do {
#20 lore raw
In reply to Dimitri Sivanich
Quoted 7 lines · Dimitri Sivanich

Allow manual override of the tick_do_timer_cpu.

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.

I really wonder about this changelog. The only case where jiffies updates bounces around is the NOHZ case. In all other modes (periodic or highres) the boot cpu gets the do_timer() duty and it's never assigned to any other cpu.

So what's the point of this exercise? Moving it away from CPU0 for acedemic reasons or what?

Thanks,

tglx

author
#21 lore raw
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Dimitri Sivanich

Allow manual override of the tick_do_timer_cpu.

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 8 lines

I really wonder about this changelog. The only case where jiffies updates bounces around is the NOHZ case. In all other modes (periodic or highres) the boot cpu gets the do_timer() duty and it's never assigned to any other cpu.

So what's the point of this exercise? Moving it away from CPU0 for acedemic reasons or what?

I wasn't specifically trying to move it away from CPU0 (having jiffies updates on CPU0 was and would be just fine for the nohz=off case). The issue was that the tick_do_timer_cpu could be any cpu even in the nohz=off case (maybe something has changed that since?). After the point of assignment it is static, but you never know which cpu it is.

#22 lore raw
Quoted 15 lines · Dimitri Sivanich
Quoted 14 lines · Thomas Gleixner
Quoted 7 lines · Dimitri Sivanich

Allow manual override of the tick_do_timer_cpu.

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.

I really wonder about this changelog. The only case where jiffies updates bounces around is the NOHZ case. In all other modes (periodic or highres) the boot cpu gets the do_timer() duty and it's never assigned to any other cpu.

Quoted 8 lines
Quoted 3 lines

So what's the point of this exercise? Moving it away from CPU0 for acedemic reasons or what?

I wasn't specifically trying to move it away from CPU0 (having jiffies updates on CPU0 was and would be just fine for the nohz=off case). The issue was that the tick_do_timer_cpu could be any cpu even in the nohz=off case (maybe something has changed that since?). After the point of assignment it is static, but you never know which cpu it is.

It's always the boot cpu and that has been there from day one of that code.

tick_setup_device()

{
	/*
	 * First device setup ?
	 */
	if (!td->evtdev) {
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (tick_do_timer_cpu == TICK_DO_TIMER_BOOT) {
			tick_do_timer_cpu = cpu;

So the first CPU which registers a clock event device takes it. That's the boot CPU, no matter what.

Thanks,

tglx

author
#23 lore raw
Quoted 41 lines · Thomas Gleixner
Quoted 15 lines · Dimitri Sivanich
Quoted 14 lines · Thomas Gleixner
Quoted 7 lines · Dimitri Sivanich

Allow manual override of the tick_do_timer_cpu.

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.

I really wonder about this changelog. The only case where jiffies updates bounces around is the NOHZ case. In all other modes (periodic or highres) the boot cpu gets the do_timer() duty and it's never assigned to any other cpu.

Quoted 8 lines
Quoted 3 lines

So what's the point of this exercise? Moving it away from CPU0 for acedemic reasons or what?

I wasn't specifically trying to move it away from CPU0 (having jiffies updates on CPU0 was and would be just fine for the nohz=off case). The issue was that the tick_do_timer_cpu could be any cpu even in the nohz=off case (maybe something has changed that since?). After the point of assignment it is static, but you never know which cpu it is.

It's always the boot cpu and that has been there from day one of that code.

tick_setup_device()

{
	/*
	 * First device setup ?
	 */
	if (!td->evtdev) {
		/*
		 * If no cpu took the do_timer update, assign it to
		 * this cpu:
		 */
		if (tick_do_timer_cpu == TICK_DO_TIMER_BOOT) {
			tick_do_timer_cpu = cpu;
Quoted 3 lines

So the first CPU which registers a clock event device takes it. That's the boot CPU, no matter what.

Both kernel tracing and the original patch that I proposed for this showed plainly (at the time) that the tick_do_timer_cpu was not always cpu 0 prior to modifying it for nohz=off. Maybe that is no longer the case?

#24 lore raw
Quoted 7 lines · Dimitri Sivanich
Quoted 3 lines · Thomas Gleixner

So the first CPU which registers a clock event device takes it. That's the boot CPU, no matter what.

Both kernel tracing and the original patch that I proposed for this showed plainly (at the time) that the tick_do_timer_cpu was not always cpu 0 prior to modifying it for nohz=off. Maybe that is no longer the case?

This logic has not been changed in years.

tick_do_timer_cpu is initialized to TICK_DO_TIMER_BOOT and the first cpu which registers either a global or a per cpu clock event device takes it over. This is at least on x86 always the boot cpu, i.e. cpu0. After that point nothing touches that variable when nohz is disabled (runtime or compile time).

So I really want to see proper proof why that would not be the case. If it really happens then we fix the root cause instead of adding random sysfs interfaces.

Thanks,

tglx

author
#25 lore raw
Quoted 10 lines · Thomas Gleixner
Quoted 7 lines · Dimitri Sivanich
Quoted 3 lines · Thomas Gleixner

So the first CPU which registers a clock event device takes it. That's the boot CPU, no matter what.

Both kernel tracing and the original patch that I proposed for this showed plainly (at the time) that the tick_do_timer_cpu was not always cpu 0 prior to modifying it for nohz=off. Maybe that is no longer the case?

This logic has not been changed in years.

I did some tracing of all points where tick_do_timer_cpu is set in the 3.3.0-rc3+ kernel.

Quoted 6 lines

tick_do_timer_cpu is initialized to TICK_DO_TIMER_BOOT and the first cpu which registers either a global or a per cpu clock event device takes it over. This is at least on x86 always the boot cpu, i.e. cpu0. After that point nothing touches that variable when nohz is disabled (runtime or compile time).

At that point it is set to cpu 0. However, when we go into highres mode it does change. Below are the two places it was set during boot with nohz=off on one of our x86 based machines.

[    0.000000] tick_setup_device: tick_do_timer_cpu 0
[    1.924098] tick_broadcast_setup_oneshot: tick_do_timer_cpu 17

So in this example it's now cpu 17, and it stays that way from that point on.

A traceback at that point shows tick_init_highres is indeed initiating this:

[    1.924863]  [<ffffffff81087e01>] tick_broadcast_setup_oneshot+0x71/0x160
[    1.924863]  [<ffffffff81087f23>] tick_broadcast_switch_to_oneshot+0x33/0x50
[    1.924863]  [<ffffffff81088841>] tick_switch_to_oneshot+0x81/0xd0
[    1.924863]  [<ffffffff810888a0>] tick_init_highres+0x10/0x20
[    1.924863]  [<ffffffff81061e71>] hrtimer_run_pending+0x71/0xd0
Quoted 4 lines

So I really want to see proper proof why that would not be the case. If it really happens then we fix the root cause instead of adding random sysfs interfaces.

If you're willing to consider it a bug that tick_do_timer_cpu is not cpu 0, than I'm very much in agreement.

Quoted 4 lines

Thanks,

tglx

#26 lore raw

Hi,

following up on a very old thread:
http://thread.gmane.org/gmane.linux.kernel/1212777

Quoted 31 lines · Dimitri Sivanich
Quoted 10 lines · Thomas Gleixner
Quoted 7 lines · Dimitri Sivanich
Quoted 3 lines · Thomas Gleixner

So the first CPU which registers a clock event device takes it. That's the boot CPU, no matter what.

Both kernel tracing and the original patch that I proposed for this showed plainly (at the time) that the tick_do_timer_cpu was not always cpu 0 prior to modifying it for nohz=off. Maybe that is no longer the case?

This logic has not been changed in years.

I did some tracing of all points where tick_do_timer_cpu is set in the 3.3.0-rc3+ kernel.

Quoted 6 lines

tick_do_timer_cpu is initialized to TICK_DO_TIMER_BOOT and the first cpu which registers either a global or a per cpu clock event device takes it over. This is at least on x86 always the boot cpu, i.e. cpu0. After that point nothing touches that variable when nohz is disabled (runtime or compile time).

At that point it is set to cpu 0. However, when we go into highres mode it does change. Below are the two places it was set during boot with nohz=off on one of our x86 based machines.

[    0.000000] tick_setup_device: tick_do_timer_cpu 0
[    1.924098] tick_broadcast_setup_oneshot: tick_do_timer_cpu 17

So in this example it's now cpu 17, and it stays that way from that point on.

Quoted 12 lines

A traceback at that point shows tick_init_highres is indeed initiating this:

[    1.924863]  [<ffffffff81087e01>] tick_broadcast_setup_oneshot+0x71/0x160
[    1.924863]  [<ffffffff81087f23>] tick_broadcast_switch_to_oneshot+0x33/0x50
[    1.924863]  [<ffffffff81088841>] tick_switch_to_oneshot+0x81/0xd0
[    1.924863]  [<ffffffff810888a0>] tick_init_highres+0x10/0x20
[    1.924863]  [<ffffffff81061e71>] hrtimer_run_pending+0x71/0xd0
Quoted 4 lines

So I really want to see proper proof why that would not be the case. If it really happens then we fix the root cause instead of adding random sysfs interfaces.

As Dimitri wrote above, the switch from cpu 0 is done by tick_broadcast_setup_oneshot. The first CPU switching to highres takes the broadcast responsibility and also sets tick_do_timer_cpu to itself.

This behaviour has been introduced by 7300711e
(clockevents: broadcast fixup possible waiters).

I don't see a good reason assign tick_do_timer_cpu to the CPU doing the one-shot timer broadcasts. The timer interrupt will be generated on any other CPU as well, be it through the broadcast IPI or a per-CPU clockevent device. Any online CPU can do that job, so how about just dropping the assignment?

The do_timer() code should not suffer from the jitter introduced by the interrupt being generated by the broadcast, should it?

Signed-off-by Jiri Bohac <[email protected]>
kernel/time/tick-broadcast.c +0 -3
@@ -572,9 +572,6 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc)
 
 		bc->event_handler = tick_handle_oneshot_broadcast;
 
-		/* Take the do_timer update */
-		tick_do_timer_cpu = cpu;
-
 		/*
 		 * We must be careful here. There might be other CPUs
 		 * waiting for periodic broadcast. We need to set the
Signature · 2 lines
Jiri Bohac <[email protected]>
SUSE Labs, SUSE CZ