diff mbox series

[v22,8/8] x86/crash: optimize CPU changes

Message ID 20230503224145.7405-9-eric.devolder@oracle.com
State New
Headers
Series crash: Kernel handling of CPU and memory hot un/plug |

Commit Message

Eric DeVolder May 3, 2023, 10:41 p.m. UTC 
  This patch is dependent upon the patch 'crash: change
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.

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.

This code is intentionally *NOT* hoisted into
crash_handle_hotplug_event() as it would prevent the arch-specific
handler from running for CPU changes. This would break PPC, for
example, which needs to update other information besides the
elfcorehdr, on CPU changes.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>
Acked-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/kernel/crash.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Thomas Gleixner May 9, 2023, 10:39 p.m. UTC | #1 
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/

> 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.

> 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.

> 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.

> +	 * 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?

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
Eric DeVolder May 10, 2023, 10:49 p.m. UTC | #2 
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
diff mbox series

Patch

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
+	 * 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.
+	 */
+	if ((image->file_mode || image->elfcorehdr_updated) &&
+		((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.