linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Add CONFIG_PERMANENT_CPU_TOPOLOGY [v2]
@ 2015-10-21 16:06 Prarit Bhargava
  2015-10-21 16:06 ` [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2] Prarit Bhargava
  2015-10-21 16:06 ` [PATCH 2/2] base, cpu, remove hotplugable_cpu_attr_groups Prarit Bhargava
  0 siblings, 2 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-10-21 16:06 UTC (permalink / raw)
  To: linux-kernel; +Cc: trenn, Prarit Bhargava

The information in /sys/devices/system/cpu/cpuX/topology
directory is useful for userspace monitoring applications and in-tree
utilities like cpupower & turbostat.

When down'ing a CPU the /sys/devices/system/cpu/cpuX/topology directory is
removed during the CPU_DEAD hotplug callback in the kernel.  The problem
with this model is that the CPU has not been physically removed and the
data in the topology directory is still valid.

This patchset adds CONFIG_PERMANENT_CPU_TOPOLOGY, and is Y by default for
x86, an N for all other arches.  When enabled the kernel is modified so
that the topology directory is added to the core cpu sysfs files so that
the topology directory exists for the lifetime of the CPU.  When
disabled, the behavior of the current kernel is maintained (that is, the
topology directory is removed on a down and added on an up).  Adding
CONFIG_PERMANENT_CPU_TOPOLOGY may require additional architecture so that
the cpumask data the CPU's topology is not cleared during a CPU down.

This patchset combines drivers/base/topology.c and drivers/base/cpu.c to
implement CONFIG_PERMANENT_CPU_TOPOLOGY, and leaves all arches except
x86 with the current behavior, and adds an additional minor cleanup to
drivers/base/cpu.c.

[v2]: core_siblings, and thread_siblings are "numa siblings that are online"
and "thread siblings that are online" and are used as such within the kernel.
They must be zero'd out when the CPU is offline.

Prarit Bhargava (2):
  cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology
    directory for lifetime of CPU [v2]
  base, cpu, remove hotplugable_cpu_attr_groups

 arch/x86/kernel/smpboot.c |    3 -
 drivers/base/Kconfig      |   13 ++++
 drivers/base/Makefile     |    2 +-
 drivers/base/cpu.c        |  141 ++++++++++++++++++++++++++++++++++++++---
 drivers/base/topology.c   |  155 ---------------------------------------------
 5 files changed, 146 insertions(+), 168 deletions(-)
 delete mode 100644 drivers/base/topology.c

-- 
1.7.9.3


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]
  2015-10-21 16:06 [PATCH 0/2] Add CONFIG_PERMANENT_CPU_TOPOLOGY [v2] Prarit Bhargava
@ 2015-10-21 16:06 ` Prarit Bhargava
  2015-10-25 19:15   ` kbuild test robot
  2015-10-27 15:50   ` Thomas Renninger
  2015-10-21 16:06 ` [PATCH 2/2] base, cpu, remove hotplugable_cpu_attr_groups Prarit Bhargava
  1 sibling, 2 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-10-21 16:06 UTC (permalink / raw)
  To: linux-kernel
  Cc: trenn, Prarit Bhargava, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, x86, Greg Kroah-Hartman, Borislav Petkov,
	Len Brown, Andy Lutomirski, Zhu Guihua, Denys Vlasenko,
	Jan H. Schönherr, Boris Ostrovsky, Paul E. McKenney

The information in /sys/devices/system/cpu/cpuX/topology
directory is useful for userspace monitoring applications and in-tree
utilities like cpupower & turbostat.

When down'ing a CPU the /sys/devices/system/cpu/cpuX/topology directory is
removed during the CPU_DEAD hotplug callback in the kernel.  The problem
with this model is that the CPU has not been physically removed and the
data in the topology directory is still valid.

This patch adds CONFIG_PERMANENT_CPU_TOPOLOGY, and is Y by default for
x86, an N for all other arches.  When enabled the kernel is modified so
that the topology directory is added to the core cpu sysfs files so that
the topology directory exists for the lifetime of the CPU.  When
disabled, the behavior of the current kernel is maintained (that is, the
topology directory is removed on a down and added on an up).  Adding
CONFIG_PERMANENT_CPU_TOPOLOGY may require additional architecture so that
the cpumask data the CPU's topology is not cleared during a CPU down.

