Eric reported that handling corresponding crash hotplug event can be
failed easily when many momery hotplug event are notified in a short period.
They failed because failing to take __kexec_lock.
=======
[ 78.714569] Fallback order for Node 0: 0
[ 78.714575] Built 1 zonelists, mobility grouping on. Total pages: 1817886
[ 78.717133] Policy zone: Normal
[ 78.724423] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
[ 78.727207] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
[ 80.056643] PEFILE: Unsigned PE binary
=======
The memory hotplug events are notified very quickly and very many,
while the handling of crash hotplug is much slower relatively. So the
atomic variable __kexec_lock and kexec_trylock() can't guarantee the
serialization of crash hotplug handling.
Here, add a new mutex lock __crash_hotplug_lock to serialize crash
hotplug handling specifically. This doesn't impact the usage of
__kexec_lock.
Signed-off-by: Baoquan He <[email protected]>
---
kernel/crash_core.c | 3 +++
kernel/kexec_core.c | 1 +
kernel/kexec_internal.h | 11 +++++++++++
3 files changed, 15 insertions(+)
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 03a7932cde0a..e8851724a530 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -783,9 +783,11 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
{
struct kimage *image;
+ crash_hotplug_lock();
/* Obtain lock while changing crash information */
if (!kexec_trylock()) {
pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
+ crash_hotplug_unlock();
return;
}
@@ -852,6 +854,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
out:
/* Release lock now that update complete */
kexec_unlock();
+ crash_hotplug_unlock();
}
static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 9dc728982d79..b95a73f35d9a 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -48,6 +48,7 @@
#include "kexec_internal.h"
atomic_t __kexec_lock = ATOMIC_INIT(0);
+DEFINE_MUTEX(__crash_hotplug_lock);
/* Flag to indicate we are going to kexec a new kernel */
bool kexec_in_progress = false;
diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
index 74da1409cd14..1db31625ef20 100644
--- a/kernel/kexec_internal.h
+++ b/kernel/kexec_internal.h
@@ -28,6 +28,17 @@ static inline void kexec_unlock(void)
atomic_set_release(&__kexec_lock, 0);
}
+/*
+ * Different than kexec/kdump loading/unloading/crash or kexec jumping/shrinking
+ * which usually rarely happen, there will be many crash hotplug events notified
+ * during one short period, e.g one memory board is hot added and memory regions
+ * are online. So mutex lock __crash_hotplug_lock is used to serialize the crash
+ * hotplug handling specificially.
+ * */
+extern struct mutex __crash_hotplug_lock;
+#define crash_hotplug_lock() mutex_lock(&__crash_hotplug_lock)
+#define crash_hotplug_unlock() mutex_unlock(&__crash_hotplug_lock)
+
#ifdef CONFIG_KEXEC_FILE
#include <linux/purgatory.h>
void kimage_file_post_load_cleanup(struct kimage *image);
--
2.41.0
On 9/22/23 18:54, Baoquan He wrote:
> Eric reported that handling corresponding crash hotplug event can be
> failed easily when many momery hotplug event are notified in a short period.
> They failed because failing to take __kexec_lock.
>
> =======
> [ 78.714569] Fallback order for Node 0: 0
> [ 78.714575] Built 1 zonelists, mobility grouping on. Total pages: 1817886
> [ 78.717133] Policy zone: Normal
> [ 78.724423] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
> [ 78.727207] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
> [ 80.056643] PEFILE: Unsigned PE binary
> =======
>
> The memory hotplug events are notified very quickly and very many,
> while the handling of crash hotplug is much slower relatively. So the
> atomic variable __kexec_lock and kexec_trylock() can't guarantee the
> serialization of crash hotplug handling.
>
> Here, add a new mutex lock __crash_hotplug_lock to serialize crash
> hotplug handling specifically. This doesn't impact the usage of
> __kexec_lock.
>
> Signed-off-by: Baoquan He <[email protected]>
> ---
> kernel/crash_core.c | 3 +++
> kernel/kexec_core.c | 1 +
> kernel/kexec_internal.h | 11 +++++++++++
> 3 files changed, 15 insertions(+)
>
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 03a7932cde0a..e8851724a530 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -783,9 +783,11 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> {
> struct kimage *image;
>
> + crash_hotplug_lock();
> /* Obtain lock while changing crash information */
> if (!kexec_trylock()) {
> pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> + crash_hotplug_unlock();
> return;
> }
>
> @@ -852,6 +854,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> out:
> /* Release lock now that update complete */
> kexec_unlock();
> + crash_hotplug_unlock();
> }
>
The crash_check_update_elfcorehdr() also has kexec_trylock() and needs similar treatment.
> static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 9dc728982d79..b95a73f35d9a 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -48,6 +48,7 @@
> #include "kexec_internal.h"
>
> atomic_t __kexec_lock = ATOMIC_INIT(0);
> +DEFINE_MUTEX(__crash_hotplug_lock);
>
> /* Flag to indicate we are going to kexec a new kernel */
> bool kexec_in_progress = false;
> diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
> index 74da1409cd14..1db31625ef20 100644
> --- a/kernel/kexec_internal.h
> +++ b/kernel/kexec_internal.h
> @@ -28,6 +28,17 @@ static inline void kexec_unlock(void)
> atomic_set_release(&__kexec_lock, 0);
> }
>
> +/*
> + * Different than kexec/kdump loading/unloading/crash or kexec jumping/shrinking
> + * which usually rarely happen, there will be many crash hotplug events notified
> + * during one short period, e.g one memory board is hot added and memory regions
> + * are online. So mutex lock __crash_hotplug_lock is used to serialize the crash
> + * hotplug handling specificially.
> + * */
> +extern struct mutex __crash_hotplug_lock;
> +#define crash_hotplug_lock() mutex_lock(&__crash_hotplug_lock)
> +#define crash_hotplug_unlock() mutex_unlock(&__crash_hotplug_lock)
> +
> #ifdef CONFIG_KEXEC_FILE
> #include <linux/purgatory.h>
> void kimage_file_post_load_cleanup(struct kimage *image);
The new content for kexec_internal.h and kexec_core.c could/should probably be
moved into crash_core.c, within the CONFIG_CRASH_HOTPLUG?
eric
On 09/23/23 at 07:10am, Eric DeVolder wrote:
>
>
> On 9/22/23 18:54, Baoquan He wrote:
> > Eric reported that handling corresponding crash hotplug event can be
> > failed easily when many momery hotplug event are notified in a short period.
> > They failed because failing to take __kexec_lock.
> >
> > =======
> > [ 78.714569] Fallback order for Node 0: 0
> > [ 78.714575] Built 1 zonelists, mobility grouping on. Total pages: 1817886
> > [ 78.717133] Policy zone: Normal
> > [ 78.724423] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
> > [ 78.727207] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
> > [ 80.056643] PEFILE: Unsigned PE binary
> > =======
> >
> > The memory hotplug events are notified very quickly and very many,
> > while the handling of crash hotplug is much slower relatively. So the
> > atomic variable __kexec_lock and kexec_trylock() can't guarantee the
> > serialization of crash hotplug handling.
> >
> > Here, add a new mutex lock __crash_hotplug_lock to serialize crash
> > hotplug handling specifically. This doesn't impact the usage of
> > __kexec_lock.
> >
> > Signed-off-by: Baoquan He <[email protected]>
> > ---
> > kernel/crash_core.c | 3 +++
> > kernel/kexec_core.c | 1 +
> > kernel/kexec_internal.h | 11 +++++++++++
> > 3 files changed, 15 insertions(+)
> >
> > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > index 03a7932cde0a..e8851724a530 100644
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -783,9 +783,11 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> > {
> > struct kimage *image;
> > + crash_hotplug_lock();
> > /* Obtain lock while changing crash information */
> > if (!kexec_trylock()) {
> > pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> > + crash_hotplug_unlock();
> > return;
> > }
> > @@ -852,6 +854,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> > out:
> > /* Release lock now that update complete */
> > kexec_unlock();
> > + crash_hotplug_unlock();
> > }
>
> The crash_check_update_elfcorehdr() also has kexec_trylock() and needs similar treatment.
>
> > static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index 9dc728982d79..b95a73f35d9a 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -48,6 +48,7 @@
> > #include "kexec_internal.h"
> > atomic_t __kexec_lock = ATOMIC_INIT(0);
> > +DEFINE_MUTEX(__crash_hotplug_lock);
> > /* Flag to indicate we are going to kexec a new kernel */
> > bool kexec_in_progress = false;
> > diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
> > index 74da1409cd14..1db31625ef20 100644
> > --- a/kernel/kexec_internal.h
> > +++ b/kernel/kexec_internal.h
> > @@ -28,6 +28,17 @@ static inline void kexec_unlock(void)
> > atomic_set_release(&__kexec_lock, 0);
> > }
> > +/*
> > + * Different than kexec/kdump loading/unloading/crash or kexec jumping/shrinking
> > + * which usually rarely happen, there will be many crash hotplug events notified
> > + * during one short period, e.g one memory board is hot added and memory regions
> > + * are online. So mutex lock __crash_hotplug_lock is used to serialize the crash
> > + * hotplug handling specificially.
> > + * */
> > +extern struct mutex __crash_hotplug_lock;
> > +#define crash_hotplug_lock() mutex_lock(&__crash_hotplug_lock)
> > +#define crash_hotplug_unlock() mutex_unlock(&__crash_hotplug_lock)
> > +
> > #ifdef CONFIG_KEXEC_FILE
> > #include <linux/purgatory.h>
> > void kimage_file_post_load_cleanup(struct kimage *image);
>
> The new content for kexec_internal.h and kexec_core.c could/should probably be
> moved into crash_core.c, within the CONFIG_CRASH_HOTPLUG?
That makes sense, I will spin v2 and post.