diff mbox series

[v4,05/11] cxl: Split out combine_coordinates() for common shared usage

Message ID 170568501456.1008395.7903809557943927970.stgit@djiang5-mobl3
State Superseded
Headers show
Series cxl: Add support to report region access coordinates to numa nodes | expand

Commit Message

Dave Jiang Jan. 19, 2024, 5:23 p.m. UTC
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>
---
 drivers/cxl/core/cdat.c |   32 +++++++++++++++++++++++---------
 drivers/cxl/core/port.c |   18 ++----------------
 drivers/cxl/cxl.h       |    4 ++++
 3 files changed, 29 insertions(+), 25 deletions(-)

Comments

Dan Williams Jan. 20, 2024, 12:35 a.m. UTC | #1
Dave Jiang 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>
> ---
>  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 cd84d87f597a..4d542627d02d 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -183,15 +183,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(root_port, &dent->coord, 1, &qos_class);
>  		if (rc != 1)
> @@ -514,4 +506,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
> + *
> + * @c1: first coordinate, to be written to
> + * @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);
> +		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;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_coordinates_combine, CXL);

There is no need for EXPORT_SYMBOL() when the definition and the only
caller exist within the same compilation unit, cxl_core.o.

However, given there is nothing "CXL" about this function it likely wants
to move out of cxl_core.o if another caller ever arrives.
Dave Jiang Jan. 22, 2024, 4:19 p.m. UTC | #2
On 1/19/24 17:35, Dan Williams wrote:
> Dave Jiang 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>
>> ---
>>  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 cd84d87f597a..4d542627d02d 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -183,15 +183,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(root_port, &dent->coord, 1, &qos_class);
>>  		if (rc != 1)
>> @@ -514,4 +506,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
>> + *
>> + * @c1: first coordinate, to be written to
>> + * @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);
>> +		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;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_coordinates_combine, CXL);
> 
> There is no need for EXPORT_SYMBOL() when the definition and the only
> caller exist within the same compilation unit, cxl_core.o.
> 
> However, given there is nothing "CXL" about this function it likely wants
> to move out of cxl_core.o if another caller ever arrives.

It's mostly used by core/cdat.c but eventually also used by core/port.c. So it's all within the core. 
>
diff mbox series

Patch

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index cd84d87f597a..4d542627d02d 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -183,15 +183,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(root_port, &dent->coord, 1, &qos_class);
 		if (rc != 1)
@@ -514,4 +506,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
+ *
+ * @c1: first coordinate, to be written to
+ * @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);
+		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;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_coordinates_combine, CXL);
+
 MODULE_IMPORT_NS(CXL);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 7a5eff45e1e2..1d3a04ef8b4f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2095,20 +2095,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
@@ -2142,7 +2128,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;
 
@@ -2151,7 +2137,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 0fc06455233d..0a0a121ee780 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -879,6 +879,10 @@  void cxl_switch_parse_cdat(struct cxl_port *port);
 int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord);
 
+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/.