Skip to contentSkip to conversation

nvme: find numa distance only if controller has valid numa id

PATCH Superseded Nilay Shroff 2 participants 4 messages Apr 15, 2024 – Apr 16, 2024 NVMe

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 nvme: find numa distance only if controller has valid numa id

On system where native nvme multipath is configured and iopolicy is set to numa but the nvme controller numa node id is undefined or -1 (NUMA_NO_NODE) then avoid calculating node distance for finding optimal io path. In such case we may access numa distance table with invalid index and that may potentially refer to incorrect memory. So this patch ensures that if the nvme controller numa node id is -1 then instead of calculating node distance for finding optimal io path, we set the numa node distance of such controller to default 10 (LOCAL_DISTANCE).

---
Changes from v1:
Formatting cleanups (Christoph)

 drivers/nvme/host/multipath.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/nvme/host/multipath.c +2 -1
index 5397fb428b24..08e598c87b08 100644
@@ -247,7 +247,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
 		if (nvme_path_is_disabled(ns))
 			continue;
 
-		if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
+		if (ns->ctrl->numa_node != NUMA_NO_NODE &&
+				READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
 			distance = node_distance(node, ns->ctrl->numa_node);
 		else
 			distance = LOCAL_DISTANCE;
Signature · 1 line
2.44.0
#1 lore raw
Quoted 24 lines · Nilay Shroff

On system where native nvme multipath is configured and iopolicy is set to numa but the nvme controller numa node id is undefined or -1 (NUMA_NO_NODE) then avoid calculating node distance for finding optimal io path. In such case we may access numa distance table with invalid index and that may potentially refer to incorrect memory. So this patch ensures that if the nvme controller numa node id is -1 then instead of calculating node distance for finding optimal io path, we set the numa node distance of such controller to default 10 (LOCAL_DISTANCE).

---
Changes from v1:
Formatting cleanups (Christoph)

  drivers/nvme/host/multipath.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/nvme/host/multipath.c +0 -0
index 5397fb428b24..08e598c87b08 100644

drivers/nvme/host/multipath.c static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node) · line 251

Quoted 6 lines
drivers/nvme/host/multipath.c +2 -1
@@ -247,7 +247,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
  		if (nvme_path_is_disabled(ns))
  			continue;
 
-		if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
+		if (ns->ctrl->numa_node != NUMA_NO_NODE &&
+				READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)

unless something is wrong when I apply this patch, above line should be kept under 80 char ?

something like following totally untested :-

drivers/nvme/host/multipath.c +0 -0
index 5397fb428b24..d16e976ae1a4 100644
@@ -247,7 +247,8 @@ static struct nvme_ns *__nvme_find_path(struct 

nvme_ns_head *head, int node)
                if (nvme_path_is_disabled(ns))
                        continue;

-               if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA) +               if (ns->ctrl->numa_node != NUMA_NO_NODE &&
+                   READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
                        distance = node_distance(node,
ns->ctrl->numa_node);
                else
                        distance = LOCAL_DISTANCE;

-ck

#2 lore raw
Quoted 54 lines · Chaitanya Kulkarni
Quoted 24 lines · Nilay Shroff

On system where native nvme multipath is configured and iopolicy is set to numa but the nvme controller numa node id is undefined or -1 (NUMA_NO_NODE) then avoid calculating node distance for finding optimal io path. In such case we may access numa distance table with invalid index and that may potentially refer to incorrect memory. So this patch ensures that if the nvme controller numa node id is -1 then instead of calculating node distance for finding optimal io path, we set the numa node distance of such controller to default 10 (LOCAL_DISTANCE).

---
Changes from v1:
Formatting cleanups (Christoph)

  drivers/nvme/host/multipath.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/nvme/host/multipath.c +0 -0
index 5397fb428b24..08e598c87b08 100644

drivers/nvme/host/multipath.c static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node) · line 251

Quoted 6 lines
drivers/nvme/host/multipath.c +2 -1
@@ -247,7 +247,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
  		if (nvme_path_is_disabled(ns))
  			continue;
 
-		if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
+		if (ns->ctrl->numa_node != NUMA_NO_NODE &&
+				READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)

unless something is wrong when I apply this patch, above line should be kept under 80 char ?

something like following totally untested :-

drivers/nvme/host/multipath.c +2 -1
index 5397fb428b24..d16e976ae1a4 100644
@@ -247,7 +247,8 @@ static struct nvme_ns *__nvme_find_path(struct 
 nvme_ns_head *head, int node)
                 if (nvme_path_is_disabled(ns))
                         continue;
 
-               if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
+               if (ns->ctrl->numa_node != NUMA_NO_NODE &&
+                   READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
                         distance = node_distance(node, 
 ns->ctrl->numa_node);

                else
                        distance = LOCAL_DISTANCE;

Quoted 3 lines

-ck

I don't know but checkpatch.pl didn't complain about my patch overshooting 80 chars width. Anyways, I will reformat the patch and resend.

Thanks,
--Nilay

#3 lore raw
Quoted 5 lines · Nilay Shroff

I don't know but checkpatch.pl didn't complain about my patch overshooting 80 chars width. Anyways, I will reformat the patch and resend.

Thanks,
--Nilay

checkpatch doesn't warn that anymore from what I remember, I've been victim of relying on checkpatch then corrected on the list about 80 char.

-ck