From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: <linux-kernel@vger.kernel.org>, <gregkh@linuxfoundation.org>,
<rafael@kernel.org>, <linux-acpi@vger.kernel.org>,
Dan Williams <dan.j.williams@intel.com>,
<linux-cxl@vger.kernel.org>
Subject: Re: [PATCH] base/node / acpi: Change 'node_hmem_attrs' to 'access_coordinates'
Date: Wed, 10 May 2023 17:14:45 +0100 [thread overview]
Message-ID: <20230510171445.00004cea@Huawei.com> (raw)
In-Reply-To: <168332248685.2190392.1983307884583782116.stgit@djiang5-mobl3>
On Fri, 05 May 2023 14:34:46 -0700
Dave Jiang <dave.jiang@intel.com> 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.
Not that it hugely matters, but why the term "coordinates"?
Looks like Dan used that term, but I've not come across it being applied
in this circumstances and it isn't a case of being immediately obvious
to me what it means.
If it is just another vague entry in kernel word soup then I don't really
mind the term, but nice to give some reasoning in patch description.
Patch otherwise looks fine to me.
Jonathan
>
> [1]: http://lore.kernel.org/r/64471313421f7_1b66294d5@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>
> ---
> 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)
> {
> }
>
>
next prev parent reply other threads:[~2023-05-10 16:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-05 21:34 [PATCH] base/node / acpi: Change 'node_hmem_attrs' to 'access_coordinates' Dave Jiang
2023-05-10 16:14 ` Jonathan Cameron [this message]
2023-05-12 15:58 ` Dan Williams
2023-05-12 16:47 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230510171445.00004cea@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox