| Message ID | 79901165.5342369.1493805915415.JavaMail.zimbra@kalray.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@kalray.eu> > Signed-off-by: Samuel Jones <sjones@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme
> +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)