CPU and memory change notifications are received in order to
regenerate the elfcorehdr.
To support cpu hotplug, a callback is registered to capture the
CPUHP_AP_ONLINE_DYN online and offline events via
cpuhp_setup_state_nocalls().
To support memory hotplug, a notifier is registered to capture the
MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
The cpu callback and memory notifiers call handle_hotplug_event()
to handle the hot plug/unplug event. Then handle_hotplug_event()
dispatches the event to the architecture specific
arch_crash_handle_hotplug_event(). During the process, the
kexec_mutex is held.
Signed-off-by: Eric DeVolder <[email protected]>
---
include/linux/crash_core.h | 10 +++++
include/linux/kexec.h | 5 +++
kernel/crash_core.c | 92 ++++++++++++++++++++++++++++++++++++++
3 files changed, 107 insertions(+)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e..a240f84348aa 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
unsigned long long *crash_size, unsigned long long *crash_base);
+#define KEXEC_CRASH_HP_REMOVE_CPU 0
+#define KEXEC_CRASH_HP_ADD_CPU 1
+#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
+#define KEXEC_CRASH_HP_ADD_MEMORY 3
+#define KEXEC_CRASH_HP_INVALID_CPU -1U
+
+struct kimage;
+void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
+ unsigned int cpu);
+
#endif /* LINUX_CRASH_CORE_H */
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index f93f2591fc1e..5935bc78ed7f 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -308,6 +308,11 @@ struct kimage {
struct purgatory_info purgatory_info;
#endif
+ bool hotplug_event;
+ unsigned int offlinecpu;
+ bool elfcorehdr_index_valid;
+ int elfcorehdr_index;
+
#ifdef CONFIG_IMA_KEXEC
/* Virtual address of IMA measurement buffer for kexec syscall */
void *ima_buffer;
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 256cf6db573c..f197af50def6 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -9,12 +9,17 @@
#include <linux/init.h>
#include <linux/utsname.h>
#include <linux/vmalloc.h>
+#include <linux/highmem.h>
+#include <linux/memory.h>
+#include <linux/cpuhotplug.h>
#include <asm/page.h>
#include <asm/sections.h>
#include <crypto/sha1.h>
+#include "kexec_internal.h"
+
/* vmcoreinfo stuff */
unsigned char *vmcoreinfo_data;
size_t vmcoreinfo_size;
@@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
}
subsys_initcall(crash_save_vmcoreinfo_init);
+
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
+void __weak arch_crash_handle_hotplug_event(struct kimage *image,
+ unsigned int hp_action, unsigned int cpu)
+{
+ WARN(1, "crash hotplug handler not implemented");
+}
+
+static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
+{
+ /* Obtain lock while changing crash information */
+ if (!mutex_trylock(&kexec_mutex))
+ return;
+
+ /* Check kdump is loaded */
+ if (kexec_crash_image) {
+ pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
+
+ /* Needed in order for the segments to be updated */
+ arch_kexec_unprotect_crashkres();
+
+ /* Flag to differentiate between normal load and hotplug */
+ kexec_crash_image->hotplug_event = true;
+
+ /* Now invoke arch-specific update handler */
+ arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
+
+ /* No longer handling a hotplug event */
+ kexec_crash_image->hotplug_event = false;
+
+ /* Change back to read-only */
+ arch_kexec_protect_crashkres();
+ }
+
+ /* Release lock now that update complete */
+ mutex_unlock(&kexec_mutex);
+}
+
+static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
+{
+ switch (val) {
+ case MEM_ONLINE:
+ handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
+ break;
+
+ case MEM_OFFLINE:
+ handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
+ break;
+ }
+ return NOTIFY_OK;
+}
+
+static struct notifier_block crash_memhp_nb = {
+ .notifier_call = crash_memhp_notifier,
+ .priority = 0
+};
+
+static int crash_cpuhp_online(unsigned int cpu)
+{
+ handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
+ return 0;
+}
+
+static int crash_cpuhp_offline(unsigned int cpu)
+{
+ handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
+ return 0;
+}
+
+static int __init crash_hotplug_init(void)
+{
+ int result = 0;
+
+ if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
+ register_memory_notifier(&crash_memhp_nb);
+
+ if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
+ result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+ "crash/cpuhp",
+ crash_cpuhp_online,
+ crash_cpuhp_offline);
+
+ return result;
+}
+
+subsys_initcall(crash_hotplug_init);
+#endif
--
2.27.0
On 05/05/22 at 02:45pm, Eric DeVolder wrote:
......
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 256cf6db573c..f197af50def6 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -9,12 +9,17 @@
> #include <linux/init.h>
> #include <linux/utsname.h>
> #include <linux/vmalloc.h>
> +#include <linux/highmem.h>
Wondering where highmem.h is needed. Just curious.
> +#include <linux/memory.h>
> +#include <linux/cpuhotplug.h>
>
> #include <asm/page.h>
> #include <asm/sections.h>
>
> #include <crypto/sha1.h>
>
> +#include "kexec_internal.h"
> +
> /* vmcoreinfo stuff */
> unsigned char *vmcoreinfo_data;
> size_t vmcoreinfo_size;
> @@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
> }
>
> subsys_initcall(crash_save_vmcoreinfo_init);
> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> + unsigned int hp_action, unsigned int cpu)
> +{
> + WARN(1, "crash hotplug handler not implemented");
> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> + /* Obtain lock while changing crash information */
> + if (!mutex_trylock(&kexec_mutex))
> + return;
> +
> + /* Check kdump is loaded */
> + if (kexec_crash_image) {
> + pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> + /* Needed in order for the segments to be updated */
> + arch_kexec_unprotect_crashkres();
> +
> + /* Flag to differentiate between normal load and hotplug */
> + kexec_crash_image->hotplug_event = true;
> +
> + /* Now invoke arch-specific update handler */
> + arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> + /* No longer handling a hotplug event */
> + kexec_crash_image->hotplug_event = false;
> +
> + /* Change back to read-only */
> + arch_kexec_protect_crashkres();
> + }
> +
> + /* Release lock now that update complete */
> + mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> + switch (val) {
> + case MEM_ONLINE:
> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> + break;
> +
> + case MEM_OFFLINE:
> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> + break;
> + }
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> + .notifier_call = crash_memhp_notifier,
> + .priority = 0
> +};
> +
> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> + return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> + return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> + int result = 0;
> +
> + if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> + register_memory_notifier(&crash_memhp_nb);
> +
> + if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> + result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> + "crash/cpuhp",
> + crash_cpuhp_online,
> + crash_cpuhp_offline);
> +
> + return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif
> --
> 2.27.0
>
On 5/6/22 02:12, Baoquan He wrote:
> On 05/05/22 at 02:45pm, Eric DeVolder wrote:
> ......
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index 256cf6db573c..f197af50def6 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -9,12 +9,17 @@
>> #include <linux/init.h>
>> #include <linux/utsname.h>
>> #include <linux/vmalloc.h>
>> +#include <linux/highmem.h>
>
> Wondering where highmem.h is needed. Just curious.
Ahh, I missed that. At one point in time we moved map_crash_pages() into this file, which brought
highmem.h along with it. But we have since moved map_crash_pages() into x86/crash.c. And I missed
eliminating highmem.h at that time.
I have removed this for v9.
eric
>
>> +#include <linux/memory.h>
>> +#include <linux/cpuhotplug.h>
>>
>> #include <asm/page.h>
>> #include <asm/sections.h>
>>
>> #include <crypto/sha1.h>
>>
>> +#include "kexec_internal.h"
>> +
>> /* vmcoreinfo stuff */
>> unsigned char *vmcoreinfo_data;
>> size_t vmcoreinfo_size;
>> @@ -491,3 +496,90 @@ static int __init crash_save_vmcoreinfo_init(void)
>> }
>>
>> subsys_initcall(crash_save_vmcoreinfo_init);
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>> + unsigned int hp_action, unsigned int cpu)
>> +{
>> + WARN(1, "crash hotplug handler not implemented");
>> +}
>> +
>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> + /* Obtain lock while changing crash information */
>> + if (!mutex_trylock(&kexec_mutex))
>> + return;
>> +
>> + /* Check kdump is loaded */
>> + if (kexec_crash_image) {
>> + pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
>> +
>> + /* Needed in order for the segments to be updated */
>> + arch_kexec_unprotect_crashkres();
>> +
>> + /* Flag to differentiate between normal load and hotplug */
>> + kexec_crash_image->hotplug_event = true;
>> +
>> + /* Now invoke arch-specific update handler */
>> + arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>> +
>> + /* No longer handling a hotplug event */
>> + kexec_crash_image->hotplug_event = false;
>> +
>> + /* Change back to read-only */
>> + arch_kexec_protect_crashkres();
>> + }
>> +
>> + /* Release lock now that update complete */
>> + mutex_unlock(&kexec_mutex);
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> + switch (val) {
>> + case MEM_ONLINE:
>> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
>> + break;
>> +
>> + case MEM_OFFLINE:
>> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
>> + break;
>> + }
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> + .notifier_call = crash_memhp_notifier,
>> + .priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> + return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> + return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> + int result = 0;
>> +
>> + if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> + register_memory_notifier(&crash_memhp_nb);
>> +
>> + if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
>> + result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> + "crash/cpuhp",
>> + crash_cpuhp_online,
>> + crash_cpuhp_offline);
>> +
>> + return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>> --
>> 2.27.0
>>
>
On 05/09/22 at 10:43am, Eric DeVolder wrote:
>
>
> On 5/6/22 02:12, Baoquan He wrote:
> > On 05/05/22 at 02:45pm, Eric DeVolder wrote:
> > ......
> > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > > index 256cf6db573c..f197af50def6 100644
> > > --- a/kernel/crash_core.c
> > > +++ b/kernel/crash_core.c
> > > @@ -9,12 +9,17 @@
> > > #include <linux/init.h>
> > > #include <linux/utsname.h>
> > > #include <linux/vmalloc.h>
> > > +#include <linux/highmem.h>
> >
> > Wondering where highmem.h is needed. Just curious.
>
> Ahh, I missed that. At one point in time we moved map_crash_pages() into
> this file, which brought highmem.h along with it. But we have since moved
> map_crash_pages() into x86/crash.c. And I missed eliminating highmem.h at
> that time.
>
> I have removed this for v9.
That's nice, and you can add my ack when repost.
Acked-by: Baoquan He <[email protected]>
On 05.05.22 20:45, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
>
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
>
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
>
> Signed-off-by: Eric DeVolder <[email protected]>
[...]
> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> + unsigned int hp_action, unsigned int cpu)
> +{
> + WARN(1, "crash hotplug handler not implemented");
Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
triggering?
> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> + /* Obtain lock while changing crash information */
> + if (!mutex_trylock(&kexec_mutex))
> + return;
This looks wrong. What if you offline memory but for some reason the mutex
is currently locked? You'd miss updating the vmcore, which would be broken.
Why is this trylock in place? Some workaround for potential locking issues,
or what is the rationale?
> +
> + /* Check kdump is loaded */
> + if (kexec_crash_image) {
> + pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> + /* Needed in order for the segments to be updated */
> + arch_kexec_unprotect_crashkres();
> +
> + /* Flag to differentiate between normal load and hotplug */
> + kexec_crash_image->hotplug_event = true;
1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
information? I mean, *hotplug* in the anme implies that the function should be
aware what's happening.
2. Why can't the unprotect+reprotect not be done inside
arch_crash_handle_hotplug_event() ? It's all arch specific either way.
IMHO, this code here should be as simple as
if (kexec_crash_image)
arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
3. Why do we have to forward the CPU for CPU onlining/offlining but not the
memory block id (or similar) when onlining/offlining a memory block?
> +
> + /* Now invoke arch-specific update handler */
> + arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> + /* No longer handling a hotplug event */
> + kexec_crash_image->hotplug_event = false;
> +
> + /* Change back to read-only */
> + arch_kexec_protect_crashkres();
> + }
> +
> + /* Release lock now that update complete */
> + mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> + switch (val) {
> + case MEM_ONLINE:
> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> + break;
> +
> + case MEM_OFFLINE:
> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> + break;
> + }
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> + .notifier_call = crash_memhp_notifier,
> + .priority = 0
> +};
> +
> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> + return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> + return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> + int result = 0;
> +
> + if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> + register_memory_notifier(&crash_memhp_nb);
> +
> + if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> + result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> + "crash/cpuhp",
> + crash_cpuhp_online,
> + crash_cpuhp_offline);
Ehm, this indentation looks very weird.
> +
> + return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif
--
Thanks,
David / dhildenb
David,
Great questions! See inline responses below.
eric
On 5/12/22 03:52, David Hildenbrand wrote:
> On 05.05.22 20:45, Eric DeVolder wrote:
>> CPU and memory change notifications are received in order to
>> regenerate the elfcorehdr.
>>
>> To support cpu hotplug, a callback is registered to capture the
>> CPUHP_AP_ONLINE_DYN online and offline events via
>> cpuhp_setup_state_nocalls().
>>
>> To support memory hotplug, a notifier is registered to capture the
>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The cpu callback and memory notifiers call handle_hotplug_event()
>> to handle the hot plug/unplug event. Then handle_hotplug_event()
>> dispatches the event to the architecture specific
>> arch_crash_handle_hotplug_event(). During the process, the
>> kexec_mutex is held.
>>
>> Signed-off-by: Eric DeVolder <[email protected]>
>
> [...]
>
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>> + unsigned int hp_action, unsigned int cpu)
>> +{
>> + WARN(1, "crash hotplug handler not implemented");
>
>
> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
> triggering?
>
You're correct. What about: printk_once(KERN_DEBUG "...") ?
>> +}
>> +
>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> + /* Obtain lock while changing crash information */
>> + if (!mutex_trylock(&kexec_mutex))
>> + return;
>
> This looks wrong. What if you offline memory but for some reason the mutex
> is currently locked? You'd miss updating the vmcore, which would be broken.
>
> Why is this trylock in place? Some workaround for potential locking issues,
> or what is the rationale?
I took this from kernel/kexec.c:do_my_load(), but you are right, this is not
right.
*
* KISS: always take the mutex.
*/
if (!mutex_trylock(&kexec_mutex))
return -EBUSY;
This should simply be mutex_lock(&kexec_mutex).
>
>> +
>> + /* Check kdump is loaded */
>> + if (kexec_crash_image) {
>> + pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
>> +
>> + /* Needed in order for the segments to be updated */
>> + arch_kexec_unprotect_crashkres();
>> +
>> + /* Flag to differentiate between normal load and hotplug */
>> + kexec_crash_image->hotplug_event = true;
>
> 1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
> information? I mean, *hotplug* in the anme implies that the function should be
> aware what's happening.
Member .hotplug_event is needed in crash_prepare_elf64_headers(). To date, it has made
sense (to me) to pass this parameter to crash_prepare_elf64_headers() via the
struct kimage, rather than directly. The patch "crash: prototype change for
crash_prepare_elf64_headers" changes crash_prepare_elf64_headers() to accept the
struct kimage. If it is so desired, it could just as well be changed to pass just
this one parameter; though struct kimage seems a more useful way to go.
>
> 2. Why can't the unprotect+reprotect not be done inside
> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>
> IMHO, this code here should be as simple as
>
> if (kexec_crash_image)
> arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>
The intent of this code was to be generic infrastructure. Just invoking the
arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
But there were a series of steps that seemed to be common, so those I hoisted
into this bit of code.
> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
> memory block id (or similar) when onlining/offlining a memory block?
From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
still in the for_each_present_cpu() list when within the
handle_hotplug_event(). Thus the CPU must be explicitly excluded
when building the new list of CPUs.
This change identifies in handle_hotplug_event() the CPU to be
excluded, and the check for excluding the CPU in
crash_prepare_elf64_headers().
If there is a better CPUHP_ to use than _DYN, I'd be all for that!
>
>> +
>> + /* Now invoke arch-specific update handler */
>> + arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>> +
>> + /* No longer handling a hotplug event */
>> + kexec_crash_image->hotplug_event = false;
>> +
>> + /* Change back to read-only */
>> + arch_kexec_protect_crashkres();
>> + }
>> +
>> + /* Release lock now that update complete */
>> + mutex_unlock(&kexec_mutex);
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> + switch (val) {
>> + case MEM_ONLINE:
>> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
>> + break;
>> +
>> + case MEM_OFFLINE:
>> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
>> + break;
>> + }
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> + .notifier_call = crash_memhp_notifier,
>> + .priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> + return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> + return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> + int result = 0;
>> +
>> + if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> + register_memory_notifier(&crash_memhp_nb);
>> +
>> + if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
>> + result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> + "crash/cpuhp",
>> + crash_cpuhp_online,
>> + crash_cpuhp_offline);
>
> Ehm, this indentation looks very weird.
>
>> +
>> + return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>
>
Hello Eric,
On 06/05/22 00:15, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
>
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
>
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
>
> Signed-off-by: Eric DeVolder <[email protected]>
> ---
> include/linux/crash_core.h | 10 +++++
> include/linux/kexec.h | 5 +++
> kernel/crash_core.c | 92 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 107 insertions(+)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e..a240f84348aa 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
> int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
> unsigned long long *crash_size, unsigned long long *crash_base);
>
> +#define KEXEC_CRASH_HP_REMOVE_CPU 0
> +#define KEXEC_CRASH_HP_ADD_CPU 1
> +#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
> +#define KEXEC_CRASH_HP_ADD_MEMORY 3
> +#define KEXEC_CRASH_HP_INVALID_CPU -1U
> +
> +struct kimage;
> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> + unsigned int cpu);
> +
> #endif /* LINUX_CRASH_CORE_H */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index f93f2591fc1e..5935bc78ed7f 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -308,6 +308,11 @@ struct kimage {
> struct purgatory_info purgatory_info;
> #endif
>
> + bool hotplug_event;
> + unsigned int offlinecpu;
> + bool elfcorehdr_index_valid;
> + int elfcorehdr_index;
How about keeping above kimage elements under below config?
#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
Thanks,
Sourabh Jain
On 5/23/22 03:36, Sourabh Jain wrote:
> Hello Eric,
>
> On 06/05/22 00:15, Eric DeVolder wrote:
>> CPU and memory change notifications are received in order to
>> regenerate the elfcorehdr.
>>
>> To support cpu hotplug, a callback is registered to capture the
>> CPUHP_AP_ONLINE_DYN online and offline events via
>> cpuhp_setup_state_nocalls().
>>
>> To support memory hotplug, a notifier is registered to capture the
>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The cpu callback and memory notifiers call handle_hotplug_event()
>> to handle the hot plug/unplug event. Then handle_hotplug_event()
>> dispatches the event to the architecture specific
>> arch_crash_handle_hotplug_event(). During the process, the
>> kexec_mutex is held.
>>
>> Signed-off-by: Eric DeVolder <[email protected]>
>> ---
>> include/linux/crash_core.h | 10 +++++
>> include/linux/kexec.h | 5 +++
>> kernel/crash_core.c | 92 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 107 insertions(+)
>>
>> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
>> index de62a722431e..a240f84348aa 100644
>> --- a/include/linux/crash_core.h
>> +++ b/include/linux/crash_core.h
>> @@ -84,4 +84,14 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>> int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>> unsigned long long *crash_size, unsigned long long *crash_base);
>> +#define KEXEC_CRASH_HP_REMOVE_CPU 0
>> +#define KEXEC_CRASH_HP_ADD_CPU 1
>> +#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
>> +#define KEXEC_CRASH_HP_ADD_MEMORY 3
>> +#define KEXEC_CRASH_HP_INVALID_CPU -1U
>> +
>> +struct kimage;
>> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
>> + unsigned int cpu);
>> +
>> #endif /* LINUX_CRASH_CORE_H */
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index f93f2591fc1e..5935bc78ed7f 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -308,6 +308,11 @@ struct kimage {
>> struct purgatory_info purgatory_info;
>> #endif
>> + bool hotplug_event;
>> + unsigned int offlinecpu;
>> + bool elfcorehdr_index_valid;
>> + int elfcorehdr_index;
> How about keeping above kimage elements under below config?
> #if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>
> Thanks,
> Sourabh Jain
Yes, of course.
Eric
On 12.05.22 18:10, Eric DeVolder wrote:
> David,
> Great questions! See inline responses below.
> eric
Sorry for the late reply, travel and vacation ...
>>
>>> +
>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>> + unsigned int hp_action, unsigned int cpu)
>>> +{
>>> + WARN(1, "crash hotplug handler not implemented");
>>
>>
>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>> triggering?
>>
> You're correct. What about: printk_once(KERN_DEBUG "...") ?
Why even bother about printing anything? If the feature is not
supported, there should be some way for user space to figure out that it
sill has to reload on hot(un)plug manually, no?
[...]
>
>>
>> 2. Why can't the unprotect+reprotect not be done inside
>> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>>
>> IMHO, this code here should be as simple as
>>
>> if (kexec_crash_image)
>> arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>>
>
> The intent of this code was to be generic infrastructure. Just invoking the
> arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
> But there were a series of steps that seemed to be common, so those I hoisted
> into this bit of code.
But most common parts are actually arch_* calls already? :)
Anyhow, no strong opinion.
>
>> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
>> memory block id (or similar) when onlining/offlining a memory block?
> From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
>
> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
> still in the for_each_present_cpu() list when within the
> handle_hotplug_event(). Thus the CPU must be explicitly excluded
> when building the new list of CPUs.
>
> This change identifies in handle_hotplug_event() the CPU to be
> excluded, and the check for excluding the CPU in
> crash_prepare_elf64_headers().
>
> If there is a better CPUHP_ to use than _DYN, I'd be all for that!
Ah okay, thanks.
--
Thanks,
David / dhildenb
On 5/31/22 08:15, David Hildenbrand wrote:
> On 12.05.22 18:10, Eric DeVolder wrote:
>> David,
>> Great questions! See inline responses below.
>> eric
>
> Sorry for the late reply, travel and vacation ...
No problem, greatly appreciate the feedback!
eric
>
>>>
>>>> +
>>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>>> + unsigned int hp_action, unsigned int cpu)
>>>> +{
>>>> + WARN(1, "crash hotplug handler not implemented");
>>>
>>>
>>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>>> triggering?
>>>
>> You're correct. What about: printk_once(KERN_DEBUG "...") ?
>
> Why even bother about printing anything? If the feature is not
> supported, there should be some way for user space to figure out that it
> sill has to reload on hot(un)plug manually, no?
I've changed this to WARN_ONCE(). If that isn't agreeable, I'll remove it.
>
>
> [...]
>
>>
>>>
>>> 2. Why can't the unprotect+reprotect not be done inside
>>> arch_crash_handle_hotplug_event() ? It's all arch specific either way.
>>>
>>> IMHO, this code here should be as simple as
>>>
>>> if (kexec_crash_image)
>>> arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>>>
>>
>> The intent of this code was to be generic infrastructure. Just invoking the
>> arch_crash_handle_hotplug_event() would certainly be as generic as it gets.
>> But there were a series of steps that seemed to be common, so those I hoisted
>> into this bit of code.
>
> But most common parts are actually arch_* calls already? :)
>
> Anyhow, no strong opinion.
For the time being, I'd like to leave as is. Let's see if the detection of the elfcorehdr
segment gets moved to this code too... (discussion thread on "x86/crash: Add x86 crash hotplug
support for kexec_load".
>
>>
>>> 3. Why do we have to forward the CPU for CPU onlining/offlining but not the
>>> memory block id (or similar) when onlining/offlining a memory block?
>> From patch "kexec: exclude hot remove cpu from elfcorehdr notes" commit message:
>>
>> Due to use of CPUHP_AP_ONLINE_DYN, upon CPU unplug, the CPU is
>> still in the for_each_present_cpu() list when within the
>> handle_hotplug_event(). Thus the CPU must be explicitly excluded
>> when building the new list of CPUs.
>>
>> This change identifies in handle_hotplug_event() the CPU to be
>> excluded, and the check for excluding the CPU in
>> crash_prepare_elf64_headers().
>>
>> If there is a better CPUHP_ to use than _DYN, I'd be all for that!
>
> Ah okay, thanks.
>
On 01.06.22 00:25, Eric DeVolder wrote:
>
>
> On 5/31/22 08:15, David Hildenbrand wrote:
>> On 12.05.22 18:10, Eric DeVolder wrote:
>>> David,
>>> Great questions! See inline responses below.
>>> eric
>>
>> Sorry for the late reply, travel and vacation ...
> No problem, greatly appreciate the feedback!
> eric
>
>>
>>>>
>>>>> +
>>>>> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
>>>>> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
>>>>> + unsigned int hp_action, unsigned int cpu)
>>>>> +{
>>>>> + WARN(1, "crash hotplug handler not implemented");
>>>>
>>>>
>>>> Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
>>>> I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
>>>> triggering?
>>>>
>>> You're correct. What about: printk_once(KERN_DEBUG "...") ?
>>
>> Why even bother about printing anything? If the feature is not
>> supported, there should be some way for user space to figure out that it
>> sill has to reload on hot(un)plug manually, no?
>
> I've changed this to WARN_ONCE(). If that isn't agreeable, I'll remove it.
Please don't use WARN* on expected error paths.
--
Thanks,
David / dhildenb