Re: [PATCH 1/2] kho: skip zero-sized lowmem and global bootmem areas

[Pasha Tatashin]

On 11-04 13:22, Mike Rapoport wrote:
> On Tue, Nov 03, 2026 at 02:48:36PM -0500, Pasha Tatashin wrote:
> > On 11-03 13:49, Mike Rapoport (Microsoft) wrote:
> > > The lowmem and the global bootmem areas can have zero size, for example
> > > when none or all of the memory is below ARCH_LOW_ADDRESS_LIMIT, or when
> > > either of those areas is explicitly set to zero with kho_bootmem=.
> > > 
> > > For a zero-sized allocation memblock returns an address without reserving
> > > anything, and that empty area is recorded in kho_bootmem. If there is no
> > > memory below ARCH_LOW_ADDRESS_LIMIT, allocation of the empty lowmem area
> > > fails and KHO gets disabled.
> > > 
> > > Extract a helper that checks for zero size and allocates memory from
> > > memblock, and use it to skip empty areas the same way as empty per-node
> > > areas are skipped.
> > > 
> > > Fixes: 3dc92c311498 ("kexec: add Kexec HandOver (KHO) generation helpers")
> > > Reported-by: Chen Pei <cp0613@xxxxxxxxxxxxxxxxx>
> > > Closes: https://lore.kernel.org/all/20261026131851.9942-1-cp0613@xxxxxxxxxxxxxxxxx
> > > Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> > > ---
> > >  kernel/liveupdate/kexec_handover.c | 61 ++++++++++++++++++--------------------
> > >  1 file changed, 29 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> > > index 6f1f55fe31819..f268c57413a7e 100644
> > > --- a/kernel/liveupdate/kexec_handover.c
> > > +++ b/kernel/liveupdate/kexec_handover.c
> > > @@ -820,6 +820,27 @@ bool kho_bootmem_overlap(phys_addr_t phys, size_t size)
> > >  	return false;
> > >  }
> > >  
> > > +static int __init kho_reserve_bootmem_area(int *idx, phys_addr_t size,
> > > +					   phys_addr_t end, int nid)
> > > +{
> > > +	phys_addr_t addr;
> > > +	int i = *idx;
> > > +
> > > +	if (!size)
> > > +		return 0;
> > > +
> > > +	addr = memblock_alloc_range_nid(size, BOOTMEM_ALIGNMENT_BYTES, 0, end,
> > > +					nid, true);
> > > +	if (!addr)
> > > +		return -ENOMEM;
> > > +
> > > +	kho_bootmem[i].addr = addr;
> > > +	kho_bootmem[i].size = size;
> > > +	*idx = i + 1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * kho_reserve_bootmem - Reserve a contiguous chunk of memory for kexec
> > >   *
> > > @@ -832,7 +853,7 @@ bool kho_bootmem_overlap(phys_addr_t phys, size_t size)
> > >  static void __init kho_reserve_bootmem(void)
> > >  {
> > >  	int nid, i = 0, max_nr_bootmem;
> > > -	phys_addr_t addr, size;
> > > +	phys_addr_t size;
> > >  
> > >  	if (!kho_enable)
> > >  		return;
> > > @@ -852,56 +873,32 @@ static void __init kho_reserve_bootmem(void)
> > >  	 * reserve bootmem area in low memory for lowmem allocations in the
> > >  	 * next kernel
> > >  	 */
> > > -	size = bootmem_size_lowmem;
> > > -	addr = memblock_phys_alloc_range(size, BOOTMEM_ALIGNMENT_BYTES, 0,
> > > -					 ARCH_LOW_ADDRESS_LIMIT);
> > > -	if (!addr) {
> > > +	if (kho_reserve_bootmem_area(&i, bootmem_size_lowmem,
> > > +				     ARCH_LOW_ADDRESS_LIMIT, NUMA_NO_NODE)) {
> > >  		pr_err("Failed to reserve lowmem bootmem\n");
> > 
> > You are adding a helper that returns an error code and then ignoring it 
> > at all call sites. Even though it can only return -ENOMEM right now, 
> > dropping the return value is not good practice.
> 
> I don't think error code is important here.

Perhaps, but it is just not elegant to add a function that returns an 
error, call that function from three places, and ignore that error every 
time.

What is wrong with:

err = kho_reserve_bootmem_area(...);
if (err) {
        pr_err("Failed to reserve lowmem bootmem: %pe\n", ERR_PTR(err));
        ...
}

> Even if some theoretical future update will change
> kho_reserve_bootmem_area() to return anything but -ENOMEM I don't see why
> the error code should be printed at all.
> 
> And anyway, that would be the part of that change.

I just don't see why not do it right away? If the function is only used 
as a boolean check, then just make it return a bool, and move prints 
inside.

In any case, this is not critical and I am not insisting on the change, 
but I think it would look better.

> 
> -- 
> Sincerely yours,
> Mike.

-- 
Pasha