RDMA/mlx5: Don't use cached IRQ affinity mask

Leon, I'd like to see a tested-by tag for this (at least until I get some time to test it).

The patch itself looks fine to me.

Quoted 2 lines · Sagi Grimberg

Leon, I'd like to see a tested-by tag for this (at least until I get some time to test it).

Of course.

Thanks

Quoted 2 lines

The patch itself looks fine to me.

Hi,
I've tested this patch and seems problematic at this moment.

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

BTW, when I run the blk_mq_map_queues it works for every irq affinity.

Quoted 3 lines

Hi,
I've tested this patch and seems problematic at this moment.

Problematic how? what are you seeing?

Quoted 2 lines

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

Is your device irq affinity linear?

Quoted 2 lines

BTW, when I run the blk_mq_map_queues it works for every irq affinity.

But its probably not aligned to the device vector affinity.

Quoted 5 lines · Sagi Grimberg

Quoted 3 lines

Hi,
I've tested this patch and seems problematic at this moment.

Problematic how? what are you seeing?

Connection failures and same error Steve saw:

[Mon Jul 16 16:19:11 2018] nvme nvme0: Connect command failed, error
wo/DNR bit: -16402
[Mon Jul 16 16:19:11 2018] nvme nvme0: failed to connect queue: 2 ret=-18

Quoted 6 lines

Quoted 4 lines

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

Is your device irq affinity linear?

When it's linear and the balancer is stopped the patch works.

Quoted 4 lines

Quoted 2 lines

BTW, when I run the blk_mq_map_queues it works for every irq affinity.

But its probably not aligned to the device vector affinity.

but I guess it's better in some cases.

I've checked the situation before Leon's patch and set all the vetcors to CPU 0. In this case (I think that this was the initial report by Steve), we use the affinity_hint (Israel's and Saeed's patches were we use dev->priv.irq_info[vector].mask) and it worked fine.

Steve,
Can you share your configuration (kernel, HCA, affinity map, connect command, lscpu) ?
I want to repro it in my lab.

Hey Max:

Quoted 37 lines · Sagi Grimberg

Quoted 5 lines

Quoted 3 lines

Hi,
I've tested this patch and seems problematic at this moment.

Problematic how? what are you seeing?

Connection failures and same error Steve saw:

[Mon Jul 16 16:19:11 2018] nvme nvme0: Connect command failed, error
wo/DNR bit: -16402
[Mon Jul 16 16:19:11 2018] nvme nvme0: failed to connect queue: 2 ret=-18

Quoted 6 lines

Quoted 4 lines

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

Is your device irq affinity linear?

When it's linear and the balancer is stopped the patch works.

Quoted 4 lines

Quoted 2 lines

BTW, when I run the blk_mq_map_queues it works for every irq affinity.

But its probably not aligned to the device vector affinity.

but I guess it's better in some cases.

I've checked the situation before Leon's patch and set all the vetcors to CPU 0. In this case (I think that this was the initial report by Steve), we use the affinity_hint (Israel's and Saeed's patches were we use dev->priv.irq_info[vector].mask) and it worked fine.

Quoted 5 lines

Steve,
Can you share your configuration (kernel, HCA, affinity map, connect command, lscpu) ?
I want to repro it in my lab.

- linux-4.18-rc1 + the nvme/nvmet inline_data_size patches + patches to enable ib_get_vector_affinity() in cxgb4 + sagi's patch + leon's mlx5 patch so I can change the affinity via procfs.

Quoted 3 lines · Steve Wise

Hey Max:

Hey,

Quoted 44 lines · Max Gurtovoy

Quoted 37 lines · Sagi Grimberg

Quoted 5 lines

Quoted 3 lines

Hi,
I've tested this patch and seems problematic at this moment.

Problematic how? what are you seeing?

Connection failures and same error Steve saw:

[Mon Jul 16 16:19:11 2018] nvme nvme0: Connect command failed, error
wo/DNR bit: -16402
[Mon Jul 16 16:19:11 2018] nvme nvme0: failed to connect queue: 2 ret=-18

Quoted 6 lines

Quoted 4 lines

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

Is your device irq affinity linear?

When it's linear and the balancer is stopped the patch works.

Quoted 4 lines

Quoted 2 lines

BTW, when I run the blk_mq_map_queues it works for every irq affinity.

But its probably not aligned to the device vector affinity.

but I guess it's better in some cases.

I've checked the situation before Leon's patch and set all the vetcors to CPU 0. In this case (I think that this was the initial report by Steve), we use the affinity_hint (Israel's and Saeed's patches were we use dev->priv.irq_info[vector].mask) and it worked fine.

Quoted 5 lines

Steve,
Can you share your configuration (kernel, HCA, affinity map, connect command, lscpu) ?
I want to repro it in my lab.

Quoted 3 lines

- linux-4.18-rc1 + the nvme/nvmet inline_data_size patches + patches to enable ib_get_vector_affinity() in cxgb4 + sagi's patch + leon's mlx5 patch so I can change the affinity via procfs.

