Re: [PATCH] [RFC] crash: Lock-free crash hotplug support reporting

[Baoquan He]

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