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

[Thomas Renninger <trenn@suse.de>]

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