This patch combines drivers/base/topology.c and drivers/base/cpu.c to
implement CONFIG_PERMANENT_CPU_TOPOLOGY, and leaves all arches except
x86 with the current behavior.

Before patch:

[root@hp-z620-01 ~]# grep ^ /sys/devices/system/cpu/cpu10/topology/*
/sys/devices/system/cpu/cpu10/topology/core_id:3
/sys/devices/system/cpu/cpu10/topology/core_siblings:ffff
/sys/devices/system/cpu/cpu10/topology/core_siblings_list:0-15
/sys/devices/system/cpu/cpu10/topology/physical_package_id:0
/sys/devices/system/cpu/cpu10/topology/thread_siblings:0404
/sys/devices/system/cpu/cpu10/topology/thread_siblings_list:2,10

Down a cpu

[root@hp-z620-01 ~]# echo 0 > /sys/devices/system/cpu/cpu10/online

[root@hp-z620-01 ~]# ls /sys/devices/system/cpu/cpu10/topology
ls: cannot access topology: No such file or directory

After patch:

[root@hp-z620-01 ~]# grep ^ /sys/devices/system/cpu/cpu10/topology/*
/sys/devices/system/cpu/cpu10/topology/core_id:3
/sys/devices/system/cpu/cpu10/topology/core_siblings:ffff
/sys/devices/system/cpu/cpu10/topology/core_siblings_list:0-15
/sys/devices/system/cpu/cpu10/topology/physical_package_id:0
/sys/devices/system/cpu/cpu10/topology/thread_siblings:0404
/sys/devices/system/cpu/cpu10/topology/thread_siblings_list:2,10

Down a cpu

[root@hp-z620-01 ~]# echo 0 > /sys/devices/system/cpu/cpu10/online

[root@hp-z620-01 ~]# grep ^ /sys/devices/system/cpu/cpu10/topology/*
/sys/devices/system/cpu/cpu10/topology/core_id:3
/sys/devices/system/cpu/cpu10/topology/core_siblings:0000
/sys/devices/system/cpu/cpu10/topology/core_siblings_list:
/sys/devices/system/cpu/cpu10/topology/physical_package_id:0
/sys/devices/system/cpu/cpu10/topology/thread_siblings:0000
/sys/devices/system/cpu/cpu10/topology/thread_siblings_list:

As can be seen, the core_id and physical_package_id files are left populated,
and indicate the physical location of the cpu.

I did some testing with and without BOOTPARAM_HOTPLUG_CPU0 enabled,
and up'd and down'd CPUs in sequence, randomly, by thread group, by
socket group and didn't see any issues.

