Re: [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks

[David Hildenbrand <david@redhat.com>]

On 12.10.23 07:53, Verma, Vishal L wrote:
> On Mon, 2023-10-09 at 17:04 +0200, David Hildenbrand wrote:
>> On 07.10.23 10:55, Huang, Ying wrote:
>>> Vishal Verma <vishal.l.verma@intel.com> writes:
>>>
>>>> @@ -2167,47 +2221,28 @@ static int __ref try_remove_memory(u64 start, u64 size)
>>>>          if (rc)
>>>>                  return rc;
>>>>    
>>>> +       mem_hotplug_begin();
>>>> +
>>>>          /*
>>>> -        * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
>>>> -        * the same granularity it was added - a single memory block.
>>>> +        * For memmap_on_memory, the altmaps could have been added on
>>>> +        * a per-memblock basis. Loop through the entire range if so,
>>>> +        * and remove each memblock and its altmap.
>>>>           */
>>>>          if (mhp_memmap_on_memory()) {
>>>
>>> IIUC, even if mhp_memmap_on_memory() returns true, it's still possible
>>> that the memmap is put in DRAM after [2/2].  So that,
>>> arch_remove_memory() are called for each memory block unnecessarily.  Can
>>> we detect this (via altmap?) and call remove_memory_block_and_altmap()
>>> for the whole range?
>>
>> Good point. We should handle memblock-per-memblock onny if we have to
>> handle the altmap. Otherwise, just call a separate function that doesn't
>> care about -- e.g., called remove_memory_blocks_no_altmap().
>>
>> We could simply walk all memory blocks and make sure either all have an
>> altmap or none has an altmap. If there is a mix, we should bail out with
>> WARN_ON_ONCE().
>>
> Ok I think I follow - based on both of these threads, here's my
> understanding in an incremental diff from the original patches (may not
> apply directly as I've already committed changes from the other bits of
> feedback - but this should provide an idea of the direction) -
> 
> ---
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 507291e44c0b..30addcb063b4 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -2201,6 +2201,40 @@ static void __ref remove_memory_block_and_altmap(u64 start, u64 size)
>   	}
>   }
>   
> +static bool memblocks_have_altmaps(u64 start, u64 size)
> +{
> +	unsigned long memblock_size = memory_block_size_bytes();
> +	u64 num_altmaps = 0, num_no_altmaps = 0;
> +	struct memory_block *mem;
> +	u64 cur_start;
> +	int rc = 0;
> +
> +	if (!mhp_memmap_on_memory())
> +		return false;

Probably can remove that, checked by the caller. (or drop the one in the 
caller)

> +
> +	for (cur_start = start; cur_start < start + size;
> +	     cur_start += memblock_size) {
> +		if (walk_memory_blocks(cur_start, memblock_size, &mem,
> +				       test_has_altmap_cb))
> +			num_altmaps++;
> +		else
> +			num_no_altmaps++;
> +	}

You should do that without the outer loop, by doing the counting in the 
callback function instead.	

> +
> +	if (!num_altmaps && num_no_altmaps > 0)
> +		return false;
> +
> +	if (!num_no_altmaps && num_altmaps > 0)
> +		return true;
> +
> +	/*
> +	 * If there is a mix of memblocks with and without altmaps,
> +	 * something has gone very wrong. WARN and bail.
> +	 */
> +	WARN_ONCE(1, "memblocks have a mix of missing and present altmaps");

It would be better if we could even make try_remove_memory() fail in 
this case.

> +	return false;
> +}
> +
>   static int __ref try_remove_memory(u64 start, u64 size)
>   {
>   	int rc, nid = NUMA_NO_NODE;
> @@ -2230,7 +2264,7 @@ static int __ref try_remove_memory(u64 start, u64 size)
>   	 * a per-memblock basis. Loop through the entire range if so,
>   	 * and remove each memblock and its altmap.
>   	 */
> -	if (mhp_memmap_on_memory()) {
> +	if (mhp_memmap_on_memory() && memblocks_have_altmaps(start, size)) {
>   		unsigned long memblock_size = memory_block_size_bytes();
>   		u64 cur_start;
>   
> @@ -2239,7 +2273,8 @@ static int __ref try_remove_memory(u64 start, u64 size)
>   			remove_memory_block_and_altmap(cur_start,
>   						       memblock_size);

^ probably cleaner move the loop into remove_memory_block_and_altmap() 
and call it remove_memory_blocks_and_altmaps(start, size) instead.

>   	} else {
> -		remove_memory_block_and_altmap(start, size);
> +		remove_memory_block_devices(start, size);
> +		arch_remove_memory(start, size, NULL);
>   	}
>   
>   	if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)) {
> 

-- 
Cheers,

David / dhildenb