Re: [PATCH v18 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

From: Sourabh Jain
Date: Thu Mar 02 2023 - 00:24:25 EST


On 01/03/23 00:22, Eric DeVolder wrote:


On 2/28/23 06:44, Baoquan He wrote:
On 02/13/23 at 10:10am, Sourabh Jain wrote:

On 11/02/23 06:05, Eric DeVolder wrote:


On 2/10/23 00:29, Sourabh Jain wrote:

On 10/02/23 01:09, Eric DeVolder wrote:


On 2/9/23 12:43, Sourabh Jain wrote:
Hello Eric,

On 09/02/23 23:01, Eric DeVolder wrote:


On 2/8/23 07:44, Thomas Gleixner wrote:
Eric!

On Tue, Feb 07 2023 at 11:23, Eric DeVolder wrote:
On 2/1/23 05:33, Thomas Gleixner wrote:

So my latest solution is introduce two new CPUHP
states, CPUHP_AP_ELFCOREHDR_ONLINE
for onlining and CPUHP_BP_ELFCOREHDR_OFFLINE for
offlining. I'm open to better names.

The CPUHP_AP_ELFCOREHDR_ONLINE needs to be
placed after CPUHP_BRINGUP_CPU. My
attempts at locating this state failed when
inside the STARTING section, so I located
this just inside the ONLINE sectoin. The crash
hotplug handler is registered on
this state as the callback for the .startup method.

The CPUHP_BP_ELFCOREHDR_OFFLINE needs to be
placed before CPUHP_TEARDOWN_CPU, and I
placed it at the end of the PREPARE section.
This crash hotplug handler is also
registered on this state as the callback for the .teardown method.

TBH, that's still overengineered. Something like this:

bool cpu_is_alive(unsigned int cpu)
{
     struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);

     return data_race(st->state) <= CPUHP_AP_IDLE_DEAD;
}

and use this to query the actual state at crash
time. That spares all
those callback heuristics.

I'm making my way though percpu crash_notes,
elfcorehdr, vmcoreinfo,
makedumpfile and (the consumer of it all) the
userspace crash utility,
in order to understand the impact of moving from
for_each_present_cpu()
to for_each_online_cpu().

Is the packing actually worth the trouble? What's the actual win?

Thanks,

          tglx



Thomas,
I've investigated the passing of crash notes through the
vmcore. What I've learned is that:

- linux/fs/proc/vmcore.c (which makedumpfile references
to do its job) does
   not care what the contents of cpu PT_NOTES are, but it
does coalesce them together.

- makedumpfile will count the number of cpu PT_NOTES in
order to determine its
   nr_cpus variable, which is reported in a header, but
otherwise unused (except
   for sadump method).

- the crash utility, for the purposes of determining the
cpus, does not appear to
   reference the elfcorehdr PT_NOTEs. Instead it locates the various
   cpu_[possible|present|online]_mask and computes
nr_cpus from that, and also of
   course which are online. In addition, when crash does
reference the cpu PT_NOTE,
   to get its prstatus, it does so by using a percpu
technique directly in the vmcore
   image memory, not via the ELF structure. Said
differently, it appears to me that
   crash utility doesn't rely on the ELF PT_NOTEs for
cpus; rather it obtains them
   via kernel cpumasks and the memory within the vmcore.

With this understanding, I did some testing. Perhaps the
most telling test was that I
changed the number of cpu PT_NOTEs emitted in the
crash_prepare_elf64_headers() to just 1,
hot plugged some cpus, then also took a few offline
sparsely via chcpu, then generated a
vmcore. The crash utility had no problem loading the
vmcore, it reported the proper number
of cpus and the number offline (despite only one cpu
PT_NOTE), and changing to a different
cpu via 'set -c 30' and the backtrace was completely valid.

My take away is that crash utility does not rely upon
ELF cpu PT_NOTEs, it obtains the
cpu information directly from kernel data structures.
Perhaps at one time crash relied
upon the ELF information, but no more. (Perhaps there
are other crash dump analyzers
that might rely on the ELF info?)

So, all this to say that I see no need to change
crash_prepare_elf64_headers(). There
is no compelling reason to move away from
for_each_present_cpu(), or modify the list for
online/offline.

Which then leaves the topic of the cpuhp state on which
to register. Perhaps reverting
back to the use of CPUHP_BP_PREPARE_DYN is the right
answer. There does not appear to
be a compelling need to accurately track whether the cpu
went online/offline for the
purposes of creating the elfcorehdr, as ultimately the
crash utility pulls that from
kernel data structures, not the elfcorehdr.

I think this is what Sourabh has known and has been
advocating for an optimization
path that allows not regenerating the elfcorehdr on cpu
changes (because all the percpu
structs are all laid out). I do think it best to leave
that as an arch choice.

