[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

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 signalling is done so
that it depends on the queue depth now. The magic define has
been removed completely. It also reworks the signalling
code to use atomic operations.

Signed-off-by: Marta Rybczynska <marta.rybczynska-FNhOzJFKnXGHXe+LvDLADg@public.gmane.org>
Signed-off-by: Samuel Jones <sjones-FNhOzJFKnXGHXe+LvDLADg@public.gmane.org>
[v1]

---

Changes in v4:
* use atomic operations as suggested by Sagi

Changes in v3:
* avoid division in the fast path
* reverse sig_count logic to simplify the code: it now counts down
  from the queue depth/2 to 0
* change sig_count to int to avoid overflows for big queues

Changes in v2:
* signal by queue size/2, remove hardcoded 32
* support queue depth of 1
---
 drivers/nvme/host/rdma.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 16f84eb..234b010 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -88,7 +88,7 @@ enum nvme_rdma_queue_flags {
 
 struct nvme_rdma_queue {
        struct nvme_rdma_qe     *rsp_ring;
-       u8                      sig_count;
+       atomic_t                sig_count;
        int                     queue_size;
        size_t                  cmnd_capsule_len;
        struct nvme_rdma_ctrl   *ctrl;
@@ -257,6 +257,15 @@ static int nvme_rdma_wait_for_cm(struct nvme_rdma_queue *queue)
        return queue->cm_error;
 }
 
+static inline int nvme_rdma_init_sig_count(int queue_size)
+{
+       /* 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.
+        */
+       return max(queue_size / 2, 1);
+}
+
 static int nvme_rdma_create_qp(struct nvme_rdma_queue *queue, const int factor)
 {
        struct nvme_rdma_device *dev = queue->device;
@@ -561,6 +570,8 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 
        queue->queue_size = queue_size;
 
+       atomic_set(&queue->sig_count, nvme_rdma_init_sig_count(queue_size));
+
        queue->cm_id = rdma_create_id(&init_net, nvme_rdma_cm_handler, queue,
                        RDMA_PS_TCP, IB_QPT_RC);
        if (IS_ERR(queue->cm_id)) {
@@ -1029,6 +1040,28 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
                nvme_rdma_wr_error(cq, wc, "SEND");
 }
 
+static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int v, old;
+
+       v = atomic_read(&queue->sig_count);
+       while (1) {
+               if (v > 1) {
+                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
+                       if (old == v)
+                               return false;
+               } else {
+                       int new_count;
+
+                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
+                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
+                       if (old == v)
+                               return true;
+               }
+               v = old;
+       }
+}
+
 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 +1089,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
@@ -1066,7 +1096,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;
 
        if (first)
-- 
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

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 signalling is done so
that it depends on the queue depth now. The magic define has
been removed completely. It also reworks the signalling
code to use atomic operations.

Signed-off-by: Marta Rybczynska <marta.rybczynska at kalray.eu>
Signed-off-by: Samuel Jones <sjones at kalray.eu>
[v1]

---

Changes in v4:
* use atomic operations as suggested by Sagi

Changes in v3:
* avoid division in the fast path
* reverse sig_count logic to simplify the code: it now counts down
  from the queue depth/2 to 0
* change sig_count to int to avoid overflows for big queues

Changes in v2:
* signal by queue size/2, remove hardcoded 32
* support queue depth of 1
---
 drivers/nvme/host/rdma.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 16f84eb..234b010 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -88,7 +88,7 @@ enum nvme_rdma_queue_flags {
 
 struct nvme_rdma_queue {
        struct nvme_rdma_qe     *rsp_ring;
-       u8                      sig_count;
+       atomic_t                sig_count;
        int                     queue_size;
        size_t                  cmnd_capsule_len;
        struct nvme_rdma_ctrl   *ctrl;
@@ -257,6 +257,15 @@ static int nvme_rdma_wait_for_cm(struct nvme_rdma_queue *queue)
        return queue->cm_error;
 }
 
+static inline int nvme_rdma_init_sig_count(int queue_size)
+{
+       /* 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.
+        */
+       return max(queue_size / 2, 1);
+}
+
 static int nvme_rdma_create_qp(struct nvme_rdma_queue *queue, const int factor)
 {
        struct nvme_rdma_device *dev = queue->device;
@@ -561,6 +570,8 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 
        queue->queue_size = queue_size;
 
+       atomic_set(&queue->sig_count, nvme_rdma_init_sig_count(queue_size));
+
        queue->cm_id = rdma_create_id(&init_net, nvme_rdma_cm_handler, queue,
                        RDMA_PS_TCP, IB_QPT_RC);
        if (IS_ERR(queue->cm_id)) {
@@ -1029,6 +1040,28 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
                nvme_rdma_wr_error(cq, wc, "SEND");
 }
 
+static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
+{
+       int v, old;
+
+       v = atomic_read(&queue->sig_count);
+       while (1) {
+               if (v > 1) {
+                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
+                       if (old == v)
+                               return false;
+               } else {
+                       int new_count;
+
+                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
+                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
+                       if (old == v)
+                               return true;
+               }
+               v = old;
+       }
+}
+
 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 +1089,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
@@ -1066,7 +1096,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;
 
        if (first)
-- 
1.8.3.1

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 5208 bytes --]

On Wed, May 03, 2017 at 12:05:15PM +0200, Marta Rybczynska wrote:
> 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 signalling is done so
> that it depends on the queue depth now. The magic define has
> been removed completely. It also reworks the signalling
> code to use atomic operations.
>
> Signed-off-by: Marta Rybczynska <marta.rybczynska-FNhOzJFKnXGHXe+LvDLADg@public.gmane.org>
> Signed-off-by: Samuel Jones <sjones-FNhOzJFKnXGHXe+LvDLADg@public.gmane.org>
> [v1]

^^^^ This part of commit message is not needed.
Thanks

>
> ---
>
> Changes in v4:
> * use atomic operations as suggested by Sagi
>
> Changes in v3:
> * avoid division in the fast path
> * reverse sig_count logic to simplify the code: it now counts down
>   from the queue depth/2 to 0
> * change sig_count to int to avoid overflows for big queues
>
> Changes in v2:
> * signal by queue size/2, remove hardcoded 32
> * support queue depth of 1
> ---
>  drivers/nvme/host/rdma.c | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 16f84eb..234b010 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -88,7 +88,7 @@ enum nvme_rdma_queue_flags {
>
>  struct nvme_rdma_queue {
>         struct nvme_rdma_qe     *rsp_ring;
> -       u8                      sig_count;
> +       atomic_t                sig_count;
>         int                     queue_size;
>         size_t                  cmnd_capsule_len;
>         struct nvme_rdma_ctrl   *ctrl;
> @@ -257,6 +257,15 @@ static int nvme_rdma_wait_for_cm(struct nvme_rdma_queue *queue)
>         return queue->cm_error;
>  }
>
> +static inline int nvme_rdma_init_sig_count(int queue_size)
> +{
> +       /* 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.
> +        */
> +       return max(queue_size / 2, 1);
> +}
> +
>  static int nvme_rdma_create_qp(struct nvme_rdma_queue *queue, const int factor)
>  {
>         struct nvme_rdma_device *dev = queue->device;
> @@ -561,6 +570,8 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
>
>         queue->queue_size = queue_size;
>
> +       atomic_set(&queue->sig_count, nvme_rdma_init_sig_count(queue_size));
> +
>         queue->cm_id = rdma_create_id(&init_net, nvme_rdma_cm_handler, queue,
>                         RDMA_PS_TCP, IB_QPT_RC);
>         if (IS_ERR(queue->cm_id)) {
> @@ -1029,6 +1040,28 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
>                 nvme_rdma_wr_error(cq, wc, "SEND");
>  }
>
> +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
> +{
> +       int v, old;
> +
> +       v = atomic_read(&queue->sig_count);
> +       while (1) {
> +               if (v > 1) {
> +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
> +                       if (old == v)
> +                               return false;
> +               } else {
> +                       int new_count;
> +
> +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
> +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
> +                       if (old == v)
> +                               return true;
> +               }
> +               v = old;
> +       }
> +}
> +
>  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 +1089,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
> @@ -1066,7 +1096,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;
>
>         if (first)
> --
> 1.8.3.1
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Leon Romanovsky]

On Wed, May 03, 2017@12:05:15PM +0200, Marta Rybczynska wrote:
> 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 signalling is done so
> that it depends on the queue depth now. The magic define has
> been removed completely. It also reworks the signalling
> code to use atomic operations.
>
> Signed-off-by: Marta Rybczynska <marta.rybczynska at kalray.eu>
> Signed-off-by: Samuel Jones <sjones at kalray.eu>
> [v1]

^^^^ This part of commit message is not needed.
Thanks

>
> ---
>
> Changes in v4:
> * use atomic operations as suggested by Sagi
>
> Changes in v3:
> * avoid division in the fast path
> * reverse sig_count logic to simplify the code: it now counts down
>   from the queue depth/2 to 0
> * change sig_count to int to avoid overflows for big queues
>
> Changes in v2:
> * signal by queue size/2, remove hardcoded 32
> * support queue depth of 1
> ---
>  drivers/nvme/host/rdma.c | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 16f84eb..234b010 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -88,7 +88,7 @@ enum nvme_rdma_queue_flags {
>
>  struct nvme_rdma_queue {
>         struct nvme_rdma_qe     *rsp_ring;
> -       u8                      sig_count;
> +       atomic_t                sig_count;
>         int                     queue_size;
>         size_t                  cmnd_capsule_len;
>         struct nvme_rdma_ctrl   *ctrl;
> @@ -257,6 +257,15 @@ static int nvme_rdma_wait_for_cm(struct nvme_rdma_queue *queue)
>         return queue->cm_error;
>  }
>
> +static inline int nvme_rdma_init_sig_count(int queue_size)
> +{
> +       /* 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.
> +        */
> +       return max(queue_size / 2, 1);
> +}
> +
>  static int nvme_rdma_create_qp(struct nvme_rdma_queue *queue, const int factor)
>  {
>         struct nvme_rdma_device *dev = queue->device;
> @@ -561,6 +570,8 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
>
>         queue->queue_size = queue_size;
>
> +       atomic_set(&queue->sig_count, nvme_rdma_init_sig_count(queue_size));
> +
>         queue->cm_id = rdma_create_id(&init_net, nvme_rdma_cm_handler, queue,
>                         RDMA_PS_TCP, IB_QPT_RC);
>         if (IS_ERR(queue->cm_id)) {
> @@ -1029,6 +1040,28 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
>                 nvme_rdma_wr_error(cq, wc, "SEND");
>  }
>
> +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
> +{
> +       int v, old;
> +
> +       v = atomic_read(&queue->sig_count);
> +       while (1) {
> +               if (v > 1) {
> +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
> +                       if (old == v)
> +                               return false;
> +               } else {
> +                       int new_count;
> +
> +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
> +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
> +                       if (old == v)
> +                               return true;
> +               }
> +               v = old;
> +       }
> +}
> +
>  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 +1089,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
> @@ -1066,7 +1096,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;
>
>         if (first)
> --
> 1.8.3.1
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170503/2d93fdcb/attachment.sig>

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Sagi Grimberg]

> +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
> +{
> +       int v, old;
> +
> +       v = atomic_read(&queue->sig_count);
> +       while (1) {
> +               if (v > 1) {
> +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
> +                       if (old == v)
> +                               return false;
> +               } else {
> +                       int new_count;
> +
> +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
> +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
> +                       if (old == v)
> +                               return true;
> +               }
> +               v = old;
> +       }
> +}
> +

Ugh, no...

How about just do:

	if (atomic_inc_return(queue->sig_count) % queue->sig_limit)
		return true;
	return false;

where
	queue->sig_limit = max(queue->queue_size / 2, 1);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Sagi Grimberg]

> +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
> +{
> +       int v, old;
> +
> +       v = atomic_read(&queue->sig_count);
> +       while (1) {
> +               if (v > 1) {
> +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
> +                       if (old == v)
> +                               return false;
> +               } else {
> +                       int new_count;
> +
> +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
> +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
> +                       if (old == v)
> +                               return true;
> +               }
> +               v = old;
> +       }
> +}
> +

Ugh, no...

How about just do:

	if (atomic_inc_return(queue->sig_count) % queue->sig_limit)
		return true;
	return false;

where
	queue->sig_limit = max(queue->queue_size / 2, 1);

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

>> +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
>> +{
>> +       int v, old;
>> +
>> +       v = atomic_read(&queue->sig_count);
>> +       while (1) {
>> +               if (v > 1) {
>> +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
>> +                       if (old == v)
>> +                               return false;
>> +               } else {
>> +                       int new_count;
>> +
>> +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
>> +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
>> +                       if (old == v)
>> +                               return true;
>> +               }
>> +               v = old;
>> +       }
>> +}
>> +
> 
> Ugh, no...
> 
> How about just do:
> 
>	if (atomic_inc_return(queue->sig_count) % queue->sig_limit)
>		return true;
>	return false;
> 
> where
>	queue->sig_limit = max(queue->queue_size / 2, 1);

I tried to avoid that because this adds a division in the fast path Bart
was unhappy about in v2.

Unfortunately we do not have an atomic with on overflow operation like
the one needed here.

Marta
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

>> +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
>> +{
>> +       int v, old;
>> +
>> +       v = atomic_read(&queue->sig_count);
>> +       while (1) {
>> +               if (v > 1) {
>> +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
>> +                       if (old == v)
>> +                               return false;
>> +               } else {
>> +                       int new_count;
>> +
>> +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
>> +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
>> +                       if (old == v)
>> +                               return true;
>> +               }
>> +               v = old;
>> +       }
>> +}
>> +
> 
> Ugh, no...
> 
> How about just do:
> 
>	if (atomic_inc_return(queue->sig_count) % queue->sig_limit)
>		return true;
>	return false;
> 
> where
>	queue->sig_limit = max(queue->queue_size / 2, 1);

I tried to avoid that because this adds a division in the fast path Bart
was unhappy about in v2.

Unfortunately we do not have an atomic with on overflow operation like
the one needed here.

Marta

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03 at 17:19 +0200, Marta Rybczynska wrote:
> > > +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
> > > +{
> > > +       int v, old;
> > > +
> > > +       v = atomic_read(&queue->sig_count);
> > > +       while (1) {
> > > +               if (v > 1) {
> > > +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
> > > +                       if (old == v)
> > > +                               return false;
> > > +               } else {
> > > +                       int new_count;
> > > +
> > > +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
> > > +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
> > > +                       if (old == v)
> > > +                               return true;
> > > +               }
> > > +               v = old;
> > > +       }
> > > +}
> > > +
> > 
> > Ugh, no...
> > 
> > How about just do:
> > 
> > 	if (atomic_inc_return(queue->sig_count) % queue->sig_limit)
> > 		return true;
> > 	return false;
> > 
> > where
> > 	queue->sig_limit = max(queue->queue_size / 2, 1);
> 
> I tried to avoid that because this adds a division in the fast path Bart
> was unhappy about in v2.
> 
> Unfortunately we do not have an atomic with on overflow operation like
> the one needed here.

Hello Marta,

The approach I proposed works well if sig_count is modified by a single thread
at a time. Seeing your code made me realize that it is nontrivial to implement
that approach if multiple threads can change sig_count concurrently. Since
atomic_cmpxchg() is relatively expensive, what Sagi proposed may be a better
alternative. Sorry that I sent you in the wrong direction.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03@17:19 +0200, Marta Rybczynska wrote:
> > > +static inline bool nvme_rdma_queue_sig_limit(struct nvme_rdma_queue *queue)
> > > +{
> > > +       int v, old;
> > > +
> > > +       v = atomic_read(&queue->sig_count);
> > > +       while (1) {
> > > +               if (v > 1) {
> > > +                       old = atomic_cmpxchg(&queue->sig_count, v, v - 1);
> > > +                       if (old == v)
> > > +                               return false;
> > > +               } else {
> > > +                       int new_count;
> > > +
> > > +                       new_count = nvme_rdma_init_sig_count(queue->queue_size);
> > > +                       old = atomic_cmpxchg(&queue->sig_count, v, new_count);
> > > +                       if (old == v)
> > > +                               return true;
> > > +               }
> > > +               v = old;
> > > +       }
> > > +}
> > > +
> > 
> > Ugh, no...
> > 
> > How about just do:
> > 
> > 	if (atomic_inc_return(queue->sig_count) % queue->sig_limit)
> > 		return true;
> > 	return false;
> > 
> > where
> > 	queue->sig_limit = max(queue->queue_size / 2, 1);
> 
> I tried to avoid that because this adds a division in the fast path Bart
> was unhappy about in v2.
> 
> Unfortunately we do not have an atomic with on overflow operation like
> the one needed here.

Hello Marta,

The approach I proposed works well if sig_count is modified by a single thread
at a time. Seeing your code made me realize that it is nontrivial to implement
that approach if multiple threads can change sig_count concurrently. Since
atomic_cmpxchg() is relatively expensive, what Sagi proposed may be a better
alternative. Sorry that I sent you in the wrong direction.

Bart.

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Jason Gunthorpe]

On Wed, May 03, 2017 at 05:19:27PM +0200, Marta Rybczynska wrote:

> > where
> >	queue->sig_limit = max(queue->queue_size / 2, 1);
> 
> I tried to avoid that because this adds a division in the fast path Bart
> was unhappy about in v2.

The compiler switches divide/multiply by powers of two into fast bit shifts.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Jason Gunthorpe]

On Wed, May 03, 2017@05:19:27PM +0200, Marta Rybczynska wrote:

> > where
> >	queue->sig_limit = max(queue->queue_size / 2, 1);
> 
> I tried to avoid that because this adds a division in the fast path Bart
> was unhappy about in v2.

The compiler switches divide/multiply by powers of two into fast bit shifts.

Jason

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03 at 09:53 -0600, Jason Gunthorpe wrote:
> On Wed, May 03, 2017 at 05:19:27PM +0200, Marta Rybczynska wrote:
> 
> > > where
> > > 	queue->sig_limit = max(queue->queue_size / 2, 1);
> > 
> > I tried to avoid that because this adds a division in the fast path Bart
> > was unhappy about in v2.
> 
> The compiler switches divide/multiply by powers of two into fast bit shifts.

Hello Jason,

As far as I know the compiler only does that for compile-time constants. In
this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03@09:53 -0600, Jason Gunthorpe wrote:
> On Wed, May 03, 2017@05:19:27PM +0200, Marta Rybczynska wrote:
> 
> > > where
> > > 	queue->sig_limit = max(queue->queue_size / 2, 1);
> > 
> > I tried to avoid that because this adds a division in the fast path Bart
> > was unhappy about in v2.
> 
> The compiler switches divide/multiply by powers of two into fast bit shifts.

Hello Jason,

As far as I know the compiler only does that for compile-time constants. In
this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.

Bart.

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Sagi Grimberg]

> Hello Jason,
>
> As far as I know the compiler only does that for compile-time constants. In
> this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.

We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
that would only do well with power of 2 sized queues...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Sagi Grimberg]

