nvme-rdma: support devices with queue size < 32

drivers/nvme/host/rdma.c static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc) · line 1042

Quoted 11 lines

drivers/nvme/host/rdma.c +11 -0

+static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int sig_limit;
+
+       /* We signal completion every queue depth/2 and also
+        * handle the case of possible device with queue_depth=1,
+        * where we would need to signal every message.
+        */
+       sig_limit = max(queue->queue_size / 2, 1);
+       return (++queue->sig_count % sig_limit) == 0;
+}

I would love if we could do this at the ib_qp level. I also know you found a real bug and we want it fixed ASAP, so maybe for now we should merge this one:

Reviewed-by Christoph Hellwig <hch at lst.de>

and then do the generic version. Marta, do you want to tackle the generic version or should I look into this once I get a little time.

Quoted 5 lines

drivers/nvme/host/rdma.c +5 -0

+static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int sig_limit;
+
+       /* We signal completion every queue depth/2 and also

drivers/nvme/host/rdma.c static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc) · line 1042

Quoted 6 lines

drivers/nvme/host/rdma.c +6 -0

+        * handle the case of possible device with queue_depth=1,
+        * where we would need to signal every message.
+        */
+       sig_limit = max(queue->queue_size / 2, 1);
+       return (++queue->sig_count % sig_limit) == 0;
+}

I would love if we could do this at the ib_qp level.

Quoted 1 line

I would love if we could do this at the ib_qp level.

Hello Christoph,

Please keep in mind that some but not all RDMA drivers need a construct like this.

In the case of small NVMe-oF queue size (<32) we may enter a deadlock caused by the fact that the IB completions aren't sent waiting for 32 and the send queue will fill up.

The error is seen as (using mlx5):

[ 2048.693355] mlx5_0:mlx5_ib_post_send:3765:(pid 7273):
[ 2048.693360] nvme nvme1: nvme_rdma_post_send failed with error code -12

This patch changes the way the signaling is done so that it depends on the queue depth now. The magic define has been removed completely.

Signed-off-by Marta Rybczynska <marta.rybczynska at kalray.eu>

Signed-off-by Samuel Jones <sjones at kalray.eu>

---
Changes from v1:
* signal by queue size/2, remove hardcoded 32
* support queue depth of 1

 drivers/nvme/host/rdma.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/nvme/host/rdma.c +12 -3

index 47a479f..4de1b92 100644
@@ -1029,6 +1029,18 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
                nvme_rdma_wr_error(cq, wc, "SEND");
 }
 
+static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int sig_limit;
+
+       /* We signal completion every queue depth/2 and also
+        * handle the case of possible device with queue_depth=1,
+        * where we would need to signal every message.
+        */
+       sig_limit = max(queue->queue_size / 2, 1);
+       return (++queue->sig_count % sig_limit) == 0;
+}
+
 static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
                struct nvme_rdma_qe *qe, struct ib_sge *sge, u32 num_sge,
                struct ib_send_wr *first, bool flush)

@@ -1056,9 +1068,6 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
         * Would have been way to obvious to handle this in hardware or
         * at least the RDMA stack..
         *
-        * This messy and racy code sniplet is copy and pasted from the iSER
-        * initiator, and the magic '32' comes from there as well.
-        *
         * Always signal the flushes. The magic request used for the flush
         * sequencer is not allocated in our driver's tagset and it's
         * triggered to be freed by blk_cleanup_queue(). So we need to

Quoted 66 lines

In the case of small NVMe-oF queue size (<32) we may enter a deadlock caused by the fact that the IB completions aren't sent waiting for 32 and the send queue will fill up.

The error is seen as (using mlx5):

[ 2048.693355] mlx5_0:mlx5_ib_post_send:3765:(pid 7273):
[ 2048.693360] nvme nvme1: nvme_rdma_post_send failed with error code -12

This patch changes the way the signaling is done so that it depends on the queue depth now. The magic define has been removed completely.

Signed-off-by Marta Rybczynska <marta.rybczynska at kalray.eu>

Signed-off-by Samuel Jones <sjones at kalray.eu>

---
Changes from v1:
* signal by queue size/2, remove hardcoded 32
* support queue depth of 1

 drivers/nvme/host/rdma.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/nvme/host/rdma.c +12 -0

index 47a479f..4de1b92 100644
@@ -1029,6 +1029,18 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct
 ib_wc *wc)
                nvme_rdma_wr_error(cq, wc, "SEND");
 }
 
+static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int sig_limit;
+
+       /* We signal completion every queue depth/2 and also
+        * handle the case of possible device with queue_depth=1,
+        * where we would need to signal every message.
+        */
+       sig_limit = max(queue->queue_size / 2, 1);
+       return (++queue->sig_count % sig_limit) == 0;
+}
+
 static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
                struct nvme_rdma_qe *qe, struct ib_sge *sge, u32 num_sge,

struct ib_send_wr *first, bool flush)

drivers/nvme/host/rdma.c +0 -3

