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

[Dan Williams <dan.j.williams@intel.com>]

Verma, Vishal L wrote:
> 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.

If this was the kernel I would say squash, since bisecting might be
impacted, but as long as the fix is there later in the series I think
that's ok.

Note, I was less worried about the cxl-cli tool tripping over this, and
more about 3rd party applications using libcxl, but those are even less
likely to use a mid-release commit.