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

[Alison Schofield <alison.schofield@intel.com>]

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

> 
>