[PATCH v3 0/3] cxl/region: Support to calculate memory tier abstract distance

[PATCH v3 0/3] cxl/region: Support to calculate memory tier abstract distance

[Huang Ying]

This series add support to calculate memory tier abstract distance for
the node backed by a cxl region.

[2/3] implements the feature.  [1/3] fixes a race condition of
dependency code.  [3/3] does some code simplification.

Changes:

v3:

- Collected acked-by from Dan, Thanks!

- Added a race fixing patch [1/3].

- Revised patch description of 2/3 (Thanks Alison!)

- Added missing unregister_mt_adistance_algorithm() call.

- Added a code simplification patch [3/3] (Thanks Alison!)

- Link to v2: https://lore.kernel.org/linux-cxl/20240611055423.470574-1-ying.huang@intel.com/

v2:

- Added comments to struct cxl_region and minor fixes (Thanks Jonathan!)

- Link to v1: https://lore.kernel.org/linux-cxl/20240531024852.282767-1-ying.huang@intel.com/


--
Best Regards,
Huang, Ying

[PATCH v3 1/3] cxl/region: Fix a race condition in memory hotplug notifier

[Huang Ying]

In the memory hotplug notifier function of the CXL region,
cxl_region_perf_attrs_callback(), the node ID is obtained by checking
the host address range of the region. However, the address range
information is not available when the region is registered in
devm_cxl_add_region(). Additionally, this information may be removed
or added under the protection of cxl_region_rwsem during runtime. If
the memory notifier is called for nodes other than that backed by the
region, a race condition may occur, potentially leading to a NULL
dereference or an invalid address range.

The race condition is addressed by checking the availability of the
address range information under the protection of cxl_region_rwsem. To
enhance code readability and use guard(), the relevant code has been
moved into a newly added function: cxl_region_nid().

Fixes: 067353a46d8c ("cxl/region: Add memory hotplug notifier for cxl region")
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Bharata B Rao <bharata@amd.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/region.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3c2b6144be23..51aeef2c012c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2304,14 +2304,25 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
 	return true;
 }
 
+static int cxl_region_nid(struct cxl_region *cxlr)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_endpoint_decoder *cxled;
+	struct cxl_decoder *cxld;
+
+	guard(rwsem_read)(&cxl_region_rwsem);
+	cxled = p->targets[0];
+	if (!cxled)
+		return NUMA_NO_NODE;
+	cxld = &cxled->cxld;
+	return phys_to_target_node(cxld->hpa_range.start);
+}
+
 static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
 					  unsigned long action, void *arg)
 {
 	struct cxl_region *cxlr = container_of(nb, struct cxl_region,
 					       memory_notifier);
-	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_endpoint_decoder *cxled = p->targets[0];
-	struct cxl_decoder *cxld = &cxled->cxld;
 	struct memory_notify *mnb = arg;
 	int nid = mnb->status_change_nid;
 	int region_nid;
@@ -2319,7 +2330,7 @@ static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
 	if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
 		return NOTIFY_DONE;
 
-	region_nid = phys_to_target_node(cxld->hpa_range.start);
+	region_nid = cxl_region_nid(cxlr);
 	if (nid != region_nid)
 		return NOTIFY_DONE;
 
-- 
2.39.2

Re: [PATCH v3 1/3] cxl/region: Fix a race condition in memory hotplug notifier

[Jonathan Cameron]

On Tue, 18 Jun 2024 16:46:37 +0800
Huang Ying <ying.huang@intel.com> wrote:

> In the memory hotplug notifier function of the CXL region,
> cxl_region_perf_attrs_callback(), the node ID is obtained by checking
> the host address range of the region. However, the address range
> information is not available when the region is registered in
> devm_cxl_add_region(). Additionally, this information may be removed
> or added under the protection of cxl_region_rwsem during runtime. If
> the memory notifier is called for nodes other than that backed by the
> region, a race condition may occur, potentially leading to a NULL
> dereference or an invalid address range.
> 
> The race condition is addressed by checking the availability of the
> address range information under the protection of cxl_region_rwsem. To
> enhance code readability and use guard(), the relevant code has been
> moved into a newly added function: cxl_region_nid().
> 
> Fixes: 067353a46d8c ("cxl/region: Add memory hotplug notifier for cxl region")
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Looks good to me and matches similar cases.  Thanks for the detailed patch
description btw

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

Re: [PATCH v3 1/3] cxl/region: Fix a race condition in memory hotplug notifier

[Davidlohr Bueso]

On Tue, 18 Jun 2024, Huang Ying wrote:

>In the memory hotplug notifier function of the CXL region,
>cxl_region_perf_attrs_callback(), the node ID is obtained by checking
>the host address range of the region. However, the address range
>information is not available when the region is registered in
>devm_cxl_add_region(). Additionally, this information may be removed
>or added under the protection of cxl_region_rwsem during runtime. If
>the memory notifier is called for nodes other than that backed by the
>region, a race condition may occur, potentially leading to a NULL
>dereference or an invalid address range.
>
>The race condition is addressed by checking the availability of the
>address range information under the protection of cxl_region_rwsem. To
>enhance code readability and use guard(), the relevant code has been
>moved into a newly added function: cxl_region_nid().

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v3 1/3] cxl/region: Fix a race condition in memory hotplug notifier

