Message-ID: <40cacc6b-c3d1-c63b-e307-d95b2c53e399@oracle.com>
Date:   Tue, 13 Jun 2023 14:58:14 -0500
From:   Eric DeVolder <eric.devolder@...cle.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, david@...hat.com, osalvador@...e.de,
        corbet@....net, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, bhe@...hat.com,
        ebiederm@...ssion.com, kexec@...ts.infradead.org, hpa@...or.com,
        rafael@...nel.org, vgoyal@...hat.com, dyoung@...hat.com,
        lf32.dev@...il.com, akpm@...ux-foundation.org,
        naveen.n.rao@...ux.vnet.ibm.com, zohar@...ux.ibm.com,
        bhelgaas@...gle.com, vbabka@...e.cz, tiwai@...e.de,
        seanjc@...gle.com, linux@...ssschuh.net, vschneid@...hat.com,
        linux-mm@...ck.org, linux-doc@...r.kernel.org,
        sourabhjain@...ux.ibm.com, konrad.wilk@...cle.com,
        boris.ostrovsky@...cle.com
Subject: Re: [PATCH v23 4/8] crash: memory and CPU hotplug sysfs attributes



On 6/13/23 10:24, Eric DeVolder wrote:
> 
> 
> On 6/13/23 03:03, Greg KH wrote:
>> On Mon, Jun 12, 2023 at 05:07:08PM -0400, Eric DeVolder wrote:
>>> Introduce the crash_hotplug attribute for memory and CPUs for
>>> use by userspace.  These attributes directly facilitate the udev
>>> rule for managing userspace re-loading of the crash kernel upon
>>> hot un/plug changes.
>>>
>>> For memory, expose the crash_hotplug attribute to the
>>> /sys/devices/system/memory directory. For example:
>>>
>>>   # udevadm info --attribute-walk /sys/devices/system/memory/memory81
>>>    looking at device '/devices/system/memory/memory81':
>>>      KERNEL=="memory81"
>>>      SUBSYSTEM=="memory"
>>>      DRIVER==""
>>>      ATTR{online}=="1"
>>>      ATTR{phys_device}=="0"
>>>      ATTR{phys_index}=="00000051"
>>>      ATTR{removable}=="1"
>>>      ATTR{state}=="online"
>>>      ATTR{valid_zones}=="Movable"
>>>
>>>    looking at parent device '/devices/system/memory':
>>>      KERNELS=="memory"
>>>      SUBSYSTEMS==""
>>>      DRIVERS==""
>>>      ATTRS{auto_online_blocks}=="offline"
>>>      ATTRS{block_size_bytes}=="8000000"
>>>      ATTRS{crash_hotplug}=="1"
>>>
>>> For CPUs, expose the crash_hotplug attribute to the
>>> /sys/devices/system/cpu directory. For example:
>>>
>>>   # udevadm info --attribute-walk /sys/devices/system/cpu/cpu0
>>>    looking at device '/devices/system/cpu/cpu0':
>>>      KERNEL=="cpu0"
>>>      SUBSYSTEM=="cpu"
>>>      DRIVER=="processor"
>>>      ATTR{crash_notes}=="277c38600"
>>>      ATTR{crash_notes_size}=="368"
>>>      ATTR{online}=="1"
>>>
>>>    looking at parent device '/devices/system/cpu':
>>>      KERNELS=="cpu"
>>>      SUBSYSTEMS==""
>>>      DRIVERS==""
>>>      ATTRS{crash_hotplug}=="1"
>>>      ATTRS{isolated}==""
>>>      ATTRS{kernel_max}=="8191"
>>>      ATTRS{nohz_full}=="  (null)"
>>>      ATTRS{offline}=="4-7"
>>>      ATTRS{online}=="0-3"
>>>      ATTRS{possible}=="0-7"
>>>      ATTRS{present}=="0-3"
>>>
>>> With these sysfs attributes in place, it is possible to efficiently
>>> instruct the udev rule to skip crash kernel reloading for kernels
>>> configured with crash hotplug support.
>>>
>>> For example, the following is the proposed udev rule change for RHEL
>>> system 98-kexec.rules (as the first lines of the rule file):
>>>
>>>   # The kernel updates the crash elfcorehdr for CPU and memory changes
>>>   SUBSYSTEM=="cpu", ATTRS{crash_hotplug}=="1", GOTO="kdump_reload_end"
>>>   SUBSYSTEM=="memory", ATTRS{crash_hotplug}=="1", GOTO="kdump_reload_end"
>>>
>>> When examined in the context of 98-kexec.rules, the above rules
>>> test if crash_hotplug is set, and if so, the userspace initiated
>>> unload-then-reload of the crash kernel is skipped.
>>>
>>> CPU and memory checks are separated in accordance with
>>> CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG kernel config options.
>>> If an architecture supports, for example, memory hotplug but not
>>> CPU hotplug, then the /sys/devices/system/memory/crash_hotplug
>>> attribute file is present, but the /sys/devices/system/cpu/crash_hotplug
>>> attribute file will NOT be present. Thus the udev rule skips
>>> userspace processing of memory hot un/plug events, but the udev
>>> rule will evaluate false for CPU events, thus allowing userspace to
>>> process CPU hot un/plug events (ie the unload-then-reload of the kdump
>>> capture kernel).
>>>
>>> Signed-off-by: Eric DeVolder <eric.devolder@...cle.com>
>>> Reviewed-by: Sourabh Jain <sourabhjain@...ux.ibm.com>
>>> Acked-by: Hari Bathini <hbathini@...ux.ibm.com>
>>> Acked-by: Baoquan He <bhe@...hat.com>
>>> ---
>>>   .../admin-guide/mm/memory-hotplug.rst          |  8 ++++++++
>>>   Documentation/core-api/cpu_hotplug.rst         | 18 ++++++++++++++++++
>>>   drivers/base/cpu.c                             | 14 ++++++++++++++
>>>   drivers/base/memory.c                          | 13 +++++++++++++
>>>   include/linux/kexec.h                          |  8 ++++++++
>>>   5 files changed, 61 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst 
>>> b/Documentation/admin-guide/mm/memory-hotplug.rst
>>> index 1b02fe5807cc..eb99d79223a3 100644
>>> --- a/Documentation/admin-guide/mm/memory-hotplug.rst
>>> +++ b/Documentation/admin-guide/mm/memory-hotplug.rst
>>> @@ -291,6 +291,14 @@ The following files are currently defined:
>>>                  Availability depends on the CONFIG_ARCH_MEMORY_PROBE
>>>                  kernel configuration option.
>>>   ``uevent``           read-write: generic udev file for device subsystems.
>>> +``crash_hotplug``      read-only: when changes to the system memory map
>>> +               occur due to hot un/plug of memory, this file contains
>>> +               '1' if the kernel updates the kdump capture kernel memory
>>> +               map itself (via elfcorehdr), or '0' if userspace must update
>>> +               the kdump capture kernel memory map.
>>> +
>>> +               Availability depends on the CONFIG_MEMORY_HOTPLUG kernel
>>> +               configuration option.
>>>   ====================== =========================================================
>>>   .. note::
>>> diff --git a/Documentation/core-api/cpu_hotplug.rst b/Documentation/core-api/cpu_hotplug.rst
>>> index f75778d37488..0c8dc3fe5f94 100644
>>> --- a/Documentation/core-api/cpu_hotplug.rst
>>> +++ b/Documentation/core-api/cpu_hotplug.rst
>>> @@ -750,6 +750,24 @@ will receive all events. A script like::
>>>   can process the event further.
>>> +When changes to the CPUs in the system occur, the sysfs file
>>> +/sys/devices/system/cpu/crash_hotplug contains '1' if the kernel
>>> +updates the kdump capture kernel list of CPUs itself (via elfcorehdr),
>>> +or '0' if userspace must update the kdump capture kernel list of CPUs.
>>> +
>>> +The availability depends on the CONFIG_HOTPLUG_CPU kernel configuration
>>> +option.
>>> +
>>> +To skip userspace processing of CPU hot un/plug events for kdump
>>> +(ie the unload-then-reload to obtain a current list of CPUs), this sysfs
>>> +file can be used in a udev rule as follows:
>>> +
>>> + SUBSYSTEM=="cpu", ATTRS{crash_hotplug}=="1", GOTO="kdump_reload_end"
>>> +
>>> +For a cpu hot un/plug event, if the architecture supports kernel updates
>>> +of the elfcorehdr (which contains the list of CPUs), then the rule skips
>>> +the unload-then-reload of the kdump capture kernel.
>>> +
>>>   Kernel Inline Documentations Reference
>>>   ======================================
>>> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
>>> index c1815b9dae68..06a0c22b37b8 100644
>>> --- a/drivers/base/cpu.c
>>> +++ b/drivers/base/cpu.c
>>> @@ -282,6 +282,17 @@ static ssize_t print_cpus_nohz_full(struct device *dev,
>>>   static DEVICE_ATTR(nohz_full, 0444, print_cpus_nohz_full, NULL);
>>>   #endif
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +#include <linux/kexec.h>
>>> +static ssize_t crash_hotplug_show(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +    return sprintf(buf, "%d\n", crash_hotplug_cpu_support());
>>> +}
>>> +static DEVICE_ATTR_ADMIN_RO(crash_hotplug);
>>> +#endif
>>> +
>>>   static void cpu_device_release(struct device *dev)
>>>   {
>>>       /*
>>> @@ -469,6 +480,9 @@ static struct attribute *cpu_root_attrs[] = {
>>>   #ifdef CONFIG_NO_HZ_FULL
>>>       &dev_attr_nohz_full.attr,
>>>   #endif
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +    &dev_attr_crash_hotplug.attr,
>>> +#endif
>>>   #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
>>>       &dev_attr_modalias.attr,
>>>   #endif
>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>> index b456ac213610..24b8ef4c830c 100644
>>> --- a/drivers/base/memory.c
>>> +++ b/drivers/base/memory.c
>>> @@ -490,6 +490,16 @@ static ssize_t auto_online_blocks_store(struct device *dev,
>>>   static DEVICE_ATTR_RW(auto_online_blocks);
>>> +#ifdef CONFIG_MEMORY_HOTPLUG
>>> +#include <linux/kexec.h>
>>> +static ssize_t crash_hotplug_show(struct device *dev,
>>> +                       struct device_attribute *attr, char *buf)
>>> +{
>>> +    return sprintf(buf, "%d\n", crash_hotplug_memory_support());
>>> +}
>>
>> This sysfs file has to be documented in Documentation/ABI/ right?
> 
> I will add these entries.
> 
>>
>> And did you use checkpatch?  It should have told you to use sysfs_emit()
>> instead...
> I did use checkpatch, and it did not reveal that.
> 
> [root@...alhost linux]# ./scripts/checkpatch.pl --patch 
> ../v23/0004-crash-memory-and-CPU-hotplug-sysfs-attributes.patch
> total: 0 errors, 0 warnings, 103 lines checked
> 
> ../v23/0004-crash-memory-and-CPU-hotplug-sysfs-attributes.patch has no obvious style problems and is 
> ready for submission.
> 
> This is not the first time it's been suggested to me that checkpatch should have caught a problem; 
> is there a different invocation than what I'm doing?
> 
>>
>>> +static DEVICE_ATTR_RO(crash_hotplug);
>>> +#endif
>>
>> All of these #ifdefs should all be removed and instead use the
>> is_visible() callback to determine if the attribute is shown or not,
>> using the IS_ENABLED() test in the function.
> 
> ok, I'll correct this.

I've been examining drivers/base/cacheinfo.c as a template for how to remove the
#ifdefs and use the is_visible() callback for the drivers/base/cpu|memory.c files.

I'm attempting to apply this technique to drivers/base/cpu.c. In this file, there
are features that are compiled in/out based on the CONFIG settings, for example 
CONFIG_ARCH_CPU_PROBE_RELEASE. My attempts at applying the technique thus far have
resulted in link-time errors for missing symbols, ie. arch_cpu_probe() and
arch_cpu_release().

As I understand it, to use IS_ENABLED(XYZ) to guard-band conditional code, the contents
of that code still needs to be compile-able (eg. no references to struct members with
surrounding #ifdef CONFIG_XYZ) and link-able (eg. any called functions must also be
compiled).

Given my understanding of the request and IS_ENABLED, I'm not seeing a path forward.
I'm thinking the approach works well in cacheinfo.c for example because cache info is
always compiled-in.

I suspect I am misunderstanding, and would appreciate a pointer/guidance.

Thanks!
eric


> 
> Thank you for looking at this!
> eric
> 
>>
>> thanks,
>>
>> greg k-h

Powered by blists - more mailing lists