[PATCH 0/4] cxl, acpi/hmat, node: Update CXL access coordinates to node directly

[PATCH 0/4] cxl, acpi/hmat, node: Update CXL access coordinates to node directly

[Dave Jiang]

I plan to take this series through the CXL tree when all the necessary tags
are received.

Andrew or David, can you please ack patch 1/4 if you are okay with it.

Greg or Rafael, please ack patch 2/4 if you are satisfied with it. The helper
function's usage is in patch 3/4.

Rafael, please ack patches 3/4 and 4/4 if you are happy with the changes.

Thank you all!

The series aim to clean up the current CXL memory region hotplug notifier by
removing the update path through HMAT and updating the node access coordinates
directly. With the existing implementation, the CXL memory hotplug notifier
gets called first. It updates the HMAT target access coordinates. And then
the HMAT notifier gets called and create the node sysfs attribs. The new
implemenation flips the callback ordering and directly updates the sysfs
attribs already created in the node and leaves HMAT data structures alone.

# Dave Jiang (4):
#       mm/memory_hotplug: Update comment for hotplug memory callback priorities
#       drivers/base/node: Add a helper function node_update_perf_attrs()
#       cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT
#       acpi/hmat: Remove now unused hmat_update_target_coordinates()
#
#  drivers/acpi/numa/hmat.c  | 34 ----------------------------------
#  drivers/base/node.c       | 39 +++++++++++++++++++++++++++++++++++++++
#  drivers/cxl/core/cdat.c   | 11 -----------
#  drivers/cxl/core/core.h   |  3 ---
#  drivers/cxl/core/region.c | 10 ++--------
#  include/linux/acpi.h      | 12 ------------
#  include/linux/memory.h    |  4 ++--
#  include/linux/node.h      |  8 ++++++++
#  8 files changed, 51 insertions(+), 70 deletions(-)
# </REPLACE>


Dave Jiang (4):
  mm/memory_hotplug: Update comment for hotplug memory callback
    priorities
  drivers/base/node: Add a helper function node_update_perf_attrs()
  cxl, acpi/hmat: Update CXL access coordinates directly instead of
    through HMAT
  acpi/hmat: Remove now unused hmat_update_target_coordinates()

 drivers/acpi/numa/hmat.c  | 34 ----------------------------------
 drivers/base/node.c       | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/cdat.c   | 11 -----------
 drivers/cxl/core/core.h   |  3 ---
 drivers/cxl/core/region.c | 10 ++--------
 include/linux/acpi.h      | 12 ------------
 include/linux/memory.h    |  4 ++--
 include/linux/node.h      |  8 ++++++++
 8 files changed, 51 insertions(+), 70 deletions(-)


base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.50.1

[PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[Dave Jiang]

Add clarification to comment for memory hotplug callback ordering as the
current comment does not provide clear language on which callback happens
first.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 include/linux/memory.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/memory.h b/include/linux/memory.h
index 40eb70ccb09d..02314723e5bd 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -116,7 +116,7 @@ struct mem_section;
 
 /*
  * Priorities for the hotplug memory callback routines (stored in decreasing
- * order in the callback chain)
+ * order in the callback chain). The callback ordering happens from high to low.
  */
 #define DEFAULT_CALLBACK_PRI	0
 #define SLAB_CALLBACK_PRI	1
-- 
2.50.1

[PATCH 2/4] drivers/base/node: Add a helper function node_update_perf_attrs()

[Dave Jiang]

Add helper function node_update_perf_attrs() to allow update of node access
coordinates computed by an external agent such as CXL. The helper allows
updating of coordinates after the attribute being created by HMAT.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/base/node.c  | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/node.h |  8 ++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 3399594136b2..cf395da18c9b 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -248,6 +248,45 @@ void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
 }
 EXPORT_SYMBOL_GPL(node_set_perf_attrs);
 
+/**
+ * node_update_perf_attrs - Update the performance values for given access class
+ * @nid: Node identifier to be updated
+ * @coord: Heterogeneous memory performance coordinates
+ * @access: The access class the for the given attributes
+ */
+void node_update_perf_attrs(unsigned int nid, struct access_coordinate *coord,
+			    enum access_coordinate_class access)
+{
+	struct node_access_nodes *access_node;
+	struct node *node;
+	int i;
+
+	if (WARN_ON_ONCE(!node_online(nid)))
+		return;
+
+	node = node_devices[nid];
+	list_for_each_entry(access_node, &node->access_list, list_node) {
+		if (access_node->access != access)
+			continue;
+
+		access_node->coord = *coord;
+		for (i = 0; access_attrs[i]; i++) {
+			sysfs_notify(&access_node->dev.kobj,
+				     NULL, access_attrs[i]->name);
+		}
+		break;
+	}
+
+	/* When setting CPU access coordinates, update mempolicy */
+	if (access == ACCESS_COORDINATE_CPU) {
+		if (mempolicy_set_node_perf(nid, coord)) {
+			pr_info("failed to set mempolicy attrs for node %d\n",
+				nid);
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(node_update_perf_attrs);
+
 /**
  * struct node_cache_info - Internal tracking for memory node caches
  * @dev:	Device represeting the cache level
diff --git a/include/linux/node.h b/include/linux/node.h
index 2c7529335b21..866e3323f1fd 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -85,6 +85,8 @@ struct node_cache_attrs {
 void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs);
 void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
 			 enum access_coordinate_class access);
+void node_update_perf_attrs(unsigned int nid, struct access_coordinate *coord,
+			    enum access_coordinate_class access);
 #else
 static inline void node_add_cache(unsigned int nid,
 				  struct node_cache_attrs *cache_attrs)
@@ -96,6 +98,12 @@ static inline void node_set_perf_attrs(unsigned int nid,
 				       enum access_coordinate_class access)
 {
 }
+
+static inline void node_update_perf_attrs(unsigned int nid,
+					  struct access_coordinate *coord,
+					  enum access_coordinate_class access)
+{
+}
 #endif
 
 struct node {
-- 
2.50.1

[PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[Dave Jiang]

The current implementation of CXL memory hotplug notifier gets called
before the HMAT memory hotplug notifier. The CXL driver calculates the
access coordinates (bandwidth and latency values) for the CXL end to
end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
memory hotplug notifier writes the access coordinates to the HMAT target
structs. Then the HMAT memory hotplug notifier is called and it creates
the access coordinates for the node sysfs attributes.

The original intent of the 'ext_updated' flag in HMAT handling code was to
stop HMAT memory hotplug callback from clobbering the access coordinates
after CXL has injected its calculated coordinates and replaced the generic
target access coordinates provided by the HMAT table in the HMAT target
structs. However the flag is hacky at best and blocks the updates from
other CXL regions that are onlined in the same node later on. Remove the
'ext_updated' flag usage and just update the access coordinates for the
nodes directly without touching HMAT target data.

The hotplug memory callback ordering is changed. Instead of changing CXL,
move HMAT back so there's room for the levels rather than have CXL share
the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
callback to be executed after the HMAT callback.

With the change, the CXL hotplug memory notifier runs after the HMAT
callback. The HMAT callback will create the node sysfs attributes for
access coordinates. The CXL callback will write the access coordinates to
the now created node sysfs attributes directly and will not pollute the
HMAT target values.

Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
Tested-by: Marc Herbert <marc.herbert@linux.intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c  |  6 ------
 drivers/cxl/core/cdat.c   |  5 -----
 drivers/cxl/core/core.h   |  1 -
 drivers/cxl/core/region.c | 10 ++--------
 include/linux/memory.h    |  2 +-
 5 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 4958301f5417..5d32490dc4ab 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -74,7 +74,6 @@ struct memory_target {
 	struct node_cache_attrs cache_attrs;
 	u8 gen_port_device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
 	bool registered;
-	bool ext_updated;	/* externally updated */
 };
 
 struct memory_initiator {
@@ -391,7 +390,6 @@ int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
 				  coord->read_bandwidth, access);
 	hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
 				  coord->write_bandwidth, access);
-	target->ext_updated = true;
 
 	return 0;
 }
