diff mbox series

[04/16] libnvdimm: Move nd_numa_attribute_group to device_type

Message ID 157309901655.1582359.18126990555058555754.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
State Superseded
Headers show
Series Memory Hierarchy: Enable target node lookups for reserved memory | expand

Commit Message

Dan Williams Nov. 7, 2019, 3:56 a.m. UTC
A 'struct device_type' instance can carry default attributes for the
device. Use this facility to remove the export of
nd_numa_attribute_group and put the responsibility on the core rather
than leaf implementations to define this attribute.

Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c |    1 -
 drivers/acpi/nfit/core.c                  |    1 -
 drivers/nvdimm/bus.c                      |    3 +--
 drivers/nvdimm/nd.h                       |    1 +
 drivers/nvdimm/region_devs.c              |    1 +
 include/linux/libnvdimm.h                 |    1 -
 6 files changed, 3 insertions(+), 5 deletions(-)

Comments

Aneesh Kumar K.V Nov. 12, 2019, 9:22 a.m. UTC | #1
Dan Williams <dan.j.williams@intel.com> writes:

> A 'struct device_type' instance can carry default attributes for the
> device. Use this facility to remove the export of
> nd_numa_attribute_group and put the responsibility on the core rather
> than leaf implementations to define this attribute.
>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: "Oliver O'Halloran" <oohall@gmail.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>


can we also expose target_node in a similar way? This allows application
to better understand the node locality of the SCM device.

-aneesh
Dan Williams Nov. 13, 2019, 1:26 a.m. UTC | #2
On Tue, Nov 12, 2019 at 1:23 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > A 'struct device_type' instance can carry default attributes for the
> > device. Use this facility to remove the export of
> > nd_numa_attribute_group and put the responsibility on the core rather
> > than leaf implementations to define this attribute.
> >
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: "Oliver O'Halloran" <oohall@gmail.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
>
> can we also expose target_node in a similar way? This allows application
> to better understand the node locality of the SCM device.

It is already exported for device-dax instances. See
DEVICE_ATTR_RO(target_node) in drivers/dax/bus.c. I did not see a use
case for it to be exported for other nvdimm device types.
Aneesh Kumar K.V Nov. 13, 2019, 6:02 a.m. UTC | #3
On 11/13/19 6:56 AM, Dan Williams wrote:
> On Tue, Nov 12, 2019 at 1:23 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>>> A 'struct device_type' instance can carry default attributes for the
>>> device. Use this facility to remove the export of
>>> nd_numa_attribute_group and put the responsibility on the core rather
>>> than leaf implementations to define this attribute.
>>>
>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: "Oliver O'Halloran" <oohall@gmail.com>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>
>>
>> can we also expose target_node in a similar way? This allows application
>> to better understand the node locality of the SCM device.
> 
> It is already exported for device-dax instances. See
> DEVICE_ATTR_RO(target_node) in drivers/dax/bus.c. I did not see a use
> case for it to be exported for other nvdimm device types.
> 

some applications do want to access the fsdax namspace as different 
mount points based on numa affinity. If can differentiate the two 
regions with different target_node and same numa_node, that will help 
them better isolate these mounts.

-aneesh
Dan Williams Nov. 13, 2019, 6:14 a.m. UTC | #4
On Tue, Nov 12, 2019 at 10:02 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 11/13/19 6:56 AM, Dan Williams wrote:
> > On Tue, Nov 12, 2019 at 1:23 AM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> Dan Williams <dan.j.williams@intel.com> writes:
> >>
> >>> A 'struct device_type' instance can carry default attributes for the
> >>> device. Use this facility to remove the export of
> >>> nd_numa_attribute_group and put the responsibility on the core rather
> >>> than leaf implementations to define this attribute.
> >>>
> >>> Cc: Ira Weiny <ira.weiny@intel.com>
> >>> Cc: Michael Ellerman <mpe@ellerman.id.au>
> >>> Cc: "Oliver O'Halloran" <oohall@gmail.com>
> >>> Cc: Vishal Verma <vishal.l.verma@intel.com>
> >>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >>
> >>
> >> can we also expose target_node in a similar way? This allows application
> >> to better understand the node locality of the SCM device.
> >
> > It is already exported for device-dax instances. See
> > DEVICE_ATTR_RO(target_node) in drivers/dax/bus.c. I did not see a use
> > case for it to be exported for other nvdimm device types.
> >
>
> some applications do want to access the fsdax namspace as different
> mount points based on numa affinity. If can differentiate the two
> regions with different target_node and same numa_node, that will help
> them better isolate these mounts.

Can you be more explicit than "some", and what's the impact if the
kernel continues with the status quo and does not export this data?
I'm trying to come up with the justification to include into the
changelog that adds that information.

At least on the pmem platforms I am working with the pmem ranges with
different target nodes also appear on different numa nodes so there is
no incremental benefit for target node there.
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 04726f8fd189..6ffda03a6349 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -287,7 +287,6 @@  int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 static const struct attribute_group *region_attr_groups[] = {
 	&nd_region_attribute_group,
 	&nd_mapping_attribute_group,
-	&nd_numa_attribute_group,
 	NULL,
 };
 
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index dec7c2b08672..b3213faf37b5 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2198,7 +2198,6 @@  static const struct attribute_group acpi_nfit_region_attribute_group = {
 static const struct attribute_group *acpi_nfit_region_attribute_groups[] = {
 	&nd_region_attribute_group,
 	&nd_mapping_attribute_group,
-	&nd_numa_attribute_group,
 	&acpi_nfit_region_attribute_group,
 	NULL,
 };
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index eb422527dd57..28e1b265aa63 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -697,11 +697,10 @@  static umode_t nd_numa_attr_visible(struct kobject *kobj, struct attribute *a,
 /*
  * nd_numa_attribute_group - NUMA attributes for all devices on an nd bus
  */
-struct attribute_group nd_numa_attribute_group = {
+const struct attribute_group nd_numa_attribute_group = {
 	.attrs = nd_numa_attributes,
 	.is_visible = nd_numa_attr_visible,
 };
-EXPORT_SYMBOL_GPL(nd_numa_attribute_group);
 
 int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus)
 {
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 3f509bb6b5c0..e7f3a2fee2ab 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -299,6 +299,7 @@  struct device *nd_pfn_devinit(struct nd_pfn *nd_pfn,
 int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig);
 extern const struct attribute_group *nd_pfn_attribute_groups[];
 extern const struct attribute_group nd_device_attribute_group;
+extern const struct attribute_group nd_numa_attribute_group;
 #else
 static inline int nd_pfn_probe(struct device *dev,
 		struct nd_namespace_common *ndns)
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 710b5111eaa8..e4281f806adc 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -765,6 +765,7 @@  EXPORT_SYMBOL_GPL(nd_region_attribute_group);
 
 static const struct attribute_group *nd_region_attribute_groups[] = {
 	&nd_device_attribute_group,
+	&nd_numa_attribute_group,
 	NULL,
 };
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index d7dbf42498af..e9a4e25fc708 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -67,7 +67,6 @@  enum {
 
 extern struct attribute_group nvdimm_bus_attribute_group;
 extern struct attribute_group nvdimm_attribute_group;
-extern struct attribute_group nd_numa_attribute_group;
 extern struct attribute_group nd_region_attribute_group;
 extern struct attribute_group nd_mapping_attribute_group;