| Message ID | 170441210328.3574076.8557138214621981436.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support to report region access coordinates to numa nodes | expand |
Dave Jiang wrote: > Calculate and store the performance data for a CXL region. Find the worst > read and write latency for all the included ranges from each of the devices > that attributes to the region and designate that as the latency data. Sum > all the read and write bandwidth data for each of the device region and > that is the total bandwidth for the region. > > The perf list is expected to be constructed before the endpoint decoders > are registered and thus there should be no early reading of the entries > from the region assemble action. The calling of the region qos calculate > function is under the protection of cxl_dpa_rwsem and will ensure that > all DPA associated work has completed. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > v3: > - Clarify calculated data is same base as the coordinates computed from the > HMAT tables. (Jonathan) > --- > drivers/cxl/core/cdat.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/core/region.c | 2 ++ > drivers/cxl/cxl.h | 5 ++++ > 3 files changed, 60 insertions(+) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index cd84d87f597a..78e1cdcb9d89 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -515,3 +515,56 @@ void cxl_switch_parse_cdat(struct cxl_port *port) > EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL); > > MODULE_IMPORT_NS(CXL); > + > +void cxl_region_perf_data_calculate(struct cxl_region *cxlr, > + struct cxl_endpoint_decoder *cxled) > +{ > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > + struct range dpa = { > + .start = cxled->dpa_res->start, > + .end = cxled->dpa_res->end, > + }; > + struct list_head *perf_list; > + struct cxl_dpa_perf *perf; > + bool found = false; > + > + switch (cxlr->mode) { > + case CXL_DECODER_RAM: > + perf_list = &mds->ram_perf_list; > + break; > + case CXL_DECODER_PMEM: > + perf_list = &mds->pmem_perf_list; > + break; > + default: > + return; > + } > + Given how far away this function is from any refactoring that might happen in region.c, and that the locking documentation in this changelog will not be readily available when reading the code later, it would be nice to "document" the locking here with a: lockdep_assert_held(&cxl_dpa_rwsem); > + list_for_each_entry(perf, perf_list, list) { > + if (range_contains(&perf->dpa_range, &dpa)) { > + found = true; > + break; > + } > + } > + > + if (!found) > + return; > + > + /* Get total bandwidth and the worst latency for the cxl region */ > + cxlr->coord.read_latency = max_t(unsigned int, > + cxlr->coord.read_latency, > + perf->coord.read_latency); > + cxlr->coord.write_latency = max_t(unsigned int, > + cxlr->coord.write_latency, > + perf->coord.write_latency); > + cxlr->coord.read_bandwidth += perf->coord.read_bandwidth; > + cxlr->coord.write_bandwidth += perf->coord.write_bandwidth; > + > + /* > + * Convert latency to nanosec from picosec to be consistent with the > + * resulting latency coordinates computed by HMAT code. Do you meen the HMEM_REPORTING code? Since access_coordinate is parsed from HMAT data, it isn't the HMAT data directly.
On 1/4/24 17:07, Dan Williams wrote: > Dave Jiang wrote: >> Calculate and store the performance data for a CXL region. Find the worst >> read and write latency for all the included ranges from each of the devices >> that attributes to the region and designate that as the latency data. Sum >> all the read and write bandwidth data for each of the device region and >> that is the total bandwidth for the region. >> >> The perf list is expected to be constructed before the endpoint decoders >> are registered and thus there should be no early reading of the entries >> from the region assemble action. The calling of the region qos calculate >> function is under the protection of cxl_dpa_rwsem and will ensure that >> all DPA associated work has completed. >> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> v3: >> - Clarify calculated data is same base as the coordinates computed from the >> HMAT tables. (Jonathan) >> --- >> drivers/cxl/core/cdat.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/cxl/core/region.c | 2 ++ >> drivers/cxl/cxl.h | 5 ++++ >> 3 files changed, 60 insertions(+) >> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> index cd84d87f597a..78e1cdcb9d89 100644 >> --- a/drivers/cxl/core/cdat.c >> +++ b/drivers/cxl/core/cdat.c >> @@ -515,3 +515,56 @@ void cxl_switch_parse_cdat(struct cxl_port *port) >> EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL); >> >> MODULE_IMPORT_NS(CXL); >> + >> +void cxl_region_perf_data_calculate(struct cxl_region *cxlr, >> + struct cxl_endpoint_decoder *cxled) >> +{ >> + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); >> + struct cxl_dev_state *cxlds = cxlmd->cxlds; >> + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); >> + struct range dpa = { >> + .start = cxled->dpa_res->start, >> + .end = cxled->dpa_res->end, >> + }; >> + struct list_head *perf_list; >> + struct cxl_dpa_perf *perf; >> + bool found = false; >> + >> + switch (cxlr->mode) { >> + case CXL_DECODER_RAM: >> + perf_list = &mds->ram_perf_list; >> + break; >> + case CXL_DECODER_PMEM: >> + perf_list = &mds->pmem_perf_list; >> + break; >> + default: >> + return; >> + } >> + > > Given how far away this function is from any refactoring that might > happen in region.c, and that the locking documentation in this changelog > will not be readily available when reading the code later, it would be > nice to "document" the locking here with a: > > lockdep_assert_held(&cxl_dpa_rwsem); Ok I'll add. > >> + list_for_each_entry(perf, perf_list, list) { >> + if (range_contains(&perf->dpa_range, &dpa)) { >> + found = true; >> + break; >> + } >> + } >> + >> + if (!found) >> + return; >> + >> + /* Get total bandwidth and the worst latency for the cxl region */ >> + cxlr->coord.read_latency = max_t(unsigned int, >> + cxlr->coord.read_latency, >> + perf->coord.read_latency); >> + cxlr->coord.write_latency = max_t(unsigned int, >> + cxlr->coord.write_latency, >> + perf->coord.write_latency); >> + cxlr->coord.read_bandwidth += perf->coord.read_bandwidth; >> + cxlr->coord.write_bandwidth += perf->coord.write_bandwidth; >> + >> + /* >> + * Convert latency to nanosec from picosec to be consistent with the >> + * resulting latency coordinates computed by HMAT code. > > Do you meen the HMEM_REPORTING code? Since access_coordinate is parsed > from HMAT data, it isn't the HMAT data directly. Yes. I was trying to say HMAT parsing/handling code.
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index cd84d87f597a..78e1cdcb9d89 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -515,3 +515,56 @@ void cxl_switch_parse_cdat(struct cxl_port *port) EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL); MODULE_IMPORT_NS(CXL); + +void cxl_region_perf_data_calculate(struct cxl_region *cxlr, + struct cxl_endpoint_decoder *cxled) +{ + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct cxl_dev_state *cxlds = cxlmd->cxlds; + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); + struct range dpa = { + .start = cxled->dpa_res->start, + .end = cxled->dpa_res->end, + }; + struct list_head *perf_list; + struct cxl_dpa_perf *perf; + bool found = false; + + switch (cxlr->mode) { + case CXL_DECODER_RAM: + perf_list = &mds->ram_perf_list; + break; + case CXL_DECODER_PMEM: + perf_list = &mds->pmem_perf_list; + break; + default: + return; + } + + list_for_each_entry(perf, perf_list, list) { + if (range_contains(&perf->dpa_range, &dpa)) { + found = true; + break; + } + } + + if (!found) + return; + + /* Get total bandwidth and the worst latency for the cxl region */ + cxlr->coord.read_latency = max_t(unsigned int, + cxlr->coord.read_latency, + perf->coord.read_latency); + cxlr->coord.write_latency = max_t(unsigned int, + cxlr->coord.write_latency, + perf->coord.write_latency); + cxlr->coord.read_bandwidth += perf->coord.read_bandwidth; + cxlr->coord.write_bandwidth += perf->coord.write_bandwidth; + + /* + * Convert latency to nanosec from picosec to be consistent with the + * resulting latency coordinates computed by HMAT code. + */ + cxlr->coord.read_latency = DIV_ROUND_UP(cxlr->coord.read_latency, 1000); + cxlr->coord.write_latency = DIV_ROUND_UP(cxlr->coord.write_latency, 1000); +} diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 57a5901d5a60..7f19b533c5ae 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1722,6 +1722,8 @@ static int cxl_region_attach(struct cxl_region *cxlr, return -EINVAL; } + cxl_region_perf_data_calculate(cxlr, cxled); + if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) { int i; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 492dbf63935f..4639d0d6ef54 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -519,6 +519,7 @@ struct cxl_region_params { * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge * @flags: Region state flags * @params: active + config params for the region + * @coord: QoS access coordinates for the region */ struct cxl_region { struct device dev; @@ -529,6 +530,7 @@ struct cxl_region { struct cxl_pmem_region *cxlr_pmem; unsigned long flags; struct cxl_region_params params; + struct access_coordinate coord; }; struct cxl_nvdimm_bridge { @@ -879,6 +881,9 @@ 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_region_perf_data_calculate(struct cxl_region *cxlr, + struct cxl_endpoint_decoder *cxled); + /* * Unit test builds overrides this to __weak, find the 'strong' version * of these symbols in tools/testing/cxl/.