@@ -773,10 +771,6 @@ static void hmat_update_target_attrs(struct memory_target *target,
 	u32 best = 0;
 	int i;
 
-	/* Don't update if an external agent has changed the data.  */
-	if (target->ext_updated)
-		return;
-
 	/* Don't update for generic port if there's no device handle */
 	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
 	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index c0af645425f4..c891fd618cfd 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -1081,8 +1081,3 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
 {
 	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
 }
-
-bool cxl_need_node_perf_attrs_update(int nid)
-{
-	return !acpi_node_backed_by_real_pxm(nid);
-}
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 2669f251d677..a253d308f3c9 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -139,7 +139,6 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
 int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
 int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
 				       enum access_coordinate_class access);
-bool cxl_need_node_perf_attrs_update(int nid);
 int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
 					struct access_coordinate *c);
 
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 71cc42d05248..1580e19f13a5 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2442,14 +2442,8 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
 
 	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
 		if (cxlr->coord[i].read_bandwidth) {
-			rc = 0;
-			if (cxl_need_node_perf_attrs_update(nid))
-				node_set_perf_attrs(nid, &cxlr->coord[i], i);
-			else
-				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
-
-			if (rc == 0)
-				cset++;
+			node_update_perf_attrs(nid, &cxlr->coord[i], i);
+			cset++;
 		}
 	}
 
diff --git a/include/linux/memory.h b/include/linux/memory.h
index 02314723e5bd..b41872c478e3 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -120,8 +120,8 @@ struct mem_section;
  */
 #define DEFAULT_CALLBACK_PRI	0
 #define SLAB_CALLBACK_PRI	1
-#define HMAT_CALLBACK_PRI	2
 #define CXL_CALLBACK_PRI	5
+#define HMAT_CALLBACK_PRI	6
 #define MM_COMPUTE_BATCH_PRI	10
 #define CPUSET_CALLBACK_PRI	10
 #define MEMTIER_HOTPLUG_PRI	100
-- 
2.50.1

[PATCH 4/4] acpi/hmat: Remove now unused hmat_update_target_coordinates()

[Dave Jiang]

Remove deadcode since CXL no longer calls hmat_update_target_coordinates().

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c | 28 ----------------------------
 drivers/cxl/core/cdat.c  |  6 ------
 drivers/cxl/core/core.h  |  2 --
 include/linux/acpi.h     | 12 ------------
 4 files changed, 48 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 5d32490dc4ab..5a36d57289b4 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -367,34 +367,6 @@ static void hmat_update_target_access(struct memory_target *target,
 	}
 }
 
-int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
-				   enum access_coordinate_class access)
-{
-	struct memory_target *target;
-	int pxm;
-
-	if (nid == NUMA_NO_NODE)
-		return -EINVAL;
-
-	pxm = node_to_pxm(nid);
-	guard(mutex)(&target_lock);
-	target = find_mem_target(pxm);
-	if (!target)
-		return -ENODEV;
-
-	hmat_update_target_access(target, ACPI_HMAT_READ_LATENCY,
-				  coord->read_latency, access);
-	hmat_update_target_access(target, ACPI_HMAT_WRITE_LATENCY,
-				  coord->write_latency, access);
-	hmat_update_target_access(target, ACPI_HMAT_READ_BANDWIDTH,
-				  coord->read_bandwidth, access);
-	hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
-				  coord->write_bandwidth, access);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(hmat_update_target_coordinates);
-
 static __init void hmat_add_locality(struct acpi_hmat_locality *hmat_loc)
 {
 	struct memory_locality *loc;
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index c891fd618cfd..bca1ec279651 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -1075,9 +1075,3 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 		cxlr->coord[i].write_bandwidth += perf->coord[i].write_bandwidth;
 	}
 }
-
-int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
-				       enum access_coordinate_class access)
-{
-	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
-}
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index a253d308f3c9..0476c3b648de 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -137,8 +137,6 @@ enum cxl_poison_trace_type {
 
 long cxl_pci_get_latency(struct pci_dev *pdev);
 int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
-int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
-				       enum access_coordinate_class access);
 int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
 					struct access_coordinate *c);
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1c5bb1e887cd..5ff5d99f6ead 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1595,18 +1595,6 @@ static inline void acpi_use_parent_companion(struct device *dev)
 	ACPI_COMPANION_SET(dev, ACPI_COMPANION(dev->parent));
 }
 
-#ifdef CONFIG_ACPI_HMAT
-int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
-				   enum access_coordinate_class access);
-#else
-static inline int hmat_update_target_coordinates(int nid,
-						 struct access_coordinate *coord,
-						 enum access_coordinate_class access)
-{
-	return -EOPNOTSUPP;
-}
-#endif
-
 #ifdef CONFIG_ACPI_NUMA
 bool acpi_node_backed_by_real_pxm(int nid);
 #else
-- 
2.50.1

Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[dan.j.williams]

