Re: [PATCH v2 2/3] mm: don't rely on system state to detect hot-plug operations

[Laurent Dufour <ldufour@linux.ibm.com>]

Le 14/09/2020 à 19:15, David Hildenbrand a écrit :
>>   arch/ia64/mm/init.c  |  4 +--
>>   drivers/base/node.c  | 86 ++++++++++++++++++++++++++++----------------
>>   include/linux/node.h | 11 +++---
>>   mm/memory_hotplug.c  |  5 +--
>>   4 files changed, 68 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index b5054b5e77c8..8e7b8c6c576e 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -538,7 +538,7 @@ virtual_memmap_init(u64 start, u64 end, void *arg)
>>   	if (map_start < map_end)
>>   		memmap_init_zone((unsigned long)(map_end - map_start),
>>   				 args->nid, args->zone, page_to_pfn(map_start),
>> -				 MEMPLUG_EARLY, NULL);
>> +				 MEMINIT_EARLY, NULL);
> 
> Patch #1.

Sure, this explains why I was able to build on ia64 but that's not the right place.

>>   	return 0;
>>   }
>>   
>> @@ -548,7 +548,7 @@ memmap_init (unsigned long size, int nid, unsigned long zone,
>>   {
>>   	if (!vmem_map) {
>>   		memmap_init_zone(size, nid, zone, start_pfn,
>> -				 MEMPLUG_EARLY, NULL);
>> +				 MEMINIT_EARLY, NULL);

I'll fix that too.

>>   	} else {
>>   		struct page *start;
>>   		struct memmap_init_callback_data args;
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 508b80f6329b..01ee73c9d675 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -761,14 +761,36 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
>>   	return pfn_to_nid(pfn);
>>   }
>>   
>> +static int do_register_memory_block_under_node(int nid,
>> +					       struct memory_block *mem_blk)
>> +{
>> +	int ret;
>> +
>> +	/*
>> +	 * If this memory block spans multiple nodes, we only indicate
>> +	 * the last processed node.
>> +	 */
>> +	mem_blk->nid = nid;
>> +
>> +	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> +				       &mem_blk->dev.kobj,
>> +				       kobject_name(&mem_blk->dev.kobj));
>> +	if (ret)
>> +		return ret;
>> +
>> +	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> +				&node_devices[nid]->dev.kobj,
>> +				kobject_name(&node_devices[nid]->dev.kobj));
>> +}
>> +
>>   /* register memory section under specified node if it spans that node */
>> -static int register_mem_sect_under_node(struct memory_block *mem_blk,
>> -					 void *arg)
>> +static int register_mem_block_under_node_early(struct memory_block *mem_blk,
>> +					       void *arg)
>>   {
>>   	unsigned long memory_block_pfns = memory_block_size_bytes() / PAGE_SIZE;
>>   	unsigned long start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
>>   	unsigned long end_pfn = start_pfn + memory_block_pfns - 1;
>> -	int ret, nid = *(int *)arg;
>> +	int nid = *(int *)arg;
>>   	unsigned long pfn;
>>   
>>   	for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
>> @@ -785,38 +807,34 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
>>   		}
>>   
>>   		/*
>> -		 * We need to check if page belongs to nid only for the boot
>> -		 * case, during hotplug we know that all pages in the memory
>> -		 * block belong to the same node.
>> -		 */
>> -		if (system_state == SYSTEM_BOOTING) {
>> -			page_nid = get_nid_for_pfn(pfn);
>> -			if (page_nid < 0)
>> -				continue;
>> -			if (page_nid != nid)
>> -				continue;
>> -		}
>> -
>> -		/*
>> -		 * If this memory block spans multiple nodes, we only indicate
>> -		 * the last processed node.
>> +		 * We need to check if page belongs to nid only at the boot
>> +		 * case because node's ranges can be interleaved.
>>   		 */
>> -		mem_blk->nid = nid;
>> -
>> -		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> -					&mem_blk->dev.kobj,
>> -					kobject_name(&mem_blk->dev.kobj));
>> -		if (ret)
>> -			return ret;
>> +		page_nid = get_nid_for_pfn(pfn);
>> +		if (page_nid < 0)
>> +			continue;
>> +		if (page_nid != nid)
>> +			continue;
>>   
>> -		return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> -				&node_devices[nid]->dev.kobj,
>> -				kobject_name(&node_devices[nid]->dev.kobj));
>> +		/* The memory block is registered to the first matching node */
> 
> That comment is misleading in that context.
> 
> A memory block is registered if there is at least a page that belongs to
> the nid. It's perfectly fine to have a single memory block belong to
> multiple NUMA nodes (when the split is within a memory block). I'd just
> drop it.

I agree the comment is not accurate, I'll drop it.