> Hello Jason,
>
> As far as I know the compiler only does that for compile-time constants. In
> this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.

We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
that would only do well with power of 2 sized queues...

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1255 bytes --]

On 5/3/2017 11:58 AM, Bart Van Assche wrote:
> On Wed, 2017-05-03 at 09:53 -0600, Jason Gunthorpe wrote:
>> On Wed, May 03, 2017 at 05:19:27PM +0200, Marta Rybczynska wrote:
>>
>>>> where
>>>> 	queue->sig_limit = max(queue->queue_size / 2, 1);
>>>
>>> I tried to avoid that because this adds a division in the fast path Bart
>>> was unhappy about in v2.
>>
>> The compiler switches divide/multiply by powers of two into fast bit shifts.
> 
> Hello Jason,
> 
> As far as I know the compiler only does that for compile-time constants. In
> this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.

Sure it is.  The only thing that needs to be constant for the compiler
to do the right thing is the '/ 2' part.  queue_size need not be
constant, and the max is performed after the division.  I would fully
expect the compiler to get this right and convert it internally to the
equivalent bit shift, but if it didn't you could always just write it
that way in the first place:

queue->sig_limit = max(queue->queue_size >> 1, 1);


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Doug Ledford]

On 5/3/2017 11:58 AM, Bart Van Assche wrote:
> On Wed, 2017-05-03@09:53 -0600, Jason Gunthorpe wrote:
>> On Wed, May 03, 2017@05:19:27PM +0200, Marta Rybczynska wrote:
>>
>>>> where
>>>> 	queue->sig_limit = max(queue->queue_size / 2, 1);
>>>
>>> I tried to avoid that because this adds a division in the fast path Bart
>>> was unhappy about in v2.
>>
>> The compiler switches divide/multiply by powers of two into fast bit shifts.
> 
> Hello Jason,
> 
> As far as I know the compiler only does that for compile-time constants. In
> this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.

