| Message ID | 170199191618.3543815.17768111410214136858.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support to report region access coordinates to numa nodes | expand |
Le 08/12/2023 à 00:31, Dave Jiang a écrit : > Add read/write latencies and bandwidth sysfs attributes for the enabled CXL > region. The bandwidth is the aggregated bandwidth of all devices that > contributes to the CXL region. The latency is the worst latency of the > device amongst all the devices that contributes to the CXL region. Hello Which initiator do these bandwidths/latencies refer to? Local CPUs near the root port? This should be specified in the doc. Brice > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ > drivers/cxl/core/region.c | 24 +++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index fff2581b8033..e96f172eb6a6 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -552,3 +552,43 @@ Description: > attribute is only visible for devices supporting the > capability. The retrieved errors are logged as kernel > events when cxl_poison event tracing is enabled. > + > + > +What: /sys/bus/cxl/devices/regionZ/read_bandwidth > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The aggregated read bandwidth of the region. The number is > + the accumulated read bandwidth of all CXL memory devices that > + contributes to the region. > + > + > +What: /sys/bus/cxl/devices/regionZ/write_bandwidth > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The aggregated write bandwidth of the region. The number is > + the accumulated write bandwidth of all CXL memory devices that > + contributes to the region. > + > + > +What: /sys/bus/cxl/devices/regionZ/read_latency > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The read latency of the region. The number is > + the worst read latency of all CXL memory devices that > + contributes to the region. > + > + > +What: /sys/bus/cxl/devices/regionZ/write_latency > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The write latency of the region. The number is > + the worst write latency of all CXL memory devices that > + contributes to the region. > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index d879f5702cf2..72c47f624d63 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, > } > static DEVICE_ATTR_RW(size); > > +#define ACCESS_ATTR(attrib) \ > +static ssize_t attrib##_show(struct device *dev, \ > + struct device_attribute *attr, \ > + char *buf) \ > +{ \ > + struct cxl_region *cxlr = to_cxl_region(dev); \ > + \ > + if (!cxlr->coord) \ > + return 0; \ > + \ > + return sysfs_emit(buf, "%u\n", \ > + cxlr->coord->attrib); \ > +} \ > +static DEVICE_ATTR_RO(attrib) > + > +ACCESS_ATTR(read_bandwidth); > +ACCESS_ATTR(read_latency); > +ACCESS_ATTR(write_bandwidth); > +ACCESS_ATTR(write_latency); > + > static struct attribute *cxl_region_attrs[] = { > &dev_attr_uuid.attr, > &dev_attr_commit.attr, > @@ -653,6 +673,10 @@ static struct attribute *cxl_region_attrs[] = { > &dev_attr_resource.attr, > &dev_attr_size.attr, > &dev_attr_mode.attr, > + &dev_attr_read_bandwidth.attr, > + &dev_attr_write_bandwidth.attr, > + &dev_attr_read_latency.attr, > + &dev_attr_write_latency.attr, > NULL, > }; > > > >
On Thu, Dec 07, 2023 at 04:31:56PM -0700, Dave Jiang wrote: > Add read/write latencies and bandwidth sysfs attributes for the enabled CXL > region. The bandwidth is the aggregated bandwidth of all devices that > contributes to the CXL region. The latency is the worst latency of the > device amongst all the devices that contributes to the CXL region. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ > drivers/cxl/core/region.c | 24 +++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index fff2581b8033..e96f172eb6a6 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -552,3 +552,43 @@ Description: > attribute is only visible for devices supporting the > capability. The retrieved errors are logged as kernel > events when cxl_poison event tracing is enabled. > + > + > +What: /sys/bus/cxl/devices/regionZ/read_bandwidth > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The aggregated read bandwidth of the region. The number is > + the accumulated read bandwidth of all CXL memory devices that > + contributes to the region. > + > + > +What: /sys/bus/cxl/devices/regionZ/write_bandwidth > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The aggregated write bandwidth of the region. The number is > + the accumulated write bandwidth of all CXL memory devices that > + contributes to the region. > + > + > +What: /sys/bus/cxl/devices/regionZ/read_latency > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The read latency of the region. The number is > + the worst read latency of all CXL memory devices that > + contributes to the region. > + > + > +What: /sys/bus/cxl/devices/regionZ/write_latency > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The write latency of the region. The number is > + the worst write latency of all CXL memory devices that > + contributes to the region. > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index d879f5702cf2..72c47f624d63 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, > } > static DEVICE_ATTR_RW(size); > > +#define ACCESS_ATTR(attrib) \ > +static ssize_t attrib##_show(struct device *dev, \ > + struct device_attribute *attr, \ > + char *buf) \ > +{ \ > + struct cxl_region *cxlr = to_cxl_region(dev); \ > + \ > + if (!cxlr->coord) \ > + return 0; \ > + \ > + return sysfs_emit(buf, "%u\n", \ > + cxlr->coord->attrib); \ > +} \ > +static DEVICE_ATTR_RO(attrib) > + > +ACCESS_ATTR(read_bandwidth); > +ACCESS_ATTR(read_latency); > +ACCESS_ATTR(write_bandwidth); > +ACCESS_ATTR(write_latency); > + > static struct attribute *cxl_region_attrs[] = { > &dev_attr_uuid.attr, > &dev_attr_commit.attr, > @@ -653,6 +673,10 @@ static struct attribute *cxl_region_attrs[] = { > &dev_attr_resource.attr, > &dev_attr_size.attr, > &dev_attr_mode.attr, > + &dev_attr_read_bandwidth.attr, > + &dev_attr_write_bandwidth.attr, > + &dev_attr_read_latency.attr, > + &dev_attr_write_latency.attr, > NULL, > }; > This way latency and bandwidth ABI are defined seems not to be consistent with what we have now. For example, for other attributes like interleave_ways, we only define one attribute "interleave_ways", and use two separate functions interleave_ways_show and interleave_ways_store for read/write operation. for ABI interface, only one is provided "/sys/bus/cxl/devices/regionZ/interleave_granularity" for latency and bandwidth, should we provide two interfaces like below instead? /sys/bus/cxl/devices/regionZ/bandwidth --rw /sys/bus/cxl/devices/regionZ/latency --rw Fan > >
On 12/11/23 11:03, fan wrote: > On Thu, Dec 07, 2023 at 04:31:56PM -0700, Dave Jiang wrote: >> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL >> region. The bandwidth is the aggregated bandwidth of all devices that >> contributes to the CXL region. The latency is the worst latency of the >> device amongst all the devices that contributes to the CXL region. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ >> drivers/cxl/core/region.c | 24 +++++++++++++++++++ >> 2 files changed, 64 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl >> index fff2581b8033..e96f172eb6a6 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-cxl >> +++ b/Documentation/ABI/testing/sysfs-bus-cxl >> @@ -552,3 +552,43 @@ Description: >> attribute is only visible for devices supporting the >> capability. The retrieved errors are logged as kernel >> events when cxl_poison event tracing is enabled. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/read_bandwidth >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The aggregated read bandwidth of the region. The number is >> + the accumulated read bandwidth of all CXL memory devices that >> + contributes to the region. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/write_bandwidth >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The aggregated write bandwidth of the region. The number is >> + the accumulated write bandwidth of all CXL memory devices that >> + contributes to the region. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/read_latency >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The read latency of the region. The number is >> + the worst read latency of all CXL memory devices that >> + contributes to the region. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/write_latency >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The write latency of the region. The number is >> + the worst write latency of all CXL memory devices that >> + contributes to the region. >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index d879f5702cf2..72c47f624d63 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, >> } >> static DEVICE_ATTR_RW(size); >> >> +#define ACCESS_ATTR(attrib) \ >> +static ssize_t attrib##_show(struct device *dev, \ >> + struct device_attribute *attr, \ >> + char *buf) \ >> +{ \ >> + struct cxl_region *cxlr = to_cxl_region(dev); \ >> + \ >> + if (!cxlr->coord) \ >> + return 0; \ >> + \ >> + return sysfs_emit(buf, "%u\n", \ >> + cxlr->coord->attrib); \ >> +} \ >> +static DEVICE_ATTR_RO(attrib) >> + >> +ACCESS_ATTR(read_bandwidth); >> +ACCESS_ATTR(read_latency); >> +ACCESS_ATTR(write_bandwidth); >> +ACCESS_ATTR(write_latency); >> + >> static struct attribute *cxl_region_attrs[] = { >> &dev_attr_uuid.attr, >> &dev_attr_commit.attr, >> @@ -653,6 +673,10 @@ static struct attribute *cxl_region_attrs[] = { >> &dev_attr_resource.attr, >> &dev_attr_size.attr, >> &dev_attr_mode.attr, >> + &dev_attr_read_bandwidth.attr, >> + &dev_attr_write_bandwidth.attr, >> + &dev_attr_read_latency.attr, >> + &dev_attr_write_latency.attr, >> NULL, >> }; >> > This way latency and bandwidth ABI are defined seems not to be consistent > with what we have now. > > For example, for other attributes like interleave_ways, we only define > one attribute "interleave_ways", and use two separate functions > interleave_ways_show and interleave_ways_store for read/write operation. > for ABI interface, only one is provided > "/sys/bus/cxl/devices/regionZ/interleave_granularity" > > for latency and bandwidth, should we provide two interfaces like below > instead? > /sys/bus/cxl/devices/regionZ/bandwidth --rw > /sys/bus/cxl/devices/regionZ/latency --rw Hi Fan. I think there's a misunderstanding here. This is not reading and writing of bandwidth and latency. This is read latency and write latency and read bandwidth and write bandwidth. They are separate and unique properties of the path. i.e. upstream and downstream direction traffic performances. > > Fan >> >>
On Mon, Dec 11, 2023 at 11:13:42AM -0700, Dave Jiang wrote: > > > On 12/11/23 11:03, fan wrote: > > On Thu, Dec 07, 2023 at 04:31:56PM -0700, Dave Jiang wrote: > >> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL > >> region. The bandwidth is the aggregated bandwidth of all devices that > >> contributes to the CXL region. The latency is the worst latency of the s/contributes/contribute/ > >> device amongst all the devices that contributes to the CXL region. s/contributes/contribute/ > >> device amongst all the devices that contributes to the CXL region. > >> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >> --- > >> Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ > >> drivers/cxl/core/region.c | 24 +++++++++++++++++++ > >> 2 files changed, 64 insertions(+) > >> > >> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > >> index fff2581b8033..e96f172eb6a6 100644 > >> --- a/Documentation/ABI/testing/sysfs-bus-cxl > >> +++ b/Documentation/ABI/testing/sysfs-bus-cxl > >> @@ -552,3 +552,43 @@ Description: > >> attribute is only visible for devices supporting the > >> capability. The retrieved errors are logged as kernel > >> events when cxl_poison event tracing is enabled. > >> + > >> + > >> +What: /sys/bus/cxl/devices/regionZ/read_bandwidth > >> +Date: Apr, 2023 > >> +KernelVersion: v6.8 > >> +Contact: linux-cxl@vger.kernel.org > >> +Description: > >> + (RO) The aggregated read bandwidth of the region. The number is > >> + the accumulated read bandwidth of all CXL memory devices that > >> + contributes to the region. > >> + > >> + > >> +What: /sys/bus/cxl/devices/regionZ/write_bandwidth > >> +Date: Apr, 2023 > >> +KernelVersion: v6.8 > >> +Contact: linux-cxl@vger.kernel.org > >> +Description: > >> + (RO) The aggregated write bandwidth of the region. The number is > >> + the accumulated write bandwidth of all CXL memory devices that > >> + contributes to the region. > >> + > >> + > >> +What: /sys/bus/cxl/devices/regionZ/read_latency > >> +Date: Apr, 2023 > >> +KernelVersion: v6.8 > >> +Contact: linux-cxl@vger.kernel.org > >> +Description: > >> + (RO) The read latency of the region. The number is > >> + the worst read latency of all CXL memory devices that > >> + contributes to the region. > >> + > >> + > >> +What: /sys/bus/cxl/devices/regionZ/write_latency > >> +Date: Apr, 2023 > >> +KernelVersion: v6.8 > >> +Contact: linux-cxl@vger.kernel.org > >> +Description: > >> + (RO) The write latency of the region. The number is > >> + the worst write latency of all CXL memory devices that > >> + contributes to the region. > >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > >> index d879f5702cf2..72c47f624d63 100644 > >> --- a/drivers/cxl/core/region.c > >> +++ b/drivers/cxl/core/region.c > >> @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, > >> } > >> static DEVICE_ATTR_RW(size); > >> > >> +#define ACCESS_ATTR(attrib) \ > >> +static ssize_t attrib##_show(struct device *dev, \ > >> + struct device_attribute *attr, \ > >> + char *buf) \ > >> +{ \ > >> + struct cxl_region *cxlr = to_cxl_region(dev); \ > >> + \ > >> + if (!cxlr->coord) \ > >> + return 0; \ > >> + \ > >> + return sysfs_emit(buf, "%u\n", \ > >> + cxlr->coord->attrib); \ > >> +} \ > >> +static DEVICE_ATTR_RO(attrib) > >> + > >> +ACCESS_ATTR(read_bandwidth); > >> +ACCESS_ATTR(read_latency); > >> +ACCESS_ATTR(write_bandwidth); > >> +ACCESS_ATTR(write_latency); > >> + > >> static struct attribute *cxl_region_attrs[] = { > >> &dev_attr_uuid.attr, > >> &dev_attr_commit.attr, > >> @@ -653,6 +673,10 @@ static struct attribute *cxl_region_attrs[] = { > >> &dev_attr_resource.attr, > >> &dev_attr_size.attr, > >> &dev_attr_mode.attr, > >> + &dev_attr_read_bandwidth.attr, > >> + &dev_attr_write_bandwidth.attr, > >> + &dev_attr_read_latency.attr, > >> + &dev_attr_write_latency.attr, > >> NULL, > >> }; > >> > > This way latency and bandwidth ABI are defined seems not to be consistent > > with what we have now. > > > > For example, for other attributes like interleave_ways, we only define > > one attribute "interleave_ways", and use two separate functions > > interleave_ways_show and interleave_ways_store for read/write operation. > > for ABI interface, only one is provided > > "/sys/bus/cxl/devices/regionZ/interleave_granularity" > > > > for latency and bandwidth, should we provide two interfaces like below > > instead? > > /sys/bus/cxl/devices/regionZ/bandwidth --rw > > /sys/bus/cxl/devices/regionZ/latency --rw > > Hi Fan. I think there's a misunderstanding here. This is not reading and writing of bandwidth and latency. This is read latency and write latency and read bandwidth and write bandwidth. They are separate and unique properties of the path. i.e. upstream and downstream direction traffic performances. > Oh. I totally misunderstood, thanks for clarification. Then the patch looks good to me expect for a minor typo in the commit comments as shown above. Fan > > > > > Fan > >> > >>
Dave Jiang wrote: > Add read/write latencies and bandwidth sysfs attributes for the enabled CXL > region. The bandwidth is the aggregated bandwidth of all devices that > contributes to the CXL region. The latency is the worst latency of the > device amongst all the devices that contributes to the CXL region. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ > drivers/cxl/core/region.c | 24 +++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index fff2581b8033..e96f172eb6a6 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -552,3 +552,43 @@ Description: > attribute is only visible for devices supporting the > capability. The retrieved errors are logged as kernel > events when cxl_poison event tracing is enabled. > + > + > +What: /sys/bus/cxl/devices/regionZ/read_bandwidth > +Date: Apr, 2023 > +KernelVersion: v6.8 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The aggregated read bandwidth of the region. The number is > + the accumulated read bandwidth of all CXL memory devices that > + contributes to the region. Units? ...ditto for the rest. Ideally these descriptions match the units of the same named fields in Documentation/ABI/stable/sysfs-devices-node for the HMEM_REPORTING attributes. You might even link that.
Le 08/12/2023 à 00:31, Dave Jiang a écrit : > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index d879f5702cf2..72c47f624d63 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, > } > static DEVICE_ATTR_RW(size); > > +#define ACCESS_ATTR(attrib) \ > +static ssize_t attrib##_show(struct device *dev, \ > + struct device_attribute *attr, \ > + char *buf) \ > +{ \ > + struct cxl_region *cxlr = to_cxl_region(dev); \ > + \ > + if (!cxlr->coord) \ > + return 0; \ > + \ > + return sysfs_emit(buf, "%u\n", \ > + cxlr->coord->attrib); \ > +} \ Hello Latencies ares off by a factor of 1000 here (I see 586/686 for r/w attributes for NUMA nodes in Qemu but 1000x higher for region attributes). For NUMA node attributes, you're dividing latencies by a factor a 1000 in cxl_region_perf_attrs_callback(): /* Adjust latencies from psec to nsec to be consistent with HMAT targets */ coord = *cxlr->coord; coord.read_latency = DIV_ROUND_UP(coord.read_latency, 1000); coord.write_latency = DIV_ROUND_UP(coord.write_latency, 1000); For region attributes, I think you're missing the same? Brice
On 12/12/23 06:46, Brice Goglin wrote: > Le 08/12/2023 à 00:31, Dave Jiang a écrit : > >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index d879f5702cf2..72c47f624d63 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, >> } >> static DEVICE_ATTR_RW(size); >> +#define ACCESS_ATTR(attrib) \ >> +static ssize_t attrib##_show(struct device *dev, \ >> + struct device_attribute *attr, \ >> + char *buf) \ >> +{ \ >> + struct cxl_region *cxlr = to_cxl_region(dev); \ >> + \ >> + if (!cxlr->coord) \ >> + return 0; \ >> + \ >> + return sysfs_emit(buf, "%u\n", \ >> + cxlr->coord->attrib); \ >> +} \ > > Hello > > Latencies ares off by a factor of 1000 here (I see 586/686 for r/w attributes > for NUMA nodes in Qemu but 1000x higher for region attributes). For NUMA node attributes, > you're dividing latencies by a factor a 1000 in cxl_region_perf_attrs_callback(): > > /* Adjust latencies from psec to nsec to be consistent with HMAT targets */ > coord = *cxlr->coord; > coord.read_latency = DIV_ROUND_UP(coord.read_latency, 1000); > coord.write_latency = DIV_ROUND_UP(coord.write_latency, 1000); > > For region attributes, I think you're missing the same? Yes.... I was keeping the original computed raw data in picosecond base. Maybe I should just convert it to nanoseconds here since that's what Linux uses to begin with just to be consistent everywhere. > > Brice >
On 12/11/23 02:06, Brice Goglin wrote: > Le 08/12/2023 à 00:31, Dave Jiang a écrit : >> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL >> region. The bandwidth is the aggregated bandwidth of all devices that >> contributes to the CXL region. The latency is the worst latency of the >> device amongst all the devices that contributes to the CXL region. > > > Hello > > Which initiator do these bandwidths/latencies refer to? Local CPUs near the root port? This should be specified in the doc. Currently I'm only storing the numbers from initiator pxm 0 to these targets in the hmat target list. I'm open to suggestions as to if a different way would be better. > > Brice > > > >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ >> drivers/cxl/core/region.c | 24 +++++++++++++++++++ >> 2 files changed, 64 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl >> index fff2581b8033..e96f172eb6a6 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-cxl >> +++ b/Documentation/ABI/testing/sysfs-bus-cxl >> @@ -552,3 +552,43 @@ Description: >> attribute is only visible for devices supporting the >> capability. The retrieved errors are logged as kernel >> events when cxl_poison event tracing is enabled. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/read_bandwidth >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The aggregated read bandwidth of the region. The number is >> + the accumulated read bandwidth of all CXL memory devices that >> + contributes to the region. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/write_bandwidth >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The aggregated write bandwidth of the region. The number is >> + the accumulated write bandwidth of all CXL memory devices that >> + contributes to the region. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/read_latency >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The read latency of the region. The number is >> + the worst read latency of all CXL memory devices that >> + contributes to the region. >> + >> + >> +What: /sys/bus/cxl/devices/regionZ/write_latency >> +Date: Apr, 2023 >> +KernelVersion: v6.8 >> +Contact: linux-cxl@vger.kernel.org >> +Description: >> + (RO) The write latency of the region. The number is >> + the worst write latency of all CXL memory devices that >> + contributes to the region. >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index d879f5702cf2..72c47f624d63 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, >> } >> static DEVICE_ATTR_RW(size); >> +#define ACCESS_ATTR(attrib) \ >> +static ssize_t attrib##_show(struct device *dev, \ >> + struct device_attribute *attr, \ >> + char *buf) \ >> +{ \ >> + struct cxl_region *cxlr = to_cxl_region(dev); \ >> + \ >> + if (!cxlr->coord) \ >> + return 0; \ >> + \ >> + return sysfs_emit(buf, "%u\n", \ >> + cxlr->coord->attrib); \ >> +} \ >> +static DEVICE_ATTR_RO(attrib) >> + >> +ACCESS_ATTR(read_bandwidth); >> +ACCESS_ATTR(read_latency); >> +ACCESS_ATTR(write_bandwidth); >> +ACCESS_ATTR(write_latency); >> + >> static struct attribute *cxl_region_attrs[] = { >> &dev_attr_uuid.attr, >> &dev_attr_commit.attr, >> @@ -653,6 +673,10 @@ static struct attribute *cxl_region_attrs[] = { >> &dev_attr_resource.attr, >> &dev_attr_size.attr, >> &dev_attr_mode.attr, >> + &dev_attr_read_bandwidth.attr, >> + &dev_attr_write_bandwidth.attr, >> + &dev_attr_read_latency.attr, >> + &dev_attr_write_latency.attr, >> NULL, >> }; >> >> >>
On Tue, 12 Dec 2023 12:30:47 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > On 12/11/23 02:06, Brice Goglin wrote: > > Le 08/12/2023 à 00:31, Dave Jiang a écrit : > >> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL > >> region. The bandwidth is the aggregated bandwidth of all devices that > >> contributes to the CXL region. The latency is the worst latency of the > >> device amongst all the devices that contributes to the CXL region. > > > > > > Hello > > > > Which initiator do these bandwidths/latencies refer to? Local CPUs near the root port? This should be specified in the doc. > > Currently I'm only storing the numbers from initiator pxm 0 to these targets in the hmat target list. I'm open to suggestions as to if a different way would be better. Nearest node with CPUs would be better I think. Jonathan
On 12/19/23 09:44, Jonathan Cameron wrote: > On Tue, 12 Dec 2023 12:30:47 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> On 12/11/23 02:06, Brice Goglin wrote: >>> Le 08/12/2023 à 00:31, Dave Jiang a écrit : >>>> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL >>>> region. The bandwidth is the aggregated bandwidth of all devices that >>>> contributes to the CXL region. The latency is the worst latency of the >>>> device amongst all the devices that contributes to the CXL region. >>> >>> >>> Hello >>> >>> Which initiator do these bandwidths/latencies refer to? Local CPUs near the root port? This should be specified in the doc. >> >> Currently I'm only storing the numbers from initiator pxm 0 to these targets in the hmat target list. I'm open to suggestions as to if a different way would be better. > Nearest node with CPUs would be better I think. Is it possible to discover that? The SRAT MPDAS structure indicates the association of initiator and memory target domains. But it does not seem to make such associations for generic targets. > > Jonathan >
On 12/20/23 13:26, Dave Jiang wrote: > > > On 12/19/23 09:44, Jonathan Cameron wrote: >> On Tue, 12 Dec 2023 12:30:47 -0700 >> Dave Jiang <dave.jiang@intel.com> wrote: >> >>> On 12/11/23 02:06, Brice Goglin wrote: >>>> Le 08/12/2023 à 00:31, Dave Jiang a écrit : >>>>> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL >>>>> region. The bandwidth is the aggregated bandwidth of all devices that >>>>> contributes to the CXL region. The latency is the worst latency of the >>>>> device amongst all the devices that contributes to the CXL region. >>>> >>>> >>>> Hello >>>> >>>> Which initiator do these bandwidths/latencies refer to? Local CPUs near the root port? This should be specified in the doc. >>> >>> Currently I'm only storing the numbers from initiator pxm 0 to these targets in the hmat target list. I'm open to suggestions as to if a different way would be better. >> Nearest node with CPUs would be better I think. > > Is it possible to discover that? The SRAT MPDAS structure indicates the association of initiator and memory target domains. But it does not seem to make such associations for generic targets. Answering my own question, looks like we need to go through hmat_update_target_attrs() and retrieve the best perf numbers via that function. Will make some mods to do that and post v15. This seems to be the way linux HMAT handling code is doing for normal initiator/target setup. > >> >> Jonathan >> >
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl index fff2581b8033..e96f172eb6a6 100644 --- a/Documentation/ABI/testing/sysfs-bus-cxl +++ b/Documentation/ABI/testing/sysfs-bus-cxl @@ -552,3 +552,43 @@ Description: attribute is only visible for devices supporting the capability. The retrieved errors are logged as kernel events when cxl_poison event tracing is enabled. + + +What: /sys/bus/cxl/devices/regionZ/read_bandwidth +Date: Apr, 2023 +KernelVersion: v6.8 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) The aggregated read bandwidth of the region. The number is + the accumulated read bandwidth of all CXL memory devices that + contributes to the region. + + +What: /sys/bus/cxl/devices/regionZ/write_bandwidth +Date: Apr, 2023 +KernelVersion: v6.8 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) The aggregated write bandwidth of the region. The number is + the accumulated write bandwidth of all CXL memory devices that + contributes to the region. + + +What: /sys/bus/cxl/devices/regionZ/read_latency +Date: Apr, 2023 +KernelVersion: v6.8 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) The read latency of the region. The number is + the worst read latency of all CXL memory devices that + contributes to the region. + + +What: /sys/bus/cxl/devices/regionZ/write_latency +Date: Apr, 2023 +KernelVersion: v6.8 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) The write latency of the region. The number is + the worst write latency of all CXL memory devices that + contributes to the region. diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index d879f5702cf2..72c47f624d63 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RW(size); +#define ACCESS_ATTR(attrib) \ +static ssize_t attrib##_show(struct device *dev, \ + struct device_attribute *attr, \ + char *buf) \ +{ \ + struct cxl_region *cxlr = to_cxl_region(dev); \ + \ + if (!cxlr->coord) \ + return 0; \ + \ + return sysfs_emit(buf, "%u\n", \ + cxlr->coord->attrib); \ +} \ +static DEVICE_ATTR_RO(attrib) + +ACCESS_ATTR(read_bandwidth); +ACCESS_ATTR(read_latency); +ACCESS_ATTR(write_bandwidth); +ACCESS_ATTR(write_latency); + static struct attribute *cxl_region_attrs[] = { &dev_attr_uuid.attr, &dev_attr_commit.attr, @@ -653,6 +673,10 @@ static struct attribute *cxl_region_attrs[] = { &dev_attr_resource.attr, &dev_attr_size.attr, &dev_attr_mode.attr, + &dev_attr_read_bandwidth.attr, + &dev_attr_write_bandwidth.attr, + &dev_attr_read_latency.attr, + &dev_attr_write_latency.attr, NULL, };
Add read/write latencies and bandwidth sysfs attributes for the enabled CXL region. The bandwidth is the aggregated bandwidth of all devices that contributes to the CXL region. The latency is the worst latency of the device amongst all the devices that contributes to the CXL region. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- Documentation/ABI/testing/sysfs-bus-cxl | 40 +++++++++++++++++++++++++++++++ drivers/cxl/core/region.c | 24 +++++++++++++++++++ 2 files changed, 64 insertions(+)