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

[David Hildenbrand <david@redhat.com>]

Tow NITs

> +bool mhp_supports_memmap_on_memory(unsigned long size)
> +{
> +	unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
> +	unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
> +	unsigned long remaining_size = size - vmemmap_size;
> +
> +	/*
> +	 * Besides having arch support and the feature enabled at runtime, we
> +	 * need a few more assumptions to hold true:
> +	 *
> +	 * a) We span a single memory block: memory onlining/offlinin;g happens

s/offlinin;g/offlining;/

> +	 *    in memory block granularity. We don't want the vmemmap of online
> +	 *    memory blocks to reside on offline memory blocks. In the future,
> +	 *    we might want to support variable-sized memory blocks to make the
> +	 *    feature more versatile.
> +	 *
> +	 * b) The vmemmap pages span complete PMDs: We don't want vmemmap code
> +	 *    to populate memory from the altmap for unrelated parts (i.e.,
> +	 *    other memory blocks)
> +	 *
> +	 * c) The vmemmap pages (and thereby the pages that will be exposed to
> +	 *    the buddy) have to cover full pageblocks: memory onlining/offlining
> +	 *    code requires applicable ranges to be page-aligned, for example, to
> +	 *    set the migratetypes properly.
> +	 *
> +	 * TODO: Although we have a check here to make sure that vmemmap pages
> +	 *	 fully populate a PMD, it is not the right place to check for
> +	 *	 this. A much better solution involves improving vmemmap code
> +	 *	 to fallback to base pages when trying to populate vmemmap using
> +	 *	 altmap as an alternative source of memory, and we do not exactly
> +	 *	 populate a single PMD.
> +	 */
> +	return memmap_on_memory &&
> +	       IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> +	       size == memory_block_size_bytes() &&
> +	       IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
> +	       IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));

IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);

LGTM, thanks!

(another pair of eyes certainly wouldn't hurt :) )

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb