lkml.org 
[lkml]   [2023]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v22 8/8] x86/crash: optimize CPU changes
From


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

\
 
 \ /
  Last update: 2023-05-11 00:50    [W:0.245 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site