Add CONFIG_PERMANENT_CPU_TOPOLOGY [v2]

PATCH Superseded Prarit Bhargava 3 participants 6 messages Oct 21, 2015 – Oct 30, 2015

Subsystems (2)

x86 Driver core

lore

Share & tools

lore

Prarit Bhargava

author

2015-10-21 16:06 UTC #0 lore raw

cover Add CONFIG_PERMANENT_CPU_TOPOLOGY [v2]

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

Signature · 1 line

1.7.9.3

Prarit Bhargava

author

2015-10-21 16:06 UTC #1 lore raw

PATCH 1/2 cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]

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

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:

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.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Greg Kroah-Hartman <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Len Brown <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Zhu Guihua <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc "Jan H. Schönherr" <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Thomas Renninger <[email protected]>

Signed-off-by Prarit Bhargava <[email protected]>

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

arch/x86/kernel/smpboot.c +0 -3

index e0c198e..949375f 100644
@@ -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);
 }

drivers/base/Kconfig +13 -0

index 98504ec..321d261 100644
@@ -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

drivers/base/Makefile +1 -1

index 6b2a84e..567ab7c 100644
@@ -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/

drivers/base/cpu.c +135 -0

162 lines not on this page · raw message

drivers/base/topology.c deleted +0 ~~-155~~

155 lines not on this page · raw message

Signature · 1 line

1.7.9.3

kbuild test robot

build bot

2015-10-25 19:16 UTC #2 lore raw

Re: [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]

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…

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

Thomas Renninger

2015-10-27 15:51 UTC #3 lore raw

Re: [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]

In reply to Prarit Bhargava

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

...

drivers/base/Kconfig config CMA_ALIGNMENT

Quoted 8 lines

drivers/base/Kconfig +8 -0

+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:

Review

Quoted 1 line

(quoted diff) +1 -0

+	  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 <[email protected]> if you plan to re-submit.

Thanks!

Thomas

Prarit Bhargava

author

2015-10-30 12:19 UTC #4 lore raw

Re: [PATCH 1/2] cpu hotplug, add CONFIG_PERMANENT_CPU_TOPOLOGY and keep topology directory for lifetime of CPU [v2]

Quoted 4 lines · Thomas Renninger

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

Quoted 12 lines

drivers/base/Kconfig config CMA_ALIGNMENT
Quoted 8 lines
drivers/base/Kconfig +8 -0
+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).

Quoted 11 lines

Puhh, not sure, maybe this:
Review
Quoted 1 line
(quoted diff) +1 -0
+	  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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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

:) !!!

Quoted 10 lines

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.

Quoted 2 lines

Feel free to add a Reviewed-by: Thomas Renninger <[email protected]> if you plan to re-submit.

Will do! Thanks for the review!

Prarit Bhargava

author

2015-10-21 16:06 UTC #5 lore raw

PATCH 2/2 base, cpu, remove hotplugable_cpu_attr_groups

In reply to Prarit Bhargava

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 <[email protected]>

CC Thomas Renninger <[email protected]>

Signed-off-by Prarit Bhargava <[email protected]>

---
 drivers/base/cpu.c |   16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

drivers/base/cpu.c +2 ~~-14~~

index 9c30782..8520c68 100644
@@ -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;

Signature · 1 line

1.7.9.3