Re: [NDCTL PATCH v7 3/4] ndctl: cxl: add QoS class check for CXL region creation

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

On 2/23/24 3:48 PM, Verma, Vishal L wrote:
> On Thu, 2024-02-08 at 13:11 -0700, Dave Jiang wrote:
>> The CFMWS provides a QTG ID. The kernel driver creates a root decoder that
>> represents the CFMWS. A qos_class attribute is exported via sysfs for the root
>> decoder.
>>
>> One or more qos_class tokens are retrieved via QTG ID _DSM from the ACPI0017
>> device for a CXL memory device. The input for the _DSM is the read and write
>> latency and bandwidth for the path between the device and the CPU. The
>> numbers are constructed by the kernel driver for the _DSM input. When a
>> device is probed, QoS class tokens  are retrieved. This is useful for a
>> hot-plugged CXL memory device that does not have regions created.
>>
>> Add a QoS check during region creation. If --enforce-qos/-Q is set and
>> the qos_class mismatches, the region creation will fail.
>>
>> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v7:
>> - Add qos_class_mismatched to region for cxl list (Vishal)
>> - Add create_region -Q check (Vishal)
>> ---
>>  Documentation/cxl/cxl-create-region.txt |  6 +++
>>  cxl/json.c                              |  6 +++
>>  cxl/lib/libcxl.c                        | 11 +++++
>>  cxl/lib/libcxl.sym                      |  2 +
>>  cxl/lib/private.h                       |  1 +
>>  cxl/libcxl.h                            |  2 +
>>  cxl/region.c                            | 56 ++++++++++++++++++++++++-
>>  7 files changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
>> index f11a412bddfe..b244af60b8a6 100644
>> --- a/Documentation/cxl/cxl-create-region.txt
>> +++ b/Documentation/cxl/cxl-create-region.txt
>> @@ -105,6 +105,12 @@ include::bus-option.txt[]
>>  	supplied, the first cross-host bridge (if available), decoder that
>>  	supports the largest interleave will be chosen.
>>  
>> +-Q::
>> +--enforce-qos::
>> +	Parameter to enforce qos_class mismatch failure. Region create operation
>> +	will fail of the qos_class of the root decoder and one of the memdev that
>> +	backs the region mismatches.
>> +
>>  include::human-option.txt[]
>>  
>>  include::debug-option.txt[]
>> diff --git a/cxl/json.c b/cxl/json.c
>> index c8bd8c27447a..27cbacc84f3a 100644
>> --- a/cxl/json.c
>> +++ b/cxl/json.c
>> @@ -1238,6 +1238,12 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
>>  		}
>>  	}
>>  
>> +	if (cxl_region_qos_class_mismatched(region)) {
>> +		jobj = json_object_new_boolean(true);
>> +		if (jobj)
>> +			json_object_object_add(jregion, "qos_class_mismatched", jobj);
>> +	}
>> +
>>  	json_object_set_userdata(jregion, region, NULL);
>>  
>>  
>> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
>> index 6c293f1dfc91..3461c4de2097 100644
>> --- a/cxl/lib/libcxl.c
>> +++ b/cxl/lib/libcxl.c
>> @@ -414,6 +414,17 @@ CXL_EXPORT int cxl_region_is_enabled(struct cxl_region *region)
>>  	return is_enabled(path);
>>  }
>>  
>> +CXL_EXPORT void cxl_region_qos_class_mismatched_set(struct cxl_region *region,
>> +						  bool mismatched)
>> +{
>> +	region->qos_mismatched = mismatched;
>> +}
> 
> This should be called cxl_region_set_qos_class_mismatched() at a
> minimum, but..
> 
>> +
>> +CXL_EXPORT bool cxl_region_qos_class_mismatched(struct cxl_region *region)
>> +{
>> +	return region->qos_mismatched;
>> +}
> 
> .. I think libcxl always perform its own qos mismatch checking when
> this is called and return appropriately, instead of relying on a user-
> set flag.

Ok. I'll add internal compare for libcxl instead and remove the flag.

> 
> Actually I don't see this interface getting called anywhere. Was there
> a patch to cxl_region_to_json() that got dropped?

It's the first code chunk above.

