Dave Jiang wrote:
> Provide a callback to parse the Switched Scoped Latency and Bandwidth
> Information Structure (DSLBIS) in the CDAT structures. The SSLBIS
> contains the bandwidth and latency information that's tied to the
> CLX switch that the data table has been read from. The extracted
s/CLX/CXL/
> values are indexed by the downstream port id.
For other readers of this patch it might be worth mentioning that this
corresponds to 'struct cxl_dport::portid'.
> It is possible the downstream port id is 0xffff which is a wildcard
> value for any port id.
>
> Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency
> and Bandwidth Information Structure (DSLBIS)
>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>
> ---
> v3:
> - Add spec section in commit header (Alison)
> - Move CDAT parse to cxl_switch_port_probe()
> - Use 'struct node_hmem_attrs'
> ---
> drivers/cxl/core/cdat.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/core/port.c | 5 +++
> drivers/cxl/cxl.h | 1 +
> drivers/cxl/cxlpci.h | 20 ++++++++++++
> drivers/cxl/port.c | 14 ++++++++-
> 5 files changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index e8b9bb99dfdf..ec3420dddf27 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -192,3 +192,79 @@ int cxl_dslbis_parse_entry(struct cdat_entry_header *header, void *arg)
> return 0;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_dslbis_parse_entry, CXL);
> +
> +int cxl_sslbis_parse_entry(struct cdat_entry_header *header, void *arg)
> +{
> + struct cdat_sslbis *sslbis = (struct cdat_sslbis *)header;
> + struct xarray *sslbis_xa = arg;
> + int remain, entries, i;
> +
> + remain = sslbis->hdr.length - sizeof(*sslbis);
> + if (!remain || remain % sizeof(struct sslbis_sslbe)) {
> + pr_warn("Malformed SSLBIS table length: (%u)\n",
> + sslbis->hdr.length);
> + return -EINVAL;
> + }
> +
> + /* Unrecognized data type, we can skip */
> + if (sslbis->data_type >= HMAT_SLLBIS_DATA_TYPE_MAX)
> + return 0;
> +
> + entries = remain / sizeof(*sslbis);
> +
> + for (i = 0; i < entries; i++) {
> + struct sslbis_sslbe *sslbe = &sslbis->sslbe[i];
> + u16 x = le16_to_cpu(sslbe->port_x_id);
> + u16 y = le16_to_cpu(sslbe->port_y_id);
> + struct node_hmem_attrs *hmem_attrs;
The more "node_hmem_attrs" get reused the more it sticks out as no
longer a good name. There's no Linux "nodes" to consider in this code,
no hmem since this is switch path and not a memory node, and no sysfs
attributes (which are typically named with "_attrs"). This data
structure is just a container for passing a tuple of r/w-latency and
r/w-bandwidth numbers. It's a performance coordinate that just happens
to get reused by the hmem sysfs nodes and now CXL cdat. Perhaps 'struct
access_coordinate'?
That would also make this code more readable:
void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs,
unsigned access);
...vs:
void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
unsigned access);
...at least that seems more readable to me.
> + u16 dsp_id;
> + u64 val;
> + int rc;
> +
> + switch (x) {
> + case SSLBIS_US_PORT:
> + dsp_id = y;
> + break;
> + case SSLBIS_ANY_PORT:
> + switch (y) {
> + case SSLBIS_US_PORT:
> + dsp_id = x;
> + break;
> + case SSLBIS_ANY_PORT:
> + dsp_id = SSLBIS_ANY_PORT;
> + break;
> + default:
> + dsp_id = y;
> + break;
> + }
> + break;
> + default:
> + dsp_id = x;
> + break;
> + }
> +
> + hmem_attrs = xa_load(sslbis_xa, dsp_id);
> + if (xa_is_err(hmem_attrs))
> + return xa_err(hmem_attrs);
> + if (!hmem_attrs) {
> + hmem_attrs = kzalloc(sizeof(*hmem_attrs), GFP_KERNEL);
> + if (!hmem_attrs)
> + return -ENOMEM;
> + }
> +
> + rc = check_mul_overflow(le64_to_cpu(sslbis->entry_base_unit),
> + le16_to_cpu(sslbe->value), &val);
> + if (unlikely(rc))
> + pr_warn("SSLBIS value overflowed!\n");
> +
> + cxl_hmem_attrs_set(hmem_attrs, sslbis->data_type, val);
> + rc = xa_insert(sslbis_xa, dsp_id, hmem_attrs, GFP_KERNEL);
I'm confused why an xarray is needed. If the sslbis indicates the access
parameters from the upstream port to the downstream port, just record
that access_coordinate and point each downstream port to the same
coordinate. Why keep an xarray full of these around?
In other words just add 'struct access_coordinate' to 'struct cxl_dport'
rather maintaining this parallel array of per-downstream port data.
When / if p2p support comes along then we can worry about dport-to-dport
performance, but for this patchset those sslbis entries are 'don't care'.
> + if (rc < 0 && rc != -EBUSY) {
> + kfree(hmem_attrs);
> + return rc;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_sslbis_parse_entry, CXL);
