On Tue, 6 Feb 2024 15:28:33 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> wrote:
> Refactor the common code of combining coordinates in order to reduce code.
> Create a new function cxl_cooordinates_combine() it combine two 'struct
> access_coordinate'.
>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> ---
> v5:
> - Fix comment header (0-day)
> - Remove EXPORT_SYMBOL (Dan)
> ---
> drivers/cxl/core/cdat.c | 32 +++++++++++++++++++++++---------
> drivers/cxl/core/port.c | 18 ++----------------
> drivers/cxl/cxl.h | 4 ++++
> 3 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 08fd0baea7a0..096320390ad9 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -185,15 +185,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
> xa_for_each(dsmas_xa, index, dent) {
> int qos_class;
>
> - dent->coord.read_latency = dent->coord.read_latency +
> - c.read_latency;
> - dent->coord.write_latency = dent->coord.write_latency +
> - c.write_latency;
> - dent->coord.read_bandwidth = min_t(int, c.read_bandwidth,
> - dent->coord.read_bandwidth);
> - dent->coord.write_bandwidth = min_t(int, c.write_bandwidth,
> - dent->coord.write_bandwidth);
> -
> + cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
> dent->entries = 1;
> rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
> &qos_class);
> @@ -484,4 +476,26 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
> }
> EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL);
>
> +/**
> + * cxl_coordinates_combine - Combine the two input coordinates into the first
> + *
> + * @out: Output coordinate of c1 and c2 combined
> + * @c1: first coordinate, to be written to
When you say to be written to, what do you mean?
Left over from adding out?
No obvious reason for this to have any idea that c1 and c2 are
ordered.
> + * @c2: second coordinate
> + */
> +void cxl_coordinates_combine(struct access_coordinate *out,
> + struct access_coordinate *c1,
> + struct access_coordinate *c2)
> +{
> + if (c2->write_bandwidth)
> + out->write_bandwidth = min(c1->write_bandwidth,
> + c2->write_bandwidth);
Why check c2->write_bandwidth?
Code already does it, but I'm not sure why + I don't think you should
treat c1 and c2 differently.
> + out->write_latency = c1->write_latency + c2->write_latency;
> +
> + if (c2->read_bandwidth)
> + out->read_bandwidth = min(c1->read_bandwidth,
> + c2->read_bandwidth);
> + out->read_latency = c1->read_latency + c2->read_latency;
> +}
> +
> MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 612bf7e1e847..af9458b2678c 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2096,20 +2096,6 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
> }
> EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
>
> -static void combine_coordinates(struct access_coordinate *c1,
> - struct access_coordinate *c2)
> -{
> - if (c2->write_bandwidth)
> - c1->write_bandwidth = min(c1->write_bandwidth,
> - c2->write_bandwidth);
> - c1->write_latency += c2->write_latency;
> -
> - if (c2->read_bandwidth)
> - c1->read_bandwidth = min(c1->read_bandwidth,
> - c2->read_bandwidth);
> - c1->read_latency += c2->read_latency;
> -}
> -
> /**
> * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
> * of CXL path
> @@ -2143,7 +2129,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
> * nothing to gather.
> */
> while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
> - combine_coordinates(&c, &dport->sw_coord);
> + cxl_coordinates_combine(&c, &c, &dport->sw_coord);
> c.write_latency += dport->link_latency;
> c.read_latency += dport->link_latency;
>
> @@ -2152,7 +2138,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
> }
>
> /* Augment with the generic port (host bridge) perf data */
> - combine_coordinates(&c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
> + cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
>
> /* Get the calculated PCI paths bandwidth */
> pdev = to_pci_dev(port->uport_dev->parent);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fe7448f2745e..fab2da4b1f04 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -882,6 +882,10 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>
> void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
>
> +void cxl_coordinates_combine(struct access_coordinate *out,
> + struct access_coordinate *c1,
> + struct access_coordinate *c2);
> +
> /*
> * Unit test builds overrides this to __weak, find the 'strong' version
> * of these symbols in tools/testing/cxl/.