Dave Jiang wrote:
> The current implementation of CXL memory hotplug notifier gets called
> before the HMAT memory hotplug notifier. The CXL driver calculates the
> access coordinates (bandwidth and latency values) for the CXL end to
> end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
> memory hotplug notifier writes the access coordinates to the HMAT target
> structs. Then the HMAT memory hotplug notifier is called and it creates
> the access coordinates for the node sysfs attributes.

Perhaps summarize quickly here the before and after of sysfs, so people
know if they are impacted by this bug, and backporters can verify they
fixed it?

> The original intent of the 'ext_updated' flag in HMAT handling code was to
> stop HMAT memory hotplug callback from clobbering the access coordinates
> after CXL has injected its calculated coordinates and replaced the generic
> target access coordinates provided by the HMAT table in the HMAT target
> structs. However the flag is hacky at best and blocks the updates from
> other CXL regions that are onlined in the same node later on. Remove the
> 'ext_updated' flag usage and just update the access coordinates for the
> nodes directly without touching HMAT target data.
> 
> The hotplug memory callback ordering is changed. Instead of changing CXL,
> move HMAT back so there's room for the levels rather than have CXL share
> the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
> callback to be executed after the HMAT callback.
> 
> With the change, the CXL hotplug memory notifier runs after the HMAT
> callback. The HMAT callback will create the node sysfs attributes for
> access coordinates. The CXL callback will write the access coordinates to
> the now created node sysfs attributes directly and will not pollute the
> HMAT target values.
> 
> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")

Why that one and not?

067353a46d8c cxl/region: Add memory hotplug notifier for cxl region

It is the ext_updated machinery that is the main problem that messes up
sysfs, right?

...and per the backport concern this should be cc: stable as well.

Other than that you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[Dave Jiang]

On 8/14/25 3:33 PM, dan.j.williams@intel.com wrote:
> Dave Jiang wrote:
>> The current implementation of CXL memory hotplug notifier gets called
>> before the HMAT memory hotplug notifier. The CXL driver calculates the
>> access coordinates (bandwidth and latency values) for the CXL end to
>> end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
>> memory hotplug notifier writes the access coordinates to the HMAT target
>> structs. Then the HMAT memory hotplug notifier is called and it creates
>> the access coordinates for the node sysfs attributes.
> 
> Perhaps summarize quickly here the before and after of sysfs, so people
> know if they are impacted by this bug, and backporters can verify they
> fixed it?

ok

> 
>> The original intent of the 'ext_updated' flag in HMAT handling code was to
>> stop HMAT memory hotplug callback from clobbering the access coordinates
>> after CXL has injected its calculated coordinates and replaced the generic
>> target access coordinates provided by the HMAT table in the HMAT target
>> structs. However the flag is hacky at best and blocks the updates from
>> other CXL regions that are onlined in the same node later on. Remove the
>> 'ext_updated' flag usage and just update the access coordinates for the
>> nodes directly without touching HMAT target data.
>>
>> The hotplug memory callback ordering is changed. Instead of changing CXL,
>> move HMAT back so there's room for the levels rather than have CXL share
>> the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
>> callback to be executed after the HMAT callback.
>>
>> With the change, the CXL hotplug memory notifier runs after the HMAT
>> callback. The HMAT callback will create the node sysfs attributes for
>> access coordinates. The CXL callback will write the access coordinates to
>> the now created node sysfs attributes directly and will not pollute the
>> HMAT target values.
>>
>> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
> 
> Why that one and not?
> 
> 067353a46d8c cxl/region: Add memory hotplug notifier for cxl region

I think I grabbed the wrong line for 'git blame'. 

> 
> It is the ext_updated machinery that is the main problem that messes up
> sysfs, right?
> 
> ...and per the backport concern this should be cc: stable as well.
> 
> Other than that you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[Marc Herbert]

On 2025-08-14 15:33, dan.j.williams@intel.com wrote:

>> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
> 
> Why that one and not?
> 
> 067353a46d8c cxl/region: Add memory hotplug notifier for cxl region
> 
> It is the ext_updated machinery that is the main problem that messes up
> sysfs, right?
> 

For sure 067353a46d8c is where my git bisect unambiguously landed.

Tested-by: Marc Herbert <marc.herbert@linux.intel.com>
(the series as a whole)

Re: [PATCH 2/4] drivers/base/node: Add a helper function node_update_perf_attrs()

[Jonathan Cameron]

On Thu, 14 Aug 2025 10:16:48 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add helper function node_update_perf_attrs() to allow update of node access
> coordinates computed by an external agent such as CXL. The helper allows
> updating of coordinates after the attribute being created by HMAT.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[Jonathan Cameron]

On Thu, 14 Aug 2025 10:16:49 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The current implementation of CXL memory hotplug notifier gets called
> before the HMAT memory hotplug notifier. The CXL driver calculates the
> access coordinates (bandwidth and latency values) for the CXL end to
> end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
> memory hotplug notifier writes the access coordinates to the HMAT target
> structs. Then the HMAT memory hotplug notifier is called and it creates
> the access coordinates for the node sysfs attributes.
> 
> The original intent of the 'ext_updated' flag in HMAT handling code was to
> stop HMAT memory hotplug callback from clobbering the access coordinates
> after CXL has injected its calculated coordinates and replaced the generic
> target access coordinates provided by the HMAT table in the HMAT target
> structs. However the flag is hacky at best and blocks the updates from
> other CXL regions that are onlined in the same node later on. 

After all removed, or when a second region onlined in that node whilst the
first is still online?  In that second case I think we should not update
the access properties as that would surprise anything already using the
earlier one.

Jonathan