ohh, now I understand that you where complaining regarding the affinity change reflection to mlx5_ib_get_vector_affinity and not regarding the failures on connecting while the affinity overlaps (that is working good before Leon's patch).
So this is a known issue since we used a static hint that never changes from dev->priv.irq_info[vector].mask.

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort mapping according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

-Max.

Quoted 64 lines · Max Gurtovoy

Quoted 3 lines · Steve Wise

Hey Max:

Hey,

Quoted 44 lines · Max Gurtovoy

Quoted 37 lines · Sagi Grimberg

Quoted 5 lines

Quoted 3 lines

Hi,
I've tested this patch and seems problematic at this moment.

Problematic how? what are you seeing?

Connection failures and same error Steve saw:

[Mon Jul 16 16:19:11 2018] nvme nvme0: Connect command failed, error
wo/DNR bit: -16402
[Mon Jul 16 16:19:11 2018] nvme nvme0: failed to connect queue: 2 ret=-18

Quoted 6 lines

Quoted 4 lines

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

Is your device irq affinity linear?

When it's linear and the balancer is stopped the patch works.

Quoted 4 lines

Quoted 2 lines

BTW, when I run the blk_mq_map_queues it works for every irq affinity.

But its probably not aligned to the device vector affinity.

but I guess it's better in some cases.

I've checked the situation before Leon's patch and set all the vetcors to CPU 0. In this case (I think that this was the initial report by Steve), we use the affinity_hint (Israel's and Saeed's patches were we use dev->priv.irq_info[vector].mask) and it worked fine.

Quoted 5 lines

Steve,
Can you share your configuration (kernel, HCA, affinity map, connect command, lscpu) ?
I want to repro it in my lab.

Quoted 3 lines

- linux-4.18-rc1 + the nvme/nvmet inline_data_size patches + patches to enable ib_get_vector_affinity() in cxgb4 + sagi's patch + leon's mlx5 patch so I can change the affinity via procfs.

ohh, now I understand that you where complaining regarding the affinity change reflection to mlx5_ib_get_vector_affinity and not regarding the failures on connecting while the affinity overlaps (that is working good before Leon's patch).
So this is a known issue since we used a static hint that never changes from dev->priv.irq_info[vector].mask.

Quoted 5 lines

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort mapping according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

Max,

I have no clue what is needed to do int blq_mq*, but my patch only gave to users ability reconfigure their affinity mask after driver is loaded.

Thanks

Quoted 3 lines

-Max.

Quoted 3 lines

Hey Max:

Hey,

Quoted 8 lines · Max Gurtovoy

Quoted 7 lines · Sagi Grimberg

Quoted 4 lines

Quoted 3 lines

Hi,
I've tested this patch and seems problematic at this moment.

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Problematic how? what are you seeing?

Connection failures and same error Steve saw:

[Mon Jul 16 16:19:11 2018] nvme nvme0: Connect command failed, error
wo/DNR bit: -16402
[Mon Jul 16 16:19:11 2018] nvme nvme0: failed to connect queue: 2 ret=-

18

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

maybe this is because of the bug that Steve mentioned in the NVMe mailing list. Sagi mentioned that we should fix it in the NVMe/RDMA initiator and I'll run his suggestion as well.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Is your device irq affinity linear?

When it's linear and the balancer is stopped the patch works.

Quoted 2 lines

Quoted 2 lines

BTW, when I run the blk_mq_map_queues it works for every irq

affinity.

Quoted 16 lines

Quoted 10 lines

Quoted 2 lines

But its probably not aligned to the device vector affinity.

but I guess it's better in some cases.

I've checked the situation before Leon's patch and set all the vetcors to CPU 0. In this case (I think that this was the initial report by Steve), we use the affinity_hint (Israel's and Saeed's patches were we use dev->priv.irq_info[vector].mask) and it worked fine.

Quoted 5 lines

Steve,
Can you share your configuration (kernel, HCA, affinity map, connect command, lscpu) ?
I want to repro it in my lab.

Quoted 3 lines

- linux-4.18-rc1 + the nvme/nvmet inline_data_size patches + patches to enable ib_get_vector_affinity() in cxgb4 + sagi's patch + leon's mlx5 patch so I can change the affinity via procfs.

ohh, now I understand that you where complaining regarding the affinity change reflection to mlx5_ib_get_vector_affinity and not regarding the failures on connecting while the affinity overlaps (that is working good before Leon's patch).
So this is a known issue since we used a static hint that never changes from dev->priv.irq_info[vector].mask.

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort mapping according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

Quoted 6 lines

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort mapping according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

That is what I would expect also. For example, in my node, where there are
16 cpus, and 2 numa nodes, I observe much better nvmf IOPS performance by setting up my 16 driver completion event queues such that each is bound to a node-local cpu. So I end up with each nodel-local cpu having 2 queues bound
to it. W/O adding support in iw_cxgb4 for ib_get_vector_affinity(), this
works fine. I assumed adding ib_get_vector_affinity() would allow this to
all "just work" by default, but I'm running into this connection failure issue.

I don't understand exactly what the blk_mq layer is trying to do, but I assume it has ingress event queues and processing that it trying to align with the drivers ingress cq event handling, so everybody stays on the same
cpu (or at least node). But something else is going on. Is there
documentation on how this works somewhere?

Quoted 16 lines

Quoted 6 lines

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort mapping according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

That is what I would expect also.   For example, in my node, where there are 16 cpus, and 2 numa nodes, I observe much better nvmf IOPS performance by setting up my 16 driver completion event queues such that each is bound to a node-local cpu.  So I end up with each nodel-local cpu having 2 queues bound to it.   W/O adding support in iw_cxgb4 for ib_get_vector_affinity(), this works fine.   I assumed adding ib_get_vector_affinity() would allow this to all "just work" by default, but I'm running into this connection failure issue.

Quoted 5 lines

I don't understand exactly what the blk_mq layer is trying to do, but I assume it has ingress event queues and processing that it trying to align with the drivers ingress cq event handling, so everybody stays on the same cpu (or at least node).   But something else is going on.  Is there documentation on how this works somewhere?

Does this (untested) patch help?

block/blk-mq-cpumap.c +2 -6

index 3eb169f15842..dbe962cb537d 100644
@@ -30,29 +30,34 @@ static int get_first_sibling(unsigned int cpu)
         return cpu;
  }
 
-int blk_mq_map_queues(struct blk_mq_tag_set *set)
+void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu)
  {
         unsigned int *map = set->mq_map;
         unsigned int nr_queues = set->nr_hw_queues;
-       unsigned int cpu, first_sibling;
+       unsigned int first_sibling;
 
-       for_each_possible_cpu(cpu) {
-               /*
-                * First do sequential mapping between CPUs and queues.
-                * In case we still have CPUs to map, and we have some 

number of
-                * threads per cores then map sibling threads to the same queue for
-                * performace optimizations.
-                */
-               if (cpu < nr_queues) {
+       /*
+        * First do sequential mapping between CPUs and queues. +        * In case we still have CPUs to map, and we have some number of +        * threads per cores then map sibling threads to the same queue for +        * performace optimizations.
+        */
+       if (cpu < nr_queues) {
+               map[cpu] = cpu_to_queue_index(nr_queues, cpu); +       } else {
+               first_sibling = get_first_sibling(cpu);
+               if (first_sibling == cpu)
                        map[cpu] = cpu_to_queue_index(nr_queues, cpu);
-               } else {
-                       first_sibling = get_first_sibling(cpu); -                       if (first_sibling == cpu)
-                               map[cpu] = cpu_to_queue_index(nr_queues, cpu);
-                       else
-                               map[cpu] = map[first_sibling]; -               }
+               else
+                       map[cpu] = map[first_sibling];
        }

block/blk-mq-cpumap.c +7 -0

+}
+EXPORT_SYMBOL_GPL(blk_mq_map_queue);
+
+int blk_mq_map_queues(struct blk_mq_tag_set *set)
+{
+       for_each_possible_cpu(cpu)
+                blk_mq_map_queue(set, cpu);
 
         return 0;
  }

block/blk-mq-rdma.c +9 -0

index 996167f1de18..5e91789bea5b 100644
@@ -35,6 +35,10 @@ int blk_mq_rdma_map_queues(struct blk_mq_tag_set *set,
         const struct cpumask *mask;
         unsigned int queue, cpu;
 
+       /* reset all to  */
+       for_each_possible_cpu(cpu)
+               set->mq_map[cpu] = UINT_MAX;
+
         for (queue = 0; queue < set->nr_hw_queues; queue++) {
                 mask = ib_get_vector_affinity(dev, first_vec + queue);
                 if (!mask)

@@ -44,6 +48,11 @@ int blk_mq_rdma_map_queues(struct blk_mq_tag_set *set,
                         set->mq_map[cpu] = queue;
         }
 
+       for_each_possible_cpu(cpu) {
+               if (set->mq_map[cpu] == UINT_MAX)
+                       blk_mq_map_queue(set, cpu);
+       }
+
         return 0;
 
  fallback:

include/linux/blk-mq.h +1 -0

index e3147eb74222..7a9848a82475 100644
@@ -283,6 +283,7 @@ int blk_mq_freeze_queue_wait_timeout(struct 
 request_queue *q,
                                      unsigned long timeout);
 
  int blk_mq_map_queues(struct blk_mq_tag_set *set);
+void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu);
  void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int 
 nr_hw_queues);

 void blk_mq_quiesce_queue_nowait(struct request_queue *q);

Signature · 1 line

It really is still a best effort thing...

-----Original Message-----
From: Max Gurtovoy <[email protected]>
Sent: Wednesday, July 18, 2018 9:14 AM
To: Sagi Grimberg <[email protected]>; Steve Wise <[email protected]>; 'Leon Romanovsky' <[email protected]>
Cc: 'Doug Ledford' <[email protected]>; 'Jason Gunthorpe' <[email protected]>; 'RDMA mailing list' <[email protected]>; 'Saeed Mahameed' <[email protected]>; 'linux-netdev' <[email protected]>
Subject: Re: [PATCH mlx5-next] RDMA/mlx5: Don't use cached IRQ affinity mask

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines

Quoted 4 lines

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort

mapping

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

That is what I would expect also. For example, in my node, where
there are
16 cpus, and 2 numa nodes, I observe much better nvmf IOPS

performance by

Quoted 11 lines

Quoted 11 lines

setting up my 16 driver completion event queues such that each is bound to a
node-local cpu. So I end up with each nodel-local cpu having 2 queues bound
to it. W/O adding support in iw_cxgb4 for ib_get_vector_affinity(),
this
works fine. I assumed adding ib_get_vector_affinity() would allow
this to
all "just work" by default, but I'm running into this connection failure issue.

Quoted 8 lines

Quoted 6 lines

I don't understand exactly what the blk_mq layer is trying to do, but I assume it has ingress event queues and processing that it trying to align with the drivers ingress cq event handling, so everybody stays on the same
cpu (or at least node). But something else is going on. Is there
documentation on how this works somewhere?

Does this (untested) patch help?

I'm not sure (I'll test it tomorrow) because the issue is the unmapped queues and not the cpus.
for example, if the affinity of q=6 and q=12 returned the same cpumask than q=6 will not be mapped and will fail to connect.

Quoted 9 lines

--

block/blk-mq-cpumap.c +0 -0

index 3eb169f15842..dbe962cb537d 100644
@@ -30,29 +30,34 @@ static int get_first_sibling(unsigned int cpu)
         return cpu;
  }
 

