| Message ID | 170612969410.2745924.538640158518770317.stgit@djiang5-mobl3 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ndctl: Add support of qos_class for CXL CLI | expand |
On Wed, Jan 24, 2024 at 01:54:54PM -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 check for config check during region creation. Emit a warning if the Maybe "Add a QoS check during region creation." > QoS class token from the root decoder is different than the mem device QoS > class token. User parameter options are provided to fail instead of just > warning. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > v3: > - Rebase to pending branch > --- > Documentation/cxl/cxl-create-region.txt | 9 ++++ > cxl/region.c | 67 +++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt > index f11a412bddfe..9ab2e0fee152 100644 > --- a/Documentation/cxl/cxl-create-region.txt > +++ b/Documentation/cxl/cxl-create-region.txt > @@ -105,6 +105,15 @@ include::bus-option.txt[] > supplied, the first cross-host bridge (if available), decoder that > supports the largest interleave will be chosen. > > +-e:: > +--strict:: > + Enforce strict execution where any potential error will force failure. > + For example, if QTG ID mismatches will cause failure. The definition of this 'Enforce ...' sounds very broad like it's going to be used for more that this QTG ID check. Is it? Maybe I missed some earlier reviews and that is intentional. I was expecting it to say something like: Enforce strict QTG ID matching. Fail to create region on any mismatch. > + > +-q:: > +--no-enforce-qtg:: > + Parameter to bypass QTG ID mismatch failure. Will only emit warning. > + > include::human-option.txt[] > > include::debug-option.txt[] snip >
On 1/26/24 11:14, Alison Schofield wrote: > On Wed, Jan 24, 2024 at 01:54:54PM -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 check for config check during region creation. Emit a warning if the > > Maybe "Add a QoS check during region creation." > >> QoS class token from the root decoder is different than the mem device QoS >> class token. User parameter options are provided to fail instead of just >> warning. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> v3: >> - Rebase to pending branch >> --- >> Documentation/cxl/cxl-create-region.txt | 9 ++++ >> cxl/region.c | 67 +++++++++++++++++++++++++++++++ >> 2 files changed, 75 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt >> index f11a412bddfe..9ab2e0fee152 100644 >> --- a/Documentation/cxl/cxl-create-region.txt >> +++ b/Documentation/cxl/cxl-create-region.txt >> @@ -105,6 +105,15 @@ include::bus-option.txt[] >> supplied, the first cross-host bridge (if available), decoder that >> supports the largest interleave will be chosen. >> >> +-e:: >> +--strict:: >> + Enforce strict execution where any potential error will force failure. >> + For example, if QTG ID mismatches will cause failure. > > The definition of this 'Enforce ...' sounds very broad like it's > going to be used for more that this QTG ID check. Is it? Maybe I > missed some earlier reviews and that is intentional. I left it intentionally broad to allow it be used in other strict checking in the future and not exclusively to qos_class. And then the -q parameter below is used to override for qos_class. Speaking of which, I should fixup the documentation to use qos_class rather than QTG ID. > > I was expecting it to say something like: Enforce strict QTG ID matching. > Fail to create region on any mismatch. > > >> + >> +-q:: >> +--no-enforce-qtg:: >> + Parameter to bypass QTG ID mismatch failure. Will only emit warning. >> + >> include::human-option.txt[] >> >> include::debug-option.txt[] > > > snip > >>
diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt index f11a412bddfe..9ab2e0fee152 100644 --- a/Documentation/cxl/cxl-create-region.txt +++ b/Documentation/cxl/cxl-create-region.txt @@ -105,6 +105,15 @@ include::bus-option.txt[] supplied, the first cross-host bridge (if available), decoder that supports the largest interleave will be chosen. +-e:: +--strict:: + Enforce strict execution where any potential error will force failure. + For example, if QTG ID mismatches will cause failure. + +-q:: +--no-enforce-qtg:: + Parameter to bypass QTG ID mismatch failure. Will only emit warning. + include::human-option.txt[] include::debug-option.txt[] diff --git a/cxl/region.c b/cxl/region.c index 3a762db4800e..00dd63e160df 100644 --- a/cxl/region.c +++ b/cxl/region.c @@ -32,6 +32,8 @@ static struct region_params { bool force; bool human; bool debug; + bool strict; + bool no_qtg; } param = { .ways = INT_MAX, .granularity = INT_MAX, @@ -49,6 +51,8 @@ struct parsed_params { const char **argv; struct cxl_decoder *root_decoder; enum cxl_decoder_mode mode; + bool strict; + bool no_qtg; }; enum region_actions { @@ -81,7 +85,9 @@ 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('e', "strict", ¶m.strict, "strict execution enforcement"), \ +OPT_BOOLEAN('q', "no-enforce-qtg", ¶m.no_qtg, "no enforce of QTG ID") static const struct option create_options[] = { BASE_OPTIONS(), @@ -360,6 +366,9 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, } } + p->strict = param.strict; + p->no_qtg = param.no_qtg; + return 0; err: @@ -467,6 +476,60 @@ static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) p->mode = CXL_DECODER_MODE_PMEM; } +static bool region_qos_match_decoder(struct qos_class *qos_class, int decoder_qc) +{ + int i; + + for (i = 0; i < qos_class->nr; i++) { + if (qos_class->qos[i] == decoder_qc) + return true; + } + + return false; +} + +static int create_region_validate_qtg_id(struct cxl_ctx *ctx, + struct parsed_params *p) +{ + struct qos_class *qos_class; + int root_qos_class, i; + + 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->nr == 0) + break; + + if (!region_qos_match_decoder(qos_class, root_qos_class)) { + if (p->strict && !p->no_qtg) { + log_err(&rl, "%s QoS Class mismatches %s\n", + cxl_decoder_get_devname(p->root_decoder), + cxl_memdev_get_devname(memdev)); + + return -ENXIO; + } + + log_notice(&rl, "%s QoS Class mismatches %s\n", + cxl_decoder_get_devname(p->root_decoder), + cxl_memdev_get_devname(memdev)); + } + } + + return 0; +} + static int create_region_validate_config(struct cxl_ctx *ctx, struct parsed_params *p) { @@ -507,6 +570,8 @@ found: return rc; collect_minsize(ctx, p); + create_region_validate_qtg_id(ctx, p); + return 0; }
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 check for config check during region creation. Emit a warning if the QoS class token from the root decoder is different than the mem device QoS class token. User parameter options are provided to fail instead of just warning. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v3: - Rebase to pending branch --- Documentation/cxl/cxl-create-region.txt | 9 ++++ cxl/region.c | 67 +++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-)