lkml.org 
[lkml]   [2023]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH v2 11/35] arch_topology: Make register_cpu_capacity_sysctl() tolerant to late CPUs
    Okay, after 25 days, I'm now changing James' comment to:

    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. However, attempts to discuss this just end
    up in a black hole, so this is a non-starter. Thus, if this needs
    to be done, it can be done as a separate patch.

    and thus I'm going to consider this patch acceptable to everyone.

    On Fri, Oct 20, 2023 at 02:44:35PM +0100, Russell King (Oracle) wrote:
    > On Thu, Sep 14, 2023 at 01:01:26PM +0100, Jonathan Cameron wrote:
    > > On Wed, 13 Sep 2023 16:37:59 +0000
    > > James Morse <james.morse@arm.com> wrote:
    > >
    > > > 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@arm.com>
    > > > ---
    > > > 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 think we should do that anyway and then use an is_visible() if we want to
    > > change whether it is visible in offline cpus.
    > >
    > > Dynamic sysfs file creation is horrible - particularly when done
    > > from an totally different file from where the rest of the attributes
    > > are registered. I'm curious what the history behind that is.
    > >
    > > Whilst here, why is there a common_cpu_attr_groups which is
    > > identical to the hotpluggable_cpu_attr_groups in base/cpu.c?
    >
    > 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.
    >
    > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
    > index 9ccb7daee78e..06c9fc6620d2 100644
    > --- a/drivers/base/arch_topology.c
    > +++ b/drivers/base/arch_topology.c
    > @@ -215,43 +215,24 @@ static ssize_t cpu_capacity_show(struct device *dev,
    > return sysfs_emit(buf, "%lu\n", topology_get_cpu_scale(cpu->dev.id));
    > }
    >
    > -static void update_topology_flags_workfn(struct work_struct *work);
    > -static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
    > -
    > static DEVICE_ATTR_RO(cpu_capacity);
    >
    > -static int cpu_capacity_sysctl_add(unsigned int cpu)
    > -{
    > - struct device *cpu_dev = get_cpu_device(cpu);
    > -
    > - if (!cpu_dev)
    > - return -ENOENT;
    > -
    > - device_create_file(cpu_dev, &dev_attr_cpu_capacity);
    > -
    > - return 0;
    > -}
    > -
    > -static int cpu_capacity_sysctl_remove(unsigned int cpu)
    > +static umode_t cpu_present_attrs_visible(struct kobject *kobi,
    > + struct attribute *attr, int index)
    > {
    > - struct device *cpu_dev = get_cpu_device(cpu);
    > -
    > - if (!cpu_dev)
    > - return -ENOENT;
    > -
    > - device_remove_file(cpu_dev, &dev_attr_cpu_capacity);
    > + struct device *dev = kobj_to_dev(kobj);
    > + struct cpu *cpu = container_of(dev, struct cpu, dev);
    >
    > - return 0;
    > + return cpu_present(cpu->dev.id) ? attr->mode : 0;
    > }
    >
    > -static int register_cpu_capacity_sysctl(void)
    > -{
    > - cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "topology/cpu-capacity",
    > - cpu_capacity_sysctl_add, cpu_capacity_sysctl_remove);
    > +const struct attribute_group cpu_capacity_attr_group = {
    > + .is_visible = cpu_present_attrs_visible,
    > + .attrs = cpu_capacity_attrs
    > +};
    >
    > - return 0;
    > -}
    > -subsys_initcall(register_cpu_capacity_sysctl);
    > +static void update_topology_flags_workfn(struct work_struct *work);
    > +static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
    >
    > static int update_topology;
    >
    > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
    > index a19a8be93102..954b045705c2 100644
    > --- a/drivers/base/cpu.c
    > +++ b/drivers/base/cpu.c
    > @@ -192,6 +192,9 @@ static const struct attribute_group crash_note_cpu_attr_group = {
    > static const struct attribute_group *common_cpu_attr_groups[] = {
    > #ifdef CONFIG_KEXEC
    > &crash_note_cpu_attr_group,
    > +#endif
    > +#ifdef CONFIG_GENERIC_ARCH_TOPOLOGY
    > + &cpu_capacity_attr_group,
    > #endif
    > NULL
    > };
    > diff --git a/include/linux/cpu.h b/include/linux/cpu.h
    > index e117c06e0c6b..745ad21e3dc8 100644
    > --- a/include/linux/cpu.h
    > +++ b/include/linux/cpu.h
    > @@ -30,6 +30,8 @@ struct cpu {
    > struct device dev;
    > };
    >
    > +extern const struct attribute_group cpu_capacity_attr_group;
    > +
    > extern void boot_cpu_init(void);
    > extern void boot_cpu_hotplug_init(void);
    > extern void cpu_init(void);
    >
    > --
    > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
    > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

    \
     
     \ /
      Last update: 2023-11-20 13:58    [W:5.869 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site