| Message ID | 20200707055917.143653-3-justin.he@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix and enable pmem as RAM device on arm64 | expand |
[+] add Powerpc maintainers to check my concern about memory_add_physaddr_to_nid Exporting -- Cheers, Justin (Jia He) > -----Original Message----- > From: Jia He <justin.he@arm.com> > Sent: Tuesday, July 7, 2020 1:59 PM > To: Catalin Marinas <Catalin.Marinas@arm.com>; Will Deacon > <will@kernel.org>; Dan Williams <dan.j.williams@intel.com>; Vishal Verma > <vishal.l.verma@intel.com>; Dave Jiang <dave.jiang@intel.com> > Cc: Michal Hocko <mhocko@suse.com>; Andrew Morton <akpm@linux- > foundation.org>; Mike Rapoport <rppt@linux.ibm.com>; Baoquan He > <bhe@redhat.com>; Chuhong Yuan <hslester96@gmail.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > mm@kvack.org; linux-nvdimm@lists.01.org; Kaly Xin <Kaly.Xin@arm.com>; > Justin He <Justin.He@arm.com> > Subject: [RFC PATCH v2 2/3] device-dax: use fallback nid when numa_node is > invalid > > Previously, numa_off is set unconditionally at the end of > dummy_numa_init(), > even with a fake numa node. Then ACPI detects node id as NUMA_NO_NODE(-1) > in > acpi_map_pxm_to_node() because it regards numa_off as turning off the numa > node. Hence dev_dax->target_node is NUMA_NO_NODE on arm64 with fake numa. > > Without this patch, pmem can't be probed as a RAM device on arm64 if SRAT > table > isn't present: > $ndctl create-namespace -fe namespace0.0 --mode=devdax --map=dev -s 1g -a > 64K > kmem dax0.0: rejecting DAX region [mem 0x240400000-0x2bfffffff] with > invalid node: -1 > kmem: probe of dax0.0 failed with error -22 > > This fixes it by using fallback memory_add_physaddr_to_nid() as nid. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Jia He <justin.he@arm.com> > --- > I noticed that on powerpc memory_add_physaddr_to_nid is not exported for > module > driver. Set it to RFC due to this concern. > > drivers/dax/kmem.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > index 275aa5f87399..68e693ca6d59 100644 > --- a/drivers/dax/kmem.c > +++ b/drivers/dax/kmem.c > @@ -28,20 +28,22 @@ int dev_dax_kmem_probe(struct device *dev) > resource_size_t kmem_end; > struct resource *new_res; > const char *new_res_name; > - int numa_node; > + int numa_node, new_node; > int rc; > > /* > * Ensure good NUMA information for the persistent memory. > - * Without this check, there is a risk that slow memory > - * could be mixed in a node with faster memory, causing > - * unavoidable performance issues. > + * Without this check, there is a risk but not fatal that slow > + * memory could be mixed in a node with faster memory, causing > + * unavoidable performance issues. Furthermore, fallback node > + * id can be used when numa_node is invalid. > */ > numa_node = dev_dax->target_node; > if (numa_node < 0) { > - dev_warn(dev, "rejecting DAX region %pR with invalid > node: %d\n", > - res, numa_node); > - return -EINVAL; > + new_node = memory_add_physaddr_to_nid(kmem_start); > + dev_info(dev, "changing nid from %d to %d for DAX > region %pR\n", > + numa_node, new_node, res); > + numa_node = new_node; > } > > /* Hotplug starting at the beginning of the next block: */ > @@ -100,6 +102,7 @@ static int dev_dax_kmem_remove(struct device *dev) > resource_size_t kmem_start = res->start; > resource_size_t kmem_size = resource_size(res); > const char *res_name = res->name; > + int numa_node = dev_dax->target_node; > int rc; > > /* > @@ -108,7 +111,10 @@ static int dev_dax_kmem_remove(struct device *dev) > * there is no way to hotremove this memory until reboot because > device > * unbind will succeed even if we return failure. > */ > - rc = remove_memory(dev_dax->target_node, kmem_start, kmem_size); > + if (numa_node < 0) > + numa_node = memory_add_physaddr_to_nid(kmem_start); > + > + rc = remove_memory(numa_node, kmem_start, kmem_size); > if (rc) { > any_hotremove_failed = true; > dev_err(dev, > -- > 2.17.1
On 07.07.20 07:59, Jia He wrote: > Previously, numa_off is set unconditionally at the end of dummy_numa_init(), > even with a fake numa node. Then ACPI detects node id as NUMA_NO_NODE(-1) in > acpi_map_pxm_to_node() because it regards numa_off as turning off the numa > node. Hence dev_dax->target_node is NUMA_NO_NODE on arm64 with fake numa. > > Without this patch, pmem can't be probed as a RAM device on arm64 if SRAT table > isn't present: > $ndctl create-namespace -fe namespace0.0 --mode=devdax --map=dev -s 1g -a 64K > kmem dax0.0: rejecting DAX region [mem 0x240400000-0x2bfffffff] with invalid node: -1 > kmem: probe of dax0.0 failed with error -22 > > This fixes it by using fallback memory_add_physaddr_to_nid() as nid. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Jia He <justin.he@arm.com> > --- > I noticed that on powerpc memory_add_physaddr_to_nid is not exported for module > driver. Set it to RFC due to this concern. > > drivers/dax/kmem.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > index 275aa5f87399..68e693ca6d59 100644 > --- a/drivers/dax/kmem.c > +++ b/drivers/dax/kmem.c > @@ -28,20 +28,22 @@ int dev_dax_kmem_probe(struct device *dev) > resource_size_t kmem_end; > struct resource *new_res; > const char *new_res_name; > - int numa_node; > + int numa_node, new_node; > int rc; > > /* > * Ensure good NUMA information for the persistent memory. > - * Without this check, there is a risk that slow memory > - * could be mixed in a node with faster memory, causing > - * unavoidable performance issues. > + * Without this check, there is a risk but not fatal that slow > + * memory could be mixed in a node with faster memory, causing > + * unavoidable performance issues. Furthermore, fallback node > + * id can be used when numa_node is invalid. > */ > numa_node = dev_dax->target_node; > if (numa_node < 0) { > - dev_warn(dev, "rejecting DAX region %pR with invalid node: %d\n", > - res, numa_node); > - return -EINVAL; > + new_node = memory_add_physaddr_to_nid(kmem_start); > + dev_info(dev, "changing nid from %d to %d for DAX region %pR\n", > + numa_node, new_node, res); > + numa_node = new_node; Now, the warning does not really make sense. We have NUMA_NO_NODE (< 0), that is not a change in the nid, but a selection of a nid. Printing NUMA_NO_NODE does not make too much sense. I suggest just getting rid of new_node and turning the dev_info() into something like dev_info(dev, "using nid %d for DAX region with undefined nid %pR\n", numa_node, res);
Hi David > -----Original Message----- > From: David Hildenbrand <david@redhat.com> > Sent: Tuesday, July 7, 2020 7:34 PM > To: Justin He <Justin.He@arm.com>; Catalin Marinas > <Catalin.Marinas@arm.com>; Will Deacon <will@kernel.org>; Dan Williams > <dan.j.williams@intel.com>; Vishal Verma <vishal.l.verma@intel.com>; Dave > Jiang <dave.jiang@intel.com> > Cc: Michal Hocko <mhocko@suse.com>; Andrew Morton <akpm@linux- > foundation.org>; Mike Rapoport <rppt@linux.ibm.com>; Baoquan He > <bhe@redhat.com>; Chuhong Yuan <hslester96@gmail.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux- > mm@kvack.org; linux-nvdimm@lists.01.org; Kaly Xin <Kaly.Xin@arm.com> > Subject: Re: [RFC PATCH v2 2/3] device-dax: use fallback nid when > numa_node is invalid > > On 07.07.20 07:59, Jia He wrote: > > Previously, numa_off is set unconditionally at the end of > dummy_numa_init(), > > even with a fake numa node. Then ACPI detects node id as NUMA_NO_NODE(-1) > in > > acpi_map_pxm_to_node() because it regards numa_off as turning off the > numa > > node. Hence dev_dax->target_node is NUMA_NO_NODE on arm64 with fake numa. > > > > Without this patch, pmem can't be probed as a RAM device on arm64 if > SRAT table > > isn't present: > > $ndctl create-namespace -fe namespace0.0 --mode=devdax --map=dev -s 1g - > a 64K > > kmem dax0.0: rejecting DAX region [mem 0x240400000-0x2bfffffff] with > invalid node: -1 > > kmem: probe of dax0.0 failed with error -22 > > > > This fixes it by using fallback memory_add_physaddr_to_nid() as nid. > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Jia He <justin.he@arm.com> > > --- > > I noticed that on powerpc memory_add_physaddr_to_nid is not exported for > module > > driver. Set it to RFC due to this concern. > > > > drivers/dax/kmem.c | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > > index 275aa5f87399..68e693ca6d59 100644 > > --- a/drivers/dax/kmem.c > > +++ b/drivers/dax/kmem.c > > @@ -28,20 +28,22 @@ int dev_dax_kmem_probe(struct device *dev) > > resource_size_t kmem_end; > > struct resource *new_res; > > const char *new_res_name; > > - int numa_node; > > + int numa_node, new_node; > > int rc; > > > > /* > > * Ensure good NUMA information for the persistent memory. > > - * Without this check, there is a risk that slow memory > > - * could be mixed in a node with faster memory, causing > > - * unavoidable performance issues. > > + * Without this check, there is a risk but not fatal that slow > > + * memory could be mixed in a node with faster memory, causing > > + * unavoidable performance issues. Furthermore, fallback node > > + * id can be used when numa_node is invalid. > > */ > > numa_node = dev_dax->target_node; > > if (numa_node < 0) { > > - dev_warn(dev, "rejecting DAX region %pR with invalid > node: %d\n", > > - res, numa_node); > > - return -EINVAL; > > + new_node = memory_add_physaddr_to_nid(kmem_start); > > + dev_info(dev, "changing nid from %d to %d for DAX > region %pR\n", > > + numa_node, new_node, res); > > + numa_node = new_node; > > Now, the warning does not really make sense. We have NUMA_NO_NODE (< 0), > that is not a change in the nid, but a selection of a nid. Printing > NUMA_NO_NODE does not make too much sense. I suggest just getting rid of > new_node and turning the dev_info() into something like > > dev_info(dev, "using nid %d for DAX region with undefined nid %pR\n", > numa_node, res); > Okay, I will update it as your sugguestion. Thanks -- Cheers, Justin (Jia He)
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c index 275aa5f87399..68e693ca6d59 100644 --- a/drivers/dax/kmem.c +++ b/drivers/dax/kmem.c @@ -28,20 +28,22 @@ int dev_dax_kmem_probe(struct device *dev) resource_size_t kmem_end; struct resource *new_res; const char *new_res_name; - int numa_node; + int numa_node, new_node; int rc; /* * Ensure good NUMA information for the persistent memory. - * Without this check, there is a risk that slow memory - * could be mixed in a node with faster memory, causing - * unavoidable performance issues. + * Without this check, there is a risk but not fatal that slow + * memory could be mixed in a node with faster memory, causing + * unavoidable performance issues. Furthermore, fallback node + * id can be used when numa_node is invalid. */ numa_node = dev_dax->target_node; if (numa_node < 0) { - dev_warn(dev, "rejecting DAX region %pR with invalid node: %d\n", - res, numa_node); - return -EINVAL; + new_node = memory_add_physaddr_to_nid(kmem_start); + dev_info(dev, "changing nid from %d to %d for DAX region %pR\n", + numa_node, new_node, res); + numa_node = new_node; } /* Hotplug starting at the beginning of the next block: */ @@ -100,6 +102,7 @@ static int dev_dax_kmem_remove(struct device *dev) resource_size_t kmem_start = res->start; resource_size_t kmem_size = resource_size(res); const char *res_name = res->name; + int numa_node = dev_dax->target_node; int rc; /* @@ -108,7 +111,10 @@ static int dev_dax_kmem_remove(struct device *dev) * there is no way to hotremove this memory until reboot because device * unbind will succeed even if we return failure. */ - rc = remove_memory(dev_dax->target_node, kmem_start, kmem_size); + if (numa_node < 0) + numa_node = memory_add_physaddr_to_nid(kmem_start); + + rc = remove_memory(numa_node, kmem_start, kmem_size); if (rc) { any_hotremove_failed = true; dev_err(dev,
Previously, numa_off is set unconditionally at the end of dummy_numa_init(), even with a fake numa node. Then ACPI detects node id as NUMA_NO_NODE(-1) in acpi_map_pxm_to_node() because it regards numa_off as turning off the numa node. Hence dev_dax->target_node is NUMA_NO_NODE on arm64 with fake numa. Without this patch, pmem can't be probed as a RAM device on arm64 if SRAT table isn't present: $ndctl create-namespace -fe namespace0.0 --mode=devdax --map=dev -s 1g -a 64K kmem dax0.0: rejecting DAX region [mem 0x240400000-0x2bfffffff] with invalid node: -1 kmem: probe of dax0.0 failed with error -22 This fixes it by using fallback memory_add_physaddr_to_nid() as nid. Suggested-by: David Hildenbrand <david@redhat.com> Signed-off-by: Jia He <justin.he@arm.com> --- I noticed that on powerpc memory_add_physaddr_to_nid is not exported for module driver. Set it to RFC due to this concern. drivers/dax/kmem.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)