[PATCH v18 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

[Sourabh Jain]

On 25/02/23 01:46, Eric DeVolder wrote:
>
>
> On 2/24/23 02:34, Sourabh Jain wrote:
>>
>> On 24/02/23 02:04, Eric DeVolder wrote:
>>>
>>>
>>> On 2/10/23 00:29, Sourabh Jain wrote:
>>>>
>>>> On 10/02/23 01:09, Eric DeVolder wrote:
>>>>>
>>>>>
>>>>> On 2/9/23 12:43, Sourabh Jain wrote:
>>>>>> Hello Eric,
>>>>>>
>>>>>> On 09/02/23 23:01, Eric DeVolder wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/8/23 07:44, Thomas Gleixner wrote:
>>>>>>>> Eric!
>>>>>>>>
>>>>>>>> On Tue, Feb 07 2023 at 11:23, Eric DeVolder wrote:
>>>>>>>>> On 2/1/23 05:33, Thomas Gleixner wrote:
>>>>>>>>>
>>>>>>>>> So my latest solution is introduce two new CPUHP states, 
>>>>>>>>> CPUHP_AP_ELFCOREHDR_ONLINE
>>>>>>>>> for onlining and CPUHP_BP_ELFCOREHDR_OFFLINE for offlining. 
>>>>>>>>> I'm open to better names.
>>>>>>>>>
>>>>>>>>> The CPUHP_AP_ELFCOREHDR_ONLINE needs to be placed after 
>>>>>>>>> CPUHP_BRINGUP_CPU. My
>>>>>>>>> attempts at locating this state failed when inside the 
>>>>>>>>> STARTING section, so I located
>>>>>>>>> this just inside the ONLINE sectoin. The crash hotplug handler 
>>>>>>>>> is registered on
>>>>>>>>> this state as the callback for the .startup method.
>>>>>>>>>
>>>>>>>>> The CPUHP_BP_ELFCOREHDR_OFFLINE needs to be placed before 
>>>>>>>>> CPUHP_TEARDOWN_CPU, and I
>>>>>>>>> placed it at the end of the PREPARE section. This crash 
>>>>>>>>> hotplug handler is also
>>>>>>>>> registered on this state as the callback for the .teardown 
>>>>>>>>> method.
>>>>>>>>
>>>>>>>> TBH, that's still overengineered. Something like this:
>>>>>>>>
>>>>>>>> bool cpu_is_alive(unsigned int cpu)
>>>>>>>> {
>>>>>>>>     struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
>>>>>>>>
>>>>>>>>     return data_race(st->state) <= CPUHP_AP_IDLE_DEAD;
>>>>>>>> }
>>>>>>>>
>>>>>>>> and use this to query the actual state at crash time. That 
>>>>>>>> spares all
>>>>>>>> those callback heuristics.
>>>>>>>>
>>>>>>>>> I'm making my way though percpu crash_notes, elfcorehdr, 
>>>>>>>>> vmcoreinfo,
>>>>>>>>> makedumpfile and (the consumer of it all) the userspace crash 
>>>>>>>>> utility,
>>>>>>>>> in order to understand the impact of moving from 
>>>>>>>>> for_each_present_cpu()
>>>>>>>>> to for_each_online_cpu().
>>>>>>>>
>>>>>>>> Is the packing actually worth the trouble? What's the actual win?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>>          tglx
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Thomas,
>>>>>>> I've investigated the passing of crash notes through the vmcore. 
>>>>>>> What I've learned is that:
>>>>>>>
>>>>>>> - linux/fs/proc/vmcore.c (which makedumpfile references to do 
>>>>>>> its job) does
>>>>>>>   not care what the contents of cpu PT_NOTES are, but it does 
>>>>>>> coalesce them together.
>>>>>>>
>>>>>>> - makedumpfile will count the number of cpu PT_NOTES in order to 
>>>>>>> determine its
>>>>>>>   nr_cpus variable, which is reported in a header, but otherwise 
>>>>>>> unused (except
>>>>>>>   for sadump method).
>>>>>>>
>>>>>>> - the crash utility, for the purposes of determining the cpus, 
>>>>>>> does not appear to
>>>>>>>   reference the elfcorehdr PT_NOTEs. Instead it locates the various
>>>>>>>   cpu_[possible|present|online]_mask and computes nr_cpus from 
>>>>>>> that, and also of
>>>>>>>   course which are online. In addition, when crash does 
>>>>>>> reference the cpu PT_NOTE,
>>>>>>>   to get its prstatus, it does so by using a percpu technique 
>>>>>>> directly in the vmcore
>>>>>>>   image memory, not via the ELF structure. Said differently, it 
>>>>>>> appears to me that
>>>>>>>   crash utility doesn't rely on the ELF PT_NOTEs for cpus; 
>>>>>>> rather it obtains them
>>>>>>>   via kernel cpumasks and the memory within the vmcore.
>>>>>>>
>>>>>>> With this understanding, I did some testing. Perhaps the most 
>>>>>>> telling test was that I
>>>>>>> changed the number of cpu PT_NOTEs emitted in the 
>>>>>>> crash_prepare_elf64_headers() to just 1,
>>>>>>> hot plugged some cpus, then also took a few offline sparsely via 
>>>>>>> chcpu, then generated a
>>>>>>> vmcore. The crash utility had no problem loading the vmcore, it 
>>>>>>> reported the proper number
>>>>>>> of cpus and the number offline (despite only one cpu PT_NOTE), 
>>>>>>> and changing to a different
>>>>>>> cpu via 'set -c 30' and the backtrace was completely valid.
>>>>>>>
>>>>>>> My take away is that crash utility does not rely upon ELF cpu 
>>>>>>> PT_NOTEs, it obtains the
>>>>>>> cpu information directly from kernel data structures. Perhaps at 
>>>>>>> one time crash relied
>>>>>>> upon the ELF information, but no more. (Perhaps there are other 
>>>>>>> crash dump analyzers
>>>>>>> that might rely on the ELF info?)
>>>>>>>
>>>>>>> So, all this to say that I see no need to change 
>>>>>>> crash_prepare_elf64_headers(). There
>>>>>>> is no compelling reason to move away from 
>>>>>>> for_each_present_cpu(), or modify the list for
>>>>>>> online/offline.
>>>>>>>
>>>>>>> Which then leaves the topic of the cpuhp state on which to 
>>>>>>> register. Perhaps reverting
>>>>>>> back to the use of CPUHP_BP_PREPARE_DYN is the right answer. 
>>>>>>> There does not appear to
>>>>>>> be a compelling need to accurately track whether the cpu went 
>>>>>>> online/offline for the
>>>>>>> purposes of creating the elfcorehdr, as ultimately the crash 
>>>>>>> utility pulls that from
>>>>>>> kernel data structures, not the elfcorehdr.
>>>>>>>
>>>>>>> I think this is what Sourabh has known and has been advocating 
>>>>>>> for an optimization
>>>>>>> path that allows not regenerating the elfcorehdr on cpu changes 
>>>>>>> (because all the percpu
>>>>>>> structs are all laid out). I do think it best to leave that as 
>>>>>>> an arch choice.
>>>>>>
>>>>>> Since things are clear on how the PT_NOTES are consumed in kdump 
>>>>>> kernel [fs/proc/vmcore.c],
>>>>>> makedumpfile, and crash tool I need your opinion on this:
>>>>>>
>>>>>> Do we really need to regenerate elfcorehdr for CPU hotplug events?
>>>>>> If yes, can you please list the elfcorehdr components that 
>>>>>> changes due to CPU hotplug.
>>>>> Due to the use of for_each_present_cpu(), it is possible for the 
>>>>> number of cpu PT_NOTEs
>>>>> to fluctuate as cpus are un/plugged. Onlining/offlining of cpus 
>>>>> does not impact the
>>>>> number of cpu PT_NOTEs (as the cpus are still present).
>>>>>
>>>>>>
>>>>>>  From what I understood, crash notes are prepared for possible 
>>>>>> CPUs as system boots and
>>>>>> could be used to create a PT_NOTE section for each possible CPU 
>>>>>> while generating the elfcorehdr
>>>>>> during the kdump kernel load.
>>>>>>
>>>>>> Now once the elfcorehdr is loaded with PT_NOTEs for every 
>>>>>> possible CPU there is no need to
>>>>>> regenerate it for CPU hotplug events. Or do we?
>>>>>
>>>>> For onlining/offlining of cpus, there is no need to regenerate the 
>>>>> elfcorehdr. However,
>>>>> for actual hot un/plug of cpus, the answer is yes due to 
>>>>> for_each_present_cpu(). The
>>>>> caveat here of course is that if crash utility is the only 
>>>>> coredump analyzer of concern,
>>>>> then it doesn't care about these cpu PT_NOTEs and there would be 
>>>>> no need to re-generate them.
>>>>>
>>>>> Also, I'm not sure if ARM cpu hotplug, which is just now coming 
>>>>> into mainstream, impacts
>>>>> any of this.
>>>>>
>>>>> Perhaps the one item that might help here is to distinguish 
>>>>> between actual hot un/plug of
>>>>> cpus, versus onlining/offlining. At the moment, I can not 
>>>>> distinguish between a hot plug
>>>>> event and an online event (and unplug/offline). If those were 
>>>>> distinguishable, then we
>>>>> could only regenerate on un/plug events.
>>>>>
>>>>> Or perhaps moving to for_each_possible_cpu() is the better choice?
>>>>
>>>> Yes, because once elfcorehdr is built with possible CPUs we don't 
>>>> have to worry about
>>>> hot[un]plug case.
>>>>
>>>> Here is my view on how things should be handled if a core-dump 
>>>> analyzer is dependent on
>>>> elfcorehdr PT_NOTEs to find online/offline CPUs.
>>>>
>>>> A PT_NOTE in elfcorehdr holds the address of the corresponding 
>>>> crash notes (kernel has
>>>> one crash note per CPU for every possible CPU). Though the crash 
>>>> notes are allocated
>>>> during the boot time they are populated when the system is on the 
>>>> crash path.
>>>>
>>>> This is how crash notes are populated on PowerPC and I am expecting 
>>>> it would be something
>>>> similar on other architectures too.
>>>>
>>>> The crashing CPU sends IPI to every other online CPU with a 
>>>> callback function that updates the
>>>> crash notes of that specific CPU. Once the IPI completes the 
>>>> crashing CPU updates its own crash
>>>> note and proceeds further.
>>>>
>>>> The crash notes of CPUs remain uninitialized if the CPUs were 
>>>> offline or hot unplugged at the time
>>>> system crash. The core-dump analyzer should be able to identify 
>>>> [un]/initialized crash notes
>>>> and display the information accordingly.
>>>>
>>>> Thoughts?
>>>>
>>>> - Sourabh
>>>
>>> I've been examining what it would mean to move to 
>>> for_each_possible_cpu() in crash_prepare_elf64_headers(). I think it 
>>> means:
>>>
>>> - Changing for_each_present_cpu() to for_each_possible_cpu() in 
>>> crash_prepare_elf64_headers().
>>> - For kexec_load() syscall path, rewrite the incoming/supplied 
>>> elfcorehdr immediately on the load with the elfcorehdr generated by 
>>> crash_prepare_elf64_headers().
>>> - Eliminate/remove the cpuhp machinery for handling crash hotplug 
>>> events.
>>
>> If for_each_present_cpu is replaced with for_each_possible_cpu I 
>> still need cpuhp machinery
>> to update FDT kexec segment for CPU hot add case.
>
> Ah, ok, that's important! So the cpuhp callbacks are still needed.
>>
>>
>>>
>>> This would then setup PT_NOTEs for all possible cpus, which should 
>>> in theory accommodate crash analyzers that rely on ELF PT_NOTEs for 
>>> crash_notes.
>>>
>>> If staying with for_each_present_cpu() is ultimately decided, then I 
>>> think leaving the cpuhp machinery in place and each arch could 
>>> decide how to handle crash cpu hotplug events. The overhead for 
>>> doing this is very minimal, and the events are likely very infrequent.
>>
>> I agree. Some architectures may need cpuhp machinery to update kexec 
>> segment[s] other then elfcorehdr. For example FDT on PowerPC.
>>
>> - Sourabh Jain
>
> OK, I was thinking that the desire was to eliminate the cpuhp 
> callbacks. In reality, the desire is to change to 
> for_each_possible_cpu(). Given that the kernel creates crash_notes for 
> all possible cpus upon kernel boot, there seems to be no reason to not 
> do this?
>
> HOWEVER...
>
> It's not clear to me that this particular change needs to be part of 
> this series. It's inclusion would facilitate PPC support, but doesn't 
> "solve" anything in general. In fact it causes kexec_load and 
> kexec_file_load to deviate (kexec_load via userspace kexec does the 
> equivalent of for_each_present_cpu() where as with this change 
> kexec_file_load would do for_each_possible_cpu(); until a hot plug 
> event then both would do for_each_possible_cpu()). And if this change 
> were to arrive as part of Sourabh's PPC support, then it does not 
> appear to impact x86 (not sure about other arches). And the 'crash' 
> dump analyzer doesn't care either way.
>
> Including this change would enable an optimization path (for x86 at 
> least) that short-circuits cpu hotplug changes in the arch crash 
> handler, for example:
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index aca3f1817674..0883f6b11de4 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -473,6 +473,11 @@ void arch_crash_handle_hotplug_event(struct 
> kimage *image)
>     unsigned long mem, memsz;
>     unsigned long elfsz = 0;
>
> +   if (image->file_mode && (
> +       image->hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> +       image->hp_action == KEXEC_CRASH_HP_REMOVE_CPU))
> +       return;
> +
>     /*
>      * Create the new elfcorehdr reflecting the changes to CPU and/or
>      * memory resources.
>
> I'm not sure that is compelling given the infrequent nature of cpu 
> hotplug events.
It certainly closes/reduces the window where kdump is not active due 
kexec segment update.|

>
> In my mind I still have a question about kexec_load() path. The 
> userspace kexec can not do the equivalent of for_each_possible_cpu(). 
> It can obtain max possible cpus from /sys/devices/system/cpu/possible, 
> but for those cpus not present the /sys/devices/system/cpu/cpuXX is 
> not available and so the crash_notes entries is not available. My 
> attempts to expose all cpuXX lead to odd behavior that was requiring 
> changes in ACPI and arch code that looked untenable.
>
> There seem to be these options available for kexec_load() path:
> - immediately rewrite the elfcorehdr upon load via a call to 
> crash_prepare_elf64_headers(). I've made this work with the following, 
> as proof of concept:
Yes regenerating/patching the elfcorehdr could be an option for 
kexec_load syscall.

>
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index cb8e6e6f983c..4eb201270f97 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -163,6 +163,12 @@ static int do_kexec_load(unsigned long entry, 
> unsigned long
>     kimage_free(image);
>  out_unlock:
>     kexec_unlock();
> +   if (IS_ENABLED(CONFIG_CRASH_HOTPLUG)) {
> +       if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) {
> +           crash_handle_hotplug_event(KEXEC_CRASH_HP_NONE, 
> KEXEC_CRASH_HP_INVALID_CPU);
> +       }
> +   }
>     return ret;
>  }
>
> - Another option is spend the time to determine whether exposing all 
> cpuXX is a viable solution; I have no idea what impacts to userspace 
> would be for possible-but-not-yet-present cpuXX entries would be. It 
> might also mean requiring a 'present' entry available within the cpuXX.
>
> - Another option is to simply let the hot plug events rewrite the 
> elfcorehdr on demand. This is what I've originally put forth, but not 
> sure how this impacts PPC given for_each_possible_cpu() change.
Given that /sys/devices/system/cpu/cpuXX is not present for 
possbile-but-not-yet-present CPUs, I am wondering do we even have crash 
notes for possible CPUs on x86?
>
> The concern is that today, both kexec_load and kexec_file_load mirror 
> each other with respect to for_each_present_cpu(); that is userspace 
> kexec is able to generate the elfcorehdr the same as would 
> kexec_file_load, for cpus. But by changing to for_each_possible_cpu(), 
> the two would deviate.

Thanks,
Sourabh Jain