Sure it is.  The only thing that needs to be constant for the compiler
to do the right thing is the '/ 2' part.  queue_size need not be
constant, and the max is performed after the division.  I would fully
expect the compiler to get this right and convert it internally to the
equivalent bit shift, but if it didn't you could always just write it
that way in the first place:

queue->sig_limit = max(queue->queue_size >> 1, 1);


-- 
Doug Ledford <dledford at redhat.com>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170503/c13ad3ac/attachment-0001.sig>

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03 at 12:17 -0400, Doug Ledford wrote:
> On 5/3/2017 11:58 AM, Bart Van Assche wrote:
> > On Wed, 2017-05-03 at 09:53 -0600, Jason Gunthorpe wrote:
> > > On Wed, May 03, 2017 at 05:19:27PM +0200, Marta Rybczynska wrote:
> > > 
> > > > > where
> > > > > 	queue->sig_limit = max(queue->queue_size / 2, 1);
> > > > 
> > > > I tried to avoid that because this adds a division in the fast path Bart
> > > > was unhappy about in v2.
> > > 
> > > The compiler switches divide/multiply by powers of two into fast bit shifts.
> > 
> > Hello Jason,
> > 
> > As far as I know the compiler only does that for compile-time constants. In
> > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
> 
> Sure it is.  The only thing that needs to be constant for the compiler
> to do the right thing is the '/ 2' part.  queue_size need not be
> constant, and the max is performed after the division.  I would fully
> expect the compiler to get this right and convert it internally to the
> equivalent bit shift, but if it didn't you could always just write it
> that way in the first place:
> 
> queue->sig_limit = max(queue->queue_size >> 1, 1);

