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

[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