lkml.org 
[lkml]   [2022]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 4/8] crash: add generic infrastructure for crash hotplug support
From


On 4/11/22 04:20, Baoquan He wrote:
> Hi Eric,
>
> On 04/01/22 at 02:30pm, Eric DeVolder wrote:
> ... ...
>
>> +static void crash_hotplug_handler(unsigned int hp_action,
>> + unsigned long a, unsigned long b)
>
> I am still struggling to consider if these unused parameters should be
> kept or removed. Do you foresee or feel on which ARCH they could be used?
>
> Considering our elfcorehdr updating method, once memory or cpu changed,
> we will update elfcorehdr and cpu notes to reflect all existing memory
> regions and cpu in the current system. We could end up with having them
> but never being used. Then we may finally need to clean them up.
>
> If you have investigated and foresee or feel they could be used on a
> certain architecture, we can keep them for the time being.

So 'hp_action' and 'a' are used within the existing patch series.
In crash_core.c, there is this bit of code:

+ kexec_crash_image->offlinecpu =
+ (hp_action == KEXEC_CRASH_HP_REMOVE_CPU) ?
+ (unsigned int)a : ~0U;

which is referencing both 'hp_action' and using 'a' from the cpu notifier handler.
I looked into removing 'a' and setting offlinecpu directly, but I thought
it better that offlinecpu be set within the safety of the kexec_mutex.
Also, Sourabh Jain's work with PowerPC utilizing this framework directly
references hp_action in the arch-specific handler.

The cpu and memory notifier handlers set hp_action accordingly. For cpu handler,
the 'a' is set with the impacted cpu. For memory handler, 'a' and 'b' form the
impacted memory range. I agree it looks like the memory range is currently
not useful.

Let me know what you think.
eric

>
>> +{
>> + /* Obtain lock while changing crash information */
>> + if (!mutex_trylock(&kexec_mutex))
>> + return;
>> +
>> + /* Check kdump is loaded */
>> + if (kexec_crash_image) {
>> + pr_debug("crash hp: hp_action %u, a %lu, b %lu", hp_action,
>> + a, b);
>> +
>> + /* 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;
>> +
>> + /* Now invoke arch-specific update handler */
>> + arch_crash_hotplug_handler(kexec_crash_image, hp_action, a, b);
>> +
>> + /* 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);
>> +}
>> +
>> +#if defined(CONFIG_MEMORY_HOTPLUG)
>> +static int crash_memhp_notifier(struct notifier_block *nb,
>> + unsigned long val, void *v)
>> +{
>> + struct memory_notify *mhp = v;
>> + unsigned long start, end;
>> +
>> + start = mhp->start_pfn << PAGE_SHIFT;
>> + end = ((mhp->start_pfn + mhp->nr_pages) << PAGE_SHIFT) - 1;
>> +
>> + switch (val) {
>> + case MEM_ONLINE:
>> + crash_hotplug_handler(KEXEC_CRASH_HP_ADD_MEMORY,
>> + start, end-start);
>> + break;
>> +
>> + case MEM_OFFLINE:
>> + crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_MEMORY,
>> + start, end-start);
>> + break;
>> + }
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> + .notifier_call = crash_memhp_notifier,
>> + .priority = 0
>> +};
>> +#endif
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU)
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> + crash_hotplug_handler(KEXEC_CRASH_HP_ADD_CPU, cpu, 0);
>> + return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> + crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_CPU, cpu, 0);
>> + return 0;
>> +}
>> +#endif
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> + int result = 0;
>> +
>> +#if defined(CONFIG_MEMORY_HOTPLUG)
>> + register_memory_notifier(&crash_memhp_nb);
>> +#endif
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU)
>> + result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> + "crash/cpuhp",
>> + crash_cpuhp_online, crash_cpuhp_offline);
>> +#endif
>> +
>> + return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif /* CONFIG_CRASH_HOTPLUG */
>> --
>> 2.27.0
>>
>

\
 
 \ /
  Last update: 2022-04-11 15:58    [W:0.106 / U:0.556 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site