On Tue, 18 Jun 2024 16:16:41 -0700 Dave Jiang <dave.jiang@xxxxxxxxx> wrote: > The current bandwidth calculation aggregates all the targets. This simple > method does not take into account where multiple targets sharing under > a switch where the aggregated bandwidth can be less or greater than the > upstream link of the switch. > > To accurately account for the shared upstream uplink cases, a new update > function is introduced by walking from the leaves to the root of the > hierarchy and adjust the bandwidth in the process. This process is done > when all the targets for a region are present but before the final values > are send to the HMAT handling code cached access_coordinate targets. > > The original perf calculation path was kept to calculate the latency > performance data that does not require the shared link consideration. > The shared upstream link calculation is done as a second pass when all > the endpoints have arrived. > > Suggested-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> > Link: https://lore.kernel.org/linux-cxl/20240501152503.00002e60@xxxxxxxxxx/ > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx> A few trivial things inline. I haven't tested this yet in anger (as for 'reasons' my test machine is running an emulation of s390 emulating x86 this morning and so is a bit busy). I'll try and get at least some tests done in the near future and the additional control patches for doing so posted for the QEMU emulation. Feel free to poke me if I've not given a tested by the end of next week. In meantime, lgtm. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > --- > v5: > - Fix cxl_memdev_get_dpa_perf() default return. (Jonathan) > - Direct return dpa_perf_contains(). (Jonathan) > - Fix incorrect bandwidth member reference. (Jonathan) > - Direct return error for pcie_link_speed_mbps(). (Jonathan) > - Remove stray edit. (Jonathan) > - Adjust calculated bandwidth of RPs sharing same host bridge. (Jonathan) > - Fix uninit var gp_is_root. (kernel test robot) > --- > drivers/cxl/core/cdat.c | 411 ++++++++++++++++++++++++++++++++++++-- > drivers/cxl/core/core.h | 4 +- > drivers/cxl/core/pci.c | 23 +++ > drivers/cxl/core/port.c | 20 ++ > drivers/cxl/core/region.c | 4 + > drivers/cxl/cxl.h | 1 + > 6 files changed, 446 insertions(+), 17 deletions(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index fea214340d4b..72f86bc99750 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c ... > +static struct cxl_dpa_perf *cxl_memdev_get_dpa_perf(struct cxl_memdev_state *mds, > + enum cxl_decoder_mode mode) > +{ > + switch (mode) { > case CXL_DECODER_RAM: > - perf = &mds->ram_perf; > - break; > + return &mds->ram_perf; > case CXL_DECODER_PMEM: > - perf = &mds->pmem_perf; > - break; > + return &mds->pmem_perf; > default: > + return ERR_PTR(-EINVAL); > + } > + Can't get here so delete the below return. > + return ERR_PTR(-EINVAL); > +} > +static int cxl_endpoint_gather_coordinates(struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled, > + struct xarray *usp_xa) > +{ > + struct cxl_port *endpoint = to_cxl_port(cxled->cxld.dev.parent); > + struct access_coordinate pci_coord[ACCESS_COORDINATE_MAX]; > + struct access_coordinate sw_coord[ACCESS_COORDINATE_MAX]; > + struct access_coordinate ep_coord[ACCESS_COORDINATE_MAX]; > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > + struct pci_dev *pdev = to_pci_dev(cxlds->dev); > + struct cxl_port *parent_port, *gp_port; > + struct cxl_perf_ctx *perf_ctx; > + struct cxl_dpa_perf *perf; > + bool gp_is_root = false; > + unsigned long index; > + void *ptr; > + int rc; > + > + if (cxlds->rcd) > + return -ENODEV; > + > + parent_port = to_cxl_port(endpoint->dev.parent); > + gp_port = to_cxl_port(parent_port->dev.parent); > + if (is_cxl_root(gp_port)) > + gp_is_root = true; gp_is_root = is_cxl_root(gp_port); and drop the initialization at declaration. Or just use is_cxl_root(gp_port) at the check below. Could set all these at declaration though I guess a few lines would be a bit long. > + > + perf = cxl_memdev_get_dpa_perf(mds, cxlr->mode); > + if (IS_ERR(perf)) > + return PTR_ERR(perf); > + > + if (!dpa_perf_contains(perf, cxled->dpa_res)) > + return -EINVAL; > + > + /* > + * The index for the xarray is the upstream port device of the upstream > + * CXL switch. > + */ > + index = (unsigned long)parent_port->uport_dev; > + perf_ctx = xa_load(usp_xa, index); > + if (!perf_ctx) { > + struct cxl_perf_ctx *c __free(kfree) = > + kzalloc(sizeof(*perf_ctx), GFP_KERNEL); > + > + if (!c) > + return -ENOMEM; > + ptr = xa_store(usp_xa, index, c, GFP_KERNEL); > + if (xa_is_err(ptr)) > + return xa_err(ptr); > + perf_ctx = no_free_ptr(c); > + } > + > + /* Direct upstream link from EP bandwidth */ > + rc = cxl_pci_get_bandwidth(pdev, pci_coord); > + if (rc < 0) > + return rc; > + > + /* > + * Min of upstream link bandwidth and Endpoint CDAT bandwidth from > + * DSLBIS. > + */ > + cxl_coordinates_combine(ep_coord, pci_coord, perf->cdat_coord); > + > + /* > + * If grandparent port is root, then there's no switch involved and > + * the endpoint is connected to a root port. > + */ > + if (!gp_is_root) { > + /* > + * Retrieve the switch SSLBIS for switch downstream port > + * associated with the endpoint bandwidth. > + */ > + rc = cxl_port_get_switch_dport_bandwidth(endpoint, sw_coord); > + if (rc) > + return rc; > + > + /* > + * Min of the earlier coordinates with the switch SSLBIS > + * bandwidth > + */ > + cxl_coordinates_combine(ep_coord, ep_coord, sw_coord); > + } > + > + /* > + * Aggregate the computed bandwidth with the current aggregated bandwidth > + * of the endpoints with the same switch upstream port. > + */ > + cxl_bandwidth_add(perf_ctx->coord, perf_ctx->coord, ep_coord); > + perf_ctx->port = parent_port; > + > + return 0; > +} > + > +static struct xarray *cxl_switch_iterate_coordinates(struct xarray *input_xa, > + bool *parent_is_root) > +{ > + struct xarray *res_xa __free(kfree) = kzalloc(sizeof(*res_xa), GFP_KERNEL); > + struct access_coordinate coords[ACCESS_COORDINATE_MAX]; > + struct cxl_perf_ctx *ctx, *us_ctx; > + unsigned long index, us_index; > + void *ptr; > + int rc; > + > + if (!res_xa) > + return ERR_PTR(-ENOMEM); > + xa_init(res_xa); > + > + *parent_is_root = false; > + xa_for_each(input_xa, index, ctx) { > + struct cxl_port *parent_port, *port, *gp_port; > + struct device *dev = (struct device *)index; > + struct cxl_dport *dport; > + struct pci_dev *pdev; > + bool gp_is_root; > + > + gp_is_root = false; > + port = ctx->port; > + parent_port = to_cxl_port(port->dev.parent); Maybe set those at declaration? > + if (is_cxl_root(parent_port)) { > + *parent_is_root = true; > + } else { > + gp_port = to_cxl_port(parent_port->dev.parent); > + gp_is_root = is_cxl_root(gp_port); > + }
