Re: [PATCH v4 1/3] memory: implement memory_block_advise/probe_max_size

[Gregory Price <gourry@gourry.net>]

On Wed, Oct 30, 2024 at 11:25:33AM +0100, David Hildenbrand wrote:
> On 29.10.24 21:20, Gregory Price wrote:
> > Hotplug memory sources may have opinions on what the memblock size
> > should be - usually for alignment purposes.  For example, CXL memory
> > extents can be 256MB with a matching alignment. If this size/alignment
> > is smaller than the block size, it can result in stranded capacity.
> > 
> > Implement memory_block_advise_max_size for use prior to allocator init,
> > for software to advise the system on the max block size.
> > 
> > Implement memory_block_probe_max_size for use by arch init code to
> > calculate the best block size. Use of advice is architecture defined.
> > 
> > The probe value can never change after first probe. Calls to advise
> > after probe will return -EBUSY to aid debugging.
> > 
> > On systems without hotplug, always return -ENODEV and 0 respectively.
> > 
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Gregory Price <gourry@gourry.net>
> > ---
> >   drivers/base/memory.c  | 48 ++++++++++++++++++++++++++++++++++++++++++
> >   include/linux/memory.h | 10 +++++++++
> >   2 files changed, 58 insertions(+)
> > 
> > diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> > index 67858eeb92ed..099a972c52dc 100644
> > --- a/drivers/base/memory.c
> > +++ b/drivers/base/memory.c
> > @@ -110,6 +110,54 @@ static void memory_block_release(struct device *dev)
> >   	kfree(mem);
> >   }
> > +/**
> > + * memory_block_advise_max_size() - advise memory hotplug on the max suggested
> > + *				    block size, usually for alignment.
> > + * @size: suggestion for maximum block size. must be aligned on power of 2.
> > + *
> > + * Early boot software (pre-allocator init) may advise archs on the max block
> > + * size. This value can only decrease after initialization, as the intent is
> > + * to identify the largest supported alignment for all sources.
> > + *
> > + * Use of this value is arch-defined, as is min/max block size.
> > + *
> > + * Return: 0 on success
> > + *	   -EINVAL if size is 0 or not pow2 aligned
> > + *	   -EBUSY if value has already been probed
> > + */
> > +static size_t memory_block_advised_sz;
> 
> Nit: if everything is called "size", call this "size" as well.
> 

Mostly shortened here because

	if (memory_block_advised_sz)
		memory_block_advised_size = min(size, memory_block_advised_size);

is over 80 characters lol.  Happy to change if you have strong feelings.


> > +static bool memory_block_advised_size_queried;
> > +int memory_block_advise_max_size(size_t size)
> 
> Not that memory_block_size_bytes() uses "unsigned long". I don't think it
> matters here. Or could it on 32bit? (I assume that code will not really
> matter on 32bit)
> 

ack

> > +{
> > +	if (!size || !is_power_of_2(size))
> > +		return -EINVAL;
> > +
> > +	if (memory_block_advised_size_queried)
> > +		return -EBUSY;
> > +
> > +	if (memory_block_advised_sz)
> > +		memory_block_advised_sz = min(size, memory_block_advised_sz);
> > +	else
> > +		memory_block_advised_sz = size;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * memory_block_advised_max_size() - query advised max hotplug block size.
> > + *
> > + * After the first call, the value can never change. Callers looking for the
> > + * actual block size should use memory_block_size_bytes. This interface is
> > + * intended for use by arch-init when initializing the hotplug block size.
> > + *
> > + * Return: advised size in bytes, or 0 if never set.
> > + */
> > +size_t memory_block_advised_max_size(void)
> > +{
> > +	memory_block_advised_size_queried = true;
> > +	return memory_block_advised_sz;> +}
> > +
> 
> I wonder if both should.could be "__init" ? So they could only be called
> from __init ... which sounds like the tight thing to do?
> 

Was thinking the same thing in another thread, will go ahead and change it.

> Acked-by: David Hildenbrand <david@redhat.com>
> 
> -- 
> Cheers,
> 
> David / dhildenb
>