Re: [PATCH v5 05/12] cxl: Split out combine_coordinates() for common shared usage

[Dave Jiang <dave.jiang@intel.com>]

On 2/15/24 9:51 AM, Jonathan Cameron wrote:
> On Tue, 6 Feb 2024 15:28:33 -0700
> Dave Jiang <dave.jiang@intel.com> 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@intel.com>
>> ---
>> 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
>> + *
>> + * @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?

Yeah I'm not sure why it says that. Seems like half a sentence I left dangling. Going to change the block to below:

/**
 * cxl_coordinates_combine - Combine the two input coordinates
 *
 * @out: Output coordinate of c1 and c2 combined
 * @c1: input coordinates
 * @c2: input coordinates
 */


> 
> 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.

I think I need to check both and make sure that neither are 0 since we are taking the min of the two and both need to be valid.

> 
>> +		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/.
>