Re: [PATCH v4 13/28] cxl/mem: Expose DCD partition capabilities in sysfs

Ira Weiny
2024-10-16T21:34:47Z

Jonathan Cameron wrote:
> On Mon, 07 Oct 2024 18:16:19 -0500
> ira.weiny@intel.com wrote:
>
> > From: Navneet Singh <navneet.singh@intel.com>
> >
> > To properly configure CXL regions on Dynamic Capacity Devices (DCD),
> > user space will need to know the details of the DC partitions available.
> >
> > Expose dynamic capacity capabilities through sysfs.
> >
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Some trivial stuff inline that I'm not that bothered about either way.
>
> Subject to answering Fan's query
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>

[snip]

> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 3f5627a1210a..b865eefdb74c 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -54,6 +54,51 @@ Description:
> > identically named field in the Identify Memory Device Output
> > Payload in the CXL-2.0 specification.
> >
> > +What: /sys/bus/cxl/devices/memX/dcY/size
> > +Date: December, 2024
> > +KernelVersion: v6.13
> > +Contact: linux-cxl@vger.kernel.org
> > +Description:
> > + (RO) Dynamic Capacity (DC) region information. Devices only
> > + export dcY if DCD partition Y is supported.
> > + dcY/size is the size of each of those partitions.
> > +
> > +What: /sys/bus/cxl/devices/memX/dcY/read_only
> > +Date: December, 2024
> > +KernelVersion: v6.13
> > +Contact: linux-cxl@vger.kernel.org
> > +Description:
> > + (RO) Dynamic Capacity (DC) region information. Devices only
> > + export dcY if DCD partition Y is supported.
> > + dcY/read_only indicates true if the region is exported
> > + read_only from the device.
> > +
> > +What: /sys/bus/cxl/devices/memX/dcY/shareable
> > +Date: December, 2024
> > +KernelVersion: v6.13
> > +Contact: linux-cxl@vger.kernel.org
> > +Description:
> > + (RO) Dynamic Capacity (DC) region information. Devices only
> > + export dcY if DCD partition Y is supported.
> > + dcY/shareable indicates true if the region is exported
> > + shareable from the device.
> > +
> > +What: /sys/bus/cxl/devices/memX/dcY/qos_class
> > +Date: December, 2024
> > +KernelVersion: v6.13
> > +Contact: linux-cxl@vger.kernel.org
> > +Description:
> > + (RO) Dynamic Capacity (DC) region information. Devices only
> > + export dcY if DCD partition Y is supported.
>
> You can document sysfs directories I think, e.g.
> https://elixir.bootlin.com/linux/v6.12-rc2/source/Documentation/ABI/stable/sysfs-devices-node#L32
> so maybe
>
> What: /sys/bus/cxl/device/memX/dcY
> Date: December, 2024
> KernelVersion: v6.13
> Contact: linux-cxl@vger.kernel.org
> Description:
> Directory containing Dynamic Capacity (DC) region information.
> Devices only export dcY if DCD partition Y is supported.
>
> What: /sys/bus/cxl/devices/memX/dcY/qos_class
> Date: December, 2024
> KernelVersion: v6.13
> Contact: linux-cxl@vger.kernel.org
> Description:
> For CXL host...
>
> To avoid the repetition of first bit of docs?

The other docs don't do this. For example:

/sys/bus/cxl/devices/memX
/sys/bus/cxl/devices/memX/ram
/sys/bus/cxl/devices/memX/pmem

Are not documented like that. I'm inclined to leave it.

>
> > + platforms that support "QoS Telemmetry" this attribute conveys
> > + a comma delimited list of platform specific cookies that
> > + identifies a QoS performance class for the persistent partition
> > + of the CXL mem device. These class-ids can be compared against
> > + a similar "qos_class" published for a root decoder. While it is
> > + not required that the endpoints map their local memory-class to
> > + a matching platform class, mismatches are not recommended and
> > + there are platform specific performance related side-effects
> > + that may result. First class-id is displayed.
> >
> > What: /sys/bus/cxl/devices/memX/pmem/qos_class
> > Date: May, 2023
>
>
> > +static ssize_t show_shareable_dcN(struct cxl_memdev *cxlmd, char *buf, int pos)
> > +{
> > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> > +
> > + return sysfs_emit(buf, "%s\n",
> > + str_false_true(mds->dc_region[pos].shareable));
>
> Fan has already raised that these seem backwards.

Yep fixed.

[snip]

> > +static struct attribute *cxl_memdev_dc##n##_attributes[] = { \
> > + &dc##n##_size.attr, \
> > + &dc##n##_read_only.attr, \
> > + &dc##n##_shareable.attr, \
> > + &dc##n##_qos_class.attr, \
> > + NULL, \
>
> No comma needed on terminator.

Fixed.

>
> > +}; \
> > +static umode_t cxl_memdev_dc##n##_attr_visible(struct kobject *kobj, \
> > + struct attribute *a, \
> > + int pos) \
> > +{ \
> > + struct device *dev = kobj_to_dev(kobj); \
> > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); \
> > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); \
> > + \
> > + /* Not a memory device */ \
> > + if (!mds) \
> if (!to_cxl_memdev_state(cxlmd->cxlds))
> return 0;
>
> I dislike long macros so if we can shave them down that is always good!

Agreed but this was the most straight forward way to deal with this. I
could perhaps break it up by having a 'master macro' which is made of
smaller macros... But this works.

>
> We do have precedence in hdm.c

Not in hdm.c directly but all the 'to_XXX()' calls have a type check. So
it is modeled that way and is called from other places.

>
> for just checking the type directly so maybe
> if (cxlmd->cxlds->type != CXL_DEVTYPE_CLASSMEM)
>
> but the above is also fine as compiler should be able to figure out it
> doesn't need to do the second half of the inline.

I'm going to leave it.

>
>
> > + return 0; \
> > + return a->mode; \
> > +} \
> > +static umode_t cxl_memdev_dc##n##_group_visible(struct kobject *kobj) \
> > +{ \
> > + struct device *dev = kobj_to_dev(kobj); \
> > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); \
> > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); \
> > + \
> > + /* Not a memory device or partition not supported */ \
> > + if (!mds || n >= mds->nr_dc_region) \
> > + return false; \
> > + return true; \
>
> /* Memory device and partition is supported */
> return mds && n < mds->nr_dc_region;

Done.
Ira