Re: [PATCH v4 11/23] cxl: Add helper function that calculates QoS values for switches

[Dave Jiang <dave.jiang@intel.com>]

On 4/24/23 10:09 AM, Dave Jiang wrote:
> 
> 
> On 4/20/23 5:26 AM, Jonathan Cameron wrote:
>> On Wed, 19 Apr 2023 13:22:07 -0700
>> Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>> The CDAT information from the switch, Switch Scoped Latency and 
>>> Bandwidth
>>> Information Strucutre (SSLBIS), is parsed and stored in an xarray 
>>> under the
>>> cxl_port. The QoS data are indexed by the downstream port id.  Walk 
>>> the CXL
>>> ports from endpoint to root and retrieve the relevant QoS information
>>> (bandwidth and latency) that are from the switch CDAT. If read or 
>>> write QoS
>>> values are not available, then use the access QoS value.
>>
>> I'd drop the access reference.  You already did that mapping from 
>> access to read
>> and write in earlier patch. Now we have no concept of access so 
>> mentioning
>> it will only potentially cause confusion.
> 
> ok
> 
>>
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>> v3:
>>> - Move to use 'struct node_hmem_attrs'
>>> ---
>>>   drivers/cxl/core/port.c |   81 
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   drivers/cxl/cxl.h       |    2 +
>>>   2 files changed, 83 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>>> index 3fedbabac1af..770b540d5325 100644
>>> --- a/drivers/cxl/core/port.c
>>> +++ b/drivers/cxl/core/port.c
>>> @@ -1921,6 +1921,87 @@ bool schedule_cxl_memdev_detach(struct 
>>> cxl_memdev *cxlmd)
>>>   }
>>>   EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
>>> +/**
>>> + * cxl_port_get_switch_qos - retrieve QoS data for CXL switches
>>
>> Hmm. Terminology wise, this is called QoS data in either CXL spec
>> or the HMAT stuff it came from.  I'd avoid that term here.
>> Might also get confused with the QoS telemetry stuff from the CXL
>> spec which is totally different or the QoS controls on an MLD
>> which are perhaps indirectly related to these.
>>
>> QoS only gets involved once these are mapped to a QTG - assumption
>> being that a given QoS policy should apply to devices of similar access
>> characteristics.
> 
> locality_info?

Or perf_data in accordance with this doc:
https://www.kernel.org/doc/html/v5.9/admin-guide/mm/numaperf.html

> 
> 
>>
>> Other than that bikeshedding.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>>
>>
>>> + * @port: endpoint cxl_port
>>> + * @rd_bw: writeback value for min read bandwidth
>>> + * @rd_lat: writeback value for total read latency
>>> + * @wr_bw: writeback value for min write bandwidth
>>> + * @wr_lat: writeback value for total write latency
>>> + *
>>> + * Return: Errno on failure, 0 on success. -ENOENT if no switch device
>>> + */
>>> +int cxl_port_get_switch_qos(struct cxl_port *port, u64 *rd_bw, u64 
>>> *rd_lat,
>>> +                u64 *wr_bw, u64 *wr_lat)
>>> +{
>>> +    u64 min_rd_bw = ULONG_MAX;
>>> +    u64 min_wr_bw = ULONG_MAX;
>>> +    struct cxl_dport *dport;
>>> +    struct cxl_port *nport;
>>> +    u64 total_rd_lat = 0;
>>> +    u64 total_wr_lat = 0;
>>> +    struct device *next;
>>> +    int switches = 0;
>>> +    int rc = 0;
>>> +
>>> +    if (!is_cxl_endpoint(port))
>>> +        return -EINVAL;
>>> +
>>> +    /* Skip the endpoint */
>>> +    next = port->dev.parent;
>>> +    nport = to_cxl_port(next);
>>> +    dport = port->parent_dport;
>>> +
>>> +    do {
>>> +        struct node_hmem_attrs *hmem_attrs;
>>> +        u64 lat, bw;
>>> +
>>> +        if (!nport->cdat.table)
>>> +            break;
>>> +
>>> +        if (!dev_is_pci(dport->dport))
>>> +            break;
>>> +
>>> +        hmem_attrs = xa_load(&nport->cdat.sslbis_xa, dport->port_id);
>>> +        if (xa_is_err(hmem_attrs))
>>> +            return xa_err(hmem_attrs);
>>> +
>>> +        if (!hmem_attrs) {
>>> +            hmem_attrs = xa_load(&nport->cdat.sslbis_xa, 
>>> SSLBIS_ANY_PORT);
>>> +            if (xa_is_err(hmem_attrs))
>>> +                return xa_err(hmem_attrs);
>>> +            if (!hmem_attrs)
>>> +                return -ENXIO;
>>> +        }
>>> +
>>> +        bw = hmem_attrs->write_bandwidth;
>>> +        lat = hmem_attrs->write_latency;
>>> +        min_wr_bw = min_t(u64, min_wr_bw, bw);
>>> +        total_wr_lat += lat;
>>> +
>>> +        bw = hmem_attrs->read_bandwidth;
>>> +        lat = hmem_attrs->read_latency;
>>> +        min_rd_bw = min_t(u64, min_rd_bw, bw);
>>> +        total_rd_lat += lat;
>>> +
>>> +        dport = nport->parent_dport;
>>> +        next = next->parent;
>>> +        nport = to_cxl_port(next);
>>> +        switches++;
>>> +    } while (next);
>>> +
>>> +    *wr_bw = min_wr_bw;
>>> +    *wr_lat = total_wr_lat;
>>> +    *rd_bw = min_rd_bw;
>>> +    *rd_lat = total_rd_lat;
>>> +
>>> +    if (!switches)
>>> +        return -ENOENT;
>>> +
>>> +    return rc;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_port_get_switch_qos, CXL);
>>
>>