Messages in this thread | | | Date | Wed, 10 May 2023 17:49:11 -0500 | Subject | Re: [PATCH v22 8/8] x86/crash: optimize CPU changes | From | Eric DeVolder <> |
| |
On 5/9/23 17:39, Thomas Gleixner wrote: > On Wed, May 03 2023 at 18:41, Eric DeVolder wrote: >> This patch is dependent upon the patch 'crash: change > > Seriously? You send a patch series which is ordered in itself and then > tell in the changelog of patch 8/8 that it depends on patch 7/8? > > This information is complete garbage once the patches are applied and > ends up in the git logs and even for the submission it's useless > information. > > Patch series are usually ordered by dependecy, no? > > Aside of that please do: > > # git grep 'This patch' Documentation/process/ > I'll remove, and re-examine the messages to use imperative tone.
>> crash_prepare_elf64_headers() to for_each_possible_cpu()'. With that >> patch, crash_prepare_elf64_headers() writes out an ELF CPU PT_NOTE >> for all possible CPUs, thus further CPU changes to the elfcorehdr >> are not needed. > > I'm having a hard time to decode this word salad. > > crash_prepare_elf64_headers() is writing out an ELF CPU PT_NOTE for > all possible CPUs, thus further changes to the ELF core header are > not required. > > Makes some sense to me.
How about this?
crash_prepare_elf64_headers() writes into the elfcorehdr an ELF PT_NOTE for all possible CPUs. As such, subsequent changes to CPUs (ie. hot un/plug, online/offline) do not need to rewrite the elfcorehdr.
> >> This change works for kexec_file_load() and kexec_load() syscalls. >> For kexec_file_load(), crash_prepare_elf64_headers() is utilized >> directly and thus all ELF CPU PT_NOTEs are in the elfcorehdr already. >> This is the kimage->file_mode term. >> For kexec_load() syscall, one CPU or memory change will cause the >> elfcorehdr to be updated via crash_prepare_elf64_headers() and at >> that point all ELF CPU PT_NOTEs are in the elfcorehdr. This is the >> kimage->elfcorehdr_updated term. > > Sorry. I tried hard, but this is completely incomprehensible. > How about this?
The kimage->file_mode term covers kdump images loaded via the kexec_file_load() syscall. Since crash_prepare_elf64_headers() wrote the initial elfcorehdr, no update to the elfcorehdr is needed for CPU changes.
The kimage->elfcorehdr_updated term covers kdump images loaded via the kexec_load() syscall. At least one memory or CPU change must occur to cause crash_prepare_elf64_headers() to rewrite the elfcorehdr. Afterwards, no update to the elfcorehdr is needed for CPU changes.
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c >> index 8064e65de6c0..3157e6068747 100644 >> --- a/arch/x86/kernel/crash.c >> +++ b/arch/x86/kernel/crash.c >> @@ -483,6 +483,16 @@ void arch_crash_handle_hotplug_event(struct kimage *image) >> unsigned long mem, memsz; >> unsigned long elfsz = 0; >> >> + /* As crash_prepare_elf64_headers() has already described all > > This is not a proper multiline comment. Please read and follow the tip > tree documentation along with all other things which are documented > there: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html > > This documentation is not there for entertainment value or exists just > because we are bored to death. > I'll fix it; unintentional. Should checkpatch.pl catch this (it did not)?
>> + * possible CPUs, there is no need to update the elfcorehdr >> + * for additional CPU changes. This works for both kexec_load() >> + * and kexec_file_load() syscalls. > > And it does not work for what? > I'll remove this.
I keep using phrases like this since kexec_file_load() is wholly controlled by the kernel code, where as kexec_load() has userspace dependencies. In this case,the sentence isn't warranted; it will work; no exceptional cases.
> You cannot expect that anyone who reads this code is an kexec/crash* > wizard who might be able to deduce the meaning of this. > > Thanks, > > tglx
Yes, thanks for the fresh eyes! eric
| |