Re: [NDCTL PATCH v3 2/3] ndctl: cxl: Add QoS class support for the memory device

From: Dave Jiang <dave.jiang@intel.com>
To: Alison Schofield <alison.schofield@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <nvdimm@lists.linux.dev>,
	<vishal.l.verma@intel.com>
Subject: Re: [NDCTL PATCH v3 2/3] ndctl: cxl: Add QoS class support for the memory device
Date: Tue, 30 Jan 2024 13:48:26 -0700	[thread overview]
Message-ID: <b3c34787-db24-4d82-8c63-3b070df5ff60@intel.com> (raw)
In-Reply-To: <ZbPzj90keGE0zr0K@aschofie-mobl2>

On 1/26/24 11:01, Alison Schofield wrote:
> On Wed, Jan 24, 2024 at 01:54:47PM -0700, Dave Jiang wrote:
>> Add libcxl API to retrieve the QoS class tokens for the memory
>> devices. Two API calls are added. One for 'ram' or 'volatile'
>> mode and another for 'pmem' or 'persistent' mode. Support also added
>> for displaying the QoS class tokens through the 'cxl list' command.
>> There can be 1 or more QoS class tokens for the memory device if
>> they are valid. The qos_class tokens are displayed behind -vvv
>> verbose level.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> One tidbit below. 
> 
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> 
> 
>> ---
>> v3:
>> - Rebase to pending branch
>> - Skip from failing if no qos_class sysfs attrib found
>> ---
>>  cxl/json.c         |   36 +++++++++++++++++++++++++++++++++++-
>>  cxl/lib/libcxl.c   |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  cxl/lib/libcxl.sym |    2 ++
>>  cxl/lib/private.h  |    2 ++
>>  cxl/libcxl.h       |    7 +++++++
>>  5 files changed, 94 insertions(+), 1 deletion(-)
>>
>> diff --git a/cxl/json.c b/cxl/json.c
>> index 48a43ddf14b0..dcbac8c14f03 100644
>> --- a/cxl/json.c
>> +++ b/cxl/json.c
>> @@ -770,12 +770,32 @@ err_free:
>>  	return jpoison;
>>  }
>>  
>> +static struct json_object *get_qos_json_object(struct json_object *jdev,
>> +					       struct qos_class *qos_class)
>> +{
>> +	struct json_object *jqos_array = json_object_new_array();
>> +	struct json_object *jobj;
>> +	int i;
>> +
>> +	if (!jqos_array)
>> +		return NULL;
>> +
>> +	for (i = 0; i < qos_class->nr; i++) {
>> +		jobj = json_object_new_int(qos_class->qos[i]);
>> +		if (jobj)
>> +			json_object_array_add(jqos_array, jobj);
>> +	}
>> +
>> +	return jqos_array;
>> +}
>> +
>>  struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>>  		unsigned long flags)
>>  {
>>  	const char *devname = cxl_memdev_get_devname(memdev);
>> -	struct json_object *jdev, *jobj;
>> +	struct json_object *jdev, *jobj, *jqos;
> 
> Can the generic *jobj be used below rather than adding the new *jqos?

With the kernel code change, this code is simplified and jobj will be used now.

DJ

> 
> 
>>  	unsigned long long serial, size;
>> +	struct qos_class *qos_class;
>>  	int numa_node;
>>  
>>  	jdev = json_object_new_object();
>> @@ -791,6 +811,13 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>>  		jobj = util_json_object_size(size, flags);
>>  		if (jobj)
>>  			json_object_object_add(jdev, "pmem_size", jobj);
>> +
>> +		if (flags & UTIL_JSON_QOS_CLASS) {
>> +			qos_class = cxl_memdev_get_pmem_qos_class(memdev);
>> +			jqos = get_qos_json_object(jdev, qos_class);
>> +			if (jqos)
>> +				json_object_object_add(jdev, "pmem_qos_class", jqos);
>> +		}
>>  	}
>>  
>>  	size = cxl_memdev_get_ram_size(memdev);
>> @@ -798,6 +825,13 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>>  		jobj = util_json_object_size(size, flags);
>>  		if (jobj)
>>  			json_object_object_add(jdev, "ram_size", jobj);
>> +
>> +		if (flags & UTIL_JSON_QOS_CLASS) {
>> +			qos_class = cxl_memdev_get_ram_qos_class(memdev);
>> +			jqos = get_qos_json_object(jdev, qos_class);
>> +			if (jqos)
>> +				json_object_object_add(jdev, "ram_qos_class", jqos);
>> +		}
>>  	}
>>  
>>  	if (flags & UTIL_JSON_HEALTH) {
> 
> snip
> 
>>
>>