On 14.09.20 11:16, Laurent Dufour wrote:
Le 14/09/2020 à 10:31, David Hildenbrand a écrit :
static int register_mem_sect_under_node_hotplug(struct memory_block *mem_blk, void *arg) { const int nid = *(int *)arg; int ret;
/* Hotplugged memory has no holes and belongs to a single 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) returnr et; return sysfs_create_link_nowarn(&mem_blk->dev.kobj, &node_devices[nid]->dev.kobj, kobject_name(&node_devices[nid]->dev.kobj));
}
Cleaner, right? :) No unnecessary checks.
I tend to agree here, I like more a simplistic version for hotplug.
... and while we're at it, we should rename register_mem_sect_under_node to something like "register_memory_block_under_node" - "section" is a legacy leftover here.
We could factor out both sysfs_create_link_nowarn() calls into something like "do_register_memory_block_under_node" or similar, to minimize code duplication.
One could argue if link_mem_section_hotplug() would be better than passing around the context.
I am not sure if I would duplicate the code there. We could just pass the pointer of the function we want to call to link_mem_sections? either register_mem_sect_under_node_hotplug or register_mem_sect_under_node_early? Would not that be clean and clear enough?
I don't particularly like passing around function pointers where it can be avoided (e.g., here exporting 3 functions now instead 1). Makes the interface harder to get IMHO. But I don't really care about that interface, easy to change later on.
This would lead to the following.
Do everyone agree?
diff --git a/drivers/base/node.c b/drivers/base/node.c index 508b80f6329b..444808a7c9b6 100644 --- a/drivers/base/node.c +++ b/drivers/base/node.c @@ -761,9 +761,32 @@ 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)@@ -785,38 +808,35 @@ 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.
* We need to check if page belongs to nid only at the boot* case because node's ranges can be interleaved.
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.*/mem_blk->nid = nid;
page_nid = get_nid_for_pfn(pfn);if (page_nid < 0)continue;if (page_nid != nid)continue;
ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,&mem_blk->dev.kobj,kobject_name(&mem_blk->dev.kobj));
ret = do_register_memory_block_under_node(nid, mem_blk);
You have to do an unconditional
return ret;
here AFAIKS. For me this looks much better.
Reviewed-by: David Hildenbrand david@redhat.com