[PATCH v2] nvme: find numa distance only if controller has valid numa id

[PATCH v2] nvme: find numa distance only if controller has valid numa id

[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).

Link: https://lore.kernel.org/all/20240413090614.678353-1-nilay@linux.ibm.com/
Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
Changes from v1:
	Formatting cleanups (Christoph)

 drivers/nvme/host/multipath.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5397fb428b24..08e598c87b08 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -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;
-- 
2.44.0

Re: [PATCH v2] nvme: find numa distance only if controller has valid numa id

[Chaitanya Kulkarni]

On 4/15/24 07:22, Nilay Shroff wrote:
> 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).
>
> Link: https://lore.kernel.org/all/20240413090614.678353-1-nilay@linux.ibm.com/
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
> Changes from v1:
> 	Formatting cleanups (Christoph)
>
>   drivers/nvme/host/multipath.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 5397fb428b24..08e598c87b08 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -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 :-

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5397fb428b24..d16e976ae1a4 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -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

Re: [PATCH v2] nvme: find numa distance only if controller has valid numa id

[Nilay Shroff]

On 4/16/24 08:06, Chaitanya Kulkarni wrote:
> On 4/15/24 07:22, Nilay Shroff wrote:
>> 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).
>>
>> Link: https://lore.kernel.org/all/20240413090614.678353-1-nilay@linux.ibm.com/
>> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
>> ---
>> Changes from v1:
>> 	Formatting cleanups (Christoph)
>>
>>   drivers/nvme/host/multipath.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
>> index 5397fb428b24..08e598c87b08 100644
>> --- a/drivers/nvme/host/multipath.c
>> +++ b/drivers/nvme/host/multipath.c
>> @@ -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 :-
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 5397fb428b24..d16e976ae1a4 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -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
> 
> 
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

Re: [PATCH v2] nvme: find numa distance only if controller has valid numa id

[Chaitanya Kulkarni]

On 4/15/2024 11:53 PM, Nilay Shroff wrote:
> 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