| Message ID | 20230201063841.965316-5-sourabhjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | In kernel handling of CPU hotplug events for crash kernel | expand |
On 2/1/23 00:38, Sourabh Jain wrote: > On architectures like PowerPC the crash notes are available for all > possible CPUs. So let's populate the elfcorehdr for all possible > CPUs having crash notes to avoid updating elfcorehdr during in-kernel > crash update on CPU hotplug events. > > The similar technique is used in kexec-tool for kexec_load case. > > Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > --- > kernel/crash_core.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index 37c594858fd51..898d8d2fe2e2e 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -364,8 +364,8 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem, > ehdr->e_ehsize = sizeof(Elf64_Ehdr); > ehdr->e_phentsize = sizeof(Elf64_Phdr); > > - /* Prepare one phdr of type PT_NOTE for each present CPU */ > - for_each_present_cpu(cpu) { > + /* Prepare one phdr of type PT_NOTE for possible CPU with crash note. */ > + for_each_possible_cpu(cpu) { Sourabh, Thomas Gleixner is suggesting moving away from for_each_present_cpu() to for_each_online_cpu(). Using for_each_online_cpu() is going to the minimum number of needed, whereas your approach of for_each_possible_cpu() would be to the maximum number needed. What would be the ramifications to ppc for moving towards for_each_online_cpu()? In my next patch series, I have finally figured out how to use cpuhp framework to where it is possible to use for_each_online_cpu() here, but that is at odds with your changes here. Thanks, eric > #ifdef CONFIG_CRASH_HOTPLUG > if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) { > /* Skip the soon-to-be offlined cpu */ > @@ -373,8 +373,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem, > continue; > } > #endif > - phdr->p_type = PT_NOTE; > notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu)); > + if (!notes_addr) > + continue; > + > + phdr->p_type = PT_NOTE; > phdr->p_offset = phdr->p_paddr = notes_addr; > phdr->p_filesz = phdr->p_memsz = sizeof(note_buf_t); > (ehdr->e_phnum)++;
On 2/2/23 09:37, Eric DeVolder wrote: > > > On 2/1/23 00:38, Sourabh Jain wrote: >> On architectures like PowerPC the crash notes are available for all >> possible CPUs. So let's populate the elfcorehdr for all possible >> CPUs having crash notes to avoid updating elfcorehdr during in-kernel >> crash update on CPU hotplug events. >> >> The similar technique is used in kexec-tool for kexec_load case. >> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >> --- >> kernel/crash_core.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >> index 37c594858fd51..898d8d2fe2e2e 100644 >> --- a/kernel/crash_core.c >> +++ b/kernel/crash_core.c >> @@ -364,8 +364,8 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem, >> ehdr->e_ehsize = sizeof(Elf64_Ehdr); >> ehdr->e_phentsize = sizeof(Elf64_Phdr); >> - /* Prepare one phdr of type PT_NOTE for each present CPU */ >> - for_each_present_cpu(cpu) { >> + /* Prepare one phdr of type PT_NOTE for possible CPU with crash note. */ >> + for_each_possible_cpu(cpu) { > > Sourabh, > Thomas Gleixner is suggesting moving away from for_each_present_cpu() to for_each_online_cpu(). > Using for_each_online_cpu() is going to the minimum number of needed, whereas your approach of > for_each_possible_cpu() would be to the maximum number needed. > > What would be the ramifications to ppc for moving towards for_each_online_cpu()? > > In my next patch series, I have finally figured out how to use cpuhp framework to where it is > possible to use for_each_online_cpu() here, but that is at odds with your changes here. > > Thanks, > eric > > Without knowing the ramifications of changing to for_each_online_cpu(), I currently am using the following: diff --git a/kernel/crash_core.c b/kernel/crash_core.c index e1a3430f06f4..a019b691d974 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -366,6 +366,9 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, int n /* Prepare one phdr of type PT_NOTE for each present CPU */ for_each_present_cpu(cpu) { + if (IS_ENABLED(CONFIG_CRASH_HOTPLUG)) { + if (!cpu_online(cpu)) continue; + } phdr->p_type = PT_NOTE; notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu)); phdr->p_offset = phdr->p_paddr = notes_addr; Thomas points out that the above can be simply the for_each_online_cpu(), but again I'm not sure how that impacts ppc, which appears to layout all possible cpus rather than just online ones. How are present but not online cpus handled by crash analysis utility? eric > >> #ifdef CONFIG_CRASH_HOTPLUG >> if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) { >> /* Skip the soon-to-be offlined cpu */ >> @@ -373,8 +373,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem, >> continue; >> } >> #endif >> - phdr->p_type = PT_NOTE; >> notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu)); >> + if (!notes_addr) >> + continue; >> + >> + phdr->p_type = PT_NOTE; >> phdr->p_offset = phdr->p_paddr = notes_addr; >> phdr->p_filesz = phdr->p_memsz = sizeof(note_buf_t); >> (ehdr->e_phnum)++;
On 02/02/23 21:07, Eric DeVolder wrote: > > > On 2/1/23 00:38, Sourabh Jain wrote: >> On architectures like PowerPC the crash notes are available for all >> possible CPUs. So let's populate the elfcorehdr for all possible >> CPUs having crash notes to avoid updating elfcorehdr during in-kernel >> crash update on CPU hotplug events. >> >> The similar technique is used in kexec-tool for kexec_load case. >> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >> --- >> kernel/crash_core.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >> index 37c594858fd51..898d8d2fe2e2e 100644 >> --- a/kernel/crash_core.c >> +++ b/kernel/crash_core.c >> @@ -364,8 +364,8 @@ int crash_prepare_elf64_headers(struct kimage >> *image, struct crash_mem *mem, >> ehdr->e_ehsize = sizeof(Elf64_Ehdr); >> ehdr->e_phentsize = sizeof(Elf64_Phdr); >> - /* Prepare one phdr of type PT_NOTE for each present CPU */ >> - for_each_present_cpu(cpu) { >> + /* Prepare one phdr of type PT_NOTE for possible CPU with crash >> note. */ >> + for_each_possible_cpu(cpu) { > > Sourabh, > Thomas Gleixner is suggesting moving away from for_each_present_cpu() > to for_each_online_cpu(). Using for_each_online_cpu() is going to the > minimum number of needed, whereas your approach of > for_each_possible_cpu() would be to the maximum number needed. > > What would be the ramifications to ppc for moving towards > for_each_online_cpu()? I was in the impression that if CPU notes are missing for offline CPUs in the elfcorehdr then makedumpfile will mess up the CPU IDs. But somehow replacing for_each_present_cpu with for_each_online_cpu worked on PowerPC, even after disabling a couple of CPUs. So things are fine if we pack PT_LOAD for online CPUs instead of present CPUs but, I need to investigate how makedumpfile is able to map PT_LOAD to online CPUs. - Sourabh Jain
On 02/02/23 21:07, Eric DeVolder wrote: > > > On 2/1/23 00:38, Sourabh Jain wrote: >> On architectures like PowerPC the crash notes are available for all >> possible CPUs. So let's populate the elfcorehdr for all possible >> CPUs having crash notes to avoid updating elfcorehdr during in-kernel >> crash update on CPU hotplug events. >> >> The similar technique is used in kexec-tool for kexec_load case. >> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >> --- >> kernel/crash_core.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >> index 37c594858fd51..898d8d2fe2e2e 100644 >> --- a/kernel/crash_core.c >> +++ b/kernel/crash_core.c >> @@ -364,8 +364,8 @@ int crash_prepare_elf64_headers(struct kimage >> *image, struct crash_mem *mem, >> ehdr->e_ehsize = sizeof(Elf64_Ehdr); >> ehdr->e_phentsize = sizeof(Elf64_Phdr); >> - /* Prepare one phdr of type PT_NOTE for each present CPU */ >> - for_each_present_cpu(cpu) { >> + /* Prepare one phdr of type PT_NOTE for possible CPU with crash >> note. */ >> + for_each_possible_cpu(cpu) { > > Sourabh, > Thomas Gleixner is suggesting moving away from for_each_present_cpu() > to for_each_online_cpu(). Using for_each_online_cpu() is going to the > minimum number of needed, whereas your approach of > for_each_possible_cpu() would be to the maximum number needed. > > What would be the ramifications to ppc for moving towards > for_each_online_cpu()? > > In my next patch series, I have finally figured out how to use cpuhp > framework to where it is possible to use for_each_online_cpu() here, > but that is at odds with your changes here. I was in the impression that if CPU notes are missing for offline CPUs in the elfcorehdr then makedumpfile will mess up the CPU IDs. But somehow replacing for_each_present_cpu with for_each_online_cpu worked on PowerPC, even after disabling a couple of CPUs. So things are fine if we pack PT_LOAD for online CPUs instead of present CPUs but, I need to investigate how makedumpfile is able to map PT_LOAD to online CPUs. - Sourabh Jain
On 03/02/23 02:31, Eric DeVolder wrote: > > > On 2/2/23 09:37, Eric DeVolder wrote: >> >> >> On 2/1/23 00:38, Sourabh Jain wrote: >>> On architectures like PowerPC the crash notes are available for all >>> possible CPUs. So let's populate the elfcorehdr for all possible >>> CPUs having crash notes to avoid updating elfcorehdr during in-kernel >>> crash update on CPU hotplug events. >>> >>> The similar technique is used in kexec-tool for kexec_load case. >>> >>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >>> --- >>> kernel/crash_core.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >>> index 37c594858fd51..898d8d2fe2e2e 100644 >>> --- a/kernel/crash_core.c >>> +++ b/kernel/crash_core.c >>> @@ -364,8 +364,8 @@ int crash_prepare_elf64_headers(struct kimage >>> *image, struct crash_mem *mem, >>> ehdr->e_ehsize = sizeof(Elf64_Ehdr); >>> ehdr->e_phentsize = sizeof(Elf64_Phdr); >>> - /* Prepare one phdr of type PT_NOTE for each present CPU */ >>> - for_each_present_cpu(cpu) { >>> + /* Prepare one phdr of type PT_NOTE for possible CPU with crash >>> note. */ >>> + for_each_possible_cpu(cpu) { >> >> Sourabh, >> Thomas Gleixner is suggesting moving away from for_each_present_cpu() >> to for_each_online_cpu(). Using for_each_online_cpu() is going to the >> minimum number of needed, whereas your approach of >> for_each_possible_cpu() would be to the maximum number needed. >> >> What would be the ramifications to ppc for moving towards >> for_each_online_cpu()? >> >> In my next patch series, I have finally figured out how to use cpuhp >> framework to where it is possible to use for_each_online_cpu() here, >> but that is at odds with your changes here. >> >> Thanks, >> eric >> >> > Without knowing the ramifications of changing to > for_each_online_cpu(), I currently am > using the following: > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index e1a3430f06f4..a019b691d974 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -366,6 +366,9 @@ int crash_prepare_elf64_headers(struct crash_mem > *mem, int n > > /* Prepare one phdr of type PT_NOTE for each present CPU */ > for_each_present_cpu(cpu) { > + if (IS_ENABLED(CONFIG_CRASH_HOTPLUG)) { > + if (!cpu_online(cpu)) continue; > + } How about let the arch decide the list of CPUs they want to pack in? Because on PowerPC the crash notes are created for possible CPUs and we can utilize this to avoid re-generating elfcorehdr for every hotplug operation. > phdr->p_type = PT_NOTE; > notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu)); > phdr->p_offset = phdr->p_paddr = notes_addr; > > Thomas points out that the above can be simply the > for_each_online_cpu(), but again > I'm not sure how that impacts ppc, > > which appears to layout all possible cpus rather > than just online ones. How are present but not online cpus handled by > crash analysis > utility? As per my testing all worked fine if we replace for_each_present_cpu with for_each_online_cpu but again I don't know the reason why it worked. I will investigate it and let you know. How packing PT_LOAD for present CPU is impacting x86? Because on PowerPC when system is on crash path it only populates the crash notes for online CPUs, and skip all other CPUs. - Sourabh Jain
diff --git a/kernel/crash_core.c b/kernel/crash_core.c index 37c594858fd51..898d8d2fe2e2e 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -364,8 +364,8 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem, ehdr->e_ehsize = sizeof(Elf64_Ehdr); ehdr->e_phentsize = sizeof(Elf64_Phdr); - /* Prepare one phdr of type PT_NOTE for each present CPU */ - for_each_present_cpu(cpu) { + /* Prepare one phdr of type PT_NOTE for possible CPU with crash note. */ + for_each_possible_cpu(cpu) { #ifdef CONFIG_CRASH_HOTPLUG if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) { /* Skip the soon-to-be offlined cpu */ @@ -373,8 +373,11 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem, continue; } #endif - phdr->p_type = PT_NOTE; notes_addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpu)); + if (!notes_addr) + continue; + + phdr->p_type = PT_NOTE; phdr->p_offset = phdr->p_paddr = notes_addr; phdr->p_filesz = phdr->p_memsz = sizeof(note_buf_t); (ehdr->e_phnum)++;
On architectures like PowerPC the crash notes are available for all possible CPUs. So let's populate the elfcorehdr for all possible CPUs having crash notes to avoid updating elfcorehdr during in-kernel crash update on CPU hotplug events. The similar technique is used in kexec-tool for kexec_load case. Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> --- kernel/crash_core.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)