| Message ID | 20240823115226.835865-1-sourabhjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] crash: Lock-free crash hotplug support reporting | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
Hello Baoquan, Do you think this patch would help reduce lock contention when CPU/Memory resources are removed in bulk from a system? Thanks, Sourabh Jain On 23/08/24 17:22, Sourabh Jain wrote: > On a CPU/Memory hotplug event, the kexec lock is taken to update the > kdump image. At the same time, this lock is also required to report > the support for crash hotplug to user-space via the > /sys/devices/system/[cpu|memory]/crash_hotplug sysfs interface, to > avoid kdump reload. > > The kexec lock is needed to report crash hotplug support because the > crash_hotplug variable, which tracks crash hotplug support, is part of > the kdump image, and the kdump image needs to be updated during a > hotplug event. > > Given that only one kdump image can be loaded at any given time, the > crash_hotplug variable can be placed outside the kdump image and set or > reset during kdump image load and unload. This allows crash hotplug > support to be reported without taking the kexec lock. > > This would help in situation where CPU/Memory resource are hotplug from > system in bulk. > > Commit e2a8f20dd8e9 ("Crash: add lock to serialize crash hotplug > handling") introduced to serialize the kexec lock during bulk CPU/Memory > hotplug events. However, with these changes, the kexec lock for crash > hotplug support reporting can be avoided altogether. > > Cc: Hari Bathini <hbathini@linux.ibm.com> > Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> > Cc: kexec@lists.infradead.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-kernel@vger.kernel.org > Cc: x86@kernel.org > Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > --- > include/linux/kexec.h | 11 ++++------- > kernel/crash_core.c | 27 +++++++++------------------ > kernel/kexec.c | 5 ++++- > kernel/kexec_file.c | 7 ++++++- > 4 files changed, 23 insertions(+), 27 deletions(-) > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > index f0e9f8eda7a3..bd755ba6bac4 100644 > --- a/include/linux/kexec.h > +++ b/include/linux/kexec.h > @@ -318,13 +318,6 @@ struct kimage { > unsigned int preserve_context : 1; > /* If set, we are using file mode kexec syscall */ > unsigned int file_mode:1; > -#ifdef CONFIG_CRASH_HOTPLUG > - /* If set, it is safe to update kexec segments that are > - * excluded from SHA calculation. > - */ > - unsigned int hotplug_support:1; > -#endif > - > #ifdef ARCH_HAS_KIMAGE_ARCH > struct kimage_arch arch; > #endif > @@ -370,6 +363,10 @@ struct kimage { > unsigned long elf_load_addr; > }; > > +#ifdef CONFIG_CRASH_HOTPLUG > +extern unsigned int crash_hotplug_support; > +#endif > + > /* kexec interface functions */ > extern void machine_kexec(struct kimage *image); > extern int machine_kexec_prepare(struct kimage *image); > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index 63cf89393c6e..3428deba0070 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -30,6 +30,13 @@ > #include "kallsyms_internal.h" > #include "kexec_internal.h" > > +#ifdef CONFIG_CRASH_HOTPLUG > +/* if set, it is safe to update kexec segments that are > + * excluded from sha calculation. > + */ > +unsigned int crash_hotplug_support; > +#endif > + > /* Per cpu memory for storing cpu states in case of system crash. */ > note_buf_t __percpu *crash_notes; > > @@ -500,23 +507,7 @@ static DEFINE_MUTEX(__crash_hotplug_lock); > */ > int crash_check_hotplug_support(void) > { > - int rc = 0; > - > - crash_hotplug_lock(); > - /* Obtain lock while reading crash information */ > - if (!kexec_trylock()) { > - pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n"); > - crash_hotplug_unlock(); > - return 0; > - } > - if (kexec_crash_image) { > - rc = kexec_crash_image->hotplug_support; > - } > - /* Release lock now that update complete */ > - kexec_unlock(); > - crash_hotplug_unlock(); > - > - return rc; > + return crash_hotplug_support; > } > > /* > @@ -552,7 +543,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu, > image = kexec_crash_image; > > /* Check that kexec segments update is permitted */ > - if (!image->hotplug_support) > + if (!crash_hotplug_support) > goto out; > > if (hp_action == KEXEC_CRASH_HP_ADD_CPU || > diff --git a/kernel/kexec.c b/kernel/kexec.c > index a6b3f96bb50c..d5c6b51eaa8b 100644 > --- a/kernel/kexec.c > +++ b/kernel/kexec.c > @@ -116,6 +116,9 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > /* Uninstall image */ > kimage_free(xchg(dest_image, NULL)); > ret = 0; > +#ifdef CONFIG_CRASH_HOTPLUG > + crash_hotplug_support = 0; > +#endif > goto out_unlock; > } > if (flags & KEXEC_ON_CRASH) { > @@ -136,7 +139,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > #ifdef CONFIG_CRASH_HOTPLUG > if ((flags & KEXEC_ON_CRASH) && arch_crash_hotplug_support(image, flags)) > - image->hotplug_support = 1; > + crash_hotplug_support = 1; > #endif > > ret = machine_kexec_prepare(image); > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index 3d64290d24c9..b326edb90fd7 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -378,7 +378,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > #ifdef CONFIG_CRASH_HOTPLUG > if ((flags & KEXEC_FILE_ON_CRASH) && arch_crash_hotplug_support(image, flags)) > - image->hotplug_support = 1; > + crash_hotplug_support = 1; > #endif > > ret = machine_kexec_prepare(image); > @@ -432,6 +432,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > arch_kexec_protect_crashkres(); > #endif > > +#ifdef CONFIG_CRASH_HOTPLUG > + if (flags & KEXEC_FILE_UNLOAD) > + crash_hotplug_support = 0; > +#endif > + > kexec_unlock(); > kimage_free(image); > return ret;
On 09/07/24 at 10:30am, Sourabh Jain wrote: > Hello Baoquan, > > Do you think this patch would help reduce lock contention when > CPU/Memory resources are removed in bulk from a system? .....snip... -- > > include/linux/kexec.h | 11 ++++------- > > kernel/crash_core.c | 27 +++++++++------------------ > > kernel/kexec.c | 5 ++++- > > kernel/kexec_file.c | 7 ++++++- > > 4 files changed, 23 insertions(+), 27 deletions(-) > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > index f0e9f8eda7a3..bd755ba6bac4 100644 > > --- a/include/linux/kexec.h > > +++ b/include/linux/kexec.h > > @@ -318,13 +318,6 @@ struct kimage { > > unsigned int preserve_context : 1; > > /* If set, we are using file mode kexec syscall */ > > unsigned int file_mode:1; > > -#ifdef CONFIG_CRASH_HOTPLUG > > - /* If set, it is safe to update kexec segments that are > > - * excluded from SHA calculation. > > - */ > > - unsigned int hotplug_support:1; > > -#endif > > - > > #ifdef ARCH_HAS_KIMAGE_ARCH > > struct kimage_arch arch; > > #endif > > @@ -370,6 +363,10 @@ struct kimage { > > unsigned long elf_load_addr; > > }; > > +#ifdef CONFIG_CRASH_HOTPLUG > > +extern unsigned int crash_hotplug_support; > > +#endif > > + > > /* kexec interface functions */ > > extern void machine_kexec(struct kimage *image); > > extern int machine_kexec_prepare(struct kimage *image); > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > > index 63cf89393c6e..3428deba0070 100644 > > --- a/kernel/crash_core.c > > +++ b/kernel/crash_core.c > > @@ -30,6 +30,13 @@ > > #include "kallsyms_internal.h" > > #include "kexec_internal.h" > > +#ifdef CONFIG_CRASH_HOTPLUG > > +/* if set, it is safe to update kexec segments that are > > + * excluded from sha calculation. > > + */ > > +unsigned int crash_hotplug_support; > > +#endif > > + > > /* Per cpu memory for storing cpu states in case of system crash. */ > > note_buf_t __percpu *crash_notes; > > @@ -500,23 +507,7 @@ static DEFINE_MUTEX(__crash_hotplug_lock); > > */ > > int crash_check_hotplug_support(void) > > { > > - int rc = 0; > > - > > - crash_hotplug_lock(); > > - /* Obtain lock while reading crash information */ > > - if (!kexec_trylock()) { > > - pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n"); > > - crash_hotplug_unlock(); > > - return 0; > > - } > > - if (kexec_crash_image) { > > - rc = kexec_crash_image->hotplug_support; > > - } > > - /* Release lock now that update complete */ > > - kexec_unlock(); > > - crash_hotplug_unlock(); > > - > > - return rc; > > + return crash_hotplug_support; I may not understand this well. Both kexec_load and kexec_file_load set hotplug_support, crash_check_hotplug_support and crash_handle_hotplug_event are to check the flag. How do you guarantee the cpu/memory sysfs checking won't have race with kexec_load and kexec_file_load? And here I see taking crash_hotplug_lock() is unnecessary in crash_check_hotplug_support() because it does't have race with crash_handle_hotplug_event(). > > } > > /* > > @@ -552,7 +543,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu, > > image = kexec_crash_image; > > /* Check that kexec segments update is permitted */ > > - if (!image->hotplug_support) > > + if (!crash_hotplug_support) > > goto out; > > if (hp_action == KEXEC_CRASH_HP_ADD_CPU || > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > index a6b3f96bb50c..d5c6b51eaa8b 100644 > > --- a/kernel/kexec.c > > +++ b/kernel/kexec.c > > @@ -116,6 +116,9 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > /* Uninstall image */ > > kimage_free(xchg(dest_image, NULL)); > > ret = 0; > > +#ifdef CONFIG_CRASH_HOTPLUG > > + crash_hotplug_support = 0; > > +#endif > > goto out_unlock; > > } > > if (flags & KEXEC_ON_CRASH) { > > @@ -136,7 +139,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > #ifdef CONFIG_CRASH_HOTPLUG > > if ((flags & KEXEC_ON_CRASH) && arch_crash_hotplug_support(image, flags)) > > - image->hotplug_support = 1; > > + crash_hotplug_support = 1; > > #endif > > ret = machine_kexec_prepare(image); > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > index 3d64290d24c9..b326edb90fd7 100644 > > --- a/kernel/kexec_file.c > > +++ b/kernel/kexec_file.c > > @@ -378,7 +378,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > #ifdef CONFIG_CRASH_HOTPLUG > > if ((flags & KEXEC_FILE_ON_CRASH) && arch_crash_hotplug_support(image, flags)) > > - image->hotplug_support = 1; > > + crash_hotplug_support = 1; > > #endif > > ret = machine_kexec_prepare(image); > > @@ -432,6 +432,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > arch_kexec_protect_crashkres(); > > #endif > > +#ifdef CONFIG_CRASH_HOTPLUG > > + if (flags & KEXEC_FILE_UNLOAD) > > + crash_hotplug_support = 0; > > +#endif > > + > > kexec_unlock(); > > kimage_free(image); > > return ret; >
diff --git a/include/linux/kexec.h b/include/linux/kexec.h index f0e9f8eda7a3..bd755ba6bac4 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -318,13 +318,6 @@ struct kimage { unsigned int preserve_context : 1; /* If set, we are using file mode kexec syscall */ unsigned int file_mode:1; -#ifdef CONFIG_CRASH_HOTPLUG - /* If set, it is safe to update kexec segments that are - * excluded from SHA calculation. - */ - unsigned int hotplug_support:1; -#endif - #ifdef ARCH_HAS_KIMAGE_ARCH struct kimage_arch arch; #endif @@ -370,6 +363,10 @@ struct kimage { unsigned long elf_load_addr; }; +#ifdef CONFIG_CRASH_HOTPLUG +extern unsigned int crash_hotplug_support; +#endif + /* kexec interface functions */ extern void machine_kexec(struct kimage *image); extern int machine_kexec_prepare(struct kimage *image); diff --git a/kernel/crash_core.c b/kernel/crash_core.c index 63cf89393c6e..3428deba0070 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -30,6 +30,13 @@ #include "kallsyms_internal.h" #include "kexec_internal.h" +#ifdef CONFIG_CRASH_HOTPLUG +/* if set, it is safe to update kexec segments that are + * excluded from sha calculation. + */ +unsigned int crash_hotplug_support; +#endif + /* Per cpu memory for storing cpu states in case of system crash. */ note_buf_t __percpu *crash_notes; @@ -500,23 +507,7 @@ static DEFINE_MUTEX(__crash_hotplug_lock); */ int crash_check_hotplug_support(void) { - int rc = 0; - - crash_hotplug_lock(); - /* Obtain lock while reading crash information */ - if (!kexec_trylock()) { - pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n"); - crash_hotplug_unlock(); - return 0; - } - if (kexec_crash_image) { - rc = kexec_crash_image->hotplug_support; - } - /* Release lock now that update complete */ - kexec_unlock(); - crash_hotplug_unlock(); - - return rc; + return crash_hotplug_support; } /* @@ -552,7 +543,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu, image = kexec_crash_image; /* Check that kexec segments update is permitted */ - if (!image->hotplug_support) + if (!crash_hotplug_support) goto out; if (hp_action == KEXEC_CRASH_HP_ADD_CPU || diff --git a/kernel/kexec.c b/kernel/kexec.c index a6b3f96bb50c..d5c6b51eaa8b 100644 --- a/kernel/kexec.c +++ b/kernel/kexec.c @@ -116,6 +116,9 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, /* Uninstall image */ kimage_free(xchg(dest_image, NULL)); ret = 0; +#ifdef CONFIG_CRASH_HOTPLUG + crash_hotplug_support = 0; +#endif goto out_unlock; } if (flags & KEXEC_ON_CRASH) { @@ -136,7 +139,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, #ifdef CONFIG_CRASH_HOTPLUG if ((flags & KEXEC_ON_CRASH) && arch_crash_hotplug_support(image, flags)) - image->hotplug_support = 1; + crash_hotplug_support = 1; #endif ret = machine_kexec_prepare(image); diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index 3d64290d24c9..b326edb90fd7 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -378,7 +378,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, #ifdef CONFIG_CRASH_HOTPLUG if ((flags & KEXEC_FILE_ON_CRASH) && arch_crash_hotplug_support(image, flags)) - image->hotplug_support = 1; + crash_hotplug_support = 1; #endif ret = machine_kexec_prepare(image); @@ -432,6 +432,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, arch_kexec_protect_crashkres(); #endif +#ifdef CONFIG_CRASH_HOTPLUG + if (flags & KEXEC_FILE_UNLOAD) + crash_hotplug_support = 0; +#endif + kexec_unlock(); kimage_free(image); return ret;
On a CPU/Memory hotplug event, the kexec lock is taken to update the kdump image. At the same time, this lock is also required to report the support for crash hotplug to user-space via the /sys/devices/system/[cpu|memory]/crash_hotplug sysfs interface, to avoid kdump reload. The kexec lock is needed to report crash hotplug support because the crash_hotplug variable, which tracks crash hotplug support, is part of the kdump image, and the kdump image needs to be updated during a hotplug event. Given that only one kdump image can be loaded at any given time, the crash_hotplug variable can be placed outside the kdump image and set or reset during kdump image load and unload. This allows crash hotplug support to be reported without taking the kexec lock. This would help in situation where CPU/Memory resource are hotplug from system in bulk. Commit e2a8f20dd8e9 ("Crash: add lock to serialize crash hotplug handling") introduced to serialize the kexec lock during bulk CPU/Memory hotplug events. However, with these changes, the kexec lock for crash hotplug support reporting can be avoided altogether. Cc: Hari Bathini <hbathini@linux.ibm.com> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> Cc: kexec@lists.infradead.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org Cc: x86@kernel.org Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> --- include/linux/kexec.h | 11 ++++------- kernel/crash_core.c | 27 +++++++++------------------ kernel/kexec.c | 5 ++++- kernel/kexec_file.c | 7 ++++++- 4 files changed, 23 insertions(+), 27 deletions(-)