Re: [PATCH v6 10/11] cxl: Export sysfs attributes for memory device QoS class

[Dave Jiang <dave.jiang@intel.com>]

On 6/1/23 07:30, Jonathan Cameron wrote:
> On Fri, 19 May 2023 10:07:34 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> Export qos_class sysfs attributes for the CXL memory device. The QoS clas
>> should show up as /sys/bus/cxl/devices/memX/ram/qos_class for the volatile
>> partition and /sys/bus/cxl/devices/memX/pmem/qos_class for the persistent
>> partition. The QTG ID is retrieved via _DSM after supplying the
>> calculated bandwidth and latency for the entire CXL path from device to
>> the CPU. This ID is used to match up to the root decoder QoS class to
>> determine which CFMWS the memory range of a hotplugged CXL mem device
>> should be assigned under.
>>
>> While there may be multiple DSMAS exported by the device CDAT, the driver
>> will only expose the first QTG ID per partition in sysfs for now. In the
>> future when multiple QTG IDs are necessary, they can be exposed. [1]
>>
>> [1]: https://lore.kernel.org/linux-cxl/167571650007.587790.10040913293130712882.stgit@djiang5-mobl3.local/T/#md2a47b1ead3e1ba08f50eab29a4af1aed1d215ab
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> A few minor suggestions inline.
> In particular some code duplication that looks easy to avoid.
>
> Jonathan
>
>> ---
>> v6:
>> - Provide full ordered QTG IDs from _DSM. (Jonathan)
>> v5:
>> - Change qtg_id to qos_class
>> v4:
>> - Change kernel version for documentation to v6.5
>> v3:
>> - Expand description of qtg_id. (Alison)
>> ---
>>   Documentation/ABI/testing/sysfs-bus-cxl |   34 +++++++++++++++++++
>>   drivers/cxl/core/memdev.c               |   55 +++++++++++++++++++++++++++++++
>>   2 files changed, 89 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
>> index ccfc7ecc61f5..7a4cacd382f9 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-cxl
>> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
>> @@ -28,6 +28,23 @@ Description:
>>   		Payload in the CXL-2.0 specification.
>>   
>>   
>> +What:		/sys/bus/cxl/devices/memX/ram/qos_class
>> +Date:		May, 2023
>> +KernelVersion:	v6.5
>> +Contact:	linux-cxl@vger.kernel.org
>> +Description:
>> +		(RO) For CXL host platforms that support "QoS Telemmetry"
>> +		this attribute conveys a comma delimited list of platform
>> +		specific cookies that identifies a QoS performance class
>> +		for the volatile 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 side-effects that may
>> +		result.
> That sounds scary - perhaps "platform specific performance related
> side-effects that may result."

Ok will update


>> +
>> +
>>   What:		/sys/bus/cxl/devices/memX/pmem/size
>>   Date:		December, 2020
>>   KernelVersion:	v5.12
>> @@ -38,6 +55,23 @@ Description:
>>   		Payload in the CXL-2.0 specification.
>>   
>>   
>> +What:		/sys/bus/cxl/devices/memX/pmem/qos_class
>> +Date:		May, 2023
>> +KernelVersion:	v6.5
>> +Contact:	linux-cxl@vger.kernel.org
>> +Description:
>> +		(RO) For CXL host 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 side-effects that may
>> +		result.
>> +
>> +
>>   What:		/sys/bus/cxl/devices/memX/serial
>>   Date:		January, 2022
>>   KernelVersion:	v5.18
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 057a43267290..9366257e3183 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -77,6 +77,32 @@ static ssize_t ram_size_show(struct device *dev, struct device_attribute *attr,
>>   static struct device_attribute dev_attr_ram_size =
>>   	__ATTR(size, 0444, ram_size_show, NULL);
>>   
>> +static ssize_t ram_qos_class_show(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct qos_class *qos_class = cxlds->ram_qos_class;
>> +	int count, i;
>> +
>> +	if (!qos_class)
>> +		return 0;
>> +
>> +	for (i = 0, count = 0; i < qos_class->nr; i++) {
>> +		count += sysfs_emit_at(buf, count, "%d",
>> +				       qos_class->entries[i]);
>> +		if (i + 1 == qos_class->nr)
>> +			count += sysfs_emit_at(buf, count, "\n");
>> +		else
>> +			count += sysfs_emit_at(buf, count, ", ");
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static struct device_attribute dev_attr_ram_qos_class =
>> +	__ATTR(qos_class, 0444, ram_qos_class_show, NULL);
>> +
>>   static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>>   			      char *buf)
>>   {
>> @@ -90,6 +116,33 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>>   static struct device_attribute dev_attr_pmem_size =
>>   	__ATTR(size, 0444, pmem_size_show, NULL);
>>   
>> +static ssize_t pmem_qos_class_show(struct device *dev,
>> +				   struct device_attribute *attr, char *buf)
>> +{
>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct qos_class *qos_class = cxlds->pmem_qos_class;
>> +	int count, i;
>> +
>> +	if (!qos_class)
>> +		return 0;
>> +
>> +	for (i = 0, count = 0; i < qos_class->nr; i++) {
>> +		count += sysfs_emit_at(buf, count, "%d",
>> +				       qos_class->entries[i]);
>> +
>> +		if (i + 1 == qos_class->nr)
>> +			count += sysfs_emit_at(buf, count, "\n");
>> +		else
>> +			count += sysfs_emit_at(buf, count, ", ");
>> +	}
> Bunch of shred code here with the volatile case.
> Add a little utility function to avoid that?

ok


>> +
>> +	return count;
>> +}
>> +
>> +static struct device_attribute dev_attr_pmem_qos_class =
>> +	__ATTR(qos_class, 0444, pmem_qos_class_show, NULL);
>> +
>>   static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
>>   			   char *buf)
>>   {
>> @@ -344,11 +397,13 @@ static struct attribute *cxl_memdev_attributes[] = {
>>   
>>   static struct attribute *cxl_memdev_pmem_attributes[] = {
>>   	&dev_attr_pmem_size.attr,
>> +	&dev_attr_pmem_qos_class.attr,
>>   	NULL,
>>   };
>>   
>>   static struct attribute *cxl_memdev_ram_attributes[] = {
>>   	&dev_attr_ram_size.attr,
>> +	&dev_attr_ram_qos_class.attr,
>>   	NULL,
>>   };
>>   
>>
>>