Message ID | 168660916231.1965241.248859226126456131.stgit@djiang5-mobl3 |
---|---|
State | New, archived |
Headers | show |
Series | [v2] base/node / acpi: Change 'node_hmem_attrs' to 'access_coordinates' | expand |
On Mon, Jun 12, 2023 at 03:32:42PM -0700, Dave Jiang wrote: > Dan Williams suggested changing the struct 'node_hmem_attrs' to > 'access_coordinates' [1]. The struct is a container of r/w-latency and > r/w-bandwidth numbers. Moving forward, this container will also be used by > CXL to store the performance characteristics of each link hop in > the PCIE/CXL topology. So, where node_hmem_attrs is just the access > parameters of a memory-node, access_coordinates applies more broadly > to hardware topology characteristics. The observation is that seemed like > an excercise in having the application identify "where" it falls on a > spectrum of bandwidth and latency needs. For the tuple of read/write-latency > and read/write-bandwidth, "coordinates" is not a perfect fit. Sometimes it > is just conveying values in isolation and not a "location" relative to > other performance points, but in the end this data is used to identify the > performance operation point of a given memory-node. [2] > > Link: http://lore.kernel.org/r/64471313421f7_1b66294d5@dwillia2-xfh.jf.intel.com.notmuch/ > Link: https://lore.kernel.org/linux-cxl/645e6215ee0de_1e6f2945e@dwillia2-xfh.jf.intel.com.notmuch/ > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > > Hi Greg and Rafael, > please consider ACK this patch and Dan can take it through the > CXL upstream tree. The remaining ACPI [1] and CXL [2] patches for enabling > CXL QoS class data have dependency on this patch. Thank you! > > [1]: https://lore.kernel.org/linux-cxl/168333141100.2290593.16294670316057617744.stgit@djiang5-mobl3/T/#t > [2]: https://lore.kernel.org/linux-cxl/168451588868.3470703.3527256859632103687.stgit@djiang5-mobl3/T/#t Isn't this going to conflict with the version that I have in the driver-core-next tree as commit 7810f4dc8795 ("base/node: Use 'property' to identify an access parameter")? Or was that a different thing? thanks, greg k-h
On 6/13/23 00:57, Greg KH wrote: > On Mon, Jun 12, 2023 at 03:32:42PM -0700, Dave Jiang wrote: >> Dan Williams suggested changing the struct 'node_hmem_attrs' to >> 'access_coordinates' [1]. The struct is a container of r/w-latency and >> r/w-bandwidth numbers. Moving forward, this container will also be used by >> CXL to store the performance characteristics of each link hop in >> the PCIE/CXL topology. So, where node_hmem_attrs is just the access >> parameters of a memory-node, access_coordinates applies more broadly >> to hardware topology characteristics. The observation is that seemed like >> an excercise in having the application identify "where" it falls on a >> spectrum of bandwidth and latency needs. For the tuple of read/write-latency >> and read/write-bandwidth, "coordinates" is not a perfect fit. Sometimes it >> is just conveying values in isolation and not a "location" relative to >> other performance points, but in the end this data is used to identify the >> performance operation point of a given memory-node. [2] >> >> Link: http://lore.kernel.org/r/64471313421f7_1b66294d5@dwillia2-xfh.jf.intel.com.notmuch/ >> Link: https://lore.kernel.org/linux-cxl/645e6215ee0de_1e6f2945e@dwillia2-xfh.jf.intel.com.notmuch/ >> Suggested-by: Dan Williams <dan.j.williams@intel.com> >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> >> --- >> >> Hi Greg and Rafael, >> please consider ACK this patch and Dan can take it through the >> CXL upstream tree. The remaining ACPI [1] and CXL [2] patches for enabling >> CXL QoS class data have dependency on this patch. Thank you! >> >> [1]: https://lore.kernel.org/linux-cxl/168333141100.2290593.16294670316057617744.stgit@djiang5-mobl3/T/#t >> [2]: https://lore.kernel.org/linux-cxl/168451588868.3470703.3527256859632103687.stgit@djiang5-mobl3/T/#t > Isn't this going to conflict with the version that I have in the > driver-core-next tree as commit 7810f4dc8795 ("base/node: Use 'property' > to identify an access parameter")? > > Or was that a different thing? Yes this is a different thing and should not conflict. But a small dependency on the mentioned commit above however: @@ -168,7 +168,7 @@ static ssize_t property##_show(struct device *dev, \ char *buf) \ { \ return sysfs_emit(buf, "%u\n", \ - to_access_nodes(dev)->hmem_attrs.property); \ + to_access_nodes(dev)->coord.property); \ } \ static DEVICE_ATTR_RO(property) > > thanks, > > greg k-h
On Mon, Jun 12, 2023 at 03:32:42PM -0700, Dave Jiang wrote: > Dan Williams suggested changing the struct 'node_hmem_attrs' to > 'access_coordinates' [1]. The struct is a container of r/w-latency and > r/w-bandwidth numbers. Moving forward, this container will also be used by > CXL to store the performance characteristics of each link hop in > the PCIE/CXL topology. So, where node_hmem_attrs is just the access > parameters of a memory-node, access_coordinates applies more broadly > to hardware topology characteristics. The observation is that seemed like > an excercise in having the application identify "where" it falls on a > spectrum of bandwidth and latency needs. For the tuple of read/write-latency > and read/write-bandwidth, "coordinates" is not a perfect fit. Sometimes it > is just conveying values in isolation and not a "location" relative to > other performance points, but in the end this data is used to identify the > performance operation point of a given memory-node. [2] > > Link: http://lore.kernel.org/r/64471313421f7_1b66294d5@dwillia2-xfh.jf.intel.com.notmuch/ > Link: https://lore.kernel.org/linux-cxl/645e6215ee0de_1e6f2945e@dwillia2-xfh.jf.intel.com.notmuch/ > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > > Hi Greg and Rafael, > please consider ACK this patch and Dan can take it through the > CXL upstream tree. The remaining ACPI [1] and CXL [2] patches for enabling > CXL QoS class data have dependency on this patch. Thank you! > > [1]: https://lore.kernel.org/linux-cxl/168333141100.2290593.16294670316057617744.stgit@djiang5-mobl3/T/#t > [2]: https://lore.kernel.org/linux-cxl/168451588868.3470703.3527256859632103687.stgit@djiang5-mobl3/T/#t Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c index bba268ecd802..f9ff992038fa 100644 --- a/drivers/acpi/numa/hmat.c +++ b/drivers/acpi/numa/hmat.c @@ -62,7 +62,7 @@ struct memory_target { unsigned int memory_pxm; unsigned int processor_pxm; struct resource memregions; - struct node_hmem_attrs hmem_attrs[2]; + struct access_coordinate coord[2]; struct list_head caches; struct node_cache_attrs cache_attrs; bool registered; @@ -227,24 +227,24 @@ static void hmat_update_target_access(struct memory_target *target, { switch (type) { case ACPI_HMAT_ACCESS_LATENCY: - target->hmem_attrs[access].read_latency = value; - target->hmem_attrs[access].write_latency = value; + target->coord[access].read_latency = value; + target->coord[access].write_latency = value; break; case ACPI_HMAT_READ_LATENCY: - target->hmem_attrs[access].read_latency = value; + target->coord[access].read_latency = value; break; case ACPI_HMAT_WRITE_LATENCY: - target->hmem_attrs[access].write_latency = value; + target->coord[access].write_latency = value; break; case ACPI_HMAT_ACCESS_BANDWIDTH: - target->hmem_attrs[access].read_bandwidth = value; - target->hmem_attrs[access].write_bandwidth = value; + target->coord[access].read_bandwidth = value; + target->coord[access].write_bandwidth = value; break; case ACPI_HMAT_READ_BANDWIDTH: - target->hmem_attrs[access].read_bandwidth = value; + target->coord[access].read_bandwidth = value; break; case ACPI_HMAT_WRITE_BANDWIDTH: - target->hmem_attrs[access].write_bandwidth = value; + target->coord[access].write_bandwidth = value; break; default: break; @@ -701,7 +701,7 @@ static void hmat_register_target_cache(struct memory_target *target) static void hmat_register_target_perf(struct memory_target *target, int access) { unsigned mem_nid = pxm_to_node(target->memory_pxm); - node_set_perf_attrs(mem_nid, &target->hmem_attrs[access], access); + node_set_perf_attrs(mem_nid, &target->coord[access], access); } static void hmat_register_target_devices(struct memory_target *target) diff --git a/drivers/base/node.c b/drivers/base/node.c index 2cada01c70da..fc0444b617d0 100644 --- a/drivers/base/node.c +++ b/drivers/base/node.c @@ -75,14 +75,14 @@ static BIN_ATTR_RO(cpulist, CPULIST_FILE_MAX_BYTES); * @dev: Device for this memory access class * @list_node: List element in the node's access list * @access: The access class rank - * @hmem_attrs: Heterogeneous memory performance attributes + * @coord: Heterogeneous memory performance coordinates */ struct node_access_nodes { struct device dev; struct list_head list_node; unsigned int access; #ifdef CONFIG_HMEM_REPORTING - struct node_hmem_attrs hmem_attrs; + struct access_coordinate coord; #endif }; #define to_access_nodes(dev) container_of(dev, struct node_access_nodes, dev) @@ -168,7 +168,7 @@ static ssize_t property##_show(struct device *dev, \ char *buf) \ { \ return sysfs_emit(buf, "%u\n", \ - to_access_nodes(dev)->hmem_attrs.property); \ + to_access_nodes(dev)->coord.property); \ } \ static DEVICE_ATTR_RO(property) @@ -188,10 +188,10 @@ static struct attribute *access_attrs[] = { /** * node_set_perf_attrs - Set the performance values for given access class * @nid: Node identifier to be set - * @hmem_attrs: Heterogeneous memory performance attributes + * @coord: Heterogeneous memory performance coordinates * @access: The access class the for the given attributes */ -void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs, +void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord, unsigned int access) { struct node_access_nodes *c; @@ -206,7 +206,7 @@ void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs, if (!c) return; - c->hmem_attrs = *hmem_attrs; + c->coord = *coord; for (i = 0; access_attrs[i] != NULL; i++) { if (sysfs_add_file_to_group(&c->dev.kobj, access_attrs[i], "initiators")) { diff --git a/include/linux/node.h b/include/linux/node.h index 427a5975cf40..25b66d705ee2 100644 --- a/include/linux/node.h +++ b/include/linux/node.h @@ -20,14 +20,14 @@ #include <linux/list.h> /** - * struct node_hmem_attrs - heterogeneous memory performance attributes + * struct access_coordinate - generic performance coordinates container * * @read_bandwidth: Read bandwidth in MB/s * @write_bandwidth: Write bandwidth in MB/s * @read_latency: Read latency in nanoseconds * @write_latency: Write latency in nanoseconds */ -struct node_hmem_attrs { +struct access_coordinate { unsigned int read_bandwidth; unsigned int write_bandwidth; unsigned int read_latency; @@ -65,7 +65,7 @@ struct node_cache_attrs { #ifdef CONFIG_HMEM_REPORTING void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs); -void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs, +void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord, unsigned access); #else static inline void node_add_cache(unsigned int nid, @@ -74,7 +74,7 @@ static inline void node_add_cache(unsigned int nid, } static inline void node_set_perf_attrs(unsigned int nid, - struct node_hmem_attrs *hmem_attrs, + struct access_coordinate *coord, unsigned access) { }