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

[Sourabh Jain <sourabhjain@linux.ibm.com>]

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;