Re: [PATCH v22 8/8] x86/crash: optimize CPU changes

[Eric DeVolder <eric.devolder@oracle.com>]

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec