| Message ID | 166777848122.1238089.2150948506074701593.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl-cli test and usability updates | expand |
On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote: > Allow for: > > cxl create-region -d decoderX.Y > > ...to assume (-m -w $(count of memdevs beneath decoderX.Y)) I'm not understanding what the change is here. Poked around a bit and still didn't get it. Help! Leaving out the -m leads to this: $ cxl create-region -d decoder3.3 mem0 mem1 cxl region: parse_create_options: must specify option for target object types (-m) cxl region: cmd_create_region: created 0 regions Leaving out the the -m and the memdevs fails because the memdev order is not correct. $ cxl create-region -d decoder3.3 cxl region: create_region: region5: failed to set target0 to mem1 cxl region: cmd_create_region: created 0 regions This still works, where I give the -m and the correct order of memdevs. cxl create-region -m -d decoder3.3 mem0 mem1 > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > cxl/region.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/cxl/region.c b/cxl/region.c > index aa0735194fa1..c0cf4ab350da 100644 > --- a/cxl/region.c > +++ b/cxl/region.c > @@ -227,10 +227,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, > } > > /* > - * For all practical purposes, -m is the default target type, but > - * hold off on actively making that decision until a second target > - * option is available. > + * For all practical purposes, -m is the default target type, but hold > + * off on actively making that decision until a second target option is > + * available. Unless there are no arguments then just assume memdevs. > */ > + if (!count) > + param.memdevs = true; > + > if (!param.memdevs) { > log_err(&rl, > "must specify option for target object types (-m)\n"); > @@ -272,11 +275,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, > p->ways = count; > if (!validate_ways(p, count)) > return -EINVAL; > - } else { > - log_err(&rl, > - "couldn't determine interleave ways from options or arguments\n"); > - return -EINVAL; > - } > + } else > + p->ways = p->num_memdevs; > > if (param.granularity < INT_MAX) { > if (param.granularity <= 0) { >
Alison Schofield wrote: > On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote: > > Allow for: > > > > cxl create-region -d decoderX.Y > > > > ...to assume (-m -w $(count of memdevs beneath decoderX.Y)) > > I'm not understanding what the change is here. Poked around a bit > and still didn't get it. Help! > > Leaving out the -m leads to this: > $ cxl create-region -d decoder3.3 mem0 mem1 > cxl region: parse_create_options: must specify option for target object types (-m) > cxl region: cmd_create_region: created 0 regions > > Leaving out the the -m and the memdevs fails because the memdev order is > not correct. > $ cxl create-region -d decoder3.3 > cxl region: create_region: region5: failed to set target0 to mem1 > cxl region: cmd_create_region: created 0 regions > > This still works, where I give the -m and the correct order of memdevs. > cxl create-region -m -d decoder3.3 mem0 mem1 Oh, I was not expecting the lack of automatic memdev sorting to rear its head so quickly, and thought that "cxl list" order was good enough for most configurations. Can provide more details on your configuration in this case? If this is current cxl_test then I already do not expect it to work with anything but decoder3.4 since the other decoders have more complicated ordering constraints. I.e. your: cxl create-region -d decoder3.3 ...worked as expected in that it found some memdevs to attempt to create the region, but you got unlucky in the sense that the default order that 'cxl list' returns memdevs was incompatible with creating a region.
On Mon, Nov 07, 2022 at 03:48:43PM -0800, Dan Williams wrote: > Alison Schofield wrote: > > On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote: > > > Allow for: > > > > > > cxl create-region -d decoderX.Y > > > > > > ...to assume (-m -w $(count of memdevs beneath decoderX.Y)) > > > > I'm not understanding what the change is here. Poked around a bit > > and still didn't get it. Help! > > > > Leaving out the -m leads to this: > > $ cxl create-region -d decoder3.3 mem0 mem1 > > cxl region: parse_create_options: must specify option for target object types (-m) > > cxl region: cmd_create_region: created 0 regions > > > > Leaving out the the -m and the memdevs fails because the memdev order is > > not correct. > > $ cxl create-region -d decoder3.3 > > cxl region: create_region: region5: failed to set target0 to mem1 > > cxl region: cmd_create_region: created 0 regions > > > > This still works, where I give the -m and the correct order of memdevs. > > cxl create-region -m -d decoder3.3 mem0 mem1 > > Oh, I was not expecting the lack of automatic memdev sorting to rear its > head so quickly, and thought that "cxl list" order was good enough for > most configurations. I wasn't clear on what was being advertised as supported with this change. I didn't read this as an announcement of automatic region creation, but it seemed you were hinting at it. > > Can provide more details on your configuration in this case? If this is > current cxl_test then I already do not expect it to work with anything > but decoder3.4 since the other decoders have more complicated ordering > constraints. > > I.e. your: > > cxl create-region -d decoder3.3 > > ...worked as expected in that it found some memdevs to attempt to create > the region, but you got unlucky in the sense that the default order that > 'cxl list' returns memdevs was incompatible with creating a region. Pretty much exactly as you say above. My example was w cxl_test decoder3.3, w 2 HB's. The automagic worked fine w decoder3.4 w 1 HB.
Alison Schofield wrote: > On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote: > > Allow for: > > > > cxl create-region -d decoderX.Y > > > > ...to assume (-m -w $(count of memdevs beneath decoderX.Y)) > > I'm not understanding what the change is here. Poked around a bit > and still didn't get it. Help! > > Leaving out the -m leads to this: > $ cxl create-region -d decoder3.3 mem0 mem1 > cxl region: parse_create_options: must specify option for target object types (-m) > cxl region: cmd_create_region: created 0 regions > > Leaving out the the -m and the memdevs fails because the memdev order is > not correct. > $ cxl create-region -d decoder3.3 > cxl region: create_region: region5: failed to set target0 to mem1 > cxl region: cmd_create_region: created 0 regions > > This still works, where I give the -m and the correct order of memdevs. > cxl create-region -m -d decoder3.3 mem0 mem1 I fixed up the man page to clarify that this is a possibility when eliding the target list: diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt index e0e6818cfdd1..286779eff9ed 100644 --- a/Documentation/cxl/cxl-create-region.txt +++ b/Documentation/cxl/cxl-create-region.txt @@ -53,16 +53,18 @@ OPTIONS ------- <target(s)>:: The CXL targets that should be used to form the region. The number of -'target' arguments must match the '--ways' option (if provided). The -targets are memdev names such as 'mem0', 'mem1' etc. +'target' arguments must match the '--ways' option (if provided). include::bus-option.txt[] -m:: --memdevs:: Indicate that the non-option arguments for 'target(s)' refer to memdev - names. Currently this is the only option supported, and must be - specified. + device names. If this option is omitted and no targets are specified + then create-region uses the equivalent of 'cxl list -M -d $decoder' + internally as the target list. Note that depending on the topology, for + example with switches, the automatic target list ordering may not be + valid and manual specification of the target list is required. -s:: --size=:: > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > cxl/region.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/cxl/region.c b/cxl/region.c > > index aa0735194fa1..c0cf4ab350da 100644 > > --- a/cxl/region.c > > +++ b/cxl/region.c > > @@ -227,10 +227,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, > > } > > > > /* > > - * For all practical purposes, -m is the default target type, but > > - * hold off on actively making that decision until a second target > > - * option is available. > > + * For all practical purposes, -m is the default target type, but hold > > + * off on actively making that decision until a second target option is > > + * available. Unless there are no arguments then just assume memdevs. > > */ > > + if (!count) > > + param.memdevs = true; > > + > > if (!param.memdevs) { > > log_err(&rl, > > "must specify option for target object types (-m)\n"); > > @@ -272,11 +275,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, > > p->ways = count; > > if (!validate_ways(p, count)) > > return -EINVAL; > > - } else { > > - log_err(&rl, > > - "couldn't determine interleave ways from options or arguments\n"); > > - return -EINVAL; > > - } > > + } else > > + p->ways = p->num_memdevs; > > > > if (param.granularity < INT_MAX) { > > if (param.granularity <= 0) { > >
diff --git a/cxl/region.c b/cxl/region.c index aa0735194fa1..c0cf4ab350da 100644 --- a/cxl/region.c +++ b/cxl/region.c @@ -227,10 +227,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, } /* - * For all practical purposes, -m is the default target type, but - * hold off on actively making that decision until a second target - * option is available. + * For all practical purposes, -m is the default target type, but hold + * off on actively making that decision until a second target option is + * available. Unless there are no arguments then just assume memdevs. */ + if (!count) + param.memdevs = true; + if (!param.memdevs) { log_err(&rl, "must specify option for target object types (-m)\n"); @@ -272,11 +275,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count, p->ways = count; if (!validate_ways(p, count)) return -EINVAL; - } else { - log_err(&rl, - "couldn't determine interleave ways from options or arguments\n"); - return -EINVAL; - } + } else + p->ways = p->num_memdevs; if (param.granularity < INT_MAX) { if (param.granularity <= 0) {
Allow for: cxl create-region -d decoderX.Y ...to assume (-m -w $(count of memdevs beneath decoderX.Y)) Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- cxl/region.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)