| Message ID | 20220111014145.2374669-1-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [rdma-core] providers/rxe: Replace '%' with '&' in check_qp_queue_full() | expand |
On 2022/1/11 9:41, Xiao Yang wrote: > The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly > assumes the queue is full when qp->cur_index is equal to "maximum - 1" > (maximum is correct). Hi All, The commit message seems inappropriate so I will resend this patch. Best Regards, Xiao Yang > For example: > If cons and qp->cur_index are 0 and q->index_mask is 1, check_qp_queue_full() > reports ENOSPC. > > Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb") > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > --- > providers/rxe/rxe_queue.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/providers/rxe/rxe_queue.h b/providers/rxe/rxe_queue.h > index 6de8140c..708e76ac 100644 > --- a/providers/rxe/rxe_queue.h > +++ b/providers/rxe/rxe_queue.h > @@ -205,7 +205,7 @@ static inline int check_qp_queue_full(struct rxe_qp *qp) > if (qp->err) > goto err; > > - if (cons == ((qp->cur_index + 1) % q->index_mask)) > + if (cons == ((qp->cur_index + 1) & q->index_mask)) > qp->err = ENOSPC; > err: > return qp->err;
On 1/10/22 20:22, yangx.jy@fujitsu.com wrote: > On 2022/1/11 9:41, Xiao Yang wrote: >> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly >> assumes the queue is full when qp->cur_index is equal to "maximum - 1" >> (maximum is correct). > Hi All, > > The commit message seems inappropriate so I will resend this patch. > > Best Regards, > Xiao Yang >> For example: >> If cons and qp->cur_index are 0 and q->index_mask is 1, check_qp_queue_full() >> reports ENOSPC. >> >> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb") >> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> >> --- >> providers/rxe/rxe_queue.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/providers/rxe/rxe_queue.h b/providers/rxe/rxe_queue.h >> index 6de8140c..708e76ac 100644 >> --- a/providers/rxe/rxe_queue.h >> +++ b/providers/rxe/rxe_queue.h >> @@ -205,7 +205,7 @@ static inline int check_qp_queue_full(struct rxe_qp *qp) >> if (qp->err) >> goto err; >> >> - if (cons == ((qp->cur_index + 1) % q->index_mask)) >> + if (cons == ((qp->cur_index + 1) & q->index_mask)) >> qp->err = ENOSPC; >> err: >> return qp->err; The way these queues work they can only hold 2^n - 1 entries. The reason for this is to distinguish empty and full. If you let the last entry be written then producer would advance to equal consumer which is the initial empty condition. So a queue which can hold 1 entry has to have two slots (n=1) but can only hold one entry. It begins with prod = cons = 0 and is empty and is *not* full Adding one entry gives slot[0] = value, prod = 1, cons = 0 and is full and not empty After reading this value slot[0] = old value, prod = cons = 1 and queue is empty again. Writing a new value slot[1] = new value, slot[0] = old value, prod = 0 and cons = 1 and queue is full again. The expression full = (cons == ((qp->cur_index + 1) % q->index_mask)) is correct. Please do not make this change. Bob
> On 13 Jan 2022, at 21:51, Bob Pearson <rpearsonhpe@gmail.com> wrote: > > On 1/10/22 20:22, yangx.jy@fujitsu.com wrote: >> On 2022/1/11 9:41, Xiao Yang wrote: >>> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly >>> assumes the queue is full when qp->cur_index is equal to "maximum - 1" >>> (maximum is correct). >> Hi All, >> >> The commit message seems inappropriate so I will resend this patch. >> >> Best Regards, >> Xiao Yang >>> For example: >>> If cons and qp->cur_index are 0 and q->index_mask is 1, check_qp_queue_full() >>> reports ENOSPC. >>> >>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb") >>> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> >>> --- >>> providers/rxe/rxe_queue.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/providers/rxe/rxe_queue.h b/providers/rxe/rxe_queue.h >>> index 6de8140c..708e76ac 100644 >>> --- a/providers/rxe/rxe_queue.h >>> +++ b/providers/rxe/rxe_queue.h >>> @@ -205,7 +205,7 @@ static inline int check_qp_queue_full(struct rxe_qp *qp) >>> if (qp->err) >>> goto err; >>> >>> - if (cons == ((qp->cur_index + 1) % q->index_mask)) >>> + if (cons == ((qp->cur_index + 1) & q->index_mask)) >>> qp->err = ENOSPC; >>> err: >>> return qp->err; > > The way these queues work they can only hold 2^n - 1 entries. The reason for this is > to distinguish empty and full. You can simply mitigate that by having free-running counters, right? Thxs, Håkon > If you let the last entry be written then producer would advance to equal consumer which is the initial empty condition. So a queue which can hold 1 entry has to have two slots (n=1) but can only hold one entry. It begins with > > prod = cons = 0 and is empty and is *not* full > > Adding one entry gives > > slot[0] = value, prod = 1, cons = 0 and is full and not empty > > After reading this value > > slot[0] = old value, prod = cons = 1 and queue is empty again. > > Writing a new value > > slot[1] = new value, slot[0] = old value, prod = 0 and cons = 1 and queue is full again. > > The expression full = (cons == ((qp->cur_index + 1) % q->index_mask)) is correct. > > Please do not make this change. > > Bob
On Mon, Jan 17, 2022 at 11:12:12AM +0000, Haakon Bugge wrote: > > The way these queues work they can only hold 2^n - 1 entries. The > > reason for this is to distinguish empty and full. > > You can simply mitigate that by having free-running counters, right? That is generally the best design, with the cost of doing the & on every use of queue data vs only on ++ Jason
diff --git a/providers/rxe/rxe_queue.h b/providers/rxe/rxe_queue.h index 6de8140c..708e76ac 100644 --- a/providers/rxe/rxe_queue.h +++ b/providers/rxe/rxe_queue.h @@ -205,7 +205,7 @@ static inline int check_qp_queue_full(struct rxe_qp *qp) if (qp->err) goto err; - if (cons == ((qp->cur_index + 1) % q->index_mask)) + if (cons == ((qp->cur_index + 1) & q->index_mask)) qp->err = ENOSPC; err: return qp->err;
The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly assumes the queue is full when qp->cur_index is equal to "maximum - 1" (maximum is correct). For example: If cons and qp->cur_index are 0 and q->index_mask is 1, check_qp_queue_full() reports ENOSPC. Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb") Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> --- providers/rxe/rxe_queue.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)