[v2]: core_siblings, and thread_siblings are "numa siblings that are online"
and "thread siblings that are online" and are used as such within the kernel.
They must be zero'd out when the CPU is offline.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Len Brown <len.brown@intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: "Jan H. Schönherr" <jschoenh@amazon.de>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Thomas Renninger <trenn@suse.de>
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 arch/x86/kernel/smpboot.c |    3 -
 drivers/base/Kconfig      |   13 ++++
 drivers/base/Makefile     |    2 +-
 drivers/base/cpu.c        |  135 +++++++++++++++++++++++++++++++++++++++
 drivers/base/topology.c   |  155 ---------------------------------------------
 5 files changed, 149 insertions(+), 159 deletions(-)
 delete mode 100644 drivers/base/topology.c

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index e0c198e..949375f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1325,7 +1325,6 @@ __init void prefill_possible_map(void)
 static void remove_siblinginfo(int cpu)
 {
 	int sibling;
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
 	for_each_cpu(sibling, topology_core_cpumask(cpu)) {
 		cpumask_clear_cpu(cpu, topology_core_cpumask(sibling));
@@ -1343,8 +1342,6 @@ static void remove_siblinginfo(int cpu)
 	cpumask_clear(cpu_llc_shared_mask(cpu));
 	cpumask_clear(topology_sibling_cpumask(cpu));
 	cpumask_clear(topology_core_cpumask(cpu));
-	c->phys_proc_id = 0;
-	c->cpu_core_id = 0;
 	cpumask_clear_cpu(cpu, cpu_sibling_setup_mask);
 }
 
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 98504ec..321d261 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -324,4 +324,17 @@ config CMA_ALIGNMENT
 
 endif
 
+config PERMANENT_CPU_TOPOLOGY
+	bool "Permanent CPU Topology"
+	depends on HOTPLUG_CPU
+	default 1 if X86
+	default 0
+	help
+	  This option configures CPU topology to be permanent for the lifetime
+	  of the CPU (until it is physically removed).  Selecting Y here
+	  results in the kernel reporting the physical location for offlined
+	  CPUs.
+
+	  If unsure, leave the default value as is.
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 6b2a84e..567ab7c 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o cacheinfo.o
+			   container.o property.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 91bbb19..9c30782 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -17,6 +17,8 @@
 #include <linux/of.h>
 #include <linux/cpufeature.h>
 #include <linux/tick.h>
+#include <linux/mm.h>
+#include <linux/hardirq.h>
 
 #include "base.h"
 
@@ -175,15 +177,145 @@ static struct attribute *crash_note_cpu_attrs[] = {
 	NULL
 };
 
+#define define_id_show_func(name)				\
+static ssize_t name##_show(struct device *dev,			\
+		struct device_attribute *attr, char *buf)	\
+{								\
+	return sprintf(buf, "%d\n", topology_##name(dev->id));	\
+}
+
+#define define_siblings_show_map(name, mask)				\
+static ssize_t name##_show(struct device *dev,				\
+			   struct device_attribute *attr, char *buf)	\
+{									\
+	return cpumap_print_to_pagebuf(false, buf, topology_##mask(dev->id));\
+}
+
+#define define_siblings_show_list(name, mask)				\
+static ssize_t name##_list_show(struct device *dev,			\
+				struct device_attribute *attr,		\
+				char *buf)				\
+{									\
+	return cpumap_print_to_pagebuf(true, buf, topology_##mask(dev->id));\
+}
+
+#define define_siblings_show_func(name, mask)	\
+	define_siblings_show_map(name, mask);	\
+	define_siblings_show_list(name, mask)
+
+define_id_show_func(physical_package_id);
+static DEVICE_ATTR_RO(physical_package_id);
+
+define_id_show_func(core_id);
+static DEVICE_ATTR_RO(core_id);
+
+define_siblings_show_func(thread_siblings, sibling_cpumask);
+static DEVICE_ATTR_RO(thread_siblings);
+static DEVICE_ATTR_RO(thread_siblings_list);
+
+define_siblings_show_func(core_siblings, core_cpumask);
+static DEVICE_ATTR_RO(core_siblings);
+static DEVICE_ATTR_RO(core_siblings_list);
+
+#ifdef CONFIG_SCHED_BOOK
+define_id_show_func(book_id);
+static DEVICE_ATTR_RO(book_id);
+define_siblings_show_func(book_siblings, book_cpumask);
+static DEVICE_ATTR_RO(book_siblings);
+static DEVICE_ATTR_RO(book_siblings_list);
+#endif
+
+static struct attribute *topology_attrs[] = {
+	&dev_attr_physical_package_id.attr,
+	&dev_attr_core_id.attr,
+	&dev_attr_thread_siblings.attr,
+	&dev_attr_thread_siblings_list.attr,
+	&dev_attr_core_siblings.attr,
+	&dev_attr_core_siblings_list.attr,
+#ifdef CONFIG_SCHED_BOOK
+	&dev_attr_book_id.attr,
+	&dev_attr_book_siblings.attr,
+	&dev_attr_book_siblings_list.attr,
+#endif
+	NULL
+};
+
 static struct attribute_group crash_note_cpu_attr_group = {
 	.attrs = crash_note_cpu_attrs,
 };
 #endif
 
+static struct attribute_group topology_attr_group = {
+	.attrs = topology_attrs,
+	.name = "topology"
+};
+
+#ifndef CONFIG_PERMANENT_CPU_TOPOLOGY
+/* Add/Remove cpu_topology interface for CPU device */
+static int topology_add_dev(unsigned int cpu)
+{
+	struct device *dev = get_cpu_device(cpu);
+
+	return sysfs_create_group(&dev->kobj, &topology_attr_group);
+}
+
+static void topology_remove_dev(unsigned int cpu)
+{
+	struct device *dev = get_cpu_device(cpu);
+
+	sysfs_remove_group(&dev->kobj, &topology_attr_group);
+}
+
+static int topology_cpu_callback(struct notifier_block *nfb,
+				 unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	int rc = 0;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+	case CPU_UP_PREPARE_FROZEN:
+		rc = topology_add_dev(cpu);
+		break;
+	case CPU_UP_CANCELED:
+	case CPU_UP_CANCELED_FROZEN:
+	case CPU_DEAD:
+	case CPU_DEAD_FROZEN:
+		topology_remove_dev(cpu);
+		break;
+	}
+	return notifier_from_errno(rc);
+}
+
+static int topology_sysfs_init(void)
+{
+	int cpu;
+	int rc = 0;
+
+	cpu_notifier_register_begin();
+
+	for_each_online_cpu(cpu) {
+		rc = topology_add_dev(cpu);
+		if (rc)
+			goto out;
+	}
+	__hotcpu_notifier(topology_cpu_callback, 0);
+
+out:
+	cpu_notifier_register_done();
+	return rc;
+}
+
+device_initcall(topology_sysfs_init);
+#endif
+
 static const struct attribute_group *common_cpu_attr_groups[] = {
 #ifdef CONFIG_KEXEC
 	&crash_note_cpu_attr_group,
 #endif
+#ifdef CONFIG_PERMANENT_CPU_TOPOLOGY
+	&topology_attr_group,
+#endif
 	NULL
 };
 
