RE: [PATCH v4 07/23] cxl: Add callback to parse the SSLBIS subtable from CDAT

[Dan Williams <dan.j.williams@intel.com>]

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@intel.com>
> 
> ---
> 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);