| Message ID | 20240206222951.1833098-6-dave.jiang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support to report region access coordinates to numa nodes | expand |
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 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/.
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/. >
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 + * @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; +} + 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/.
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(-)