Message-ID: <0dd94920-b13f-4da7-9ea6-4f008af1f4b3@linux.ibm.com>
Date: Wed, 4 Sep 2024 14:55:15 +0530
From: Sourabh Jain <sourabhjain@...ux.ibm.com>
To: Baoquan He <bhe@...hat.com>
Cc: Michael Ellerman <mpe@...erman.id.au>,
        Hari Bathini <hbathini@...ux.ibm.com>, kexec@...ts.infradead.org,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        x86@...nel.org, Sachin P Bappalige <sachinpb@...ux.vnet.ibm.com>
Subject: Re: [PATCH] kexec/crash: no crash update when kexec in progress

Hello Baoquan,

On 30/08/24 16:47, Baoquan He wrote:
> On 08/20/24 at 12:10pm, Sourabh Jain wrote:
>> Hello Baoquan,
>>
>> On 19/08/24 11:45, Baoquan He wrote:
>>> On 08/19/24 at 09:45am, Sourabh Jain wrote:
>>>> Hello Michael and Boaquan
>>>>
>>>> On 01/08/24 12:21, Sourabh Jain wrote:
>>>>> Hello Michael,
>>>>>
>>>>> On 01/08/24 08:04, Michael Ellerman wrote:
>>>>>> Sourabh Jain <sourabhjain@...ux.ibm.com> writes:
>>>>>>> The following errors are observed when kexec is done with SMT=off on
>>>>>>> powerpc.
>>>>>>>
>>>>>>> [  358.458385] Removing IBM Power 842 compression device
>>>>>>> [  374.795734] kexec_core: Starting new kernel
>>>>>>> [  374.795748] kexec: Waking offline cpu 1.
>>>>>>> [  374.875695] crash hp: kexec_trylock() failed, elfcorehdr may
>>>>>>> be inaccurate
>>>>>>> [  374.935833] kexec: Waking offline cpu 2.
>>>>>>> [  375.015664] crash hp: kexec_trylock() failed, elfcorehdr may
>>>>>>> be inaccurate
>>>>>>> snip..
>>>>>>> [  375.515823] kexec: Waking offline cpu 6.
>>>>>>> [  375.635667] crash hp: kexec_trylock() failed, elfcorehdr may
>>>>>>> be inaccurate
>>>>>>> [  375.695836] kexec: Waking offline cpu 7.
>>>>>> Are they actually errors though? Do they block the actual kexec from
>>>>>> happening? Or are they just warnings in dmesg?
>>>>> The kexec kernel boots fine.
>>>>>
>>>>> This warning appears regardless of whether the kdump kernel is loaded.
>>>>>
>>>>> However, when the kdump kernel is loaded, we will not be able to update
>>>>> the kdump image (FDT).
>>>>> I think this should be fine given that kexec is in progress.
>>>>>
>>>>> Please let me know your opinion.
>>>>>
>>>>>> Because the fix looks like it could be racy.
>>>>> It seems like it is racy, but given that kexec takes the lock first and
>>>>> then
>>>>> brings the CPU up, which triggers the kdump image, which always fails to
>>>>> update the kdump image because it could not take the same lock.
>>>>>
>>>>> Note: the kexec lock is not released unless kexec boot fails.
>>>> Any comments or suggestions on this fix?
>>> Is this a little better?
>>>
>>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>>> index 63cf89393c6e..0355ffb712f4 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;
>>> @@ -539,7 +539,7 @@ 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()) {
>>> +	if (!kexec_trylock() && kexec_in_progress) {
>>>    		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>>    		crash_hotplug_unlock();
>>>    		return;
>> Ideally, when `kexec_in_progress` is True, there should be no way to acquire
>> the kexec lock.
>> Therefore, calling `kexec_trylock()` before checking `kexec_in_progress` is
>> not helpful.
>> The kernel will print the error message "kexec_trylock() failed, elfcorehdr
>> may be inaccurate."
>> So, with the above changes, the original problem remains unsolved.
>>
>> However, after closely inspecting the `kernel/kexec_core.c:kernel_kexec()`
>> function, I discovered
>> an exceptional case where my patch needs an update. The issue arises when
>> the system returns
>> from the `machine_kexec()` function, which indicates that kexec has failed.
>>
>> In this scenario, the kexec lock is released, but `kexec_in_progress`
>> remains True.
>>
>> I am unsure why `kexec_in_progress` is NOT set to False when kexec fails.
>> Was this by design,
>> or was it an oversight because returning from the `machine_kexec()` function
>> is highly unlikely?
>>
>> Here is my proposal to address the original problem along with the
>> exceptional case I described
>> above.
>>
>> Let's implement two patches:
>>
>> 1. A patch that sets `kexec_in_progress` to False if the system returns from
>> `machine_kexec()` before
> I don't think we have chance to return from machine_kexec() after
> triggering kexec/kdump jumping. The KEXEC_JUMP could return, but I'v
> never heard people using it.

Agree, on most arch there is no return from machine_kexec function.
So lets drop the above idea of resetting kexec_in_progress.


>
>>     unlocking the kexec lock in the `kernel_kexec()` function.
>>
>>     ```
>>     diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>>     index c0caa14880c3..b41277183455 100644
>>     --- a/kernel/kexec_core.c
>>     +++ b/kernel/kexec_core.c
>>     @@ -1069,6 +1069,7 @@ int kernel_kexec(void)
>>     #endif
>>
>>      Unlock:
>>     +      kexec_in_progress = false;
>>             kexec_unlock();
>>             return error;
>>      ```
>>
>> 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.

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();


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.


Thanks,
Sourabh Jain

Powered by blists - more mailing lists