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?
