Hello Baoquan,

On 30/08/24 16:47, Baoquan He wrote:

On 08/20/24 at 12:10pm, Sourabh Jain wrote:

quoted

Hello Baoquan,

On 19/08/24 11:45, Baoquan He wrote:

quoted

On 08/19/24 at 09:45am, Sourabh Jain wrote:

quoted

Hello Michael and Boaquan

On 01/08/24 12:21, Sourabh Jain wrote:

quoted

Hello Michael,

On 01/08/24 08:04, Michael Ellerman wrote:

quoted

Sourabh Jain [off-list ref] writes:

quoted

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.

quoted

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.

quoted

    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