block/blk-mq-cpumap.c static int get_first_sibling(unsigned int cpu) · line 34

Quoted 3 lines

block/blk-mq-cpumap.c +1 -1

-int blk_mq_map_queues(struct blk_mq_tag_set *set)
+void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu)
  {

block/blk-mq-rdma.c int blk_mq_rdma_map_queues(struct

Quoted 53 lines

block/blk-mq-cpumap.c +1 -5

         unsigned int *map = set->mq_map;
         unsigned int nr_queues = set->nr_hw_queues;
-       unsigned int cpu, first_sibling;
+       unsigned int first_sibling;
 
-       for_each_possible_cpu(cpu) {
-               /*
-                * First do sequential mapping between CPUs and queues.
-                * In case we still have CPUs to map, and we have some

number of
- * threads per cores then map sibling threads to the
same queue for

block/blk-mq-cpumap.c +11 -7

-                * performace optimizations.
-                */
-               if (cpu < nr_queues) {
+       /*
+        * First do sequential mapping between CPUs and queues.
+        * In case we still have CPUs to map, and we have some number of
+        * threads per cores then map sibling threads to the same queue for
+        * performace optimizations.
+        */
+       if (cpu < nr_queues) {
+               map[cpu] = cpu_to_queue_index(nr_queues, cpu);
+       } else {
+               first_sibling = get_first_sibling(cpu);
+               if (first_sibling == cpu)
                         map[cpu] = cpu_to_queue_index(nr_queues, cpu);
-               } else {
-                       first_sibling = get_first_sibling(cpu);
-                       if (first_sibling == cpu)
-                               map[cpu] = cpu_to_queue_index(nr_queues,

cpu);

block/blk-mq-cpumap.c +9 -3

-                       else
-                               map[cpu] = map[first_sibling];
-               }
+               else
+                       map[cpu] = map[first_sibling];
         }
+}
+EXPORT_SYMBOL_GPL(blk_mq_map_queue);
+
+int blk_mq_map_queues(struct blk_mq_tag_set *set)
+{
+       for_each_possible_cpu(cpu)
+                blk_mq_map_queue(set, cpu);
 
         return 0;
  }

block/blk-mq-rdma.c +0 -0

index 996167f1de18..5e91789bea5b 100644
@@ -35,6 +35,10 @@ int blk_mq_rdma_map_queues(struct

blk_mq_tag_set *set,

block/blk-mq-rdma.c int blk_mq_rdma_map_queues(struct

Quoted 11 lines

block/blk-mq-rdma.c +4 -0

         const struct cpumask *mask;
         unsigned int queue, cpu;
 
+       /* reset all to  */
+       for_each_possible_cpu(cpu)
+               set->mq_map[cpu] = UINT_MAX;
+
         for (queue = 0; queue < set->nr_hw_queues; queue++) {
                 mask = ib_get_vector_affinity(dev, first_vec + queue);
                 if (!mask)

@@ -44,6 +48,11 @@ int blk_mq_rdma_map_queues(struct

blk_mq_tag_set *set,

Quoted 28 lines

block/blk-mq-rdma.c +5 -0

                         set->mq_map[cpu] = queue;
         }
 
+       for_each_possible_cpu(cpu) {
+               if (set->mq_map[cpu] == UINT_MAX)
+                       blk_mq_map_queue(set, cpu);
+       }
+
         return 0;
 
  fallback:

include/linux/blk-mq.h +1 -0

index e3147eb74222..7a9848a82475 100644
@@ -283,6 +283,7 @@ int blk_mq_freeze_queue_wait_timeout(struct
 request_queue *q,
                                      unsigned long timeout);
 
  int blk_mq_map_queues(struct blk_mq_tag_set *set);
+void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu);
  void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int
 nr_hw_queues);

void blk_mq_quiesce_queue_nowait(struct request_queue *q);
--

It really is still a best effort thing...

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort

mapping

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

That is what I would expect also. For example, in my node, where
there are
16 cpus, and 2 numa nodes, I observe much better nvmf IOPS

performance by

Quoted 18 lines

Quoted 17 lines

setting up my 16 driver completion event queues such that each is bound to a
node-local cpu. So I end up with each nodel-local cpu having 2 queues bound
to it. W/O adding support in iw_cxgb4 for ib_get_vector_affinity(),
this
works fine. I assumed adding ib_get_vector_affinity() would allow
this to
all "just work" by default, but I'm running into this connection failure issue.

I don't understand exactly what the blk_mq layer is trying to do, but I assume it has ingress event queues and processing that it trying to align with the drivers ingress cq event handling, so everybody stays on the same
cpu (or at least node). But something else is going on. Is there
documentation on how this works somewhere?

Quoted 1 line

Does this (untested) patch help?

I'm not sure (I'll test it tomorrow) because the issue is the unmapped queues and not the cpus.
for example, if the affinity of q=6 and q=12 returned the same cpumask than q=6 will not be mapped and will fail to connect.

Quoted 33 lines · Sagi Grimberg

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

IMO we must fulfil the user wish to connect to N queues and not reduce it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort

mapping

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

That is what I would expect also. For example, in my node, where
there are
16 cpus, and 2 numa nodes, I observe much better nvmf IOPS

performance by

Quoted 18 lines

Quoted 17 lines

setting up my 16 driver completion event queues such that each is bound to a
node-local cpu. So I end up with each nodel-local cpu having 2 queues bound
to it. W/O adding support in iw_cxgb4 for ib_get_vector_affinity(),
this
works fine. I assumed adding ib_get_vector_affinity() would allow
this to
all "just work" by default, but I'm running into this connection failure issue.

I don't understand exactly what the blk_mq layer is trying to do, but I assume it has ingress event queues and processing that it trying to align with the drivers ingress cq event handling, so everybody stays on the same
cpu (or at least node). But something else is going on. Is there
documentation on how this works somewhere?

Quoted 7 lines

Quoted 1 line

Does this (untested) patch help?

I'm not sure (I'll test it tomorrow) because the issue is the unmapped queues and not the cpus.
for example, if the affinity of q=6 and q=12 returned the same cpumask than q=6 will not be mapped and will fail to connect.

Attached is a patch that applies cleanly for me. It has problems if vectors have affinity to more than 1 cpu:

[ 2031.988881] iw_cxgb4: comp_vector 0, irq 203 mask 0xff00
[ 2031.994706] iw_cxgb4: comp_vector 1, irq 204 mask 0xff00
[ 2032.000348] iw_cxgb4: comp_vector 2, irq 205 mask 0xff00
[ 2032.005992] iw_cxgb4: comp_vector 3, irq 206 mask 0xff00
[ 2032.011629] iw_cxgb4: comp_vector 4, irq 207 mask 0xff00
[ 2032.017271] iw_cxgb4: comp_vector 5, irq 208 mask 0xff00
[ 2032.022901] iw_cxgb4: comp_vector 6, irq 209 mask 0xff00
[ 2032.028514] iw_cxgb4: comp_vector 7, irq 210 mask 0xff00
[ 2032.034110] iw_cxgb4: comp_vector 8, irq 211 mask 0xff00
[ 2032.039677] iw_cxgb4: comp_vector 9, irq 212 mask 0xff00
[ 2032.045244] iw_cxgb4: comp_vector 10, irq 213 mask 0xff00
[ 2032.050889] iw_cxgb4: comp_vector 11, irq 214 mask 0xff00
[ 2032.056531] iw_cxgb4: comp_vector 12, irq 215 mask 0xff00
[ 2032.062174] iw_cxgb4: comp_vector 13, irq 216 mask 0xff00
[ 2032.067817] iw_cxgb4: comp_vector 14, irq 217 mask 0xff00
[ 2032.073457] iw_cxgb4: comp_vector 15, irq 218 mask 0xff00
[ 2032.079102] blk_mq_rdma_map_queues: set->mq_map[0] queue 0 vector 0
[ 2032.085621] blk_mq_rdma_map_queues: set->mq_map[1] queue 1 vector 1
[ 2032.092139] blk_mq_rdma_map_queues: set->mq_map[2] queue 2 vector 2
[ 2032.098658] blk_mq_rdma_map_queues: set->mq_map[3] queue 3 vector 3
[ 2032.105177] blk_mq_rdma_map_queues: set->mq_map[4] queue 4 vector 4
[ 2032.111689] blk_mq_rdma_map_queues: set->mq_map[5] queue 5 vector 5
[ 2032.118208] blk_mq_rdma_map_queues: set->mq_map[6] queue 6 vector 6
[ 2032.124728] blk_mq_rdma_map_queues: set->mq_map[7] queue 7 vector 7
[ 2032.131246] blk_mq_rdma_map_queues: set->mq_map[8] queue 15 vector 15
[ 2032.137938] blk_mq_rdma_map_queues: set->mq_map[9] queue 15 vector 15

[ 2032.144629] blk_mq_rdma_map_queues: set->mq_map[10] queue 15 vector 15
[ 2032.151401] blk_mq_rdma_map_queues: set->mq_map[11] queue 15 vector 15
[ 2032.158172] blk_mq_rdma_map_queues: set->mq_map[12] queue 15 vector 15
[ 2032.164940] blk_mq_rdma_map_queues: set->mq_map[13] queue 15 vector 15
[ 2032.171709] blk_mq_rdma_map_queues: set->mq_map[14] queue 15 vector 15
[ 2032.178477] blk_mq_rdma_map_queues: set->mq_map[15] queue 15 vector 15
[ 2032.187409] nvme nvme0: Connect command failed, error wo/DNR bit: -16402
[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

Quoted 82 lines · Steve Wise

Quoted 38 lines · Sagi Grimberg

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

IMO we must fulfil the user wish to connect to N queues and not reduce
it because of affinity overlaps. So in order to push Leon's patch we must also fix the blk_mq_rdma_map_queues to do a best effort

mapping

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

according the affinity and map the rest in naive way (in that way we will *always* map all the queues).

That is what I would expect also.   For example, in my node, where there are
16 cpus, and 2 numa nodes, I observe much better nvmf IOPS

performance by

Quoted 22 lines

Quoted 21 lines

setting up my 16 driver completion event queues such that each is bound to a
node-local cpu.  So I end up with each nodel-local cpu having 2 queues
bound
to it.   W/O adding support in iw_cxgb4 for ib_get_vector_affinity(), this
works fine.   I assumed adding ib_get_vector_affinity() would allow this to
all "just work" by default, but I'm running into this connection failure
issue.

I don't understand exactly what the blk_mq layer is trying to do, but I
assume it has ingress event queues and processing that it trying to align
with the drivers ingress cq event handling, so everybody stays on the same
cpu (or at least node).   But something else is going on.  Is there documentation on how this works somewhere?

Quoted 7 lines

Quoted 1 line

Does this (untested) patch help?

I'm not sure (I'll test it tomorrow) because the issue is the unmapped queues and not the cpus.
for example, if the affinity of q=6 and q=12 returned the same cpumask than q=6 will not be mapped and will fail to connect.

Attached is a patch that applies cleanly for me.  It has problems if vectors have affinity to more than 1 cpu:

[ 2031.988881] iw_cxgb4: comp_vector 0, irq 203 mask 0xff00
[ 2031.994706] iw_cxgb4: comp_vector 1, irq 204 mask 0xff00
[ 2032.000348] iw_cxgb4: comp_vector 2, irq 205 mask 0xff00
[ 2032.005992] iw_cxgb4: comp_vector 3, irq 206 mask 0xff00
[ 2032.011629] iw_cxgb4: comp_vector 4, irq 207 mask 0xff00
[ 2032.017271] iw_cxgb4: comp_vector 5, irq 208 mask 0xff00
[ 2032.022901] iw_cxgb4: comp_vector 6, irq 209 mask 0xff00
[ 2032.028514] iw_cxgb4: comp_vector 7, irq 210 mask 0xff00
[ 2032.034110] iw_cxgb4: comp_vector 8, irq 211 mask 0xff00
[ 2032.039677] iw_cxgb4: comp_vector 9, irq 212 mask 0xff00
[ 2032.045244] iw_cxgb4: comp_vector 10, irq 213 mask 0xff00
[ 2032.050889] iw_cxgb4: comp_vector 11, irq 214 mask 0xff00
[ 2032.056531] iw_cxgb4: comp_vector 12, irq 215 mask 0xff00
[ 2032.062174] iw_cxgb4: comp_vector 13, irq 216 mask 0xff00
[ 2032.067817] iw_cxgb4: comp_vector 14, irq 217 mask 0xff00
[ 2032.073457] iw_cxgb4: comp_vector 15, irq 218 mask 0xff00
[ 2032.079102] blk_mq_rdma_map_queues: set->mq_map[0] queue 0 vector 0
[ 2032.085621] blk_mq_rdma_map_queues: set->mq_map[1] queue 1 vector 1
[ 2032.092139] blk_mq_rdma_map_queues: set->mq_map[2] queue 2 vector 2
[ 2032.098658] blk_mq_rdma_map_queues: set->mq_map[3] queue 3 vector 3
[ 2032.105177] blk_mq_rdma_map_queues: set->mq_map[4] queue 4 vector 4
[ 2032.111689] blk_mq_rdma_map_queues: set->mq_map[5] queue 5 vector 5
[ 2032.118208] blk_mq_rdma_map_queues: set->mq_map[6] queue 6 vector 6
[ 2032.124728] blk_mq_rdma_map_queues: set->mq_map[7] queue 7 vector 7
[ 2032.131246] blk_mq_rdma_map_queues: set->mq_map[8] queue 15 vector 15
[ 2032.137938] blk_mq_rdma_map_queues: set->mq_map[9] queue 15 vector 15
[ 2032.144629] blk_mq_rdma_map_queues: set->mq_map[10] queue 15
vector 15
[ 2032.151401] blk_mq_rdma_map_queues: set->mq_map[11] queue 15
vector 15
[ 2032.158172] blk_mq_rdma_map_queues: set->mq_map[12] queue 15
vector 15
[ 2032.164940] blk_mq_rdma_map_queues: set->mq_map[13] queue 15

Quoted 8 lines

vector 15

[ 2032.171709] blk_mq_rdma_map_queues: set->mq_map[14] queue 15
vector 15
[ 2032.178477] blk_mq_rdma_map_queues: set->mq_map[15] queue 15
vector 15
[ 2032.187409] nvme nvme0: Connect command failed, error wo/DNR bit:
-16402
[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

Quoted 6 lines

Quoted 2 lines

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

Quoted 6 lines

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Thanks for testing this.
I slightly improved the setting of the left CPUs and actually used Sagi's initial proposal.

Sagi,
please review the attached patch and let me know if I should add your signature on it.
I'll run some perf test early next week on it (meanwhile I run login/logout with different num_queues successfully and irq settings).

Steve,
It will be great if you can apply the attached in your system and send your findings.

Regards,
Max,

Quoted 28 lines · Max Gurtovoy

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

Quoted 6 lines

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Thanks for testing this.
I slightly improved the setting of the left CPUs and actually used Sagi's initial proposal.

Sagi,
please review the attached patch and let me know if I should add your signature on it.
I'll run some perf test early next week on it (meanwhile I run login/logout with different num_queues successfully and irq settings).

Steve,
It will be great if you can apply the attached in your system and send your findings.

Quoted 2 lines

Regards,
Max,

So the conlusion to this thread is that Leon's mlx5 patch needs to wait until this block-mq patch is accepted?

Thanks,
Jason

Quoted 32 lines · Jason Gunthorpe

Quoted 31 lines · Max Gurtovoy

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

Quoted 6 lines

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Thanks for testing this.
I slightly improved the setting of the left CPUs and actually used Sagi's
initial proposal.

Sagi,
please review the attached patch and let me know if I should add your signature on it.
I'll run some perf test early next week on it (meanwhile I run login/logout
with different num_queues successfully and irq settings).

Steve,
It will be great if you can apply the attached in your system and send your
findings.

Quoted 5 lines

Quoted 2 lines

Regards,
Max,

So the conlusion to this thread is that Leon's mlx5 patch needs to wait until this block-mq patch is accepted?

Yes, since nvmf is the only user of this function. Still waiting for comments on the suggested patch :)

