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 <[email protected]> > > --- > > 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; > >