Hello Doug,

In my comment I was referring to "% max(queue_size / 2, 1)" and not to
"queue_size / 2".

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03@12:17 -0400, Doug Ledford wrote:
> On 5/3/2017 11:58 AM, Bart Van Assche wrote:
> > On Wed, 2017-05-03@09:53 -0600, Jason Gunthorpe wrote:
> > > On Wed, May 03, 2017@05:19:27PM +0200, Marta Rybczynska wrote:
> > > 
> > > > > where
> > > > > 	queue->sig_limit = max(queue->queue_size / 2, 1);
> > > > 
> > > > I tried to avoid that because this adds a division in the fast path Bart
> > > > was unhappy about in v2.
> > > 
> > > The compiler switches divide/multiply by powers of two into fast bit shifts.
> > 
> > Hello Jason,
> > 
> > As far as I know the compiler only does that for compile-time constants. In
> > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
> 
> Sure it is.  The only thing that needs to be constant for the compiler
> to do the right thing is the '/ 2' part.  queue_size need not be
> constant, and the max is performed after the division.  I would fully
> expect the compiler to get this right and convert it internally to the
> equivalent bit shift, but if it didn't you could always just write it
> that way in the first place:
> 
> queue->sig_limit = max(queue->queue_size >> 1, 1);

Hello Doug,