> Remove the
> 'ext_updated' flag usage and just update the access coordinates for the
> nodes directly without touching HMAT target data.
> 
> The hotplug memory callback ordering is changed. Instead of changing CXL,
> move HMAT back so there's room for the levels rather than have CXL share
> the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
> callback to be executed after the HMAT callback.
> 
> With the change, the CXL hotplug memory notifier runs after the HMAT
> callback. The HMAT callback will create the node sysfs attributes for
> access coordinates. The CXL callback will write the access coordinates to
> the now created node sysfs attributes directly and will not pollute the
> HMAT target values.
> 
> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
> Tested-by: Marc Herbert <marc.herbert@linux.intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/numa/hmat.c  |  6 ------
>  drivers/cxl/core/cdat.c   |  5 -----
>  drivers/cxl/core/core.h   |  1 -
>  drivers/cxl/core/region.c | 10 ++--------
>  include/linux/memory.h    |  2 +-
>  5 files changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 4958301f5417..5d32490dc4ab 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -74,7 +74,6 @@ struct memory_target {
>  	struct node_cache_attrs cache_attrs;
>  	u8 gen_port_device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
>  	bool registered;
> -	bool ext_updated;	/* externally updated */
>  };
>  
>  struct memory_initiator {
> @@ -391,7 +390,6 @@ int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
>  				  coord->read_bandwidth, access);
>  	hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
>  				  coord->write_bandwidth, access);
> -	target->ext_updated = true;
>  
>  	return 0;
>  }
> @@ -773,10 +771,6 @@ static void hmat_update_target_attrs(struct memory_target *target,
>  	u32 best = 0;
>  	int i;
>  
> -	/* Don't update if an external agent has changed the data.  */
> -	if (target->ext_updated)
> -		return;
> -
>  	/* Don't update for generic port if there's no device handle */
>  	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
>  	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index c0af645425f4..c891fd618cfd 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -1081,8 +1081,3 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>  {
>  	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
>  }
> -
> -bool cxl_need_node_perf_attrs_update(int nid)
> -{
> -	return !acpi_node_backed_by_real_pxm(nid);
> -}
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 2669f251d677..a253d308f3c9 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -139,7 +139,6 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
>  int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
>  int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>  				       enum access_coordinate_class access);
> -bool cxl_need_node_perf_attrs_update(int nid);
>  int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
>  					struct access_coordinate *c);
>  
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 71cc42d05248..1580e19f13a5 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2442,14 +2442,8 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>  
>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
>  		if (cxlr->coord[i].read_bandwidth) {
> -			rc = 0;
> -			if (cxl_need_node_perf_attrs_update(nid))
> -				node_set_perf_attrs(nid, &cxlr->coord[i], i);
> -			else
> -				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
> -
> -			if (rc == 0)
> -				cset++;
> +			node_update_perf_attrs(nid, &cxlr->coord[i], i);
> +			cset++;
>  		}
>  	}
>  
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 02314723e5bd..b41872c478e3 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -120,8 +120,8 @@ struct mem_section;
>   */
>  #define DEFAULT_CALLBACK_PRI	0
>  #define SLAB_CALLBACK_PRI	1
> -#define HMAT_CALLBACK_PRI	2
>  #define CXL_CALLBACK_PRI	5
> +#define HMAT_CALLBACK_PRI	6
>  #define MM_COMPUTE_BATCH_PRI	10
>  #define CPUSET_CALLBACK_PRI	10
>  #define MEMTIER_HOTPLUG_PRI	100

Re: [PATCH 4/4] acpi/hmat: Remove now unused hmat_update_target_coordinates()

[Jonathan Cameron]

On Thu, 14 Aug 2025 10:16:50 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Remove deadcode since CXL no longer calls hmat_update_target_coordinates().
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hard to argue with this one ;)

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[Dave Jiang]

On 8/15/25 6:31 AM, Jonathan Cameron wrote:
> On Thu, 14 Aug 2025 10:16:49 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> The current implementation of CXL memory hotplug notifier gets called
>> before the HMAT memory hotplug notifier. The CXL driver calculates the
>> access coordinates (bandwidth and latency values) for the CXL end to
>> end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
>> memory hotplug notifier writes the access coordinates to the HMAT target
>> structs. Then the HMAT memory hotplug notifier is called and it creates
>> the access coordinates for the node sysfs attributes.
>>
>> The original intent of the 'ext_updated' flag in HMAT handling code was to
>> stop HMAT memory hotplug callback from clobbering the access coordinates
>> after CXL has injected its calculated coordinates and replaced the generic
>> target access coordinates provided by the HMAT table in the HMAT target
>> structs. However the flag is hacky at best and blocks the updates from
>> other CXL regions that are onlined in the same node later on. 
> 
> After all removed, or when a second region onlined in that node whilst the
> first is still online?  In that second case I think we should not update
> the access properties as that would surprise anything already using the
> earlier one.

Are you thinking we'll need some sort of ref counting on the node?

DJ
> 
> Jonathan
> 
>> Remove the
>> 'ext_updated' flag usage and just update the access coordinates for the
>> nodes directly without touching HMAT target data.
>>
>> The hotplug memory callback ordering is changed. Instead of changing CXL,
>> move HMAT back so there's room for the levels rather than have CXL share
>> the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
>> callback to be executed after the HMAT callback.
>>
>> With the change, the CXL hotplug memory notifier runs after the HMAT
>> callback. The HMAT callback will create the node sysfs attributes for
>> access coordinates. The CXL callback will write the access coordinates to
>> the now created node sysfs attributes directly and will not pollute the
>> HMAT target values.
>>
>> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
>> Tested-by: Marc Herbert <marc.herbert@linux.intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/acpi/numa/hmat.c  |  6 ------
>>  drivers/cxl/core/cdat.c   |  5 -----
>>  drivers/cxl/core/core.h   |  1 -
>>  drivers/cxl/core/region.c | 10 ++--------
>>  include/linux/memory.h    |  2 +-
>>  5 files changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
>> index 4958301f5417..5d32490dc4ab 100644
>> --- a/drivers/acpi/numa/hmat.c
>> +++ b/drivers/acpi/numa/hmat.c
>> @@ -74,7 +74,6 @@ struct memory_target {
>>  	struct node_cache_attrs cache_attrs;
>>  	u8 gen_port_device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
>>  	bool registered;
>> -	bool ext_updated;	/* externally updated */
>>  };
>>  
>>  struct memory_initiator {
>> @@ -391,7 +390,6 @@ int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
>>  				  coord->read_bandwidth, access);
>>  	hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
>>  				  coord->write_bandwidth, access);
>> -	target->ext_updated = true;
>>  
>>  	return 0;
>>  }
>> @@ -773,10 +771,6 @@ static void hmat_update_target_attrs(struct memory_target *target,
>>  	u32 best = 0;
>>  	int i;
>>  
>> -	/* Don't update if an external agent has changed the data.  */
>> -	if (target->ext_updated)
>> -		return;
>> -
>>  	/* Don't update for generic port if there's no device handle */
>>  	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
>>  	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index c0af645425f4..c891fd618cfd 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -1081,8 +1081,3 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>>  {
>>  	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
>>  }
>> -
>> -bool cxl_need_node_perf_attrs_update(int nid)
>> -{
>> -	return !acpi_node_backed_by_real_pxm(nid);
>> -}
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 2669f251d677..a253d308f3c9 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -139,7 +139,6 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
>>  int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
>>  int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>>  				       enum access_coordinate_class access);
>> -bool cxl_need_node_perf_attrs_update(int nid);
>>  int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
>>  					struct access_coordinate *c);
>>  
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 71cc42d05248..1580e19f13a5 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -2442,14 +2442,8 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>>  
>>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
>>  		if (cxlr->coord[i].read_bandwidth) {
>> -			rc = 0;
>> -			if (cxl_need_node_perf_attrs_update(nid))
>> -				node_set_perf_attrs(nid, &cxlr->coord[i], i);
>> -			else
>> -				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
>> -
>> -			if (rc == 0)
>> -				cset++;
>> +			node_update_perf_attrs(nid, &cxlr->coord[i], i);
>> +			cset++;
>>  		}
>>  	}
>>  
>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>> index 02314723e5bd..b41872c478e3 100644
>> --- a/include/linux/memory.h
>> +++ b/include/linux/memory.h
>> @@ -120,8 +120,8 @@ struct mem_section;
>>   */
>>  #define DEFAULT_CALLBACK_PRI	0
>>  #define SLAB_CALLBACK_PRI	1
>> -#define HMAT_CALLBACK_PRI	2
>>  #define CXL_CALLBACK_PRI	5
>> +#define HMAT_CALLBACK_PRI	6
>>  #define MM_COMPUTE_BATCH_PRI	10
>>  #define CPUSET_CALLBACK_PRI	10
>>  #define MEMTIER_HOTPLUG_PRI	100
> 

