kexec.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: David Hildenbrand <david@redhat.com>
To: kexec@lists.infradead.org
Subject: [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support
Date: Thu, 12 May 2022 10:52:02 +0200	[thread overview]
Message-ID: <62089f7b-4a3e-7dc8-1cda-84583e19d6fd@redhat.com> (raw)
In-Reply-To: <20220505184603.1548-4-eric.devolder@oracle.com>

On 05.05.22 20:45, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
> 
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().
> 
> To support memory hotplug, a notifier is registered to capture the
> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
> 
> The cpu callback and memory notifiers call handle_hotplug_event()
> to handle the hot plug/unplug event. Then handle_hotplug_event()
> dispatches the event to the architecture specific
> arch_crash_handle_hotplug_event(). During the process, the
> kexec_mutex is held.
> 
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

[...]

> +
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image,
> +							unsigned int hp_action, unsigned int cpu)
> +{
> +	WARN(1, "crash hotplug handler not implemented");


Won't that trigger on any arch that has CONFIG_HOTPLUG_CPU and CONFIG_MEMORY_HOTPLUG?
I mean, you only implement it for x86 later in this series. Or what else stops this WARN from
triggering?

> +}
> +
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (!mutex_trylock(&kexec_mutex))
> +		return;

This looks wrong. What if you offline memory but for some reason the mutex
is currently locked? You'd miss updating the vmcore, which would be broken.

Why is this trylock in place? Some workaround for potential locking issues,
or what is the rationale?

> +
> +	/* Check kdump is loaded */
> +	if (kexec_crash_image) {
> +		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +
> +		/* Needed in order for the segments to be updated */
> +		arch_kexec_unprotect_crashkres();
> +
> +		/* Flag to differentiate between normal load and hotplug */
> +		kexec_crash_image->hotplug_event = true;

1. Why is that required? Why can't arch_crash_handle_hotplug_event() forward that
information? I mean, *hotplug* in the anme implies that the function should be
aware what's happening.

2. Why can't the unprotect+reprotect not be done inside
arch_crash_handle_hotplug_event() ? It's all arch specific either way.

IMHO, this code here should be as simple as

if (kexec_crash_image)
	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);

3. Why do we have to forward the CPU for CPU onlining/offlining but not the
memory block id (or similar) when onlining/offlining a memory block?

> +
> +		/* Now invoke arch-specific update handler */
> +		arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
> +
> +		/* No longer handling a hotplug event */
> +		kexec_crash_image->hotplug_event = false;
> +
> +		/* Change back to read-only */
> +		arch_kexec_protect_crashkres();
> +	}
> +
> +	/* Release lock now that update complete */
> +	mutex_unlock(&kexec_mutex);
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> +	switch (val) {
> +	case MEM_ONLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0);
> +		break;
> +
> +	case MEM_OFFLINE:
> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0);
> +		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)
> +{
> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> +	return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> +	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_AP_ONLINE_DYN,
> +									"crash/cpuhp",
> +									crash_cpuhp_online,
> +									crash_cpuhp_offline);

Ehm, this indentation looks very weird.

> +
> +	return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif


-- 
Thanks,

David / dhildenb



  parent reply	other threads:[~2022-05-12  8:52 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-05 18:45 [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug Eric DeVolder
2022-05-05 18:45 ` [PATCH v8 1/7] x86/crash: fix minor typo/bug in debug message Eric DeVolder
     [not found]   ` <72764a3c-8b8c-8652-945e-9b15f31cda15@linux.ibm.com>
2022-05-09  5:26     ` Baoquan He
2022-05-09 15:41       ` Eric DeVolder
2022-05-05 18:45 ` [PATCH v8 2/7] crash: prototype change for crash_prepare_elf64_headers Eric DeVolder
2022-05-12  8:42   ` David Hildenbrand
2022-05-12 16:10     ` Eric DeVolder
2022-05-05 18:45 ` [PATCH v8 3/7] crash: add generic infrastructure for crash hotplug support Eric DeVolder
2022-05-06  7:12   ` Baoquan He
2022-05-09 15:43     ` Eric DeVolder
2022-05-11 10:09       ` Baoquan He
2022-05-12  8:52   ` David Hildenbrand [this message]
2022-05-12 16:10     ` Eric DeVolder
2022-05-31 13:15       ` David Hildenbrand
2022-05-31 22:25         ` Eric DeVolder
2022-06-15  9:53           ` David Hildenbrand
2022-05-23  8:36   ` Sourabh Jain
2022-05-23 15:04     ` Eric DeVolder
2022-05-05 18:46 ` [PATCH v8 4/7] kexec: exclude elfcorehdr from the segment digest Eric DeVolder
2022-05-11 10:11   ` Baoquan He
2022-05-05 18:46 ` [PATCH v8 5/7] kexec: exclude hot remove cpu from elfcorehdr notes Eric DeVolder
2022-05-11 10:13   ` Baoquan He
2022-05-05 18:46 ` [PATCH v8 6/7] x86/crash: Add x86 crash hotplug support for kexec_file_load Eric DeVolder
2022-05-25  5:25   ` Sourabh Jain
2022-05-25 13:51     ` Eric DeVolder
2022-05-05 18:46 ` [PATCH v8 7/7] x86/crash: Add x86 crash hotplug support for kexec_load Eric DeVolder
2022-05-25 14:26   ` Sourabh Jain
2022-05-31 22:18     ` Eric DeVolder
2022-05-25 15:13 ` [PATCH v8 0/7] crash: Kernel handling of CPU and memory hot un/plug Sourabh Jain
2022-05-26 13:16   ` Eric DeVolder
2022-05-26 13:39     ` Sourabh Jain
2022-05-26 13:44       ` Eric DeVolder
2022-05-31 13:18       ` David Hildenbrand
2022-05-31 22:22         ` Eric DeVolder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=62089f7b-4a3e-7dc8-1cda-84583e19d6fd@redhat.com \
    --to=david@redhat.com \
    --cc=kexec@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).