> 
>> +
>>  CXL_EXPORT int cxl_region_disable(struct cxl_region *region)
>>  {
>>  	const char *devname = cxl_region_get_devname(region);
>> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
>> index 465c78dc6c70..47a9c3cafc71 100644
>> --- a/cxl/lib/libcxl.sym
>> +++ b/cxl/lib/libcxl.sym
>> @@ -285,4 +285,6 @@ global:
>>  	cxl_root_decoder_get_qos_class;
>>  	cxl_memdev_get_pmem_qos_class;
>>  	cxl_memdev_get_ram_qos_class;
>> +	cxl_region_qos_class_mismatched_set;
>> +	cxl_region_qos_class_mismatched;
>>  } LIBCXL_7;
>> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
>> index 07dc8c784f1d..88448d82d53f 100644
>> --- a/cxl/lib/private.h
>> +++ b/cxl/lib/private.h
>> @@ -174,6 +174,7 @@ struct cxl_region {
>>  	struct daxctl_region *dax_region;
>>  	struct kmod_module *module;
>>  	struct list_head mappings;
>> +	bool qos_mismatched;
>>  };
>>  
>>  struct cxl_memdev_mapping {
>> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
>> index a180f01cb05e..7795496cdbbd 100644
>> --- a/cxl/libcxl.h
>> +++ b/cxl/libcxl.h
>> @@ -335,6 +335,8 @@ int cxl_region_clear_target(struct cxl_region *region, int position);
>>  int cxl_region_clear_all_targets(struct cxl_region *region);
>>  int cxl_region_decode_commit(struct cxl_region *region);
>>  int cxl_region_decode_reset(struct cxl_region *region);
>> +void cxl_region_qos_class_mismatched_set(struct cxl_region *region, bool mismatched);
>> +bool cxl_region_qos_class_mismatched(struct cxl_region *region);
>>  
>>  #define cxl_region_foreach(decoder, region)                                    \
>>  	for (region = cxl_region_get_first(decoder); region != NULL;           \
>> diff --git a/cxl/region.c b/cxl/region.c
>> index 3a762db4800e..76df177ef246 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -32,6 +32,7 @@ static struct region_params {
>>  	bool force;
>>  	bool human;
>>  	bool debug;
>> +	bool qos_enforce;
>>  } param = {
>>  	.ways = INT_MAX,
>>  	.granularity = INT_MAX,
>> @@ -49,6 +50,8 @@ struct parsed_params {
>>  	const char **argv;
>>  	struct cxl_decoder *root_decoder;
>>  	enum cxl_decoder_mode mode;
>> +	bool qos_enforce;
>> +	bool qos_mismatched;
>>  };
>>  
>>  enum region_actions {
>> @@ -81,7 +84,8 @@ OPT_STRING('U', "uuid", &param.uuid, \
>>  	   "region uuid", "uuid for the new region (default: autogenerate)"), \
>>  OPT_BOOLEAN('m', "memdevs", &param.memdevs, \
>>  	    "non-option arguments are memdevs"), \
>> -OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats")
>> +OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats"), \
>> +OPT_BOOLEAN('Q', "enforce-qos", &param.qos_enforce, "enforce of qos_class matching")
>>  
>>  static const struct option create_options[] = {
>>  	BASE_OPTIONS(),
>> @@ -360,6 +364,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
>>  		}
>>  	}
>>  
>> +	p->qos_enforce = param.qos_enforce;
>> +
>>  	return 0;
>>  
>>  err:
>> @@ -467,6 +473,49 @@ static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p)
>>  		p->mode = CXL_DECODER_MODE_PMEM;
>>  }
>>  
>> +static int create_region_validate_qos_class(struct cxl_ctx *ctx,
> 
> ctx is never used, can be removed.

ok

> 
>> +					    struct parsed_params *p)
>> +{
>> +	int root_qos_class;
>> +	int qos_class;
>> +	int i;
>> +
>> +	if (!p->qos_enforce)
>> +		return 0;
>> +
>> +	root_qos_class = cxl_root_decoder_get_qos_class(p->root_decoder);
>> +	if (root_qos_class == CXL_QOS_CLASS_NONE)
>> +		return 0;
>> +
>> +	for (i = 0; i < p->ways; i++) {
>> +		struct json_object *jobj =
>> +			json_object_array_get_idx(p->memdevs, i);
>> +		struct cxl_memdev *memdev = json_object_get_userdata(jobj);
>> +
>> +		if (p->mode == CXL_DECODER_MODE_RAM)
>> +			qos_class = cxl_memdev_get_ram_qos_class(memdev);
>> +		else
>> +			qos_class = cxl_memdev_get_pmem_qos_class(memdev);
>> +
>> +		/* No qos_class entries. Possibly no kernel support */
>> +		if (qos_class == CXL_QOS_CLASS_NONE)
>> +			break;
>> +
>> +		if (qos_class != root_qos_class) {
>> +			p->qos_mismatched = true;
>> +			if (p->qos_enforce) {
>> +				log_err(&rl, "%s QoS Class mismatches %s\n",
>> +					cxl_decoder_get_devname(p->root_decoder),
>> +					cxl_memdev_get_devname(memdev));
>> +
>> +				return -ENXIO;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int create_region_validate_config(struct cxl_ctx *ctx,
>>  					 struct parsed_params *p)
>>  {
>> @@ -507,6 +556,10 @@ found:
>>  		return rc;
>>  
>>  	collect_minsize(ctx, p);
>> +	rc = create_region_validate_qos_class(ctx, p);
>> +	if (rc)
>> +		return rc;
>> +
> 
> Maybe this call can be moved into the existing validate_decoder() check
> since?

ok

> 
>>  	return 0;
>>  }
>>  
>> @@ -654,6 +707,7 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>>  		return -EOPNOTSUPP;
>>  	}
>>  
>> +	cxl_region_qos_class_mismatched_set(region, p->qos_mismatched);
>>  	devname = cxl_region_get_devname(region);
>>  
>>  	rc = cxl_region_determine_granularity(region, p);
> 
> I think as a future enhancement, it might be nice to add
> cxl_filter_walk() smarts to allow it to filter memdevs based on
> qos_class.  That way, when cxl create-region is called without any
> memdev arguments (i.e. it is free to select memdevs), collect_memdevs()
> can ask for memdevs that match the qos_class, and see if those can
> satisfy the interleave requirements if --enforce-qos is used.