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/[email protected]/
Link: https://lore.kernel.org/linux-cxl/[email protected]/
Suggested-by: Dan Williams <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Dave Jiang <[email protected]>
---
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
v2:
- Add additional descriptions from Dan on reasoning behind using
access_coordinate. (Jonathan)
---
drivers/acpi/numa/hmat.c | 20 ++++++++++----------
drivers/base/node.c | 12 ++++++------
include/linux/node.h | 8 ++++----
3 files changed, 20 insertions(+), 20 deletions(-)
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)
{
}
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/[email protected]/
> Link: https://lore.kernel.org/linux-cxl/[email protected]/
> Suggested-by: Dan Williams <[email protected]>
> Reviewed-by: Dan Williams <[email protected]>
> Signed-off-by: Dave Jiang <[email protected]>
>
> ---
>
> 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/[email protected]/
>> Link: https://lore.kernel.org/linux-cxl/[email protected]/
>> Suggested-by: Dan Williams <[email protected]>
>> Reviewed-by: Dan Williams <[email protected]>
>> Signed-off-by: Dave Jiang <[email protected]>
>>
>> ---
>>
>> 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/[email protected]/
> Link: https://lore.kernel.org/linux-cxl/[email protected]/
> Suggested-by: Dan Williams <[email protected]>
> Reviewed-by: Dan Williams <[email protected]>
> Signed-off-by: Dave Jiang <[email protected]>
>
> ---
>
> 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 <[email protected]>