@@ -1056,9 +1068,6 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue
 *queue,
         * Would have been way to obvious to handle this in hardware or
         * at least the RDMA stack..
         *
-        * This messy and racy code sniplet is copy and pasted from the iSER
-        * initiator, and the magic '32' comes from there as well.
-        *
         * Always signal the flushes. The magic request used for the flush
         * sequencer is not allocated in our driver's tagset and it's

* triggered to be freed by blk_cleanup_queue(). So we need to

drivers/nvme/host/rdma.c +1 -1

@@ -1066,7 +1075,7 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue
 *queue,
         * embedded in request's payload, is not freed when __ib_process_cq()
         * calls wr_cqe->done().
         */
-       if ((++queue->sig_count % 32) == 0 || flush)
+       if (nvme_rdma_queue_sig_limit(queue) || flush)
                wr.send_flags |= IB_SEND_SIGNALED;
 

Quoted 1 line

if (first)

Hello Marta,

The approach of this patch is suboptimal from a performance point of view. If the number of WRs that have been submitted since the last signaled WR was submitted would be tracked in a member variable that would allow to get rid of the (relatively slow) division operation.

Quoted 67 lines · Marta Rybczynska

Quoted 66 lines

In the case of small NVMe-oF queue size (<32) we may enter a deadlock caused by the fact that the IB completions aren't sent waiting for 32 and the send queue will fill up.

The error is seen as (using mlx5):

[ 2048.693355] mlx5_0:mlx5_ib_post_send:3765:(pid 7273):
[ 2048.693360] nvme nvme1: nvme_rdma_post_send failed with error code -12

This patch changes the way the signaling is done so that it depends on the queue depth now. The magic define has been removed completely.

Signed-off-by Marta Rybczynska <marta.rybczynska at kalray.eu>

Signed-off-by Samuel Jones <sjones at kalray.eu>

---
Changes from v1:
* signal by queue size/2, remove hardcoded 32
* support queue depth of 1

 drivers/nvme/host/rdma.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/nvme/host/rdma.c +12 -0

index 47a479f..4de1b92 100644
@@ -1029,6 +1029,18 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct
 ib_wc *wc)
                nvme_rdma_wr_error(cq, wc, "SEND");
 }
 
+static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int sig_limit;
+
+       /* We signal completion every queue depth/2 and also
+        * handle the case of possible device with queue_depth=1,
+        * where we would need to signal every message.
+        */
+       sig_limit = max(queue->queue_size / 2, 1);
+       return (++queue->sig_count % sig_limit) == 0;
+}
+
 static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
                struct nvme_rdma_qe *qe, struct ib_sge *sge, u32 num_sge,

struct ib_send_wr *first, bool flush)

drivers/nvme/host/rdma.c +0 -3

@@ -1056,9 +1068,6 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue
 *queue,
         * Would have been way to obvious to handle this in hardware or
         * at least the RDMA stack..
         *
-        * This messy and racy code sniplet is copy and pasted from the iSER
-        * initiator, and the magic '32' comes from there as well.
-        *
         * Always signal the flushes. The magic request used for the flush
         * sequencer is not allocated in our driver's tagset and it's

* triggered to be freed by blk_cleanup_queue(). So we need to

drivers/nvme/host/rdma.c +1 -1

@@ -1066,7 +1075,7 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue
 *queue,
         * embedded in request's payload, is not freed when __ib_process_cq()
         * calls wr_cqe->done().
         */
-       if ((++queue->sig_count % 32) == 0 || flush)
+       if (nvme_rdma_queue_sig_limit(queue) || flush)
                wr.send_flags |= IB_SEND_SIGNALED;
 

Quoted 9 lines

Quoted 1 line

if (first)

Hello Marta,

The approach of this patch is suboptimal from a performance point of view. If the number of WRs that have been submitted since the last signaled WR was submitted would be tracked in a member variable that would allow to get rid of the (relatively slow) division operation.

Hello Bart,
I think that we can remove the division (the modulo sig_limit). The sig_count is an u8 so it is really a type of variable you propose. It isn't used anywhere it seems so we can change the way it is used in the snippet to count until the signaling moment. It will give something like:

static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)

{
       int sig_limit;

       /* We signal completion every queue depth/2 and also
        * handle the case of possible device with queue_depth=1,
        * where we would need to signal every message.
        */
       sig_limit = max(queue->queue_size / 2, 1);
       queue->sig_count++;
       if (queue->sig_count < sig_limit)
           return 0;
       queue->sig_count = 0;
       return 1;
}

Do you like it better?

Marta

Quoted 100 lines

Quoted 67 lines · Marta Rybczynska

Quoted 66 lines

In the case of small NVMe-oF queue size (<32) we may enter a deadlock caused by the fact that the IB completions aren't sent waiting for 32 and the send queue will fill up.

The error is seen as (using mlx5):

[ 2048.693355] mlx5_0:mlx5_ib_post_send:3765:(pid 7273):
[ 2048.693360] nvme nvme1: nvme_rdma_post_send failed with error code -12

This patch changes the way the signaling is done so that it depends on the queue depth now. The magic define has been removed completely.

