On Sun, May 18, 2025 at 11:51 AM Mike Rapoport <r...@kernel.org> wrote:
>
> 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:
I wanted to do proper xa_destroy(), as the whole point of this patch
is to satisfy lockdep, and do a proper xa_init(). The patch fixes a
warning in linux-next, and I think should be taken as is. We can do a
separate clean-up once the series lands, where xa_load_or_alloc()
could either take another argument, or split into two functions.
Pasha