In my comment I was referring to "% max(queue_size / 2, 1)" and not to
"queue_size / 2".

Bart.

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Wed, May 03, 2017 at 07:01:14PM +0300, Sagi Grimberg wrote:
>
> > Hello Jason,
> >
> > As far as I know the compiler only does that for compile-time constants. In
> > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
>
> We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
> that would only do well with power of 2 sized queues...

IMHO, It is not-so-big-deal limitation.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Leon Romanovsky]

On Wed, May 03, 2017@07:01:14PM +0300, Sagi Grimberg wrote:
>
> > Hello Jason,
> >
> > As far as I know the compiler only does that for compile-time constants. In
> > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
>
> We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
> that would only do well with power of 2 sized queues...

IMHO, It is not-so-big-deal limitation.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170503/bb3a3888/attachment.sig>

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03 at 19:27 +0300, Leon Romanovsky wrote:
> On Wed, May 03, 2017 at 07:01:14PM +0300, Sagi Grimberg wrote:
> > > As far as I know the compiler only does that for compile-time constants. In
> > > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
> > 
> > We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
> > that would only do well with power of 2 sized queues...
> 
> IMHO, It is not-so-big-deal limitation.

Hello Marta, Sagi and Leon,

How about changing nvme_rdma_init_sig_count() such that it always returns
a power of two? The "+ 1" in the code below makes sure that the argument of
ilog2() is larger than zero even if queue_size == 1. I'm not sure whether
the time needed to compute ilog2() would make it necessary to cache the
nvme_rdma_init_sig_count() return value.

