Re: [PATCH RFC 01/22] arch_topology: Make register_cpu_capacity_sysctl() tolerant to late CPUs

[Russell King (Oracle)]

On Tue, Nov 28, 2023 at 02:37:22PM +0000, Jonathan Cameron wrote:
> On Tue, 07 Nov 2023 10:29:23 +0000
> Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
> 
> > From: James Morse <james.morse@xxxxxxx>
> > 
> > register_cpu_capacity_sysctl() adds a property to sysfs that describes
> > the CPUs capacity. This is done from a subsys_initcall() that assumes
> > all possible CPUs are registered.
> > 
> > With CPU hotplug, possible CPUs aren't registered until they become
> > present, (or for arm64 enabled). This leads to messages during boot:
> > | register_cpu_capacity_sysctl: too early to get CPU1 device!
> > and once these CPUs are added to the system, the file is missing.
> > 
> > Move this to a cpuhp callback, so that the file is created once
> > CPUs are brought online. This covers CPUs that are added late by
> > mechanisms like hotplug.
> > One observable difference is the file is now missing for offline CPUs.
> > 
> > Signed-off-by: James Morse <james.morse@xxxxxxx>
> > ---
> > If the offline CPUs thing is a problem for the tools that consume
> > this value, we'd need to move cpu_capacity to be part of cpu.c's
> > common_cpu_attr_groups.
> 
> I'm not keen on squirting sysfs files in from code so
> might be nice to do that anyway and use is_visible() / sysfs_update_group()
> but that would be a job for another day if at all.

I'm doing my best, but it's really not helped by the dysfunctional
nature of some parts of the kernel community. I have now decided that
this is not possible to implement. So while it's a nice idea, I don't
think we'll ever see it.

As I mentioned on the 14th November, complete with a patch (and got no
response from anyone):
> Looking into doing this, the easy bit is adding the attribute group
> with an appropriate .is_visible dependent on cpu_present(), but we
> need to be able to call sysfs_update_groups() when the state of the
> .is_visible() changes.
>
> Given the comment in sysfs_update_groups() about "if an error occurs",
> rather than making this part of common_cpu_attr_groups, would it be
> better that it's part of its own set of groups, thus limiting the
> damage from a possible error? I suspect, however, that any error at
> that point means that the system is rather fatally wounded.
>
> This is what I have so far to implement your idea, less the necessary
> sysfs_update_groups() call when we need to change the visibility of
> the attributes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!