Re: [PATCH v3 3/3] libnvdimm: Add sysfs numa_node to NVDIMM devices

[Dan Williams]

On Wed, Jun 24, 2015 at 9:38 AM, Toshi Kani <toshi.kani@xxxxxx> wrote:
> On Tue, 2015-06-23 at 17:26 -0700, Dan Williams wrote:
>> On Fri, Jun 19, 2015 at 11:18 AM, Toshi Kani <toshi.kani@xxxxxx> wrote:
>> > Add support of sysfs 'numa_node' to I/O-related NVDIMM devices
>> > under /sys/bus/nd/devices, regionN, namespaceN.0, and bttN.
>> > When bttN is not set up, its numa_node returns -1 (NUMA_NO_NODE).
>> >
>> > An example of numa_node values on a 2-socket system with a single
>> > NVDIMM range on each socket is shown below.
>> >   /sys/bus/nd/devices
>> >   |-- btt0/numa_node:-1
>> >   |-- btt1/numa_node:0
>> >   |-- namespace0.0/numa_node:0
>> >   |-- namespace1.0/numa_node:1
>> >   |-- region0/numa_node:0
>> >   |-- region1/numa_node:1
>> >
>> > These numa_node files are then linked under the block class of
>> > their device names.
>> >   /sys/class/block/pmem0/device/numa_node:0
>> >   /sys/class/block/pmem0s/device/numa_node:0
>> >   /sys/class/block/pmem1/device/numa_node:1
>> >
>> > This enables numactl(8) to accept 'block:' and 'file:' paths of
>> > pmem and btt devices as shown in the examples below.
>> >   numactl --preferred block:pmem0 --show
>> >   numactl --preferred file:/dev/pmem0s --show
>> >
>> > Signed-off-by: Toshi Kani <toshi.kani@xxxxxx>
>> > ---
>> >  drivers/acpi/nfit.c             |    1 +
>> >  drivers/nvdimm/btt_devs.c       |    1 +
>> >  drivers/nvdimm/bus.c            |   30 ++++++++++++++++++++++++++++++
>> >  drivers/nvdimm/namespace_devs.c |    1 +
>> >  include/linux/libnvdimm.h       |    1 +
>> >  5 files changed, 34 insertions(+)
>> >
>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index 5997753..9cb63ac 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -873,6 +873,7 @@ static const struct attribute_group *acpi_nfit_region_attribute_groups[] = {
>> >         &nd_region_attribute_group,
>> >         &nd_mapping_attribute_group,
>> >         &nd_device_attribute_group,
>> > +       &nd_numa_attribute_group,
>> >         &acpi_nfit_region_attribute_group,
>> >         NULL,
>> >  };
>> > diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
>> > index bcf77dc..a7b192f 100644
>> > --- a/drivers/nvdimm/btt_devs.c
>> > +++ b/drivers/nvdimm/btt_devs.c
>> > @@ -308,6 +308,7 @@ static struct attribute_group nd_btt_attribute_group = {
>> >  static const struct attribute_group *nd_btt_attribute_groups[] = {
>> >         &nd_btt_attribute_group,
>> >         &nd_device_attribute_group,
>> > +       &nd_numa_attribute_group,
>> >         NULL,
>> >  };
>> >
>> > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>> > index 67525f9..03c0ee1 100644
>> > --- a/drivers/nvdimm/bus.c
>> > +++ b/drivers/nvdimm/bus.c
>> > @@ -420,6 +420,36 @@ struct attribute_group nd_device_attribute_group = {
>> >  };
>> >  EXPORT_SYMBOL_GPL(nd_device_attribute_group);
>> >
>> > +static ssize_t numa_node_show(struct device *dev,
>> > +               struct device_attribute *attr, char *buf)
>> > +{
>> > +       return sprintf(buf, "%d\n", dev_to_node(dev));
>> > +}
>>
>> So patch 2 collided with the requested BTT stacking rework and
>> prompted me to take a closer look.  Shouldn't numa_node_show() be
>> changed like this?
>
> numa_node_show() is listed in its own nd_numa_attribute_group for using
> is_visible.  This nd_numa_attribute_group is then listed by region
> (acpi_nfit_region_attribute_groups), namespace
> (nd_namespace_attribute_groups), and btt (nd_btt_attribute_groups).
> Therefore, numa_node_show() is only called with these device objects.
> So, I do not think we need such change.  Or are you suggesting to change
> the way attribute group is set?

No, my mistake I missed this hunk in drivers/nvdimm/region.c

@@ -47,6 +47,7 @@ static int nd_region_probe(struct device *dev)

        num_ns->active = rc;
        num_ns->count = rc + err;
+       set_dev_node(dev, nd_region->numa_node);
        dev_set_drvdata(dev, num_ns);

        if (rc && err && rc == err)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/