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

[Laurent Dufour]

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.