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
