Re: [ndctl PATCH v2 05/10] libcxl: add low level APIs for region creation

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

On Wed, 2022-08-10 at 20:05 -0700, Dan Williams wrote:
> Vishal Verma wrote:
> > Add libcxl APIs to create a region under a given root decoder, and to
> > set different attributes for the new region. These allow setting the
> > size, interleave_ways, interleave_granularity, uuid, and the target
> > devices for the newly minted cxl_region object.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  Documentation/cxl/lib/libcxl.txt |  69 ++++++
> >  cxl/lib/private.h                |   2 +
> >  cxl/lib/libcxl.c                 | 377 ++++++++++++++++++++++++++++++-
> >  cxl/libcxl.h                     |  23 +-
> >  cxl/lib/libcxl.sym               |  16 ++
> >  5 files changed, 484 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> > index 7a38ce4..c3a8f36 100644
> > --- a/Documentation/cxl/lib/libcxl.txt
> > +++ b/Documentation/cxl/lib/libcxl.txt
> > @@ -508,6 +508,75 @@ device to represent the root of a PCI device hierarchy. The
> >  cxl_target_get_physical_node() helper returns the device name of that
> >  companion object in the PCI hierarchy.
> >  
> > +==== REGIONS
> > +A CXL region is composed of one or more slices of CXL memdevs, with configurable
> > +interleave settings - both the number of interleave ways, and the interleave
> > +granularity. In terms of hierarchy, it is the child of a CXL root decoder. A root
> > +decoder (recall that this corresponds to an ACPI CEDT.CFMWS 'window'), may have
> > +multiple chile regions, but a region is strictly tied to one root decoder.
> 
> Mmm, that's a spicy region.
> 
> s/chile/child/

Hah yep will fix.

> 
> > +
> > +A region also defines a set of mappings which are slices of capacity on a memdev,
> 
> Since the above already defined that a region is composed of one or more
> slices of CXL memdevs, how about:
> 
> "The slices that compose a region are called mappings. A mapping is a
> tuple of 'memdev', 'endpoint decoder', and the 'position'.

Yep sounds good.

[snip]

> 
> > +CXL_EXPORT struct cxl_region *
> > +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
> > +{
> > +       struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> > +       char *path = decoder->dev_buf;
> > +       char buf[SYSFS_ATTR_SIZE];
> > +       struct cxl_region *region;
> > +       int rc;
> > +
> > +       sprintf(path, "%s/create_pmem_region", decoder->dev_path);
> > +       rc = sysfs_read_attr(ctx, path, buf);
> > +       if (rc < 0) {
> > +               err(ctx, "failed to read new region name: %s\n",
> > +                   strerror(-rc));
> > +               return NULL;
> > +       }
> > +
> > +       rc = sysfs_write_attr(ctx, path, buf);
> > +       if (rc < 0) {
> > +               err(ctx, "failed to write new region name: %s\n",
> > +                   strerror(-rc));
> > +               return NULL;
> > +       }
> 
> I think there either needs to be a "decoder->regions_init = 0" here, or
> a direct call to "add_cxl_region(decoder...)" just in case this context
> had already listed regions before creating a new one.
> 
> I like the precision of "add_cxl_region()", but that needs to open code
> some of the internals of sysfs_device_parse(), so maybe
> "decoder->regions_init = 0" is ok for now.

Yes, I found that out - and added this - in patch 10 (I can instead
move it here - it makes sense).

Until patch 10, during region creation, nothing had done regions_init
until this point, so this happens to work. Patch 10 where we do walk
the regions before this point to calculate the max available space,
necessitates the reset here.

That being said, potentially all of patch 10 is squash-able into
different bits of the series - I left it at the end so the
max_available_extent stuff can be reviewed on its own.

I'm happy to go either way on squashing it or keeping it standalone.

> 
> > +
> > +       /* create_region was successful, walk to the new region */
> > +       cxl_region_foreach(decoder, region) {
> > +               const char *devname = cxl_region_get_devname(region);
> > +
> > +               if (strcmp(devname, buf) == 0)
> > +                       goto found;
> > +       }
> > +
> > +       /*
> > +        * If walking to the region we just created failed, something has gone
> > +        * very wrong. Attempt to delete it to avoid leaving a dangling region
> > +        * id behind.
> > +        */
> > +       err(ctx, "failed to add new region to libcxl\n");
> > +       cxl_region_delete_name(decoder, buf);
> > +       return NULL;
> > +
> > + found:
> > +       return region;
> > +}
> > +
> >  CXL_EXPORT int cxl_decoder_get_nr_targets(struct cxl_decoder *decoder)
> >  {
> >         return decoder->nr_targets;
> > @@ -1729,6 +2084,24 @@ CXL_EXPORT const char *cxl_decoder_get_devname(struct cxl_decoder *decoder)
> >         return devpath_to_devname(decoder->dev_path);
> >  }
> >  
> > +CXL_EXPORT struct cxl_memdev *
> > +cxl_ep_decoder_get_memdev(struct cxl_decoder *decoder)
> 
> Hmm, this is the only place where the API assumes the type of the
> decoder. The other root-only or endpoint-only decoder attribute getters
> are just cxl_decoder_get_*(), so I think drop the "_ep".

Agreed, will drop the ep_.

> 
> Other than the items listed above, this looks good.
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks Dan!