Signed-off-by Marta Rybczynska <marta.rybczynska at kalray.eu>

Signed-off-by Samuel Jones <sjones at kalray.eu>

---
Changes from v1:
* signal by queue size/2, remove hardcoded 32
* support queue depth of 1

 drivers/nvme/host/rdma.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/nvme/host/rdma.c +12 -0

index 47a479f..4de1b92 100644
@@ -1029,6 +1029,18 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct
 ib_wc *wc)
                nvme_rdma_wr_error(cq, wc, "SEND");
 }
 
+static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int sig_limit;
+
+       /* We signal completion every queue depth/2 and also
+        * handle the case of possible device with queue_depth=1,
+        * where we would need to signal every message.
+        */
+       sig_limit = max(queue->queue_size / 2, 1);
+       return (++queue->sig_count % sig_limit) == 0;
+}
+
 static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
                struct nvme_rdma_qe *qe, struct ib_sge *sge, u32 num_sge,

struct ib_send_wr *first, bool flush)

drivers/nvme/host/rdma.c +0 -3

@@ -1056,9 +1068,6 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue
 *queue,
         * Would have been way to obvious to handle this in hardware or
         * at least the RDMA stack..
         *
-        * This messy and racy code sniplet is copy and pasted from the iSER
-        * initiator, and the magic '32' comes from there as well.
-        *
         * Always signal the flushes. The magic request used for the flush
         * sequencer is not allocated in our driver's tagset and it's

* triggered to be freed by blk_cleanup_queue(). So we need to

drivers/nvme/host/rdma.c +1 -1

@@ -1066,7 +1075,7 @@ static int nvme_rdma_post_send(struct nvme_rdma_queue
 *queue,
         * embedded in request's payload, is not freed when __ib_process_cq()
         * calls wr_cqe->done().
         */
-       if ((++queue->sig_count % 32) == 0 || flush)
+       if (nvme_rdma_queue_sig_limit(queue) || flush)
                wr.send_flags |= IB_SEND_SIGNALED;
 

Quoted 9 lines

Quoted 1 line

if (first)

Hello Marta,

The approach of this patch is suboptimal from a performance point of view. If the number of WRs that have been submitted since the last signaled WR was submitted would be tracked in a member variable that would allow to get rid of the (relatively slow) division operation.

Hello Bart,
I think that we can remove the division (the modulo sig_limit). The sig_count is an u8 so it is really a type of variable you propose. It isn't used anywhere it seems so we can change the way it is used in the snippet to count until the signaling moment. It will give something like:

static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)

{
       int sig_limit;

       /* We signal completion every queue depth/2 and also
        * handle the case of possible device with queue_depth=1,
        * where we would need to signal every message.
        */
       sig_limit = max(queue->queue_size / 2, 1);
       queue->sig_count++;
       if (queue->sig_count < sig_limit)
           return 0;
       queue->sig_count = 0;
       return 1;
}

Quoted 1 line

Do you like it better?

Hi Marta,
I think that Bart (and I agree) meant to avoid doing the division in the fast path. You can set a new variable to rdma_ctrl called sig_limit and set once it during initialization stage. Then just do:

if ((++queue->sig_count % queue->ctrl->sig_limit) == 0 || flush)

in nvme_rdma_post_send function.

Also, as you mentioned sig_count is u8 so you need to avoid setting the sig_limit to a bigger value than 255. So please take a minimum of 32 (or any other suggestions ?? ) and the result of max(queue->queue_size / 2, 1);

I think that we can remove the division (the modulo sig_limit). The sig_count is an u8 so it is really a type of variable you propose. It isn't used anywhere it seems so we can change the way it is used in the snippet to count until the signaling moment. It will give something like:

static inline nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)

{
       int sig_limit;

       /* We signal completion every queue depth/2 and also
        * handle the case of possible device with queue_depth=1,
        * where we would need to signal every message.
        */
       sig_limit = max(queue->queue_size / 2, 1);
       queue->sig_count++;
       if (queue->sig_count < sig_limit)
           return 0;
       queue->sig_count = 0;
       return 1;
}

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

BTW, did you test with deeper queue depths (say 512)?

Quoted 5 lines

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

BTW, did you test with deeper queue depths (say 512)?

Wait, taking it back...

Can you make nvme_rdma_queue_sig_limit() return a bool instead?

Also, Looking at this closer, I'm pretty convinced that this should convert to atomic. For iSER its fine as is because we are under the iscsi connection lock, but here we need to handle mutual exclusion.

This would be an incremental change though.

I've hand applies this once with a few formatting tweaks now that I got the ACK from Sagi. ?'d love to see if someone could figure out how we could move the improved atomic + ilog version into the rdma core somehow.

Quoted 4 lines

I've hand applies this once with a few formatting tweaks now that I got the ACK from Sagi. ?'d love to see if someone could figure out how we could move the improved atomic + ilog version into the rdma core somehow.

I'll submit the atomic+ilog version in nvme next week. I have no access to the test systems this week.