Verma, Vishal L wrote:
> On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote:
> > > The core of 'cxl list' knows, among other things, how to filter memdevs by
> > > their connectivity to a root decoder, enabled status, and how to identify
> > > memdevs by name, id, serial number. Use the fact that the json-c object
> > > array returned by cxl_filter_walk() also includes the corresponding libcxl
> > > objects to populate and validate the memdev target list for 'cxl
> > > create-region'.
> > >
> > > With this in place a default set of memdev targets can be derived from the
> > > specified root decoder, and the connectivity is validated by the same
> > > logic
> > > that prepares the hierarchical json topology. The argument list becomes
> > > as tolerant of different id formats as 'cxl list'. For example "mem9" and
> > > "9" are equivalent.
> > >
> > > Comma separated lists are also allowed, e.g. "mem9,mem10". However the
> > > sorting of memdevs by filter position falls back to the 'cxl list' order
> > > in
> > > that case. In other words:
> > >
> > > arg order region position
> > > mem9 mem10 => [0]: mem9 [1]: mem10
> > > mem10 mem9 => [0]: mem10 [1]: mem9
> > > mem9,mem10 => [0]: mem9 [1]: mem10
> > > mem10,mem9 => [0]: mem9 [1]: mem10
>
> Hm, this does create an awkward situation where
>
> cxl create-region -m mem10 mem9 (same as first arg order above)
>
> can behave differently from
>
> cxl create-region -m "mem10 mem9" (behaves like 4th arg order above)
>
> i.e. if the args happen to get quoted into a single space separated
> list, then it switches back to cxl-list ordering as "mem10 mem9" gets
> treated as a single filter string.
>
> I wonder if we should add another step after collect_memdevs() (or
> change memdev_sort()) to return the arg order by default, so:
>
> mem9 mem10 => [0]: mem9 [1]: mem10
> mem10 mem9 => [0]: mem10 [1]: mem9
> mem9,mem10 => [0]: mem9 [1]: mem10
> mem10,mem9 => [0]: mem10 [1]: mem9
> "mem9 mem10" => [0]: mem9 [1]: mem10
> "mem10 mem9" => [0]: mem10 [1]: mem9
>
> i.e. region positioning stays consistent no matter what combination of
> field separators, or quoting, or word splitting ends up happening.
>
> Then (future patches) add a --relax-ordering or --allow-reordering
> option that can fix up the order for creating a region successfully
> with the given set.
>
> With this, the only two options create-region has are either try the
> user-specified order exactly, or reorder to something that wil be
> successful. The cxl-list default order doesn't seem as a useful 'mode'
> to have as it can be hit-or-miss.
Yeah, that timebomb is probably not something I should leave ticking.
Fixed up now, but with the 'fun' of C string manipulation.
> > >
> > > Note that 'cxl list' order groups memdevs by port, later this will need to
> > > augmented with a sort implementation that orders memdevs by a topology
> > > compatible decode order.
> > >
> > > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> > > ---
> > > cxl/region.c | 274
> > > +++++++++++++++++++++++++++++++++++++---------------------
> > > 1 file changed, 175 insertions(+), 99 deletions(-)
> > >
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index c6d7d1a973a8..e47709754447 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -40,8 +40,10 @@ struct parsed_params {
> > > u64 ep_min_size;
> > > int ways;
> > > int granularity;
> > > - const char **targets;
> > > - int num_targets;
> > > + struct json_object *memdevs;
> > > + int num_memdevs;
> > > + int argc;
> > > + const char **argv;
> > > struct cxl_decoder *root_decoder;
> > > enum cxl_decoder_mode mode;
> > > };
> > > @@ -99,16 +101,148 @@ static const struct option destroy_options[] = {
> > > OPT_END(),
> > > };
> > >
> > > -static int parse_create_options(int argc, const char **argv,
> > > - struct parsed_params *p)
> > > +/*
> > > + * Convert an array of strings into a single comma-separated-value
> > > + * string that can be passed as a single 'filter' string to
> > > + * cxl_filter_walk()
> > > + */
> > > +static const char *to_csv(int count, const char **strings)
> > > {
> > > + ssize_t len = count + 1, cursor = 0;
> > > + char *csv;
> > > int i;
> > >
> > > + if (!count)
> > > + return NULL;
> > > +
> > > + for (i = 0; i < count; i++)
> > > + len += strlen(strings[i]);
> > > + csv = calloc(1, len);
> > > + if (!csv)
> > > + return NULL;
> > > + for (i = 0; i < count; i++) {
> > > + cursor += snprintf(csv + cursor, len - cursor, "%s%s",
> > > + strings[i], i + 1 < count ? "," : "");
> > > + if (cursor >= len) {
> > > + csv[len] = 0;
> > > + break;
> > > + }
> > > + }
> > > + return csv;
> > > +}
> > > +
> > > +static struct sort_context {
> > > + int count;
> > > + const char **sort;
> > > +} sort_context;
> > > +
> > > +static int memdev_filter_pos(struct json_object *jobj, int count, const
> > > char **sort)
> > > +{
> > > + struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > > + int pos;
> > > +
> > > + for (pos = 0; pos < count; pos++)
> > > + if (util_cxl_memdev_filter(memdev, sort[pos], NULL))
> > > + return pos;
> > > + return count;
> > > +}
> > > +
> > > +static int memdev_sort(const void *a, const void *b)
> > > +{
> > > + int a_pos, b_pos, count = sort_context.count;
> > > + const char **sort = sort_context.sort;
> > > + struct json_object **a_obj, **b_obj;
> > > +
> > > + a_obj = (struct json_object **) a;
> > > + b_obj = (struct json_object **) b;
> > > +
> > > + a_pos = memdev_filter_pos(*a_obj, count, sort);
> > > + b_pos = memdev_filter_pos(*b_obj, count, sort);
> > > +
> > > + return a_pos - b_pos;
> > > +}
> > > +
> > > +static struct json_object *collect_memdevs(struct cxl_ctx *ctx,
> > > + const char *decoder, int count,
> > > + const char **mems)
> > > +{
> > > + const char *csv = to_csv(count, mems);
> > > + struct cxl_filter_params filter_params = {
> > > + .decoder_filter = decoder,
> > > + .memdevs = true,
> > > + .memdev_filter = csv,
> > > + };
> > > + struct json_object *jmemdevs;
> > > +
> > > + jmemdevs = cxl_filter_walk(ctx, &filter_params);
> > > +
> > > + if (!jmemdevs) {
> > > + log_err(&rl, "failed to retrieve memdevs\n");
> > > + goto out;
> > > + }
> > > +
> > > + if (json_object_array_length(jmemdevs) == 0) {
> > > + log_err(&rl,
> > > + "no active memdevs found: decoder: %s filter:
> > > %s\n",
> > > + decoder, csv ? csv : "none");
> > > + json_object_put(jmemdevs);
> > > + jmemdevs = NULL;
> > > + goto out;
> > > + }
> > > +
> > > + sort_context = (struct sort_context){
> > > + .count = count,
> > > + .sort = mems,
> > > + };
> > > + json_object_array_sort(jmemdevs, memdev_sort);
> > > +
> > > +out:
> > > + free((void *)csv);
> > > + return jmemdevs;
> > > +}
> > > +
> > > +static bool validate_ways(struct parsed_params *p, int count)
> > > +{
> > > + /*
> > > + * Validate interleave ways against targets found in the
> > > topology. If
> > > + * the targets were specified, then non-default param.ways must
> > > equal
> > > + * that number of targets.
> > > + */
> > > + if (p->ways > p->num_memdevs || (count && p->ways !=
> > > p->num_memdevs)) {
>
> This falls over when doing something like
>
> cxl create-region -m mem0,mem1
>
> as @count ends up being '1' from the single filter spec passed in.
>
> I think doing a 'count = p->num_memdevs' (below) should fix it - we
> don't care about how many separate filter specs were passed in, once
> they have been combined, and the resulting memdevfs collected.
I think it matters because if someone typos something like "mem8, mem8,
mem10" we want the tool to validate with @count at 3 even though
num_memdevs would only return 2 in that case.
I am solving this by turning the @count argument into an in/out that
gets updated to be the number of individual elements of a target list.
Where @count is 4 for this target list on entry:
"mem1,mem2" mem3 mem4 "mem5 mem6"
...and 6 on return from collect_memdevs().
>
> > > + log_err(&rl,
> > > + "Interleave ways %d is %s than number of memdevs
> > > %s: %d\n",
> > > + p->ways, p->ways > p->num_memdevs ? "greater" :
> > > "less",
> > > + count ? "specified" : "found", p->num_memdevs);
> > > + return false;
> > > + }
> > > + return true;
> > > +}
> > > +
> > > +static int parse_create_options(struct cxl_ctx *ctx, int count,
> > > + const char **mems, struct parsed_params
> > > *p)
> > > +{
> > > if (!param.root_decoder) {
> > > log_err(&rl, "no root decoder specified\n");
> > > return -EINVAL;
> > > }
> > >
> > > + /*
> > > + * 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.
> > > + */
> > > + if (!param.memdevs) {
> > > + log_err(&rl,
> > > + "must specify option for target object types
> > > (-m)\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Collect the valid memdevs relative to the given root decoder */
> > > + p->memdevs = collect_memdevs(ctx, param.root_decoder, count,
> > > mems);
> > > + if (!p->memdevs)
> > > + return -ENXIO;
> > > + p->num_memdevs = json_object_array_length(p->memdevs);
>
> Per the above comment, either
>
> count = p->num_memdevs;
>
> here, or alternatively, the interleave ways validation shouldn't use
> @count anymore.
>
> > > +
> > > if (param.type) {
> > > p->mode = cxl_decoder_mode_from_ident(param.type);
> > > if (p->mode == CXL_DECODER_MODE_NONE) {
>
> <snip>
>
> > >
> > > @@ -664,8 +738,10 @@ static int region_action(int argc, const char
> > > **argv, struct cxl_ctx *ctx,
> > > if (rc)
> > > return rc;
> > >
> > > - if (action == ACTION_CREATE)
> > > - return create_region(ctx, count, p);
> > > + if (action == ACTION_CREATE) {
> > > + rc = create_region(ctx, count, p);
> > > + json_object_put(p->memdevs);
>
> Should this dereference happen just before returning from
> create_region() since that is where cxl_filter_walk() was called from,
> and is also where p->memdevs is last used.
Yeah, not sure why I had it outside.
