Re: [ndctl PATCH v5 02/16] cxl: add a cxl utility and libcxl library

From: Dan Williams <dan.j.williams@intel.com>
To: Vishal Verma <vishal.l.verma@intel.com>
Cc: linux-cxl@vger.kernel.org, Ben Widawsky <ben.widawsky@intel.com>,
	 Linux NVDIMM <nvdimm@lists.linux.dev>
Subject: Re: [ndctl PATCH v5 02/16] cxl: add a cxl utility and libcxl library
Date: Tue, 7 Dec 2021 21:12:04 -0800	[thread overview]
Message-ID: <CAPcyv4g=Zc=KizTvVVeyA4gFJi=K8Amuok-kjBM6UqkVZHwd2g@mail.gmail.com> (raw)
In-Reply-To: <20211111204436.1560365-3-vishal.l.verma@intel.com>

On Thu, Nov 11, 2021 at 12:45 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> CXL - or Compute eXpress Link - is a new interconnect that extends PCIe
> to support a wide range of devices, including cache coherent memory
> expanders. As such, these devices can be new sources of 'persistent
> memory', and the 'ndctl' umbrella of tools and libraries needs to be able
> to interact with them.
>
> Add a new utility and library for managing these CXL memory devices. This
> is an initial bring-up for interacting with CXL devices, and only includes
> adding the utility and library infrastructure, parsing device information
> from sysfs for CXL devices, and providing a 'cxl-list' command to
> display this information in JSON formatted output.
>
> Cc: Ben Widawsky <ben.widawsky@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
[..]
> +int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
> +{
> +       const struct option options[] = {
> +               OPT_STRING('d', "memdev", &param.memdev, "memory device name",
> +                          "filter by CXL memory device name"),
> +               OPT_BOOLEAN('D', "memdevs", &list.memdevs,
> +                           "include CXL memory device info"),
> +               OPT_BOOLEAN('i', "idle", &list.idle, "include idle devices"),
> +               OPT_BOOLEAN('u', "human", &list.human,
> +                               "use human friendly number formats "),
> +               OPT_END(),
> +       };
> +       const char * const u[] = {
> +               "cxl list [<options>]",
> +               NULL
> +       };
> +       struct json_object *jdevs = NULL;
> +       unsigned long list_flags;
> +       struct cxl_memdev *memdev;
> +       int i;
> +
> +       argc = parse_options(argc, argv, options, u, 0);
> +       for (i = 0; i < argc; i++)
> +               error("unknown parameter \"%s\"\n", argv[i]);
> +
> +       if (argc)
> +               usage_with_options(u, options);
> +
> +       if (num_list_flags() == 0)
> +               list.memdevs = true;

It strikes me that we may regret this default. In the interest of
giving us flexibility to pick a new default later (i.e. to not grow
any maintenance dependencies on this default), let's fail for now if
no list options are specified. I expect "cxl list" to enumerate the
active and most relevant end user devices by default. For ndctl its
namespaces, but for cxl-cli I expect it should be regions by default
to show all the CXL address ranges. Likely it should be "--regions
--namespaces" by default since regions are the primary user relevant
devices for volatile memory and namespaces are the end user relevant
devices for persistent memory, but the "--regions" vs "--regions
--namespaces" decision can be made later.