On Tue, 6 Feb 2024 15:28:38 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
> +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@xxxxxxxxxxxxxxx
> +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@xxxxxxxxxxxxxxx
> +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@xxxxxxxxxxxxxxx
> +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,
> };
>