static inline int nvme_rdma_init_sig_count(int queue_size)
{
        /* Return the largest power of two that is not above half of (queue size + 1) */
        return 1 << ilog2((queue_size + 1) / 2);
}

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Bart Van Assche]

On Wed, 2017-05-03@19:27 +0300, Leon Romanovsky wrote:
> On Wed, May 03, 2017@07:01:14PM +0300, Sagi Grimberg wrote:
> > > As far as I know the compiler only does that for compile-time constants. In
> > > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
> > 
> > We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
> > that would only do well with power of 2 sized queues...
> 
> IMHO, It is not-so-big-deal limitation.

Hello Marta, Sagi and Leon,

How about changing nvme_rdma_init_sig_count() such that it always returns
a power of two? The "+ 1" in the code below makes sure that the argument of
ilog2() is larger than zero even if queue_size == 1. I'm not sure whether
the time needed to compute ilog2() would make it necessary to cache the
nvme_rdma_init_sig_count() return value.

static inline int nvme_rdma_init_sig_count(int queue_size)
{
        /* Return the largest power of two that is not above half of (queue size + 1) */
 ???????return 1 << ilog2((queue_size + 1) / 2);
}

Bart.

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Sagi Grimberg]

> Hello Marta, Sagi and Leon,
>
> How about changing nvme_rdma_init_sig_count() such that it always returns
> a power of two? The "+ 1" in the code below makes sure that the argument of
> ilog2() is larger than zero even if queue_size == 1. I'm not sure whether
> the time needed to compute ilog2() would make it necessary to cache the
> nvme_rdma_init_sig_count() return value.

