Re: [PATCH] kexec/crash: no crash update when kexec in progress

[Sourabh Jain <sourabhjain@linux.ibm.com>]

Hello Baoquan,

On 09/09/24 10:53, Baoquan He wrote:
> On 09/09/24 at 10:35am, Sourabh Jain wrote:
>>
>> On 08/09/24 16:00, Baoquan He wrote:
>>> On 09/05/24 at 02:07pm, Sourabh Jain wrote:
>>>> Hello Baoquan,
>>>>
>>>> On 05/09/24 08:53, Baoquan He wrote:
>>>>> On 09/04/24 at 02:55pm, Sourabh Jain wrote:
>>>>>> Hello Baoquan,
>>>>>>
>>>>>> On 30/08/24 16:47, Baoquan He wrote:
>>>>>>> On 08/20/24 at 12:10pm, Sourabh Jain wrote:
>>>>>>>> Hello Baoquan,
>>>>>>>>
>>>>> ......snip...
>>>>>>>> 2. A patch to return early from the `crash_handle_hotplug_event()` function
>>>>>>>> if `kexec_in_progress` is
>>>>>>>>        set to True. This is essentially my original patch.
>>>>>>> There's a race gap between the kexec_in_progress checking and the
>>>>>>> setting it to true which Michael has mentioned.
>>>>>> The window where kernel is holding kexec_lock to do kexec boot
>>>>>> but kexec_in_progress is yet not set to True.
>>>>>>
>>>>>> If kernel needs to handle crash hotplug event, the function
>>>>>> crash_handle_hotplug_event()  will not get the kexec_lock and
>>>>>> error out by printing error message about not able to update
>>>>>> kdump image.
>>>>> But you wanted to avoid the erroring out if it's being in
>>>>> kernel_kexec().  Now you are seeing at least one the noising
>>>>> message, aren't you?
>>>> Yes, but it is very rare to encounter.
>>>>
>>>> My comments on your updated code are inline below.
>>>>
>>>>>> I think it should be fine. Given that lock is already taken for
>>>>>> kexec kernel boot.
>>>>>>
>>>>>> Am I missing something major?
>>>>>>
>>>>>>> That's why I think
>>>>>>> maybe checking kexec_in_progress after failing to retriving
>>>>>>> __kexec_lock is a little better, not very sure.
>>>>>> Try for kexec lock before kexec_in_progress check will not solve
>>>>>> the original problem this patch trying to solve.
>>>>>>
>>>>>> You proposed the below changes earlier:
>>>>>>
>>>>>> -	if (!kexec_trylock()) {
>>>>>> +	if (!kexec_trylock() && kexec_in_progress) {
>>>>>>     		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>>>>>     		crash_hotplug_unlock();
>>>>> Ah, I meant as below, but wrote it mistakenly.
>>>>>
>>>>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>>>>> index 63cf89393c6e..e7c7aa761f46 100644
>>>>> --- a/kernel/crash_core.c
>>>>> +++ b/kernel/crash_core.c
>>>>> @@ -504,7 +504,7 @@ int crash_check_hotplug_support(void)
>>>>>     	crash_hotplug_lock();
>>>>>     	/* Obtain lock while reading crash information */
>>>>> -	if (!kexec_trylock()) {
>>>>> +	if (!kexec_trylock() && !kexec_in_progress) {
>>>>>     		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>>>>     		crash_hotplug_unlock();
>>>>>     		return 0;
>>>>>
>>>>>
>>>>>> Once the kexec_in_progress is set to True there is no way one can get
>>>>>> kexec_lock. So kexec_trylock() before kexec_in_progress is not helpful
>>>>>> for the problem I am trying to solve.
>>>>> With your patch, you could still get the error message if the race gap
>>>>> exist. With above change, you won't get it. Please correct me if I am
>>>>> wrong.
>>>> The above code will print an error message during the race gap. Here's why:
>>>>
>>>> Let’s say the kexec lock is acquired in the kernel_kexec() function,
>>>> but kexec_in_progress is not yet set to True. In this scenario, the code
>>>> will print
>>>> an error message.
>>>>
>>>> There is another issue I see with the above code:
>>>>
>>>> Consider that the system is on the kexec kernel boot path, and
>>>> kexec_in_progress
>>>> is set to True. If crash_hotplug_unlock() is called, the kernel will not
>>>> only update
>>>> the kdump image without acquiring the kexec lock, but it will also release
>>>> the
>>>> kexec lock in the out label. I believe this is incorrect.
>>>>
>>>> Please share your thoughts.
>>> How about this?
>>>
>>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>>> index 63cf89393c6e..8ba7b1da0ded 100644
>>> --- a/kernel/crash_core.c
>>> +++ b/kernel/crash_core.c
>>> @@ -505,7 +505,8 @@ int crash_check_hotplug_support(void)
>>>    	crash_hotplug_lock();
>>>    	/* Obtain lock while reading crash information */
>>>    	if (!kexec_trylock()) {
>>> -		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>> +		if (!kexec_in_progress)
>>> +			pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>>    		crash_hotplug_unlock();
>>>    		return 0;
>>>    	}
>>> @@ -540,7 +541,8 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu,
>>>    	crash_hotplug_lock();
>>>    	/* Obtain lock while changing crash information */
>>>    	if (!kexec_trylock()) {
>>> -		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>> +		if (!kexec_in_progress)
>>> +			pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>>    		crash_hotplug_unlock();
>>>    		return;
>>>    	}
>> Yes putting pr_info under kexec in progress check would work.
>>
>> I will rebase the patch on top on next-20240906 to avoid conflict with
>> https://lore.kernel.org/all/20240812041651.703156-1-sourabhjain@linux.ibm.com/T/#u
>> and send v2.
> Great. When you repost, can you please also add why ppc will hot add cpu
> into patch log when crash triggered? Otherwise other people may be
> confused when reading code here or trace back the code change.

Sure, I will add it.

- Sourabh Jain

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec