On Sun, May 18, 2025 at 07:23:15AM -0700, Changyuan Lyu wrote:
> From: Pasha Tatashin <pasha.tatas...@soleen.com>
>
> Lockdep shows the following warning:
>
> INFO: trying to register non-static key.
> The code is fine but needs lockdep annotation, or maybe
> you didn't initialize this object before use?
> turning off the locking correctness validator.
>
> [<ffffffff810133a6>] dump_stack_lvl+0x66/0xa0
> [<ffffffff8136012c>] assign_lock_key+0x10c/0x120
> [<ffffffff81358bb4>] register_lock_class+0xf4/0x2f0
> [<ffffffff813597ff>] __lock_acquire+0x7f/0x2c40
> [<ffffffff81360cb0>] ? __pfx_hlock_conflict+0x10/0x10
> [<ffffffff811707be>] ? native_flush_tlb_global+0x8e/0xa0
> [<ffffffff8117096e>] ? __flush_tlb_all+0x4e/0xa0
> [<ffffffff81172fc2>] ? __kernel_map_pages+0x112/0x140
> [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> [<ffffffff81359556>] lock_acquire+0xe6/0x280
> [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> [<ffffffff8100b9e0>] _raw_spin_lock+0x30/0x40
> [<ffffffff813ec327>] ? xa_load_or_alloc+0x67/0xe0
> [<ffffffff813ec327>] xa_load_or_alloc+0x67/0xe0
> [<ffffffff813eb4c0>] kho_preserve_folio+0x90/0x100
> [<ffffffff813ebb7f>] __kho_finalize+0xcf/0x400
> [<ffffffff813ebef4>] kho_finalize+0x34/0x70
>
> This is becase xa has its own lock, that is not initialized in
> xa_load_or_alloc.
>
> Modifiy __kho_preserve_order(), to properly call
> xa_init(&new_physxa->phys_bits);
>
> Fixes: fc33e4b44b27 ("kexec: enable KHO support for memory preservation")
> Signed-off-by: Pasha Tatashin <pasha.tatas...@soleen.com>
> Signed-off-by: Changyuan Lyu <changyu...@google.com>
> ---
> kernel/kexec_handover.c | 29 +++++++++++++++++++++++++----
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
> index 69b953551677..f0ac6a9170f8 100644
> --- a/kernel/kexec_handover.c
> +++ b/kernel/kexec_handover.c
> @@ -144,14 +144,35 @@ static int __kho_preserve_order(struct kho_mem_track
> *track, unsigned long pfn,
> unsigned int order)
> {
> struct kho_mem_phys_bits *bits;
> - struct kho_mem_phys *physxa;
> + struct kho_mem_phys *physxa, *new_physxa;
> const unsigned long pfn_high = pfn >> order;
>
> might_sleep();
>
> - physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa));
> - if (IS_ERR(physxa))
> - return PTR_ERR(physxa);
> + physxa = xa_load(&track->orders, order);
> + if (!physxa) {
> + new_physxa = kzalloc(sizeof(*physxa), GFP_KERNEL);
> + if (!new_physxa)
> + return -ENOMEM;
> +
> + xa_init(&new_physxa->phys_bits);
> + physxa = xa_cmpxchg(&track->orders, order, NULL, new_physxa,
> + GFP_KERNEL);
> + if (xa_is_err(physxa)) {
> + int err_ret = xa_err(physxa);
> +
> + xa_destroy(&new_physxa->phys_bits);
> + kfree(new_physxa);
> +
> + return err_ret;
> + }
> + if (physxa) {
> + xa_destroy(&new_physxa->phys_bits);
> + kfree(new_physxa);
> + } else {
> + physxa = new_physxa;
> + }
> + }
You are nearly duplicating xa_load_or_alloc() here.
Is xa_destroy() is really needed here? In the end we destroying an empty
xarray.
Unless xa_destroy() is a must something like this would be simpler IMHO:
diff --git a/kernel/kexec_handover.c b/kernel/kexec_handover.c
index ef21db6c59d5..4c8303fbf97a 100644
--- a/kernel/kexec_handover.c
+++ b/kernel/kexec_handover.c
@@ -91,10 +91,12 @@ struct kho_serialization {
struct khoser_mem_chunk *preserved_mem_map;
};
-static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t
sz)
+static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t
sz,
+ bool *new)
{
void *elm, *res;
+ *new = false;
elm = xa_load(xa, index);
if (elm)
return elm;
@@ -112,6 +114,7 @@ static void *xa_load_or_alloc(struct xarray *xa, unsigned
long index, size_t sz)
return res;
}
+ *new = true;
return elm;
}
@@ -146,15 +149,18 @@ static int __kho_preserve_order(struct kho_mem_track
*track, unsigned long pfn,
struct kho_mem_phys_bits *bits;
struct kho_mem_phys *physxa;
const unsigned long pfn_high = pfn >> order;
+ bool new;
might_sleep();
- physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa));
+ physxa = xa_load_or_alloc(&track->orders, order, sizeof(*physxa), &new);
if (IS_ERR(physxa))
return PTR_ERR(physxa);
+ if (new)
+ xa_init(&physxa->phys_bits);
bits = xa_load_or_alloc(&physxa->phys_bits, pfn_high / PRESERVE_BITS,
- sizeof(*bits));
+ sizeof(*bits), &new);
if (IS_ERR(bits))
return PTR_ERR(bits);
And if xa_destroy() is actually required, the allocation of new xarray
should be a helper function.
> bits = xa_load_or_alloc(&physxa->phys_bits, pfn_high / PRESERVE_BITS,
> sizeof(*bits));
> --
> 2.49.0.1101.gccaa498523-goog
--
Sincerely yours,
Mike.