Re: [PATCH 3/4] cxl, acpi/hmat: Update CXL access coordinates directly instead of through HMAT

[Jonathan Cameron]

On Fri, 15 Aug 2025 08:35:31 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 8/15/25 6:31 AM, Jonathan Cameron wrote:
> > On Thu, 14 Aug 2025 10:16:49 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> The current implementation of CXL memory hotplug notifier gets called
> >> before the HMAT memory hotplug notifier. The CXL driver calculates the
> >> access coordinates (bandwidth and latency values) for the CXL end to
> >> end path (i.e. CPU to endpoint). When the CXL region is onlined, the CXL
> >> memory hotplug notifier writes the access coordinates to the HMAT target
> >> structs. Then the HMAT memory hotplug notifier is called and it creates
> >> the access coordinates for the node sysfs attributes.
> >>
> >> The original intent of the 'ext_updated' flag in HMAT handling code was to
> >> stop HMAT memory hotplug callback from clobbering the access coordinates
> >> after CXL has injected its calculated coordinates and replaced the generic
> >> target access coordinates provided by the HMAT table in the HMAT target
> >> structs. However the flag is hacky at best and blocks the updates from
> >> other CXL regions that are onlined in the same node later on.   
> > 
> > After all removed, or when a second region onlined in that node whilst the
> > first is still online?  In that second case I think we should not update
> > the access properties as that would surprise anything already using the
> > earlier one.  
> 
> Are you thinking we'll need some sort of ref counting on the node?

Was more a question on the intent, but indeed some sort of refcount would be
needed to tear down cleanly. Might get messy though as what do we do if
two regions are added with different characteristics then the first one
is removed. Do we update?

Meh. This is a corner case and if we get different properties in one CFMWS
as a common thing then we need to figure out how to spin more NUMA nodes.
So I'm not sure this isn't a 'won't fix until the case turns out to matter'
thing.

Jonathan

> 
> DJ
> > 
> > Jonathan
> >   
> >> Remove the
> >> 'ext_updated' flag usage and just update the access coordinates for the
> >> nodes directly without touching HMAT target data.
> >>
> >> The hotplug memory callback ordering is changed. Instead of changing CXL,
> >> move HMAT back so there's room for the levels rather than have CXL share
> >> the same level as SLAB_CALLBACK_PRI. The change will resulting in the CXL
> >> callback to be executed after the HMAT callback.
> >>
> >> With the change, the CXL hotplug memory notifier runs after the HMAT
> >> callback. The HMAT callback will create the node sysfs attributes for
> >> access coordinates. The CXL callback will write the access coordinates to
> >> the now created node sysfs attributes directly and will not pollute the
> >> HMAT target values.
> >>
> >> Fixes: debdce20c4f2 ("cxl/region: Deal with numa nodes not enumerated by SRAT")
> >> Tested-by: Marc Herbert <marc.herbert@linux.intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>  drivers/acpi/numa/hmat.c  |  6 ------
> >>  drivers/cxl/core/cdat.c   |  5 -----
> >>  drivers/cxl/core/core.h   |  1 -
> >>  drivers/cxl/core/region.c | 10 ++--------
> >>  include/linux/memory.h    |  2 +-
> >>  5 files changed, 3 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> >> index 4958301f5417..5d32490dc4ab 100644
> >> --- a/drivers/acpi/numa/hmat.c
> >> +++ b/drivers/acpi/numa/hmat.c
> >> @@ -74,7 +74,6 @@ struct memory_target {
> >>  	struct node_cache_attrs cache_attrs;
> >>  	u8 gen_port_device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
> >>  	bool registered;
> >> -	bool ext_updated;	/* externally updated */
> >>  };
> >>  
> >>  struct memory_initiator {
> >> @@ -391,7 +390,6 @@ int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
> >>  				  coord->read_bandwidth, access);
> >>  	hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
> >>  				  coord->write_bandwidth, access);
> >> -	target->ext_updated = true;
> >>  
> >>  	return 0;
> >>  }
> >> @@ -773,10 +771,6 @@ static void hmat_update_target_attrs(struct memory_target *target,
> >>  	u32 best = 0;
> >>  	int i;
> >>  
> >> -	/* Don't update if an external agent has changed the data.  */
> >> -	if (target->ext_updated)
> >> -		return;
> >> -
> >>  	/* Don't update for generic port if there's no device handle */
> >>  	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
> >>  	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> >> index c0af645425f4..c891fd618cfd 100644
> >> --- a/drivers/cxl/core/cdat.c
> >> +++ b/drivers/cxl/core/cdat.c
> >> @@ -1081,8 +1081,3 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
> >>  {
> >>  	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
> >>  }
> >> -
> >> -bool cxl_need_node_perf_attrs_update(int nid)
> >> -{
> >> -	return !acpi_node_backed_by_real_pxm(nid);
> >> -}
> >> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> >> index 2669f251d677..a253d308f3c9 100644
> >> --- a/drivers/cxl/core/core.h
> >> +++ b/drivers/cxl/core/core.h
> >> @@ -139,7 +139,6 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
> >>  int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
> >>  int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
> >>  				       enum access_coordinate_class access);
> >> -bool cxl_need_node_perf_attrs_update(int nid);
> >>  int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
> >>  					struct access_coordinate *c);
> >>  
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 71cc42d05248..1580e19f13a5 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -2442,14 +2442,8 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
> >>  
> >>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
> >>  		if (cxlr->coord[i].read_bandwidth) {
> >> -			rc = 0;
> >> -			if (cxl_need_node_perf_attrs_update(nid))
> >> -				node_set_perf_attrs(nid, &cxlr->coord[i], i);
> >> -			else
> >> -				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
> >> -
> >> -			if (rc == 0)
> >> -				cset++;
> >> +			node_update_perf_attrs(nid, &cxlr->coord[i], i);
> >> +			cset++;
> >>  		}
> >>  	}
> >>  
> >> diff --git a/include/linux/memory.h b/include/linux/memory.h
> >> index 02314723e5bd..b41872c478e3 100644
> >> --- a/include/linux/memory.h
> >> +++ b/include/linux/memory.h
> >> @@ -120,8 +120,8 @@ struct mem_section;
> >>   */
> >>  #define DEFAULT_CALLBACK_PRI	0
> >>  #define SLAB_CALLBACK_PRI	1
> >> -#define HMAT_CALLBACK_PRI	2
> >>  #define CXL_CALLBACK_PRI	5
> >> +#define HMAT_CALLBACK_PRI	6
> >>  #define MM_COMPUTE_BATCH_PRI	10
> >>  #define CPUSET_CALLBACK_PRI	10
> >>  #define MEMTIER_HOTPLUG_PRI	100  
> >   
> 
> 

Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[David Hildenbrand]

