Re: [ndctl PATCH 11/15] cxl/region: Use cxl_filter_walk() to gather create-region targets

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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.
> > 
> > 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.williams@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.

> > +               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.

> > +       }
> >  
> >         cxl_bus_foreach(ctx, bus) {
> >                 struct cxl_decoder *decoder;
> >