Re: [RFC PATCH 12/15] cxl/region: Add region creation ABI

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

On Wed, May 4, 2022 at 3:57 PM Verma, Vishal L <vishal.l.verma@intel.com> wrote:
>
> On Wed, 2022-04-13 at 11:37 -0700, Ben Widawsky wrote:
> > Regions are created as a child of the decoder that encompasses an
> > address space with constraints. Regions have a number of attributes that
> > must be configured before the region can be activated.
> >
> > Multiple processes which are trying not to race with each other
> > shouldn't need special userspace synchronization to do so.
> >
> > // Allocate a new region name
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_pmem_region)
> >
> > // Create a new region by name
> > while
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_pmem_region)
> > ! echo $region > /sys/bus/cxl/devices/decoder0.0/create_pmem_region
> > do true; done
> >
> > // Region now exists in sysfs
> > stat -t /sys/bus/cxl/devices/decoder0.0/$region
> >
> > // Delete the region, and name
> > echo $region > /sys/bus/cxl/devices/decoder0.0/delete_region
>
> I noticed a slight ABI inconsistency here while working on the libcxl
> side of this - see below.
>
> > +
> > +static ssize_t create_pmem_region_show(struct device *dev,
> > +                                      struct device_attribute *attr, char *buf)
> > +{
> > +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > +       struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxld);
> > +       size_t rc;
> > +
> > +       /*
> > +        * There's no point in returning known bad answers when the lock is held
> > +        * on the store side, even though the answer given here may be
> > +        * immediately invalidated as soon as the lock is dropped it's still
> > +        * useful to throttle readers in the presence of writers.
> > +        */
> > +       rc = mutex_lock_interruptible(&cxlrd->id_lock);
> > +       if (rc)
> > +               return rc;
> > +       rc = sysfs_emit(buf, "%d\n", cxlrd->next_region_id);
>
> This emits a numeric region ID, e.g. "0", where as
>
> > +       mutex_unlock(&cxlrd->id_lock);
> > +
> > +       return rc;
> > +}
> > +
>
> <snip>
>
> > +static ssize_t delete_region_store(struct device *dev,
> > +                                  struct device_attribute *attr,
> > +                                  const char *buf, size_t len)
> > +{
> > +       struct cxl_port *port = to_cxl_port(dev->parent);
> > +       struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > +       struct cxl_region *cxlr;
> > +
> > +       cxlr = cxl_find_region_by_name(cxld, buf);
>
> This expects a full region name string e.g. "region0"
>
> Was this intentional? I don't think it's a huge deal, the library can
> certainly deal with it if needed - but would it be better to have the
> ABI symmetrical between create and delete?

Yes, makes sense to me.