| Message ID | 169575398814.4028282.11591056324662123995.stgit@djiang5-mobl3 |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] cxl: Add committed sysfs attribute to CXL decoder | expand |
On Tue, 26 Sep 2023, Dave Jiang wrote: >This attribute allows cxl-cli to determine whether a decoder is >actively participating in a region. This is only a snapshot of the >state, and doesn't offer any protection or serialization against a >concurrent disable-region operation. > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> >Suggested-by: Dan Williams <dan.j.williams@intel.com> >Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> >Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >--- >v3: >- Update commit log verbiage. (Vishal) >- Change code to conform with existing code. (Dan) >v2: >- Use FIELD_GET() (Jonathan) >--- > Documentation/ABI/testing/sysfs-bus-cxl | 7 +++++++ > drivers/cxl/core/port.c | 12 ++++++++++++ > 2 files changed, 19 insertions(+) > >diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl >index 087f762ebfd5..ef3fc9fe9d0d 100644 >--- a/Documentation/ABI/testing/sysfs-bus-cxl >+++ b/Documentation/ABI/testing/sysfs-bus-cxl >@@ -369,6 +369,13 @@ Description: > provided it is currently idle / not bound to a driver. > > >+What: /sys/bus/cxl/devices/decoderX.Y/committed >+Date: Sep, 2023 >+KernelVersion: v6.7 >+Contact: linux-cxl@vger.kernel.org >+Description: >+ (RO) Indicates whether the decoder is committed. >+ > What: /sys/bus/cxl/devices/regionZ/uuid > Date: May, 2022 > KernelVersion: v6.0 >diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >index 724be8448eb4..20919ffbc1a4 100644 >--- a/drivers/cxl/core/port.c >+++ b/drivers/cxl/core/port.c >@@ -277,12 +277,24 @@ static ssize_t interleave_ways_show(struct device *dev, > > static DEVICE_ATTR_RO(interleave_ways); > >+static ssize_t committed_show(struct device *dev, >+ struct device_attribute *attr, char *buf) >+{ >+ struct cxl_decoder *cxld = to_cxl_decoder(dev); >+ bool enabled = cxld->flags & CXL_DECODER_F_ENABLE; >+ >+ return sysfs_emit(buf, "%d\n", enabled); >+} >+ >+static DEVICE_ATTR_RO(committed); >+ > static struct attribute *cxl_decoder_base_attrs[] = { > &dev_attr_start.attr, > &dev_attr_size.attr, > &dev_attr_locked.attr, > &dev_attr_interleave_granularity.attr, > &dev_attr_interleave_ways.attr, >+ &dev_attr_committed.attr, > NULL, > }; > > >
Dave Jiang wrote: > This attribute allows cxl-cli to determine whether a decoder is > actively participating in a region. This is only a snapshot of the > state, and doesn't offer any protection or serialization against a > concurrent disable-region operation. A random thought occurred while realizing that the kernel uses a check of: port->commit_end != -1 ...to determine that a given port (switch or endpoint) has committed decoders. If the goal here is to determine when it is safe to disable an entire memdev maybe it is better to check all of the decoders at once at the port level rather than one at a time. It is already the case that CXL prevents decoders from being committed out of order so what do you think of replacing decoderX.Y/committed with portX/decoders_committed, where it just does: down_read(&cxl_region_rwsem); sysfs_emit("%d\n", port->commit_end + 1); up_read(&cxl_region_rwsem); ...and cxl-cli aborts destructive processes on that attribute being non-zero. Is there any use case for userspace to check for individual decoders being committed? It can infer "decoder committed" from "decoder_id < decoders_commited".
On 10/3/23 15:40, Dan Williams wrote: > Dave Jiang wrote: >> This attribute allows cxl-cli to determine whether a decoder is >> actively participating in a region. This is only a snapshot of the >> state, and doesn't offer any protection or serialization against a >> concurrent disable-region operation. > > A random thought occurred while realizing that the kernel uses a check > of: > > port->commit_end != -1 > > ...to determine that a given port (switch or endpoint) has committed > decoders. If the goal here is to determine when it is safe to disable an > entire memdev maybe it is better to check all of the decoders at once at > the port level rather than one at a time. It is already the case that > CXL prevents decoders from being committed out of order so what do you > think of replacing decoderX.Y/committed with portX/decoders_committed, > where it just does: > > down_read(&cxl_region_rwsem); > sysfs_emit("%d\n", port->commit_end + 1); > up_read(&cxl_region_rwsem); > > ...and cxl-cli aborts destructive processes on that attribute being > non-zero. Seems reasonable. I'll rework the patch and the cxl cli code. > > Is there any use case for userspace to check for individual decoders > being committed? It can infer "decoder committed" from "decoder_id < > decoders_commited".
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl index 087f762ebfd5..ef3fc9fe9d0d 100644 --- a/Documentation/ABI/testing/sysfs-bus-cxl +++ b/Documentation/ABI/testing/sysfs-bus-cxl @@ -369,6 +369,13 @@ Description: provided it is currently idle / not bound to a driver. +What: /sys/bus/cxl/devices/decoderX.Y/committed +Date: Sep, 2023 +KernelVersion: v6.7 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) Indicates whether the decoder is committed. + What: /sys/bus/cxl/devices/regionZ/uuid Date: May, 2022 KernelVersion: v6.0 diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 724be8448eb4..20919ffbc1a4 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -277,12 +277,24 @@ static ssize_t interleave_ways_show(struct device *dev, static DEVICE_ATTR_RO(interleave_ways); +static ssize_t committed_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct cxl_decoder *cxld = to_cxl_decoder(dev); + bool enabled = cxld->flags & CXL_DECODER_F_ENABLE; + + return sysfs_emit(buf, "%d\n", enabled); +} + +static DEVICE_ATTR_RO(committed); + static struct attribute *cxl_decoder_base_attrs[] = { &dev_attr_start.attr, &dev_attr_size.attr, &dev_attr_locked.attr, &dev_attr_interleave_granularity.attr, &dev_attr_interleave_ways.attr, + &dev_attr_committed.attr, NULL, };