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

[Prarit Bhargava]

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@xxxxxxxx>
> if you plan to re-submit.

Will do!  Thanks for the review!

P.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/