@@ -191,6 +323,9 @@ static const struct attribute_group *hotplugable_cpu_attr_groups[] = {
 #ifdef CONFIG_KEXEC
 	&crash_note_cpu_attr_group,
 #endif
+#ifdef CONFIG_PERMANENT_CPU_TOPOLOGY
+	&topology_attr_group,
+#endif
 	NULL
 };
 
diff --git a/drivers/base/topology.c b/drivers/base/topology.c
deleted file mode 100644
index 8b7d7f8..0000000
--- a/drivers/base/topology.c
+++ /dev/null
@@ -1,155 +0,0 @@
-/*
- * driver/base/topology.c - Populate sysfs with cpu topology information
- *
- * Written by: Zhang Yanmin, Intel Corporation
- *
- * Copyright (C) 2006, Intel Corp.
- *
- * All rights reserved.
- *
- * 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, GOOD TITLE or
- * NON INFRINGEMENT.  See the GNU General Public License for more
- * details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-#include <linux/mm.h>
-#include <linux/cpu.h>
-#include <linux/module.h>
-#include <linux/hardirq.h>
-#include <linux/topology.h>
-
-#define define_id_show_func(name)				\
-static ssize_t name##_show(struct device *dev,			\
-		struct device_attribute *attr, char *buf)	\
-{								\
-	return sprintf(buf, "%d\n", topology_##name(dev->id));	\
-}
-
-#define define_siblings_show_map(name, mask)				\
-static ssize_t name##_show(struct device *dev,				\
-			   struct device_attribute *attr, char *buf)	\
-{									\
-	return cpumap_print_to_pagebuf(false, buf, topology_##mask(dev->id));\
-}
-
-#define define_siblings_show_list(name, mask)				\
-static ssize_t name##_list_show(struct device *dev,			\
-				struct device_attribute *attr,		\
-				char *buf)				\
-{									\
-	return cpumap_print_to_pagebuf(true, buf, topology_##mask(dev->id));\
-}
-
-#define define_siblings_show_func(name, mask)	\
-	define_siblings_show_map(name, mask);	\
-	define_siblings_show_list(name, mask)
-
-define_id_show_func(physical_package_id);
-static DEVICE_ATTR_RO(physical_package_id);
-
-define_id_show_func(core_id);
-static DEVICE_ATTR_RO(core_id);
-
-define_siblings_show_func(thread_siblings, sibling_cpumask);
-static DEVICE_ATTR_RO(thread_siblings);
-static DEVICE_ATTR_RO(thread_siblings_list);
-
-define_siblings_show_func(core_siblings, core_cpumask);
-static DEVICE_ATTR_RO(core_siblings);
-static DEVICE_ATTR_RO(core_siblings_list);
-
-#ifdef CONFIG_SCHED_BOOK
-define_id_show_func(book_id);
-static DEVICE_ATTR_RO(book_id);
-define_siblings_show_func(book_siblings, book_cpumask);
-static DEVICE_ATTR_RO(book_siblings);
-static DEVICE_ATTR_RO(book_siblings_list);
-#endif
-
-static struct attribute *default_attrs[] = {
-	&dev_attr_physical_package_id.attr,
-	&dev_attr_core_id.attr,
-	&dev_attr_thread_siblings.attr,
-	&dev_attr_thread_siblings_list.attr,
-	&dev_attr_core_siblings.attr,
-	&dev_attr_core_siblings_list.attr,
-#ifdef CONFIG_SCHED_BOOK
-	&dev_attr_book_id.attr,
-	&dev_attr_book_siblings.attr,
-	&dev_attr_book_siblings_list.attr,
-#endif
-	NULL
-};
-
-static struct attribute_group topology_attr_group = {
-	.attrs = default_attrs,
-	.name = "topology"
-};
-
-/* Add/Remove cpu_topology interface for CPU device */
-static int topology_add_dev(unsigned int cpu)
-{
-	struct device *dev = get_cpu_device(cpu);
-
-	return sysfs_create_group(&dev->kobj, &topology_attr_group);
-}
-
-static void topology_remove_dev(unsigned int cpu)
-{
-	struct device *dev = get_cpu_device(cpu);
-
-	sysfs_remove_group(&dev->kobj, &topology_attr_group);
-}
-
-static int topology_cpu_callback(struct notifier_block *nfb,
-				 unsigned long action, void *hcpu)
-{
-	unsigned int cpu = (unsigned long)hcpu;
-	int rc = 0;
-
-	switch (action) {
-	case CPU_UP_PREPARE:
-	case CPU_UP_PREPARE_FROZEN:
-		rc = topology_add_dev(cpu);
-		break;
-	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
-	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
-		topology_remove_dev(cpu);
-		break;
-	}
-	return notifier_from_errno(rc);
-}
-
-static int topology_sysfs_init(void)
-{
-	int cpu;
-	int rc = 0;
-
-	cpu_notifier_register_begin();
-
-	for_each_online_cpu(cpu) {
-		rc = topology_add_dev(cpu);
-		if (rc)
-			goto out;
-	}
-	__hotcpu_notifier(topology_cpu_callback, 0);
-
-out:
-	cpu_notifier_register_done();
-	return rc;
-}
-
-device_initcall(topology_sysfs_init);
-- 
1.7.9.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] base, cpu, remove hotplugable_cpu_attr_groups
  2015-10-21 16:06 [PATCH 0/2] Add CONFIG_PERMANENT_CPU_TOPOLOGY [v2] Prarit Bhargava
  2015-10-21 16:06 ` [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2] Prarit Bhargava
@ 2015-10-21 16:06 ` Prarit Bhargava
  1 sibling, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-10-21 16:06 UTC (permalink / raw)
  To: linux-kernel; +Cc: trenn, Prarit Bhargava, Greg Kroah-Hartman