On 14.08.25 19:16, Dave Jiang wrote:
> Add clarification to comment for memory hotplug callback ordering as the
> current comment does not provide clear language on which callback happens
> first.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   include/linux/memory.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 40eb70ccb09d..02314723e5bd 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -116,7 +116,7 @@ struct mem_section;
>   
>   /*
>    * Priorities for the hotplug memory callback routines (stored in decreasing
> - * order in the callback chain)
> + * order in the callback chain). The callback ordering happens from high to low.
>    */
>   #define DEFAULT_CALLBACK_PRI	0
>   #define SLAB_CALLBACK_PRI	1

"stored in decreasing order in the callback chain"

is pretty clear? It's a chain after all that gets called.

-- 
Cheers

David / dhildenb

Re: [PATCH 2/4] drivers/base/node: Add a helper function node_update_perf_attrs()

[David Hildenbrand]

On 14.08.25 19:16, Dave Jiang wrote:
> Add helper function node_update_perf_attrs() to allow update of node access
> coordinates computed by an external agent such as CXL. The helper allows
> updating of coordinates after the attribute being created by HMAT.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/base/node.c  | 39 +++++++++++++++++++++++++++++++++++++++
>   include/linux/node.h |  8 ++++++++
>   2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 3399594136b2..cf395da18c9b 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -248,6 +248,45 @@ void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
>   }
>   EXPORT_SYMBOL_GPL(node_set_perf_attrs);
>   
> +/**
> + * node_update_perf_attrs - Update the performance values for given access class
> + * @nid: Node identifier to be updated
> + * @coord: Heterogeneous memory performance coordinates
> + * @access: The access class the for the given attributes

"the for": there is probably something missing

> + */
> +void node_update_perf_attrs(unsigned int nid, struct access_coordinate *coord,
> +			    enum access_coordinate_class access)
> +{
> +	struct node_access_nodes *access_node;
> +	struct node *node;
> +	int i;
> +
> +	if (WARN_ON_ONCE(!node_online(nid)))
> +		return;
> +
> +	node = node_devices[nid];
> +	list_for_each_entry(access_node, &node->access_list, list_node) {
> +		if (access_node->access != access)
> +			continue;
> +
> +		access_node->coord = *coord;
> +		for (i = 0; access_attrs[i]; i++) {
> +			sysfs_notify(&access_node->dev.kobj,
> +				     NULL, access_attrs[i]->name);
> +		}
> +		break;
> +	}
> +
> +	/* When setting CPU access coordinates, update mempolicy */
> +	if (access == ACCESS_COORDINATE_CPU) {
> +		if (mempolicy_set_node_perf(nid, coord)) {
> +			pr_info("failed to set mempolicy attrs for node %d\n",
> +				nid);
> +		}
if (access == ACCESS_COORDINATE_CPU &&
     mempolicy_set_node_perf(nid, coord))
	pr_info("failed to set mempolicy attrs for node %d\n", nid);

or

if (access != ACCESS_COORDINATE_CPU)
	return
if (mempolicy_set_node_perf(nid, coord))
	pr_info("failed to set mempolicy attrs for node %d\n", nid);


With both things sorted

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[Dave Jiang]

On 8/16/25 12:29 AM, David Hildenbrand wrote:
> On 14.08.25 19:16, Dave Jiang wrote:
>> Add clarification to comment for memory hotplug callback ordering as the
>> current comment does not provide clear language on which callback happens
>> first.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   include/linux/memory.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>> index 40eb70ccb09d..02314723e5bd 100644
>> --- a/include/linux/memory.h
>> +++ b/include/linux/memory.h
>> @@ -116,7 +116,7 @@ struct mem_section;
>>     /*
>>    * Priorities for the hotplug memory callback routines (stored in decreasing
>> - * order in the callback chain)
>> + * order in the callback chain). The callback ordering happens from high to low.
>>    */
>>   #define DEFAULT_CALLBACK_PRI    0
>>   #define SLAB_CALLBACK_PRI    1
> 
> "stored in decreasing order in the callback chain"
> 
> is pretty clear? It's a chain after all that gets called.

I can drop the patch. For some reason when I read it I'm thinking the opposite, and when Marc was also confused I started questioning things.

> 

Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[Marc Herbert]

On 2025-08-18 07:08, Dave Jiang wrote:
> 
> 
> On 8/16/25 12:29 AM, David Hildenbrand wrote:
>> On 14.08.25 19:16, Dave Jiang wrote:
>>> Add clarification to comment for memory hotplug callback ordering as the
>>> current comment does not provide clear language on which callback happens
>>> first.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>   include/linux/memory.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>> index 40eb70ccb09d..02314723e5bd 100644
>>> --- a/include/linux/memory.h
>>> +++ b/include/linux/memory.h
>>> @@ -116,7 +116,7 @@ struct mem_section;
>>>     /*
>>>    * Priorities for the hotplug memory callback routines (stored in decreasing
>>> - * order in the callback chain)
>>> + * order in the callback chain). The callback ordering happens from high to low.
>>>    */
>>>   #define DEFAULT_CALLBACK_PRI    0
>>>   #define SLAB_CALLBACK_PRI    1
>>
>> "stored in decreasing order in the callback chain"
>>
>> is pretty clear? It's a chain after all that gets called.
> 
> I can drop the patch. For some reason when I read it I'm thinking the opposite, and when Marc was also confused I started questioning things.
> 

I think we both found the current comment confusing (even together!)
because:

- It very briefly alludes to an implementation detail (the chain)
  without really getting into detail. A "chain" could be bi-directional;
  why not? This one is... "most likely" not. Doubt.

- Higher priorities can have lower numbers, example: "P1 bugs". Not the
  case here, but this "double standards" situation makes _all_
  priorities suspicious and confusing.

- Constants that come first in the file are called last.


I would go further than Dave and also drop the "chain" implementation
detail because it makes the reader to think too much.  Not needed and
distracting at this particular point in the file.

/*
 * Priorities for the hotplug memory callback routines.
 * Invoked from high to low.
 */

  => Hopefully zero cognitive load.

Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[David Hildenbrand]

On 19.08.25 05:14, Marc Herbert wrote:
> 
> 
> On 2025-08-18 07:08, Dave Jiang wrote:
>>
>>
>> On 8/16/25 12:29 AM, David Hildenbrand wrote:
>>> On 14.08.25 19:16, Dave Jiang wrote:
>>>> Add clarification to comment for memory hotplug callback ordering as the
>>>> current comment does not provide clear language on which callback happens
>>>> first.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>    include/linux/memory.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>>> index 40eb70ccb09d..02314723e5bd 100644
>>>> --- a/include/linux/memory.h
>>>> +++ b/include/linux/memory.h
>>>> @@ -116,7 +116,7 @@ struct mem_section;
>>>>      /*
>>>>     * Priorities for the hotplug memory callback routines (stored in decreasing
>>>> - * order in the callback chain)
>>>> + * order in the callback chain). The callback ordering happens from high to low.
>>>>     */
>>>>    #define DEFAULT_CALLBACK_PRI    0
>>>>    #define SLAB_CALLBACK_PRI    1
>>>
>>> "stored in decreasing order in the callback chain"
>>>
>>> is pretty clear? It's a chain after all that gets called.
>>
>> I can drop the patch. For some reason when I read it I'm thinking the opposite, and when Marc was also confused I started questioning things.
>>
> 
> I think we both found the current comment confusing (even together!)
> because:
> 
> - It very briefly alludes to an implementation detail (the chain)
>    without really getting into detail. A "chain" could be bi-directional;
>    why not? This one is... "most likely" not. Doubt.
> 

Please note that the memory notifier is really just using the generic 
*notifier chain* mechanism as documented in include/linux/notifier.h.

Here is a good summary of that mechanism. I don't quite agree with the 
"implementation detail" part, but that information might indeed not be 
required here.

https://0xax.gitbooks.io/linux-insides/content/Concepts/linux-cpu-4.html

> - Higher priorities can have lower numbers, example: "P1 bugs". Not the
>    case here, but this "double standards" situation makes _all_
>    priorities suspicious and confusing.
> 

Yes, "priorities" are handled differently in different context.

> - Constants that come first in the file are called last.

Yes, but that part makes perfect sense to me.
  > I would go further than Dave and also drop the "chain" implementation
> detail because it makes the reader to think too much.  Not needed and
> distracting at this particular point in the file.

  > /*
>   * Priorities for the hotplug memory callback routines.
>   * Invoked from high to low.
>   */
> 
>    => Hopefully zero cognitive load.

Still confusion about how a high priority translates to a number, maybe?

/*
  * Priorities for the hotplug memory callback routines. Invoked from
  * high to low. Higher priorities corresponds to higher numbers.
  */

-- 
Cheers

David / dhildenb

Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[Dave Jiang]

On 8/19/25 2:18 AM, David Hildenbrand wrote:
> On 19.08.25 05:14, Marc Herbert wrote:
>>
>>
>> On 2025-08-18 07:08, Dave Jiang wrote:
>>>
>>>
>>> On 8/16/25 12:29 AM, David Hildenbrand wrote:
>>>> On 14.08.25 19:16, Dave Jiang wrote:
>>>>> Add clarification to comment for memory hotplug callback ordering as the
>>>>> current comment does not provide clear language on which callback happens
>>>>> first.
>>>>>
>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>> ---
>>>>>    include/linux/memory.h | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>>>> index 40eb70ccb09d..02314723e5bd 100644
>>>>> --- a/include/linux/memory.h
>>>>> +++ b/include/linux/memory.h
>>>>> @@ -116,7 +116,7 @@ struct mem_section;
>>>>>      /*
>>>>>     * Priorities for the hotplug memory callback routines (stored in decreasing
>>>>> - * order in the callback chain)
>>>>> + * order in the callback chain). The callback ordering happens from high to low.
>>>>>     */
>>>>>    #define DEFAULT_CALLBACK_PRI    0
>>>>>    #define SLAB_CALLBACK_PRI    1
>>>>
>>>> "stored in decreasing order in the callback chain"
>>>>
>>>> is pretty clear? It's a chain after all that gets called.
>>>
>>> I can drop the patch. For some reason when I read it I'm thinking the opposite, and when Marc was also confused I started questioning things.
>>>
>>
>> I think we both found the current comment confusing (even together!)
>> because:
>>
>> - It very briefly alludes to an implementation detail (the chain)
>>    without really getting into detail. A "chain" could be bi-directional;
>>    why not? This one is... "most likely" not. Doubt.
>>
> 
> Please note that the memory notifier is really just using the generic *notifier chain* mechanism as documented in include/linux/notifier.h.
> 
> Here is a good summary of that mechanism. I don't quite agree with the "implementation detail" part, but that information might indeed not be required here.
> 
> https://0xax.gitbooks.io/linux-insides/content/Concepts/linux-cpu-4.html
> 
>> - Higher priorities can have lower numbers, example: "P1 bugs". Not the
>>    case here, but this "double standards" situation makes _all_
>>    priorities suspicious and confusing.
>>
> 
> Yes, "priorities" are handled differently in different context.
> 
>> - Constants that come first in the file are called last.
> 
> Yes, but that part makes perfect sense to me.
>  > I would go further than Dave and also drop the "chain" implementation
>> detail because it makes the reader to think too much.  Not needed and
>> distracting at this particular point in the file.
> 
>  > /*
>>   * Priorities for the hotplug memory callback routines.
>>   * Invoked from high to low.
>>   */
>>
>>    => Hopefully zero cognitive load.
> 
> Still confusion about how a high priority translates to a number, maybe?
> 
> /*
>  * Priorities for the hotplug memory callback routines. Invoked from
>  * high to low. Higher priorities corresponds to higher numbers.
>  */
> 

This reads clear to me. I will adopt this comment if there are no additional objections. 

Re: [PATCH 2/4] drivers/base/node: Add a helper function node_update_perf_attrs()

[Dave Jiang]

On 8/18/25 2:49 AM, David Hildenbrand wrote:
> On 14.08.25 19:16, Dave Jiang wrote:
>> Add helper function node_update_perf_attrs() to allow update of node access
>> coordinates computed by an external agent such as CXL. The helper allows
>> updating of coordinates after the attribute being created by HMAT.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/base/node.c  | 39 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/node.h |  8 ++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 3399594136b2..cf395da18c9b 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -248,6 +248,45 @@ void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
>>   }
>>   EXPORT_SYMBOL_GPL(node_set_perf_attrs);
>>   +/**
>> + * node_update_perf_attrs - Update the performance values for given access class
>> + * @nid: Node identifier to be updated
>> + * @coord: Heterogeneous memory performance coordinates
>> + * @access: The access class the for the given attributes
> 
> "the for": there is probably something missing

looks like extra 'the'

> 
>> + */
>> +void node_update_perf_attrs(unsigned int nid, struct access_coordinate *coord,
>> +                enum access_coordinate_class access)
>> +{
>> +    struct node_access_nodes *access_node;
>> +    struct node *node;
>> +    int i;
>> +
>> +    if (WARN_ON_ONCE(!node_online(nid)))
>> +        return;
>> +
>> +    node = node_devices[nid];
>> +    list_for_each_entry(access_node, &node->access_list, list_node) {
>> +        if (access_node->access != access)
>> +            continue;
>> +
>> +        access_node->coord = *coord;
>> +        for (i = 0; access_attrs[i]; i++) {
>> +            sysfs_notify(&access_node->dev.kobj,
>> +                     NULL, access_attrs[i]->name);
>> +        }
>> +        break;
>> +    }
>> +
>> +    /* When setting CPU access coordinates, update mempolicy */
>> +    if (access == ACCESS_COORDINATE_CPU) {
>> +        if (mempolicy_set_node_perf(nid, coord)) {
>> +            pr_info("failed to set mempolicy attrs for node %d\n",
>> +                nid);
>> +        }
> if (access == ACCESS_COORDINATE_CPU &&
>     mempolicy_set_node_perf(nid, coord))
>     pr_info("failed to set mempolicy attrs for node %d\n", nid);
> 
> or
> 
> if (access != ACCESS_COORDINATE_CPU)
>     return
> if (mempolicy_set_node_perf(nid, coord))
>     pr_info("failed to set mempolicy attrs for node %d\n", nid);
> 
ok


> 
> With both things sorted
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 
Thanks for the ack

Re: [PATCH 1/4] mm/memory_hotplug: Update comment for hotplug memory callback priorities

[David Hildenbrand]

On 19.08.25 17:39, Dave Jiang wrote:
> 
> 
> On 8/19/25 2:18 AM, David Hildenbrand wrote:
>> On 19.08.25 05:14, Marc Herbert wrote:
>>>
>>>
>>> On 2025-08-18 07:08, Dave Jiang wrote:
>>>>
>>>>
>>>> On 8/16/25 12:29 AM, David Hildenbrand wrote:
>>>>> On 14.08.25 19:16, Dave Jiang wrote:
>>>>>> Add clarification to comment for memory hotplug callback ordering as the
>>>>>> current comment does not provide clear language on which callback happens
>>>>>> first.
>>>>>>
>>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>>> ---
>>>>>>     include/linux/memory.h | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>>>>> index 40eb70ccb09d..02314723e5bd 100644
>>>>>> --- a/include/linux/memory.h
>>>>>> +++ b/include/linux/memory.h
>>>>>> @@ -116,7 +116,7 @@ struct mem_section;
>>>>>>       /*
>>>>>>      * Priorities for the hotplug memory callback routines (stored in decreasing
>>>>>> - * order in the callback chain)
>>>>>> + * order in the callback chain). The callback ordering happens from high to low.
>>>>>>      */
>>>>>>     #define DEFAULT_CALLBACK_PRI    0
>>>>>>     #define SLAB_CALLBACK_PRI    1
>>>>>
>>>>> "stored in decreasing order in the callback chain"
>>>>>
>>>>> is pretty clear? It's a chain after all that gets called.
>>>>
>>>> I can drop the patch. For some reason when I read it I'm thinking the opposite, and when Marc was also confused I started questioning things.
>>>>
>>>
>>> I think we both found the current comment confusing (even together!)
>>> because:
>>>
>>> - It very briefly alludes to an implementation detail (the chain)
>>>     without really getting into detail. A "chain" could be bi-directional;
>>>     why not? This one is... "most likely" not. Doubt.
>>>
>>
>> Please note that the memory notifier is really just using the generic *notifier chain* mechanism as documented in include/linux/notifier.h.
>>
>> Here is a good summary of that mechanism. I don't quite agree with the "implementation detail" part, but that information might indeed not be required here.
>>
>> https://0xax.gitbooks.io/linux-insides/content/Concepts/linux-cpu-4.html
>>
>>> - Higher priorities can have lower numbers, example: "P1 bugs". Not the
>>>     case here, but this "double standards" situation makes _all_
>>>     priorities suspicious and confusing.
>>>
>>
>> Yes, "priorities" are handled differently in different context.
>>
>>> - Constants that come first in the file are called last.
>>
>> Yes, but that part makes perfect sense to me.
>>   > I would go further than Dave and also drop the "chain" implementation
>>> detail because it makes the reader to think too much.  Not needed and
>>> distracting at this particular point in the file.
>>
>>   > /*
>>>    * Priorities for the hotplug memory callback routines.
>>>    * Invoked from high to low.
>>>    */
>>>
>>>     => Hopefully zero cognitive load.
>>
>> Still confusion about how a high priority translates to a number, maybe?
>>
>> /*
>>   * Priorities for the hotplug memory callback routines. Invoked from
>>   * high to low. Higher priorities corresponds to higher numbers.
>>   */
>>
> 
> This reads clear to me. I will adopt this comment if there are no additional objections.


Feel free to add

Acked-by: David Hildenbrand <david@redhat.com>


-- 
Cheers

David / dhildenb