ilog2 is pretty fast.

> static inline int nvme_rdma_init_sig_count(int queue_size)
> {
>         /* Return the largest power of two that is not above half of (queue size + 1) */
>         return 1 << ilog2((queue_size + 1) / 2);
> }

That'd work too I think...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Sagi Grimberg]

> Hello Marta, Sagi and Leon,
>
> How about changing nvme_rdma_init_sig_count() such that it always returns
> a power of two? The "+ 1" in the code below makes sure that the argument of
> ilog2() is larger than zero even if queue_size == 1. I'm not sure whether
> the time needed to compute ilog2() would make it necessary to cache the
> nvme_rdma_init_sig_count() return value.

ilog2 is pretty fast.

> static inline int nvme_rdma_init_sig_count(int queue_size)
> {
>         /* Return the largest power of two that is not above half of (queue size + 1) */
>         return 1 << ilog2((queue_size + 1) / 2);
> }

That'd work too I think...

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1594 bytes --]

On 5/3/2017 12:24 PM, Bart Van Assche wrote:
> On Wed, 2017-05-03 at 12:17 -0400, Doug Ledford wrote:
>> On 5/3/2017 11:58 AM, Bart Van Assche wrote:
>>> On Wed, 2017-05-03 at 09:53 -0600, Jason Gunthorpe wrote:
>>>> On Wed, May 03, 2017 at 05:19:27PM +0200, Marta Rybczynska wrote:
>>>>
>>>>>> where
>>>>>> 	queue->sig_limit = max(queue->queue_size / 2, 1);
>>>>>
>>>>> I tried to avoid that because this adds a division in the fast path Bart
>>>>> was unhappy about in v2.
>>>>
>>>> The compiler switches divide/multiply by powers of two into fast bit shifts.
>>>
>>> Hello Jason,
>>>
>>> As far as I know the compiler only does that for compile-time constants. In
>>> this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
>>
>> Sure it is.  The only thing that needs to be constant for the compiler
>> to do the right thing is the '/ 2' part.  queue_size need not be
>> constant, and the max is performed after the division.  I would fully
>> expect the compiler to get this right and convert it internally to the
>> equivalent bit shift, but if it didn't you could always just write it
>> that way in the first place:
>>
>> queue->sig_limit = max(queue->queue_size >> 1, 1);
> 
> Hello Doug,
> 
> In my comment I was referring to "% max(queue_size / 2, 1)" and not to
> "queue_size / 2".

Sorry, too much context was cut for that to come through.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Doug Ledford]

On 5/3/2017 12:24 PM, Bart Van Assche wrote:
> On Wed, 2017-05-03@12:17 -0400, Doug Ledford wrote:
>> On 5/3/2017 11:58 AM, Bart Van Assche wrote:
>>> On Wed, 2017-05-03@09:53 -0600, Jason Gunthorpe wrote:
>>>> On Wed, May 03, 2017@05:19:27PM +0200, Marta Rybczynska wrote:
>>>>
>>>>>> where
>>>>>> 	queue->sig_limit = max(queue->queue_size / 2, 1);
>>>>>
>>>>> I tried to avoid that because this adds a division in the fast path Bart
>>>>> was unhappy about in v2.
>>>>
>>>> The compiler switches divide/multiply by powers of two into fast bit shifts.
>>>
>>> Hello Jason,
>>>
>>> As far as I know the compiler only does that for compile-time constants. In
>>> this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
>>
>> Sure it is.  The only thing that needs to be constant for the compiler
>> to do the right thing is the '/ 2' part.  queue_size need not be
>> constant, and the max is performed after the division.  I would fully
>> expect the compiler to get this right and convert it internally to the
>> equivalent bit shift, but if it didn't you could always just write it
>> that way in the first place:
>>
>> queue->sig_limit = max(queue->queue_size >> 1, 1);
> 
> Hello Doug,
> 
> In my comment I was referring to "% max(queue_size / 2, 1)" and not to
> "queue_size / 2".