hotplugable_cpu_attr_groups is not different from common_cpu_attr_groups,
and can be removed.  This patchset renames common_cpu_attr_groups to
cpu_attr_groups.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Thomas Renninger <trenn@suse.de>
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/base/cpu.c |   16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 9c30782..8520c68 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -309,17 +309,7 @@ out:
 device_initcall(topology_sysfs_init);
 #endif
 
-static const struct attribute_group *common_cpu_attr_groups[] = {
-#ifdef CONFIG_KEXEC
-	&crash_note_cpu_attr_group,
-#endif
-#ifdef CONFIG_PERMANENT_CPU_TOPOLOGY
-	&topology_attr_group,
-#endif
-	NULL
-};
-
-static const struct attribute_group *hotplugable_cpu_attr_groups[] = {
+static const struct attribute_group *cpu_attr_groups[] = {
 #ifdef CONFIG_KEXEC
 	&crash_note_cpu_attr_group,
 #endif
@@ -502,9 +492,7 @@ int register_cpu(struct cpu *cpu, int num)
 #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
 	cpu->dev.bus->uevent = cpu_uevent;
 #endif
-	cpu->dev.groups = common_cpu_attr_groups;
-	if (cpu->hotpluggable)
-		cpu->dev.groups = hotplugable_cpu_attr_groups;
+	cpu->dev.groups = cpu_attr_groups;
 	error = device_register(&cpu->dev);
 	if (!error)
 		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-- 
1.7.9.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]
  2015-10-21 16:06 ` [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2] Prarit Bhargava
@ 2015-10-25 19:15   ` kbuild test robot
  2015-10-27 15:50   ` Thomas Renninger
  1 sibling, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2015-10-25 19:15 UTC (permalink / raw)
  To: Prarit Bhargava
  Cc: kbuild-all, linux-kernel, trenn, Prarit Bhargava,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Greg Kroah-Hartman, Borislav Petkov, Len Brown, Andy Lutomirski,
	Zhu Guihua, Denys Vlasenko, Jan H. Schönherr,
	Boris Ostrovsky, Paul E. McKenney

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

Hi Prarit,

[auto build test ERROR on driver-core/driver-core-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Prarit-Bhargava/Add-CONFIG_PERMANENT_CPU_TOPOLOGY-v2/20151022-001204
config: arm-allnoconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

>> drivers/base/cpu.c:249:11: error: 'topology_attrs' undeclared here (not in a function)
     .attrs = topology_attrs,
              ^

vim +/topology_attrs +249 drivers/base/cpu.c

   243	static struct attribute_group crash_note_cpu_attr_group = {
   244		.attrs = crash_note_cpu_attrs,
   245	};
   246	#endif
   247	
   248	static struct attribute_group topology_attr_group = {
 > 249		.attrs = topology_attrs,
   250		.name = "topology"
   251	};
   252	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 5497 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]
  2015-10-21 16:06 ` [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2] Prarit Bhargava
  2015-10-25 19:15   ` kbuild test robot
@ 2015-10-27 15:50   ` Thomas Renninger
  2015-10-30 12:19     ` Prarit Bhargava
  1 sibling, 1 reply; 6+ messages in thread
From: Thomas Renninger @ 2015-10-27 15:50 UTC (permalink / raw)
  To: Prarit Bhargava
  Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Greg Kroah-Hartman, Borislav Petkov, Len Brown, Andy Lutomirski,
	Zhu Guihua, Denys Vlasenko, Jan H. Schönherr,
	Boris Ostrovsky, Paul E. McKenney

On Wednesday, October 21, 2015 12:06:37 PM Prarit Bhargava wrote:

...

> +config PERMANENT_CPU_TOPOLOGY
> +	bool "Permanent CPU Topology"
> +	depends on HOTPLUG_CPU
> +	default 1 if X86
> +	default 0
> +	help
> +	  This option configures CPU topology to be permanent for the lifetime
> +	  of the CPU (until it is physically removed).

I cannot see where you differ soft offlined and physically removed?
The topology file is simply never removed or do I oversee something?
Hm, maybe this one is even correct, but could get re-phrased a bit.

Puhh, not sure, maybe this:
> +	  This option configures CPU topology to be permanent.
should be enough?
It is obvious that topology info is still correct when the CPU is software
offlined via:
echo 0 >/sys/devices/system/cpu/cpuX/online

But if someone really hit the button to eject a Numa node or
so, this info might not be up-to-date (but still avail now, because we
cannot differ software vs hardware events, at least not that easy).

But it is up-to-date, once the newly plugged in Numa node or CPU gets
added and ramped up, so this change should be very fine.

Maybe it's worth to phrase above into the changelog at least?


I think this is a sane change and works!
If physically removed, the topology info could be outdated, but userspace
knows, that the core is offlined right now. Also the info will likely
be the same when something gets re-plugged. If not, topology info will be
valid once the re-plugged core gets initialized, so everything should be fine.

I put these two patches onto our latest kernel builds, enabled the .config
option for i386 and x86_64, disabled it for other archs and things still built
nicely:
https://build.opensuse.org/project/show/home:trenn:kernel

One issue:
Why do you move topology.c into cpu.c?
It is hard to see the real diff now.
If moving makes sense, it would be nice to first submit a patch
showing the changes for this feature and another patch saying:
"Eleminating topology.c, only code move, no functional change"
in the changelog.


Feel free to add a Reviewed-by: Thomas Renninger <trenn@suse.com>
if you plan to re-submit.

Thanks!

    Thomas

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]
  2015-10-27 15:50   ` Thomas Renninger
@ 2015-10-30 12:19     ` Prarit Bhargava
  0 siblings, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-10-30 12:19 UTC (permalink / raw)
  To: Thomas Renninger
  Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Greg Kroah-Hartman, Borislav Petkov, Len Brown, Andy Lutomirski,
	Zhu Guihua, Denys Vlasenko, "Jan H. Schönherr",
	Boris Ostrovsky, Paul E. McKenney



On 10/27/2015 11:50 AM, Thomas Renninger wrote:
> On Wednesday, October 21, 2015 12:06:37 PM Prarit Bhargava wrote:
> 
> ...
> 
>> +config PERMANENT_CPU_TOPOLOGY
>> +	bool "Permanent CPU Topology"
>> +	depends on HOTPLUG_CPU
>> +	default 1 if X86
>> +	default 0
>> +	help
>> +	  This option configures CPU topology to be permanent for the lifetime
>> +	  of the CPU (until it is physically removed).
> 
> I cannot see where you differ soft offlined and physically removed?
> The topology file is simply never removed or do I oversee something?
> Hm, maybe this one is even correct, but could get re-phrased a bit.

Yeah, I can rework this Thomas.  When CONFIG_PERMANENT_CPU_TOPOLOGY=Y the sysfs
topology file will only be removed when the CPU is physically removed (that is,
when the cpu device struct is destroyed).

> 
> Puhh, not sure, maybe this:
>> +	  This option configures CPU topology to be permanent.
> should be enough?
> It is obvious that topology info is still correct when the CPU is software
> offlined via:
> echo 0 >/sys/devices/system/cpu/cpuX/online
> 
> But if someone really hit the button to eject a Numa node or
> so, this info might not be up-to-date (but still avail now, because we
> cannot differ software vs hardware events, at least not that easy).

If the device is ejected, the cpu device struct will be free'd as will be the
sysfs entries for the device.  IOW, there cannot be a stale data issue.

> 
> But it is up-to-date, once the newly plugged in Numa node or CPU gets
> added and ramped up, so this change should be very fine.
> 
> Maybe it's worth to phrase above into the changelog at least?

Yeah -- I'll do that in the next version.

> 
> 
> I think this is a sane change and works!
> If physically removed, the topology info could be outdated, but userspace
> knows, that the core is offlined right now. Also the info will likely
> be the same when something gets re-plugged. If not, topology info will be
> valid once the re-plugged core gets initialized, so everything should be fine.
> 
> I put these two patches onto our latest kernel builds, enabled the .config
> option for i386 and x86_64, disabled it for other archs and things still built
> nicely:
> https://build.opensuse.org/project/show/home:trenn:kernel

:) !!!

> 
> One issue:
> Why do you move topology.c into cpu.c?
> It is hard to see the real diff now.
> If moving makes sense, it would be nice to first submit a patch
> showing the changes for this feature and another patch saying:
> "Eleminating topology.c, only code move, no functional change"
> in the changelog.
> 
> 
> Feel free to add a Reviewed-by: Thomas Renninger <trenn@suse.com>
> if you plan to re-submit.

Will do!  Thanks for the review!

P.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-30 12:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-21 16:06 [PATCH 0/2] Add CONFIG_PERMANENT_CPU_TOPOLOGY [v2] Prarit Bhargava
2015-10-21 16:06 ` [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2] Prarit Bhargava
2015-10-25 19:15   ` kbuild test robot
2015-10-27 15:50   ` Thomas Renninger
2015-10-30 12:19     ` Prarit Bhargava
2015-10-21 16:06 ` [PATCH 2/2] base, cpu, remove hotplugable_cpu_attr_groups Prarit Bhargava

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).