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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 24 Apr 2023 10:31:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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

Either would be fine as far as I'm concerned.
I guess perf_data puts less of a spin on what it 'means' than
locality_info - so I'd slightly prefer that.

Jonathan

> 
> > 
> >   
> >>
> >> 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);  
> >>
> >>