Since things are clear on how the PT_NOTES are consumed in
kdump kernel [fs/proc/vmcore.c],
makedumpfile, and crash tool I need your opinion on this:

Do we really need to regenerate elfcorehdr for CPU hotplug events?
If yes, can you please list the elfcorehdr components that
changes due to CPU hotplug.
Due to the use of for_each_present_cpu(), it is possible for the
number of cpu PT_NOTEs
to fluctuate as cpus are un/plugged. Onlining/offlining of cpus
does not impact the
number of cpu PT_NOTEs (as the cpus are still present).


  From what I understood, crash notes are prepared for
possible CPUs as system boots and
could be used to create a PT_NOTE section for each possible
CPU while generating the elfcorehdr
during the kdump kernel load.

Now once the elfcorehdr is loaded with PT_NOTEs for every
possible CPU there is no need to
regenerate it for CPU hotplug events. Or do we?

For onlining/offlining of cpus, there is no need to regenerate
the elfcorehdr. However,
for actual hot un/plug of cpus, the answer is yes due to
for_each_present_cpu(). The
caveat here of course is that if crash utility is the only
coredump analyzer of concern,
then it doesn't care about these cpu PT_NOTEs and there would be
no need to re-generate them.

Also, I'm not sure if ARM cpu hotplug, which is just now coming
into mainstream, impacts
any of this.

Perhaps the one item that might help here is to distinguish
between actual hot un/plug of
cpus, versus onlining/offlining. At the moment, I can not
distinguish between a hot plug
event and an online event (and unplug/offline). If those were
distinguishable, then we
could only regenerate on un/plug events.

Or perhaps moving to for_each_possible_cpu() is the better choice?

Yes, because once elfcorehdr is built with possible CPUs we don't
have to worry about
hot[un]plug case.

Here is my view on how things should be handled if a core-dump
analyzer is dependent on
elfcorehdr PT_NOTEs to find online/offline CPUs.

A PT_NOTE in elfcorehdr holds the address of the corresponding crash
notes (kernel has
one crash note per CPU for every possible CPU). Though the crash
notes are allocated
during the boot time they are populated when the system is on the
crash path.

This is how crash notes are populated on PowerPC and I am expecting
it would be something
similar on other architectures too.

The crashing CPU sends IPI to every other online CPU with a callback
function that updates the
crash notes of that specific CPU. Once the IPI completes the
crashing CPU updates its own crash
note and proceeds further.

The crash notes of CPUs remain uninitialized if the CPUs were
offline or hot unplugged at the time
system crash. The core-dump analyzer should be able to identify
[un]/initialized crash notes
and display the information accordingly.

Thoughts?

- Sourabh

In general, I agree with your points. You've presented a strong case to
go with for_each_possible_cpu() in crash_prepare_elf64_headers() and
those crash notes would always be present, and we can ignore changes to
cpus wrt/ elfcorehdr updates.

But what do we do about kexec_load() syscall? The way the userspace
utility works is it determines cpus by:
  nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
which is not the equivalent of possible_cpus. So the complete list of
cpu PT_NOTEs is not generated up front. We would need a solution for
that?
Hello Eric,

The sysconf document says _SC_NPROCESSORS_CONF is processors configured,
isn't that equivalent to possible CPUs?

What exactly sysconf(_SC_NPROCESSORS_CONF) returns on x86? IIUC, on powerPC
it is possible CPUs.

Baoquan,

 From sysconf man page, with my understanding, _SC_NPROCESSORS_CONF is
returning the possible cpus, while _SC_NPROCESSORS_ONLN returns present
cpus. If these are true, we can use them.

Thomas Gleixner has pointed out that:

 glibc tries to evaluate that in the following order:
  1) /sys/devices/system/cpu/cpu*
     That's present CPUs not possible CPUs
  2) /proc/stat
     That's online CPUs
  3) sched_getaffinity()
     That's online CPUs at best. In the worst case it's an affinity mask
     which is set on a process group

meaning that _SC_NPROCESSORS_CONF is not equivalent to possible_cpus(). Furthermore, the /sys/system/devices/cpus/cpuXX entries are not available for not-present-but-possible cpus; thus userspace kexec utility can not write out the elfcorehdr with all possible cpus listed.


But I am wondering why the existing present cpu way is going to be
discarded. Sorry, I tried to go through this thread, it's too long, can
anyone summarize the reason with shorter and clear sentences. Sorry
again for that.

Hello Eric,

By utilizing for_each_possible_cpu() in crash_prepare_elf64_headers(), in the case of the kexec_file_load(), this change would simplify some issues Sourabh has encountered for PPC support.

Things are fine even with for_each_present_cpu on PPC. It is just that I want to avoid
the regeneration of elfcorehdr for every CPU change by packing possible CPUs at once.


Thanks,
Sourabh Jain