Quoted 41 lines · Max Gurtovoy

Quoted 37 lines · Jason Gunthorpe

Quoted 26 lines · Max Gurtovoy

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

Quoted 6 lines

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Thanks for testing this.
I slightly improved the setting of the left CPUs and actually used Sagi's
initial proposal.

Sagi,
please review the attached patch and let me know if I should add your signature on it.
I'll run some perf test early next week on it (meanwhile I run login/logout
with different num_queues successfully and irq settings).

Quoted 7 lines

Steve,
It will be great if you can apply the attached in your system and send your
findings.

Regards,
Max,

So the conlusion to this thread is that Leon's mlx5 patch needs to wait until this block-mq patch is accepted?

Quoted 2 lines

Yes, since nvmf is the only user of this function. Still waiting for comments on the suggested patch :)

Hey Sagi, what do you think of Max's patch?

Max, should you resend this in a form suitable for merging?

Thanks,

Steve.

Yes, since nvmf is the only user of this function. Still waiting for comments on the suggested patch :)

Hi Max,

Quoted 4 lines

Yes, since nvmf is the only user of this function. Still waiting for comments on the suggested patch :)

Sorry for the late response (but I'm on vacation so I have an excuse ;))

I'm thinking that we should avoid trying to find an assignment when stuff like irqbalance daemon is running and changing the affinitization.

This extension was made to apply optimal affinity assignment when the device irq affinity is lined up in a vector per core.

I'm thinking that when we identify this is not the case, we immediately fallback to the default mapping.

1. when we get a mask, if its weight != 1, we fallback.
2. if a queue was left unmapped, we fallback.

Maybe something like the following:

block/blk-mq-rdma.c +9 -2

index 996167f1de18..1ada6211c55e 100644
@@ -35,17 +35,26 @@ int blk_mq_rdma_map_queues(struct blk_mq_tag_set *set,
         const struct cpumask *mask;
         unsigned int queue, cpu;
 
+       /* reset all CPUs mapping */
+       for_each_possible_cpu(cpu)
+               set->mq_map[cpu] = UINT_MAX;
+
         for (queue = 0; queue < set->nr_hw_queues; queue++) {
                 mask = ib_get_vector_affinity(dev, first_vec + queue);
                 if (!mask)
                         goto fallback;
 
-               for_each_cpu(cpu, mask)
-                       set->mq_map[cpu] = queue;
+               if (cpumask_weight(mask) != 1)
+                       goto fallback;
+
+               cpu = cpumask_first(mask);
+               if (set->mq_map[cpu] != UINT_MAX)

Quoted 1 line · Sagi Grimberg

Hi Max,

Hi,

Quoted 7 lines

Quoted 4 lines

Yes, since nvmf is the only user of this function. Still waiting for comments on the suggested patch :)

Sorry for the late response (but I'm on vacation so I have an excuse ;))

NP :) currently the code works..

Quoted 1 line

Quoted 3 lines

I'm thinking that we should avoid trying to find an assignment when stuff like irqbalance daemon is running and changing the affinitization.

but this is exactly what Steve complained and Leon try to fix (and break the connection establishment).
If this is the case and we all agree then we're good without Leon's patch and without our suggestions.

Quoted 5 lines

This extension was made to apply optimal affinity assignment when the device irq affinity is lined up in a vector per core.

Quoted 7 lines

I'm thinking that when we identify this is not the case, we immediately fallback to the default mapping.

1. when we get a mask, if its weight != 1, we fallback.
2. if a queue was left unmapped, we fallback.

Maybe something like the following:

did you test it ? I think it will not work since you need to map all the queues and all the CPUs.

Quoted 33 lines

--

block/blk-mq-rdma.c +12 -3

index 996167f1de18..1ada6211c55e 100644
@@ -35,17 +35,26 @@ int blk_mq_rdma_map_queues(struct blk_mq_tag_set
 *set,
          const struct cpumask *mask;
          unsigned int queue, cpu;
 
+       /* reset all CPUs mapping */
+       for_each_possible_cpu(cpu)
+               set->mq_map[cpu] = UINT_MAX;
+
          for (queue = 0; queue < set->nr_hw_queues; queue++) {
                  mask = ib_get_vector_affinity(dev, first_vec + queue);
                  if (!mask)
                          goto fallback;
 
-               for_each_cpu(cpu, mask)
-                       set->mq_map[cpu] = queue;
+               if (cpumask_weight(mask) != 1)
+                       goto fallback;
+
+               cpu = cpumask_first(mask);
+               if (set->mq_map[cpu] != UINT_MAX)
+                       goto fallback;
+
+               set->mq_map[cpu] = queue;
          }
 
          return 0;
-

Quoted 4 lines

  fallback:
         return blk_mq_map_queues(set);
  }
--

see attached another algorithem that can improve the mapping (although it's not a short one)...

it will try to map according to affinity mask, and also in case the mask weight > 1 it will try to be better than the naive mapping I suggest in the previous email.

Quoted 18 lines · Max Gurtovoy

Quoted 1 line · Sagi Grimberg

Hi Max,

Hi,

Quoted 7 lines

Quoted 4 lines

Yes, since nvmf is the only user of this function. Still waiting for comments on the suggested patch :)

Sorry for the late response (but I'm on vacation so I have an excuse ;))

NP :) currently the code works..

Quoted 1 line

Quoted 9 lines

Quoted 3 lines

I'm thinking that we should avoid trying to find an assignment when stuff like irqbalance daemon is running and changing the affinitization.

but this is exactly what Steve complained and Leon try to fix (and break the connection establishment).
If this is the case and we all agree then we're good without Leon's patch and without our suggestions.

I don't agree.  Currently setting certain affinity mappings breaks nvme connectivity.  I don't think that is desirable.  And mlx5 is broken in that it doesn't allow changing the affinity but silently ignores the change, which misleads the admin or irqbalance...

Let me know if you want me to try this or any particular fix.

Quoted 2 lines

Let me know if you want me to try this or any particular fix.

Steve, can you test this one?

Signature · 24 lines

[PATCH rfc] block: fix rdma queue mapping

nvme-rdma attempts to map queues based on irq vector affinity.
However, for some devices, completion vector irq affinity is
configurable by the user which can break the existing assumption
that irq vectors are optimally arranged over the host cpu cores.

So we map queues in two stages:
First map queues according to corresponding to the completion
vector IRQ affinity taking the first cpu in the vector affinity map.
if the current irq affinity is arranged such that a vector is not
assigned to any distinct cpu, we map it to a cpu that is on the same
node. If numa affinity can not be sufficed, we map it to any unmapped
cpu we can find. Then, map the remaining cpus in the possible cpumap
naively.

Signed-off-by: Sagi Grimberg <[email protected]>
---
Steve, can you test out this patch?
 block/blk-mq-cpumap.c  | 39 +++++++++++++-----------
 block/blk-mq-rdma.c    | 80
+++++++++++++++++++++++++++++++++++++++++++-------
 include/linux/blk-mq.h |  1 +
 3 files changed, 93 insertions(+), 27 deletions(-)

block/blk-mq-cpumap.c +2 -6

index 3eb169f15842..34811db8cba9 100644
@@ -30,30 +30,35 @@ static int get_first_sibling(unsigned int cpu)
         return cpu;
  }
 
-int blk_mq_map_queues(struct blk_mq_tag_set *set)
+void blk_mq_map_queue_cpu(struct blk_mq_tag_set *set, unsigned int cpu)
  {
         unsigned int *map = set->mq_map;
         unsigned int nr_queues = set->nr_hw_queues;
-       unsigned int cpu, first_sibling;
+       unsigned int first_sibling;
 
-       for_each_possible_cpu(cpu) {
-               /*
-                * First do sequential mapping between CPUs and queues.
-                * In case we still have CPUs to map, and we have some

number of
-                * threads per cores then map sibling threads to the same queue for
-                * performace optimizations.
-                */
-               if (cpu < nr_queues) {
+       /*
+        * First do sequential mapping between CPUs and queues. +        * In case we still have CPUs to map, and we have some number of +        * threads per cores then map sibling threads to the same queue for
+        * performace optimizations.
+        */
+       if (cpu < nr_queues) {
+               map[cpu] = cpu_to_queue_index(nr_queues, cpu); +       } else {
+               first_sibling = get_first_sibling(cpu);
+               if (first_sibling == cpu)
                        map[cpu] = cpu_to_queue_index(nr_queues, cpu); -               } else {
-                       first_sibling = get_first_sibling(cpu); -                       if (first_sibling == cpu)
-                               map[cpu] = cpu_to_queue_index(nr_queues, cpu);
-                       else
-                               map[cpu] = map[first_sibling]; -               }
+               else
+                       map[cpu] = map[first_sibling];
        }

block/blk-mq-cpumap.c +7 -0

+}
+
+int blk_mq_map_queues(struct blk_mq_tag_set *set)
+{
+       unsigned int cpu;
 
+       for_each_possible_cpu(cpu)
+               blk_mq_map_queue_cpu(set, cpu);

        return 0;
 }
 EXPORT_SYMBOL_GPL(blk_mq_map_queues);

block/blk-mq-rdma.c +55 -0

index 996167f1de18..d04cbb1925f5 100644
@@ -14,6 +14,61 @@
  #include <linux/blk-mq-rdma.h>
  #include <rdma/ib_verbs.h>
 
+static int blk_mq_rdma_map_queue(struct blk_mq_tag_set *set,
+               struct ib_device *dev, int first_vec, unsigned int queue)
+{
+       const struct cpumask *mask;
+       unsigned int cpu;
+       bool mapped = false;
+
+       mask = ib_get_vector_affinity(dev, first_vec + queue);
+       if (!mask)
+               return -ENOTSUPP;
+
+       /* map with an unmapped cpu according to affinity mask */
+       for_each_cpu(cpu, mask) {
+               if (set->mq_map[cpu] == UINT_MAX) {
+                       set->mq_map[cpu] = queue;
+                       mapped = true;
+                       break;
+               }
+       }
+
+       if (!mapped) {
+               int n;
+
+               /* map with an unmapped cpu in the same numa node */
+               for_each_node(n) {
+                       const struct cpumask *node_cpumask =
 cpumask_of_node(n);
+
+                       if (!cpumask_intersects(mask, node_cpumask))
+                               continue;
+
+                       for_each_cpu(cpu, node_cpumask) {
+                               if (set->mq_map[cpu] == UINT_MAX) {
+                                       set->mq_map[cpu] = queue;
+                                       mapped = true;
+                                       break;
+                               }
+                       }
+               }
+       }
+
+       if (!mapped) {
+               /* map with any unmapped cpu we can find */
+               for_each_possible_cpu(cpu) {
+                       if (set->mq_map[cpu] == UINT_MAX) {
+                               set->mq_map[cpu] = queue;
+                               mapped = true;
+                               break;
+                       }
+               }
+       }
+
+       WARN_ON_ONCE(!mapped);
+       return 0;
+}
+
  /**
   * blk_mq_rdma_map_queues - provide a default queue mapping for rdma

device
  * @set:       tagset to provide the mapping for

block/blk-mq-rdma.c +0 -0

@@ -21,31 +76,36 @@
   * @first_vec: first interrupt vectors to use for queues (usually 0)
   *
   * This function assumes the rdma device @dev has at least as many

available
- * interrupt vetors as @set has queues.  It will then query it's affinity mask
- * and built queue mapping that maps a queue to the CPUs that have irq affinity
- * for the corresponding vector.
+ * interrupt vetors as @set has queues.  It will then query vector affinity mask
+ * and attempt to build irq affinity aware queue mappings. If optimal affinity
+ * aware mapping cannot be acheived for a given queue, we look for any unmapped
+ * cpu to map it. Lastly, we map naively all other unmapped cpus in the mq_map.
  *
  * In case either the driver passed a @dev with less vectors than
  * @set->nr_hw_queues, or @dev does not provide an affinity mask for a
  * vector, we fallback to the naive mapping.
  */
 int blk_mq_rdma_map_queues(struct blk_mq_tag_set *set,
-               struct ib_device *dev, int first_vec)
+                struct ib_device *dev, int first_vec)
 {
-       const struct cpumask *mask;
        unsigned int queue, cpu;

+       /* reset cpu mapping */
+       for_each_possible_cpu(cpu)
+               set->mq_map[cpu] = UINT_MAX;
+
        for (queue = 0; queue < set->nr_hw_queues; queue++) { -               mask = ib_get_vector_affinity(dev, first_vec + queue); -               if (!mask)
+               if (blk_mq_rdma_map_queue(set, dev, first_vec, queue)) goto fallback;
+       }

-               for_each_cpu(cpu, mask)
-                       set->mq_map[cpu] = queue; +       /* map any remaining unmapped cpus */
+       for_each_possible_cpu(cpu) {
+               if (set->mq_map[cpu] == UINT_MAX) +                       blk_mq_map_queue_cpu(set, cpu);; }

        return 0;
-
 fallback:
        return blk_mq_map_queues(set);
 }

include/linux/blk-mq.h +1 -0

index d710e92874cc..6eb09c4de34f 100644
@@ -285,6 +285,7 @@ int blk_mq_freeze_queue_wait_timeout(struct
 request_queue *q,
                                      unsigned long timeout);
 
  int blk_mq_map_queues(struct blk_mq_tag_set *set);
+void blk_mq_map_queue_cpu(struct blk_mq_tag_set *set, unsigned int cpu);

 void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues);

 void blk_mq_quiesce_queue_nowait(struct request_queue *q);

