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;
>
