Re: [PATCH v5 10/12] cxl/region: Add sysfs attribute for locality attributes of CXL regions

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 6 Feb 2024 15:28:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL
> region. The bandwidth is the aggregated bandwidth of all devices that
> contribute to the CXL region. The latency is the worst latency of the
> device amongst all the devices that contribute to the CXL region.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A few comments inline.  Mostly about reducing duplication.

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  60 +++++++++++
>  drivers/cxl/core/region.c               | 134 ++++++++++++++++++++++++
>  2 files changed, 194 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index fff2581b8033..5f8c26815399 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -552,3 +552,63 @@ Description:
>  		attribute is only visible for devices supporting the
>  		capability. The retrieved errors are logged as kernel
>  		events when cxl_poison event tracing is enabled.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/read_bandwidth

Up to you, but it's fairly common to have multiple what lines in these
files and you could easily combine at least the read/write descriptions.

> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The aggregated read bandwidth of the region. The number is
> +		the accumulated read bandwidth of all CXL memory devices that
> +		contributes to the region in MB/s. It is identical data that
> +		should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/read_bandwidth.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/write_bandwidth
> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The aggregated write bandwidth of the region. The number is
> +		the accumulated write bandwidth of all CXL memory devices that
> +		contributes to the region in MB/s. It is identical data that
> +		should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/write_bandwidth.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/read_latency
> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The read latency of the region. The number is
> +		the worst read latency of all CXL memory devices that
> +		contributes to the region in nanoseconds. It is identical data
> +		that should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/read_latency.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/write_latency
> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The write latency of the region. The number is
> +		the worst write latency of all CXL memory devices that
> +		contributes to the region in nanoseconds. It is identical data
> +		that should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/write_latency.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0c2337b1fd41..6347dbc746f9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -30,6 +30,138 @@
>  
>  static struct cxl_region *to_cxl_region(struct device *dev);
>  
> +#define __ACCESS0_ATTR_RO(_name) {				\
> +	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
> +	.show	= _name##_access0_show,				\
> +}
> +
> +#define ACCESS0_DEVICE_ATTR_RO(_name) \
> +	struct device_attribute dev_attr_access0_##_name = __ACCESS0_ATTR_RO(_name)
> +
> +#define ACCESS0_ATTR(attrib)					\
> +static ssize_t attrib##_access0_show(struct device *dev,	\
> +			   struct device_attribute *attr,	\
> +			   char *buf)				\
> +{								\
> +	struct cxl_region *cxlr = to_cxl_region(dev);		\
> +								\
> +	if (cxlr->coord[0].attrib == 0)				\
> +		return -ENOENT;					\
> +								\
> +	return sysfs_emit(buf, "%u\n", cxlr->coord[0].attrib);	\
> +}								\
> +static ACCESS0_DEVICE_ATTR_RO(attrib)
> +
> +ACCESS0_ATTR(read_bandwidth);
> +ACCESS0_ATTR(read_latency);
> +ACCESS0_ATTR(write_bandwidth);
> +ACCESS0_ATTR(write_latency);
> +
> +static struct attribute *access0_coordinate_attrs[] = {
> +	&dev_attr_access0_read_bandwidth.attr,
> +	&dev_attr_access0_write_bandwidth.attr,
> +	&dev_attr_access0_read_latency.attr,
> +	&dev_attr_access0_write_latency.attr,
> +	NULL,
> +};
> +
> +static umode_t cxl_region_access0_coordinate_visible(struct kobject *kobj,
> +						     struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +
> +	if (a == &dev_attr_access0_read_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].read_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access0_write_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].write_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access0_read_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].read_bandwidth == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access0_write_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].write_bandwidth == 0)
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +#define __ACCESS1_ATTR_RO(_name) {				\
> +	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
> +	.show	= _name##_access1_show,				\
> +}

Maybe define __ACCESSX_ATTR_RO(_name, _access)
etc to avoid some duplicaton.

> +
> +#define ACCESS1_DEVICE_ATTR_RO(_name) \
> +	struct device_attribute dev_attr_access1_##_name = __ACCESS1_ATTR_RO(_name)
> +
> +#define ACCESS1_ATTR(attrib)					\
> +static ssize_t attrib##_access1_show(struct device *dev,	\
> +			   struct device_attribute *attr,	\
> +			   char *buf)				\
> +{								\
> +	struct cxl_region *cxlr = to_cxl_region(dev);		\
> +								\
> +	if (cxlr->coord[1].attrib == 0)				\
> +		return -ENOENT;					\
> +								\
> +	return sysfs_emit(buf, "%u\n", cxlr->coord[1].attrib);	\
> +}								\
> +static ACCESS1_DEVICE_ATTR_RO(attrib)
> +
> +ACCESS1_ATTR(read_bandwidth);
> +ACCESS1_ATTR(read_latency);
> +ACCESS1_ATTR(write_bandwidth);
> +ACCESS1_ATTR(write_latency);
> +
> +static struct attribute *access1_coordinate_attrs[] = {
> +	&dev_attr_access1_read_bandwidth.attr,
> +	&dev_attr_access1_write_bandwidth.attr,
> +	&dev_attr_access1_read_latency.attr,
> +	&dev_attr_access1_write_latency.attr,
> +	NULL,
> +};
> +
> +static umode_t cxl_region_access1_coordinate_visible(struct kobject *kobj,
> +						     struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +
> +	if (a == &dev_attr_access1_read_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].read_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access1_write_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].write_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access1_read_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].read_bandwidth == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access1_write_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].write_bandwidth == 0)
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +static const struct attribute_group cxl_region_access0_coordinate_group = {
> +	.name = "access0",
> +	.attrs = access0_coordinate_attrs,
> +	.is_visible = cxl_region_access0_coordinate_visible,
> +};
> +
> +static const struct attribute_group cxl_region_access1_coordinate_group = {
> +	.name = "access1",
> +	.attrs = access1_coordinate_attrs,
> +	.is_visible = cxl_region_access1_coordinate_visible,
> +};
> +
>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> @@ -2039,6 +2171,8 @@ static const struct attribute_group *region_groups[] = {
>  	&cxl_base_attribute_group,
>  	&cxl_region_group,
>  	&cxl_region_target_group,
> +	&cxl_region_access0_coordinate_group,
> +	&cxl_region_access1_coordinate_group,
>  	NULL,
>  };
>