Re: [PATCH v7 2/3] cxl: Calculate region bandwidth of targets with shared upstream link

Alison Schofield
2024-07-11T01:39:38Z

On Wed, Jul 10, 2024 at 03:24:01PM -0700, Dave Jiang wrote:
> The current bandwidth calculation aggregates all the targets. This simple
> method does not take into account where multiple targets sharing under
> a switch or a root port where the aggregated bandwidth can be 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 clamp the bandwidth in the process as needed. 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.
>
> Testing is done via qemu with CXL hierachy. run_qemu[1] is modified to
> support several CXL hierachy layouts. The following layouts are tested:
>
> HB: Host Bridge
> RP: Root Port
> SW: Switch
> EP: End Point
>
> 2 HB 2 RP 2 EP: resulting bandwidth: 624
> 1 HB 2 RP 2 EP: resulting bandwidth: 624
> 2 HB 2 RP 2 SW 4 EP: resulting bandwidth: 624
>
> Current testing, perf number from SRAT/HMAT is hacked into the kernel
> code. However with new QEMU support of Generic Target Port that's
> incoming, the perf data injection is no longer needed.
>
> [1]: https://github.com/pmem/run_qemu
>
> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Link: https://lore.kernel.org/linux-cxl/20240501152503.00002e60@Huawei.com/
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> v7:
> - Add test notes in commit log. (Dan)
> - Move cxl_memdev_get_dpa_perf() to cxled_memdev_get_dpa_perf(). (Dan)
> - Add a DEFINE_FREE(free_perf_xa). (Dan)
> - Address 2hb2rp2ep issue Jonathan reported. (Jonathan)
> - Added more kdoc comment headers. (Dan)

Looks like potential kdocs are not annotated w /**

> - Rename helper functions to be more clear on what they do. (Dan)
> - Move activiation point to after cxl_region_setup_targets(). (Dan)
> ---

snip

> +/*
> + * cxl_region_shared_upstream_perf_update - Recalculate the bandwidth for the region
> + * @cxl_region: the cxl region to recalculate
> + *
> + * The function walks the topology from bottom up and calculates the bandwidth. It
> + * starts at the endpoints, processes at the switches if any, processes at the rootport
> + * level, at the host bridge level, and finally aggregates at the region.
> + */
> +void cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
> +{
> + struct xarray *usp_xa, *working_xa;
> + int root_count = 0;
> + bool is_root;
> + int rc;
> +
> + lockdep_assert_held(&cxl_dpa_rwsem);
> +
> + usp_xa = kzalloc(sizeof(*usp_xa), GFP_KERNEL);
> + if (!usp_xa)
> return;
> +
> + xa_init(usp_xa);
> +
> + /* Collect bandwidth data from all the endpoints. */
> + for (int i = 0; i < cxlr->params.nr_targets; i++) {
> + struct cxl_endpoint_decoder *cxled = cxlr->params.targets[i];
> +
> + is_root = false;
> + rc = cxl_endpoint_gather_bandwidth(cxlr, cxled, usp_xa, &is_root);
> + if (rc) {
> + free_perf_xa(usp_xa);
> + return;
> + }
> + root_count += is_root;
> }
>
> + /* Detect asymmetric hierachy with some direct attached endpoints. */
> + if (root_count && root_count != cxlr->params.nr_targets) {
> + dev_dbg(&cxlr->dev,
> + "Asymmetric hierachy detected, bandwidth not updated\n");
> + return;
> + }
> +
> + /*
> + * Walk up one or more switches to deal with the bandwidth of the
> + * switches if they exist. Endpoints directly attached to RPs skip
> + * over this part.
> + */
> + if (!root_count) {
> + do {
> + working_xa = cxl_switch_gather_bandwidth(cxlr, usp_xa,
> + &is_root);
> + if (IS_ERR(working_xa))
> + goto out;
> + free_perf_xa(usp_xa);
> + usp_xa = working_xa;
> + } while (!is_root);
> + }
> +
> + /* Handle the bandwidth at the root port of the hierachy */
> + working_xa = cxl_rp_gather_bandwidth(usp_xa);
> + if (rc)
> + goto out;
> + free_perf_xa(usp_xa);

I was going to say something about getting rid of the goto's
and just freeing and returning in line like was done ~36 lines
above..but then realize something went astray here in the code
movement. Here and below, rc isn't set.

> + usp_xa = working_xa;
> +
> + /* Handle the bandwidth at the host bridge of the hierachy */
> + working_xa = cxl_hb_gather_bandwidth(usp_xa);
> + if (rc)
> + goto out;
> + free_perf_xa(usp_xa);
> + usp_xa = working_xa;
> +
> + /*
> + * Aggregate all the bandwidth collected per CFMWS (ACPI0017) and
> + * update the region bandwidth with the final calculated values.
> + */
> + cxl_region_update_bandwidth(cxlr, usp_xa);
> +
> +out:
> + free_perf_xa(usp_xa);
> +}

snip

>
>