Re: [PATCH v2 1/7] mm,memory_hotplug: Allocate memmap from the added memory range

[Oscar Salvador <osalvador@suse.de>]

On Thu, Feb 25, 2021 at 07:58:01PM +0100, David Hildenbrand wrote:
> > In this way, we have:
> > 
> > (start_pfn, buddy_start_pfn - 1] = Initialized and PageReserved
> > (buddy_start_pfn, end_pfn]       = Initialized and sent to buddy
> 
> nit: shouldn't it be
> 
> [start_pfn, buddy_start_pfn - 1]
> [buddy_start_pfn, end_pfn - 1]
> 
> or
> 
> [start_pfn, buddy_start_pfn)
> [buddy_start_pfn, end_pfn)
> 
> (I remember that "[" means inclusive and "(" means exclusive, I might be wrong :) )
> 
> I actually prefer the first variant.

Let us go witht the first variant, I guess it is more clear.

> > -static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
> > +static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > +			       unsigned long buddy_start_pfn)
> >   {
> >   	const unsigned long end_pfn = start_pfn + nr_pages;
> > -	unsigned long pfn;
> > +	unsigned long pfn = buddy_start_pfn;
> > +
> > +	/*
> > +	 * When using memmap_on_memory, the range might be unaligned as the
> > +	 * first pfns are used for vmemmap pages. Align it in case we need to.
> > +	 */
> > +	if (pfn & ((1 << (MAX_ORDER - 1)) - 1)) {
> 
> if (!IS_ALIGNED(pfn, MAX_ORDER_NR_PAGES))

Will change

> 
> > +		(*online_page_callback)(pfn_to_page(pfn), pageblock_order);
> > +		pfn += 1 << pageblock_order;
> 
> pfn += pageblock_nr_pages;
> 
> Can you add a comment why we can be sure that we are off by  a single pageblock? What about s390x where a MAX_ORDER_NR_PAGES == 4 * pageblock_nr_pages?
> 
> Would it make thing simpler to just do a
> 
> while (!IS_ALIGNED(pfn, MAX_ORDER_NR_PAGES)) {
> 	(*online_page_callback)(pfn_to_page(pfn), 0);
> 	pfn++;
> }

Honestly, I did not spend much time thinking on other platforms other
than arm64/x86_64.
But I think that that would be the universal solution as we do not make
any assumptions.

I will replace it.

> > +bool mhp_supports_memmap_on_memory(unsigned long size)
> > +{
> > +	return memmap_on_memory_enabled &&
> > +	       size == memory_block_size_bytes();
> 
> Regarding my other comments as reply to the other patches, I'd move all magic you have when trying to enable right here.

Ok, will do.

> > @@ -1613,7 +1658,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> >   	zone_pcp_disable(zone);
> >   	/* set above range as isolated */
> > -	ret = start_isolate_page_range(start_pfn, end_pfn,
> > +	ret = start_isolate_page_range(buddy_start_pfn, end_pfn,
> >   				       MIGRATE_MOVABLE,
> >   				       MEMORY_OFFLINE | REPORT_FAILURE);
> 
> Did you take care to properly adjust undo_isolate_page_range() as well? I can't spot it.

No, I did not. Good that you noticed :-)

Will fix it up in the next version.

> > +static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
> > +{
> > +	unsigned long *nr_vmemmap_pages = (unsigned long *)arg;
> > +
> > +	*nr_vmemmap_pages += mem->nr_vmemmap_pages;
> > +	return mem->nr_vmemmap_pages;
> > +}
> > +
> 
> I think you can do this easier, all you want to know is if there
> is any block that has nr_vmemmap_pages set - and return the value.
> 
> static int first_set_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
> {
> 	/* If not set, continue with the next block. */
> 	return mem->nr_vmemmap_pages;
> }

Yeah, less code.
Will fix it.

> 
> ...
> > +		walk_memory_blocks(start, size, &nr_vmemmap_pages,
> > +					 get_nr_vmemmap_pages_cb);
> 
> ...
> 
> mem->nr_vmemmap_pages = walk_memory_blocks(start ...)
> 
> 
> 
> Looks quite promising, only a couple of things to fine-tune :) Thanks!

Thanks for having a look, that is highly appreciated!

Let us see if we can polish the minor things that are missing and target
this for the next release.


-- 
Oscar Salvador
SUSE L3