Messages in this thread | | | Date | Wed, 26 Oct 2022 12:30:02 +0530 | Subject | Re: [PATCH v12 3/7] crash: add generic infrastructure for crash hotplug support | From | Sourabh Jain <> |
| |
Hello Baoquan,
On 24/10/22 14:40, Baoquan He wrote: > Hi Eric, Sourabh, > > On 10/07/22 at 02:14pm, Eric DeVolder wrote: >> >> On 10/3/22 12:51, Sourabh Jain wrote: >>> Hello Eric, >>> >>> On 10/09/22 02:35, Eric DeVolder wrote: > ...... >>>> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu) >>>> +{ >>>> + /* Obtain lock while changing crash information */ >>>> + mutex_lock(&kexec_mutex); >>>> + >>>> + /* Check kdump is loaded */ >>>> + if (kexec_crash_image) { >>>> + struct kimage *image = kexec_crash_image; >>>> + >>>> + if (hp_action == KEXEC_CRASH_HP_ADD_CPU || >>>> + hp_action == KEXEC_CRASH_HP_REMOVE_CPU) >>>> + pr_debug("crash hp: hp_action %u, cpu %u\n", hp_action, cpu); >>>> + else >>>> + pr_debug("crash hp: hp_action %u\n", hp_action); >>>> + >>>> + /* >>>> + * When the struct kimage is allocated, it is wiped to zero, so >>>> + * the elfcorehdr_index_valid defaults to false. Find the >>>> + * segment containing the elfcorehdr, if not already found. >>>> + * This works for both the kexec_load and kexec_file_load paths. >>>> + */ >>>> + if (!image->elfcorehdr_index_valid) { >>>> + unsigned char *ptr; >>>> + unsigned long mem, memsz; >>>> + unsigned int n; >>>> + >>>> + for (n = 0; n < image->nr_segments; n++) { >>>> + mem = image->segment[n].mem; >>>> + memsz = image->segment[n].memsz; >>>> + ptr = arch_map_crash_pages(mem, memsz); >>>> + if (ptr) { >>>> + /* The segment containing elfcorehdr */ >>>> + if (memcmp(ptr, ELFMAG, SELFMAG) == 0) { >>>> + image->elfcorehdr_index = (int)n; >>>> + image->elfcorehdr_index_valid = true; >>>> + } >>>> + } >>>> + arch_unmap_crash_pages((void **)&ptr); >>>> + } >>>> + } >>>> + >>>> + if (!image->elfcorehdr_index_valid) { >>>> + pr_err("crash hp: unable to locate elfcorehdr segment"); >>>> + goto out; >>>> + } >>>> + >>>> + /* Needed in order for the segments to be updated */ >>>> + arch_kexec_unprotect_crashkres(); >>>> + >>>> + /* Flag to differentiate between normal load and hotplug */ >>>> + image->hotplug_event = true; >>>> + >>>> + /* Now invoke arch-specific update handler */ >>>> + arch_crash_handle_hotplug_event(image, hp_action); >>>> + >>>> + /* No longer handling a hotplug event */ >>>> + image->hotplug_event = false; >>>> + >>>> + /* Change back to read-only */ >>>> + arch_kexec_protect_crashkres(); >>>> + } >>>> + >>>> +out: >>>> + /* Release lock now that update complete */ >>>> + mutex_unlock(&kexec_mutex); >>>> +} >>>> + >>>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v) >>>> +{ >>>> + switch (val) { >>>> + case MEM_ONLINE: >>>> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, 0); >>>> + break; >>>> + >>>> + case MEM_OFFLINE: >>>> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, 0); >>>> + break; >>>> + } >>>> + return NOTIFY_OK; >>> Can we pass v (memory_notify) argument to arch_crash_handle_hotplug_event function >>> via handle_hotplug_event? >>> >>> Because the way memory hotplug is handled on PowerPC, it is hard to update the elfcorehdr >>> without memory_notify args. >>> >>> On PowePC memblock data structure is used to prepare elfcorehdr for kdump. Since the notifier >>> used for memory hotplug crash handler get initiated before the memblock data structure update >>> happens (as depicted below), the newly prepared elfcorehdr still holds the old memory regions. >>> So if the system crash with obsolete elfcorehdr, makedumpfile failed to collect vmcore. >>> >>> Sequence of actions done on PowerPC to server the memory hotplug: >>> >>> Initiate memory hot remove >>> | >>> v >>> offline pages >>> | >>> v >>> initiate memory notify call chain >>> for MEM_OFFLINE event. >>> (same is used for crash update) >>> | >>> v >>> prepare new elfcorehdr for kdump using >>> memblock data structure >>> | >>> v >>> update memblock data structure >>> >>> How passing memory_notify to arch crash hotplug handler will help? >>> >>> memory_notify holds the start PFN and page count, with that we can get >>> the base address and size of hot unplugged memory and can use the same >>> to avoid hot unplugged memeory region to get added in the elfcorehdr.. >>> >>> Thanks, >>> Sourabh Jain >>> >> Sourabh, let's see what Baoquan thinks. >> >> Baoquan, are you OK with this request? I once had these parameters to the >> crash hotplug handler and since they were unused at the time, you asked >> that I remove them, which I did. > Sorry to miss this mail. I thought both of you were talking about > somthing, and didn't notice this question to me. > > I think there are two ways to solve the issue Sourabh raised: > 1) make handle_hotplug_event() get and pass down the memory_notify as > Sourabh said, or the hp_action, mem_start|size as Eric suggested. I > have to admit I haven't carefully checked which one is better. > > 2) let the current code as is since it's aiming at x86 only. Later > Sourabh can modify code according to his need on ppc. This can give > satisfying why on code change each time. > > I personally like the 2nd way, while also like seeing 1st one if the > code change and log is convincing to any reviewer.
Ok let's go with second approach. I will introduce a patch in PowerPC series to update the handle_hotplug_event function signature and justify the change.
Thanks, Sourabh Jain
| |