From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Jiang, Dave" <dave.jiang@intel.com>,
"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>
Cc: "Schofield, Alison" <alison.schofield@intel.com>
Subject: Re: [NDCTL PATCH v7 3/4] ndctl: cxl: add QoS class check for CXL region creation
Date: Fri, 23 Feb 2024 22:48:03 +0000 [thread overview]
Message-ID: <681afb7ff4a05fa07b0f449b825c8dd04915c6fa.camel@intel.com> (raw)
In-Reply-To: <20240208201435.2081583-4-dave.jiang@intel.com>
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.
Actually I don't see this interface getting called anywhere. Was there
a patch to cxl_region_to_json() that got dropped?
> +
> 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", ¶m.uuid, \
> "region uuid", "uuid for the new region (default: autogenerate)"), \
> OPT_BOOLEAN('m', "memdevs", ¶m.memdevs, \
> "non-option arguments are memdevs"), \
> -OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number formats")
> +OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number formats"), \
> +OPT_BOOLEAN('Q', "enforce-qos", ¶m.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.
> + 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?
> 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.
next prev parent reply other threads:[~2024-02-23 22:48 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-08 20:11 [NDCTL PATCH v7 4/4] ndctl: Add support of qos_class for CXL CLI Dave Jiang
2024-02-08 20:11 ` [NDCTL PATCH v7 1/4] ndctl: cxl: Add QoS class retrieval for the root decoder Dave Jiang
2024-02-08 20:11 ` [NDCTL PATCH v7 2/4] ndctl: cxl: Add QoS class support for the memory device Dave Jiang
2024-02-08 20:11 ` [NDCTL PATCH v7 3/4] ndctl: cxl: add QoS class check for CXL region creation Dave Jiang
2024-02-23 22:48 ` Verma, Vishal L [this message]
2024-02-26 21:58 ` Dave Jiang
2024-02-08 20:11 ` [NDCTL PATCH v7 4/4] ndctl: add test for qos_class in CXL test suite Dave Jiang
2024-02-22 7:59 ` Verma, Vishal L
2024-02-26 22:39 ` Dave Jiang
2024-02-23 22:07 ` Verma, Vishal L
2024-02-26 23:09 ` Dave Jiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=681afb7ff4a05fa07b0f449b825c8dd04915c6fa.camel@intel.com \
--to=vishal.l.verma@intel.com \
--cc=alison.schofield@intel.com \
--cc=dave.jiang@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).