Re: [PATCH v21 2/7] crash: add generic infrastructure for crash hotplug support

[Eric DeVolder]

On 4/6/23 06:04, Baoquan He wrote:
> On 04/04/23 at 02:03pm, Eric DeVolder wrote:
> ......
> > +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> > +{
> > +	struct kimage *image;
> > +
> > +	/* Obtain lock while changing crash information */
> > +	if (!kexec_trylock()) {
> > +		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> > +		return;
> > +	}
> > +
> > +	/* Check kdump is not loaded */
> > +	if (!kexec_crash_image)
> > +		goto out;
> > +
> > +	image = kexec_crash_image;
> > +
> > +	if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> > +		hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
> > +		pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
> > +	else
> > +		pr_debug("hp_action %u\n", hp_action);
>
> Seems we passed in the cpu number just for printing here. Wondering why
> we don't print out hot added/removed memory ranges. Is the cpu number
> printing necessary?
Baoquan,

Ah, actually until recently it was used to track the 'offlinecpu' in this function, but tglx pointed 
out that was un-necessary. That resulted in dropping the code in this function dealing with 
offlinecpu, leaving this as its only use in this function.

The printing of cpu number is not necessary, but helpful; I use it for debugging.

The printing of memory range is also not necessary, but in order to do that, should we choose to do 
so, requires passing in the memory range to this function. This patch series did do this early on, 
and by v7 I dropped it at your urging 
(https://lore.kernel.org/lkml/20220401183040.1624-1-eric.devolder@xxxxxxxxxx/). At the time, I 
provided it since I considered this generic infrastructure, but I could not defend it since x86 
didn't need it. However, PPC now needs this, and is now carrying this as part of PPC support of 
CRASH_HOTPLUG 
(https://lore.kernel.org/linuxppc-dev/20230312181154.278900-6-sourabhjain@xxxxxxxxxxxxx/T/#u).

If you'd rather I pickup the memory range handling again, I can do that. I think I'd likely change 
this function to be:

  void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu,
     struct memory_notify *mhp);

where on a CPU op the 'cpu' parameter would be valid and 'mhp' NULL, and on a memory op,
the 'mhp' would be valid and 'cpu' parameter invalid(0).

I'd likely then stuff these two parameters into struct kimage so that it can be utilized by 
arch-specific handler, if needed.

And of course, would print out the memory range for debug purposes.

Let me know what you think.
eric


> > +
> > +	/*
> > +	 * When the struct kimage is allocated, the elfcorehdr_index
> > +	 * is set to -1. Find the segment containing the elfcorehdr,
> > +	 * if not already found. This works for both the kexec_load
> > +	 * and kexec_file_load paths.
> > +	 */
> > +	if (image->elfcorehdr_index < 0) {
> > +		unsigned long mem;
> > +		unsigned char *ptr;
> > +		unsigned int n;
> > +
> > +		for (n = 0; n < image->nr_segments; n++) {
> > +			mem = image->segment[n].mem;
> > +			ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
> > +			if (ptr) {
> > +				/* The segment containing elfcorehdr */
> > +				if (memcmp(ptr, ELFMAG, SELFMAG) == 0)
> > +					image->elfcorehdr_index = (int)n;
> > +				kunmap_local(ptr);
> > +			}
> > +		}
> > +	}
> > +
> > +	if (image->elfcorehdr_index < 0) {
> > +		pr_err("unable to locate elfcorehdr segment");
> > +		goto out;
> > +	}
> > +
> > +	/* Needed in order for the segments to be updated */
> > +	arch_kexec_unprotect_crashkres();
> > +
> > +	/* Differentiate between normal load and hotplug update */
> > +	image->hp_action = hp_action;
> > +
> > +	/* Now invoke arch-specific update handler */
> > +	arch_crash_handle_hotplug_event(image);
> > +
> > +	/* No longer handling a hotplug event */
> > +	image->hp_action = KEXEC_CRASH_HP_NONE;
> > +	image->elfcorehdr_updated = true;
> > +
> > +	/* Change back to read-only */
> > +	arch_kexec_protect_crashkres();
> > +
> > +out:
> > +	/* Release lock now that update complete */
> > +	kexec_unlock();
> > +}
> > +
> > +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> > +{
> > +	switch (val) {
> > +	case MEM_ONLINE:
> > +		crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
> > +			KEXEC_CRASH_HP_INVALID_CPU);
> > +		break;
> > +
> > +	case MEM_OFFLINE:
> > +		crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
> > +			KEXEC_CRASH_HP_INVALID_CPU);
> > +		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)
> > +{
> > +	crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> > +	return 0;
> > +}
> > +
> > +static int crash_cpuhp_offline(unsigned int cpu)
> > +{
> > +	crash_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_BP_PREPARE_DYN,
> > +			"crash/cpuhp", crash_cpuhp_online, crash_cpuhp_offline);
> > +	}
> > +
> > +	return result;
> > +}
> > +
> > +subsys_initcall(crash_hotplug_init);
> > +#endif
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 3d578c6fefee..8296d019737c 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -277,6 +277,12 @@ struct kimage *do_kimage_alloc_init(void)
> >   	/* Initialize the list of unusable pages */
> >   	INIT_LIST_HEAD(&image->unusable_pages);
> >   
> > +#ifdef CONFIG_CRASH_HOTPLUG
> > +	image->hp_action = KEXEC_CRASH_HP_NONE;
> > +	image->elfcorehdr_index = -1;
> > +	image->elfcorehdr_updated = false;
> > +#endif
> > +
> >   	return image;
> >   }
> >   
> > --
> > 2.31.1