[Gregory Price]

On Tue, Jun 18, 2024 at 04:46:37PM +0800, Huang Ying wrote:
> In the memory hotplug notifier function of the CXL region,
> cxl_region_perf_attrs_callback(), the node ID is obtained by checking
> the host address range of the region. However, the address range
> information is not available when the region is registered in
> devm_cxl_add_region(). Additionally, this information may be removed
> or added under the protection of cxl_region_rwsem during runtime. If
> the memory notifier is called for nodes other than that backed by the
> region, a race condition may occur, potentially leading to a NULL
> dereference or an invalid address range.
> 
> The race condition is addressed by checking the availability of the
> address range information under the protection of cxl_region_rwsem. To
> enhance code readability and use guard(), the relevant code has been
> moved into a newly added function: cxl_region_nid().
> 
> Fixes: 067353a46d8c ("cxl/region: Add memory hotplug notifier for cxl region")
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>

Good catch

Reviewed-by: Gregory Price <gourry@gourry.net>

Re: [PATCH v3 1/3] cxl/region: Fix a race condition in memory hotplug notifier

[fan]

On Tue, Jun 18, 2024 at 04:46:37PM +0800, Huang Ying wrote:
> In the memory hotplug notifier function of the CXL region,
> cxl_region_perf_attrs_callback(), the node ID is obtained by checking
> the host address range of the region. However, the address range
> information is not available when the region is registered in
> devm_cxl_add_region(). Additionally, this information may be removed
> or added under the protection of cxl_region_rwsem during runtime. If
> the memory notifier is called for nodes other than that backed by the
> region, a race condition may occur, potentially leading to a NULL
> dereference or an invalid address range.
> 
> The race condition is addressed by checking the availability of the
> address range information under the protection of cxl_region_rwsem. To
> enhance code readability and use guard(), the relevant code has been
> moved into a newly added function: cxl_region_nid().
> 
> Fixes: 067353a46d8c ("cxl/region: Add memory hotplug notifier for cxl region")
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bharata B Rao <bharata@amd.com>
> Cc: Alistair Popple <apopple@nvidia.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  drivers/cxl/core/region.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c2b6144be23..51aeef2c012c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2304,14 +2304,25 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>  	return true;
>  }
>  
> +static int cxl_region_nid(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_decoder *cxld;
> +
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	cxled = p->targets[0];
> +	if (!cxled)
> +		return NUMA_NO_NODE;
> +	cxld = &cxled->cxld;
> +	return phys_to_target_node(cxld->hpa_range.start);
> +}
> +
>  static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
>  					  unsigned long action, void *arg)
>  {
>  	struct cxl_region *cxlr = container_of(nb, struct cxl_region,
>  					       memory_notifier);
> -	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_endpoint_decoder *cxled = p->targets[0];
> -	struct cxl_decoder *cxld = &cxled->cxld;
>  	struct memory_notify *mnb = arg;
>  	int nid = mnb->status_change_nid;
>  	int region_nid;
> @@ -2319,7 +2330,7 @@ static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
>  	if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
>  		return NOTIFY_DONE;
>  
> -	region_nid = phys_to_target_node(cxld->hpa_range.start);
> +	region_nid = cxl_region_nid(cxlr);
>  	if (nid != region_nid)
>  		return NOTIFY_DONE;
>  
> -- 
> 2.39.2
> 

[PATCH v3 2/3] cxl/region: Support to calculate memory tier abstract distance

[Huang Ying]

An abstract distance value must be assigned by the driver that makes
the memory available to the system. It reflects relative performance
and is used to place memory nodes backed by CXL regions in the appropriate
memory tiers allowing promotion/demotion within the existing memory tiering
mechanism.

The abstract distance is calculated based on the memory access latency
and bandwidth of CXL regions.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Acked-by: Dan Williams <dan.j.williams@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Bharata B Rao <bharata@amd.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/region.c | 27 +++++++++++++++++++++++++++
 drivers/cxl/cxl.h         |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 51aeef2c012c..dc15ceba7ab7 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -9,6 +9,7 @@
 #include <linux/uuid.h>
 #include <linux/sort.h>
 #include <linux/idr.h>
+#include <linux/memory-tiers.h>
 #include <cxlmem.h>
 #include <cxl.h>
 #include "core.h"
@@ -2228,6 +2229,7 @@ static void unregister_region(void *_cxlr)
 	int i;
 
 	unregister_memory_notifier(&cxlr->memory_notifier);
+	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
 	device_del(&cxlr->dev);
 
 	/*
@@ -2340,6 +2342,27 @@ static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
+static int cxl_region_calculate_adistance(struct notifier_block *nb,
+					  unsigned long nid, void *data)
+{
+	struct cxl_region *cxlr = container_of(nb, struct cxl_region,
+					       adist_notifier);
+	struct access_coordinate *perf;
+	int *adist = data;
+	int region_nid;
+
+	region_nid = cxl_region_nid(cxlr);
+	if (nid != region_nid)
+		return NOTIFY_OK;
+
+	perf = &cxlr->coord[ACCESS_COORDINATE_CPU];
+
+	if (mt_perf_to_adistance(perf, adist))
+		return NOTIFY_OK;
+
+	return NOTIFY_STOP;
+}
+
 /**
  * devm_cxl_add_region - Adds a region to a decoder
  * @cxlrd: root decoder
@@ -2382,6 +2405,10 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
 	register_memory_notifier(&cxlr->memory_notifier);
 
+	cxlr->adist_notifier.notifier_call = cxl_region_calculate_adistance;
+	cxlr->adist_notifier.priority = 100;
+	register_mt_adistance_algorithm(&cxlr->adist_notifier);
+
 	rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
 	if (rc)
 		return ERR_PTR(rc);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 603c0120cff8..f46252373159 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -522,6 +522,7 @@ struct cxl_region_params {
  * @params: active + config params for the region
  * @coord: QoS access coordinates for the region
  * @memory_notifier: notifier for setting the access coordinates to node
+ * @adist_notifier: notifier for calculating the abstract distance of node
  */
 struct cxl_region {
 	struct device dev;
@@ -534,6 +535,7 @@ struct cxl_region {
 	struct cxl_region_params params;
 	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 	struct notifier_block memory_notifier;
+	struct notifier_block adist_notifier;
 };
 
 struct cxl_nvdimm_bridge {
-- 
2.39.2

Re: [PATCH v3 2/3] cxl/region: Support to calculate memory tier abstract distance

[Jonathan Cameron]

On Tue, 18 Jun 2024 16:46:38 +0800
Huang Ying <ying.huang@intel.com> wrote:

> An abstract distance value must be assigned by the driver that makes
> the memory available to the system. It reflects relative performance
> and is used to place memory nodes backed by CXL regions in the appropriate
> memory tiers allowing promotion/demotion within the existing memory tiering
> mechanism.
> 
> The abstract distance is calculated based on the memory access latency
> and bandwidth of CXL regions.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 2/3] cxl/region: Support to calculate memory tier abstract distance

[Gregory Price]

On Tue, Jun 18, 2024 at 04:46:38PM +0800, Huang Ying wrote:
> An abstract distance value must be assigned by the driver that makes
> the memory available to the system. It reflects relative performance
> and is used to place memory nodes backed by CXL regions in the appropriate
> memory tiers allowing promotion/demotion within the existing memory tiering
> mechanism.
> 
> The abstract distance is calculated based on the memory access latency
> and bandwidth of CXL regions.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Gregory Price <gourry@gourry.net>

Re: [PATCH v3 2/3] cxl/region: Support to calculate memory tier abstract distance

[fan]

On Tue, Jun 18, 2024 at 04:46:38PM +0800, Huang Ying wrote:
> An abstract distance value must be assigned by the driver that makes
> the memory available to the system. It reflects relative performance
> and is used to place memory nodes backed by CXL regions in the appropriate
> memory tiers allowing promotion/demotion within the existing memory tiering
> mechanism.
> 
> The abstract distance is calculated based on the memory access latency
> and bandwidth of CXL regions.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bharata B Rao <bharata@amd.com>
> Cc: Alistair Popple <apopple@nvidia.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  drivers/cxl/core/region.c | 27 +++++++++++++++++++++++++++
>  drivers/cxl/cxl.h         |  2 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 51aeef2c012c..dc15ceba7ab7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -9,6 +9,7 @@
>  #include <linux/uuid.h>
>  #include <linux/sort.h>
>  #include <linux/idr.h>
> +#include <linux/memory-tiers.h>
>  #include <cxlmem.h>
>  #include <cxl.h>
>  #include "core.h"
> @@ -2228,6 +2229,7 @@ static void unregister_region(void *_cxlr)
>  	int i;
>  
>  	unregister_memory_notifier(&cxlr->memory_notifier);
> +	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
>  	device_del(&cxlr->dev);
>  
>  	/*
> @@ -2340,6 +2342,27 @@ static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
>  	return NOTIFY_OK;
>  }
>  
> +static int cxl_region_calculate_adistance(struct notifier_block *nb,
> +					  unsigned long nid, void *data)
> +{
> +	struct cxl_region *cxlr = container_of(nb, struct cxl_region,
> +					       adist_notifier);
> +	struct access_coordinate *perf;
> +	int *adist = data;
> +	int region_nid;
> +
> +	region_nid = cxl_region_nid(cxlr);
> +	if (nid != region_nid)
> +		return NOTIFY_OK;
> +
> +	perf = &cxlr->coord[ACCESS_COORDINATE_CPU];
> +
> +	if (mt_perf_to_adistance(perf, adist))
> +		return NOTIFY_OK;
> +
> +	return NOTIFY_STOP;
> +}
> +
>  /**
>   * devm_cxl_add_region - Adds a region to a decoder
>   * @cxlrd: root decoder
> @@ -2382,6 +2405,10 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>  	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
>  	register_memory_notifier(&cxlr->memory_notifier);
>  
> +	cxlr->adist_notifier.notifier_call = cxl_region_calculate_adistance;
> +	cxlr->adist_notifier.priority = 100;
> +	register_mt_adistance_algorithm(&cxlr->adist_notifier);
> +
>  	rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
>  	if (rc)
>  		return ERR_PTR(rc);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 603c0120cff8..f46252373159 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -522,6 +522,7 @@ struct cxl_region_params {
>   * @params: active + config params for the region
>   * @coord: QoS access coordinates for the region
>   * @memory_notifier: notifier for setting the access coordinates to node
> + * @adist_notifier: notifier for calculating the abstract distance of node
>   */
>  struct cxl_region {
>  	struct device dev;
> @@ -534,6 +535,7 @@ struct cxl_region {
>  	struct cxl_region_params params;
>  	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
>  	struct notifier_block memory_notifier;
> +	struct notifier_block adist_notifier;
>  };
>  
>  struct cxl_nvdimm_bridge {
> -- 
> 2.39.2
> 

[PATCH v3 3/3] cxl/region: Simplify cxl_region_nid()

[Huang Ying]

The node ID of the region can be gotten via resource start address
directly.  This simplifies the implementation of cxl_region_nid().

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Alison Schofield <alison.schofield@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Bharata B Rao <bharata@amd.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/region.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index dc15ceba7ab7..605efe3562c6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2309,15 +2309,13 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
 static int cxl_region_nid(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_endpoint_decoder *cxled;
-	struct cxl_decoder *cxld;
+	struct resource *res;
 
 	guard(rwsem_read)(&cxl_region_rwsem);
-	cxled = p->targets[0];
-	if (!cxled)
+        res = p->res;
+	if (!res)
 		return NUMA_NO_NODE;
-	cxld = &cxled->cxld;
-	return phys_to_target_node(cxld->hpa_range.start);
+	return phys_to_target_node(res->start);
 }
 
 static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
-- 
2.39.2

Re: [PATCH v3 3/3] cxl/region: Simplify cxl_region_nid()

[Jonathan Cameron]

On Tue, 18 Jun 2024 16:46:39 +0800
Huang Ying <ying.huang@intel.com> wrote:

> The node ID of the region can be gotten via resource start address
> directly.  This simplifies the implementation of cxl_region_nid().
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bharata B Rao <bharata@amd.com>
> Cc: Alistair Popple <apopple@nvidia.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core/region.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index dc15ceba7ab7..605efe3562c6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2309,15 +2309,13 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>  static int cxl_region_nid(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_endpoint_decoder *cxled;
> -	struct cxl_decoder *cxld;
> +	struct resource *res;
>  
>  	guard(rwsem_read)(&cxl_region_rwsem);
> -	cxled = p->targets[0];
> -	if (!cxled)
> +        res = p->res;

Odd indent - I think spaces rather than tab.  Otherwise seems reasonable.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> +	if (!res)
>  		return NUMA_NO_NODE;
> -	cxld = &cxled->cxld;
> -	return phys_to_target_node(cxld->hpa_range.start);
> +	return phys_to_target_node(res->start);
>  }
>  
>  static int cxl_region_perf_attrs_callback(struct notifier_block *nb,

Re: [PATCH v3 3/3] cxl/region: Simplify cxl_region_nid()

[Huang, Ying]

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Tue, 18 Jun 2024 16:46:39 +0800
> Huang Ying <ying.huang@intel.com> wrote:
>
>> The node ID of the region can be gotten via resource start address
>> directly.  This simplifies the implementation of cxl_region_nid().
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Suggested-by: Alison Schofield <alison.schofield@intel.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Bharata B Rao <bharata@amd.com>
>> Cc: Alistair Popple <apopple@nvidia.com>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> ---
>>  drivers/cxl/core/region.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index dc15ceba7ab7..605efe3562c6 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -2309,15 +2309,13 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>>  static int cxl_region_nid(struct cxl_region *cxlr)
>>  {
>>  	struct cxl_region_params *p = &cxlr->params;
>> -	struct cxl_endpoint_decoder *cxled;
>> -	struct cxl_decoder *cxld;
>> +	struct resource *res;
>>  
>>  	guard(rwsem_read)(&cxl_region_rwsem);
>> -	cxled = p->targets[0];
>> -	if (!cxled)
>> +        res = p->res;
>
> Odd indent - I think spaces rather than tab.  Otherwise seems
> reasonable.

Good catch!  I used spaces accidently.

Hi, Dave,

Do you need me to send a new version?  Or you can change it?

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

Thank you very much for your review!

--
Best Regards,
Huang, Ying

>
>> +	if (!res)
>>  		return NUMA_NO_NODE;
>> -	cxld = &cxled->cxld;
>> -	return phys_to_target_node(cxld->hpa_range.start);
>> +	return phys_to_target_node(res->start);
>>  }
>>  
>>  static int cxl_region_perf_attrs_callback(struct notifier_block *nb,

Re: [PATCH v3 3/3] cxl/region: Simplify cxl_region_nid()

[Dave Jiang]

On 6/20/24 7:25 PM, Huang, Ying wrote:
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
> 
>> On Tue, 18 Jun 2024 16:46:39 +0800
>> Huang Ying <ying.huang@intel.com> wrote:
>>
>>> The node ID of the region can be gotten via resource start address
>>> directly.  This simplifies the implementation of cxl_region_nid().
>>>
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Suggested-by: Alison Schofield <alison.schofield@intel.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Cc: Dave Jiang <dave.jiang@intel.com>
>>> Cc: Bharata B Rao <bharata@amd.com>
>>> Cc: Alistair Popple <apopple@nvidia.com>
>>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> ---
>>>  drivers/cxl/core/region.c | 10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index dc15ceba7ab7..605efe3562c6 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -2309,15 +2309,13 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>>>  static int cxl_region_nid(struct cxl_region *cxlr)
>>>  {
>>>  	struct cxl_region_params *p = &cxlr->params;
>>> -	struct cxl_endpoint_decoder *cxled;
>>> -	struct cxl_decoder *cxld;
>>> +	struct resource *res;
>>>  
>>>  	guard(rwsem_read)(&cxl_region_rwsem);
>>> -	cxled = p->targets[0];
>>> -	if (!cxled)
>>> +        res = p->res;
>>
>> Odd indent - I think spaces rather than tab.  Otherwise seems
>> reasonable.
> 
> Good catch!  I used spaces accidently.
> 
> Hi, Dave,
> 
> Do you need me to send a new version?  Or you can change it?

The series LGTM. I can fix it up unless there are other comments from someone else that require changes. Also thanks for the fix in 1/3. 

> 
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Thank you very much for your review!
> 
> --
> Best Regards,
> Huang, Ying
> 
>>
>>> +	if (!res)
>>>  		return NUMA_NO_NODE;
>>> -	cxld = &cxled->cxld;
>>> -	return phys_to_target_node(cxld->hpa_range.start);
>>> +	return phys_to_target_node(res->start);
>>>  }
>>>  
>>>  static int cxl_region_perf_attrs_callback(struct notifier_block *nb,

Re: [PATCH v3 3/3] cxl/region: Simplify cxl_region_nid()

[Gregory Price]

On Tue, Jun 18, 2024 at 04:46:39PM +0800, Huang Ying wrote:
> The node ID of the region can be gotten via resource start address
> directly.  This simplifies the implementation of cxl_region_nid().
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Gregory Price <gourry@gourry.net>

Re: [PATCH v3 3/3] cxl/region: Simplify cxl_region_nid()

[fan]

On Tue, Jun 18, 2024 at 04:46:39PM +0800, Huang Ying wrote:
> The node ID of the region can be gotten via resource start address
> directly.  This simplifies the implementation of cxl_region_nid().
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Alison Schofield <alison.schofield@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bharata B Rao <bharata@amd.com>
> Cc: Alistair Popple <apopple@nvidia.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core/region.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index dc15ceba7ab7..605efe3562c6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2309,15 +2309,13 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>  static int cxl_region_nid(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_endpoint_decoder *cxled;
> -	struct cxl_decoder *cxld;
> +	struct resource *res;
>  
>  	guard(rwsem_read)(&cxl_region_rwsem);
> -	cxled = p->targets[0];
> -	if (!cxled)
> +        res = p->res;
> +	if (!res)
>  		return NUMA_NO_NODE;
> -	cxld = &cxled->cxld;
> -	return phys_to_target_node(cxld->hpa_range.start);
> +	return phys_to_target_node(res->start);
>  }
>  
>  static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
> -- 
> 2.39.2
> 

Reviewed-by: Fan Ni <fan.ni@samsung.com>