On Wed, May 4, 2022 at 3:57 PM Verma, Vishal L <[email protected]> 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.
