On 22-05-04 22:17:49, Dan Williams wrote: > 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.
It was not intentional. It's "region%u" for both create and delete now.