Quoted 4 lines

Quoted 2 lines

Let me know if you want me to try this or any particular fix.

Steve, can you test this one?

Yes! I'll try it out tomorrow.

Stevo

Hey Sagi,

The patch works allowing connections for the various affinity mappings below:

One comp_vector per core across all cores, starting with numa-local cores:

Quoted 6 lines

Hey Sagi,

The patch works allowing connections for the various affinity mappings below:

One comp_vector per core across all cores, starting with numa-local cores:

Thanks Steve, is this your "Tested by:" tag?

The new patchworks doesn't grab patches inlined in messages, so you will need to resend it.

Also, can someone remind me what the outcome is here? Does it supersede Leon's patch:

https://patchwork.kernel.org/patch/10526167/

Quoted 5 lines

Hey Sagi,

The patch works allowing connections for the various affinity mappings

below:

Quoted 2 lines

One comp_vector per core across all cores, starting with numa-local cores:

Thanks Steve, is this your "Tested by:" tag?

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

Quoted 6 lines

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Thanks for testing this.
I slightly improved the setting of the left CPUs and actually used Sagi's initial proposal.

Sagi,
please review the attached patch and let me know if I should add your signature on it.
I'll run some perf test early next week on it (meanwhile I run login/logout with different num_queues successfully and irq settings).

Steve,
It will be great if you can apply the attached in your system and send your findings.

Quoted 20 lines · Max Gurtovoy

Quoted 10 lines

Quoted 4 lines

Quoted 1 line

[ 2032.194376] nvme nvme0: failed to connect queue: 9 ret=-18

queue 9 is not mapped (overlap).
please try the bellow:

This seems to work.  Here are three mapping cases:  each vector on its own cpu, each vector on 1 cpu within the local numa node, and each vector having all cpus in its numa node.  The 2nd mapping looks kinda funny, but I think it achieved what you wanted?  And all the cases resulted in successful connections.

Thanks for testing this.
I slightly improved the setting of the left CPUs and actually used Sagi's initial proposal.

Sagi,
please review the attached patch and let me know if I should add your signature on it.
I'll run some perf test early next week on it (meanwhile I run login/logout with different num_queues successfully and irq settings).

Quoted 3 lines

Steve,
It will be great if you can apply the attached in your system and send your findings.

Sorry, I got side tracked.  I'll try and test this today and report back.

Steve.