Sorry, too much context was cut for that to come through.


-- 
Doug Ledford <dledford at redhat.com>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170503/d21eb6b0/attachment.sig>

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

> On Wed, 2017-05-03 at 19:27 +0300, Leon Romanovsky wrote:
>> On Wed, May 03, 2017 at 07:01:14PM +0300, Sagi Grimberg wrote:
>> > > As far as I know the compiler only does that for compile-time constants. In
>> > > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
>> > 
>> > We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
>> > that would only do well with power of 2 sized queues...
>> 
>> IMHO, It is not-so-big-deal limitation.
> 
> Hello Marta, Sagi and Leon,
> 
> How about changing nvme_rdma_init_sig_count() such that it always returns
> a power of two? The "+ 1" in the code below makes sure that the argument of
> ilog2() is larger than zero even if queue_size == 1. I'm not sure whether
> the time needed to compute ilog2() would make it necessary to cache the
> nvme_rdma_init_sig_count() return value.
> 
> static inline int nvme_rdma_init_sig_count(int queue_size)
> {
>        /* Return the largest power of two that is not above half of (queue size + 1) */
>        return 1 << ilog2((queue_size + 1) / 2);
> }
> 
> Bart.

This looks like a good idea to me. Then the division can change into
an AND. I'll come back with another version soon.

Marta
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

> On Wed, 2017-05-03@19:27 +0300, Leon Romanovsky wrote:
>> On Wed, May 03, 2017@07:01:14PM +0300, Sagi Grimberg wrote:
>> > > As far as I know the compiler only does that for compile-time constants. In
>> > > this case the divisor (max(queue_size / 2, 1)) is not a compile-time constant.
>> > 
>> > We could theoretically do a (sig_count & max(queue_size / 2, 1)) but
>> > that would only do well with power of 2 sized queues...
>> 
>> IMHO, It is not-so-big-deal limitation.
> 
> Hello Marta, Sagi and Leon,
> 
> How about changing nvme_rdma_init_sig_count() such that it always returns
> a power of two? The "+ 1" in the code below makes sure that the argument of
> ilog2() is larger than zero even if queue_size == 1. I'm not sure whether
> the time needed to compute ilog2() would make it necessary to cache the
> nvme_rdma_init_sig_count() return value.
> 
> static inline int nvme_rdma_init_sig_count(int queue_size)
> {
>        /* Return the largest power of two that is not above half of (queue size + 1) */
> ???????return 1 << ilog2((queue_size + 1) / 2);
> }
> 
> Bart.

This looks like a good idea to me. Then the division can change into
an AND. I'll come back with another version soon.

Marta

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Christoph Hellwig]

> This looks like a good idea to me. Then the division can change into
> an AND. I'll come back with another version soon.

Did you get a chance to look at this?  I'm tempted to just merge v2
with the division in it and leave any fixing up for the time we move this
guestimate into the rdma core..
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Christoph Hellwig]

> This looks like a good idea to me. Then the division can change into
> an AND. I'll come back with another version soon.

Did you get a chance to look at this?  I'm tempted to just merge v2
with the division in it and leave any fixing up for the time we move this
guestimate into the rdma core..

Re: [PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

>> This looks like a good idea to me. Then the division can change into
>> an AND. I'll come back with another version soon.
> 
> Did you get a chance to look at this?  I'm tempted to just merge v2
> with the division in it and leave any fixing up for the time we move this
> guestimate into the rdma core..

Yes, I've done another iteration but it needs testing. I'd prefer one of
the later versions finally. It seems that we're also seeing the
effect of the race condition on the signalling variable. 

However, we can also go incremental: do a like-v2 fix first, then fix the
races in nvme and then add a more general fix for all rdma.

Marta
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4, under testing] nvme-rdma: support devices with queue size < 32

[Marta Rybczynska]

>> This looks like a good idea to me. Then the division can change into
>> an AND. I'll come back with another version soon.
> 
> Did you get a chance to look at this?  I'm tempted to just merge v2
> with the division in it and leave any fixing up for the time we move this
> guestimate into the rdma core..

Yes, I've done another iteration but it needs testing. I'd prefer one of
the later versions finally. It seems that we're also seeing the
effect of the race condition on the signalling variable. 

However, we can also go incremental: do a like-v2 fix first, then fix the
races in nvme and then add a more general fix for all rdma.

Marta