| Message ID | 20220111022404.2375531-1-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [rdma-core,RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full() | expand |
> -----Original Message----- > From: Xiao Yang <yangx.jy@fujitsu.com> > Sent: Tuesday, January 11, 2022 7:54 AM > To: rpearsonhpe@gmail.com; leon@kernel.org > Cc: linux-rdma@vger.kernel.org; Xiao Yang <yangx.jy@fujitsu.com> > Subject: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in > check_qp_queue_full() > > The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly > assumes the queue is full when the number of entires 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(-) > > 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)) Are you sure that index_mask would always be aligned with 2^X? > qp->err = ENOSPC; > err: > return qp->err; > -- > 2.25.1 > >
On Tue, Jan 11, 2022 at 10:24:04AM +0800, Xiao Yang wrote: > The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly > assumes the queue is full when the number of entires 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(-) > > 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)) Please reuse queue_full(). Thanks > qp->err = ENOSPC; > err: > return qp->err; > -- > 2.25.1 > > >
On 2022/1/11 12:12, Devesh Sharma wrote: > >> -----Original Message----- >> From: Xiao Yang<yangx.jy@fujitsu.com> >> Sent: Tuesday, January 11, 2022 7:54 AM >> To: rpearsonhpe@gmail.com; leon@kernel.org >> Cc: linux-rdma@vger.kernel.org; Xiao Yang<yangx.jy@fujitsu.com> >> Subject: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in >> check_qp_queue_full() >> >> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly >> assumes the queue is full when the number of entires 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(-) >> >> 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)) > Are you sure that index_mask would always be aligned with 2^X? Hi Deves, I think it is. index_mask is alwasy set to 2^X -1 in kernel: ---------------------------------------------------------------- struct rxe_queue *rxe_queue_init(struct rxe_dev *rxe, int *num_elem, unsigned int elem_size, enum queue_type type) { ... num_slots = *num_elem + 1; num_slots = roundup_pow_of_two(num_slots); q->index_mask = num_slots - 1; ... } ---------------------------------------------------------------- Best Regards, Xiao Yang >> qp->err = ENOSPC; >> err: >> return qp->err; >> -- >> 2.25.1 >> >>
On 2022/1/11 14:11, Leon Romanovsky wrote: > On Tue, Jan 11, 2022 at 10:24:04AM +0800, Xiao Yang wrote: >> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly >> assumes the queue is full when the number of entires 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(-) >> >> 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)) > Please reuse queue_full(). Hi Leon, qp->cur_index and qp->err are introduced for new ibv_wr_* APIs and I am not sure if check_qp_queue_full() can be replaced with queue_full(). Bob, do you have any suggestion? Best Regards, Xiao Yang > Thanks > >> qp->err = ENOSPC; >> err: >> return qp->err; >> -- >> 2.25.1 >> >> >>
> -----Original Message----- > From: yangx.jy@fujitsu.com <yangx.jy@fujitsu.com> > Sent: Tuesday, January 11, 2022 2:41 PM > To: Devesh Sharma <devesh.s.sharma@oracle.com> > Cc: rpearsonhpe@gmail.com; leon@kernel.org; linux-rdma@vger.kernel.org > Subject: [External] : Re: [PATCH rdma-core RESEND] providers/rxe: Replace > '%' with '&' in check_qp_queue_full() > > On 2022/1/11 12:12, Devesh Sharma wrote: > > > >> -----Original Message----- > >> From: Xiao Yang<yangx.jy@fujitsu.com> > >> Sent: Tuesday, January 11, 2022 7:54 AM > >> To: rpearsonhpe@gmail.com; leon@kernel.org > >> Cc: linux-rdma@vger.kernel.org; Xiao Yang<yangx.jy@fujitsu.com> > >> Subject: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' > >> in > >> check_qp_queue_full() > >> > >> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" > >> mistakenly assumes the queue is full when the number of entires 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(-) > >> > >> 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)) > > Are you sure that index_mask would always be aligned with 2^X? > Hi Deves, > > I think it is. index_mask is alwasy set to 2^X -1 in kernel: Cool looks good, other than comments from Leon. Reviewed-By: Devesh Sharma <devesh.s.sharma@oracle.com> > ---------------------------------------------------------------- > struct rxe_queue *rxe_queue_init(struct rxe_dev *rxe, int *num_elem, > unsigned int elem_size, enum queue_type type) { > ... > num_slots = *num_elem + 1; > num_slots = roundup_pow_of_two(num_slots); > q->index_mask = num_slots - 1; > ... > } > ---------------------------------------------------------------- > > Best Regards, > Xiao Yang > >> qp->err = ENOSPC; > >> err: > >> return qp->err; > >> -- > >> 2.25.1 > >> > >>
On Tue, Jan 11, 2022 at 09:19:46AM +0000, yangx.jy@fujitsu.com wrote: > On 2022/1/11 14:11, Leon Romanovsky wrote: > > On Tue, Jan 11, 2022 at 10:24:04AM +0800, Xiao Yang wrote: > >> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly > >> assumes the queue is full when the number of entires 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(-) > >> > >> 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)) > > Please reuse queue_full(). > Hi Leon, > > qp->cur_index and qp->err are introduced for new ibv_wr_* APIs and I am > not sure if check_qp_queue_full() can be replaced with queue_full(). > > Bob, do you have any suggestion? diff --git a/providers/rxe/rxe_queue.h b/providers/rxe/rxe_queue.h index 6de8140c..83eb4a5f 100644 --- a/providers/rxe/rxe_queue.h +++ b/providers/rxe/rxe_queue.h @@ -198,14 +198,11 @@ static inline void advance_qp_cur_index(struct rxe_qp *qp) static inline int check_qp_queue_full(struct rxe_qp *qp) { struct rxe_queue_buf *q = qp->sq.queue; - uint32_t cons; - - cons = atomic_load_explicit(consumer(q), memory_order_acquire); if (qp->err) goto err; - if (cons == ((qp->cur_index + 1) % q->index_mask)) + if (queue_full(q)) qp->err = ENOSPC; err: return qp->err; (END) > > Best Regards, > Xiao Yang > > Thanks > > > >> qp->err = ENOSPC; > >> err: > >> return qp->err; > >> -- > >> 2.25.1 > >> > >> > >>
On 2022/1/11 18:47, Leon Romanovsky wrote: > On Tue, Jan 11, 2022 at 09:19:46AM +0000, yangx.jy@fujitsu.com wrote: >> On 2022/1/11 14:11, Leon Romanovsky wrote: >>> On Tue, Jan 11, 2022 at 10:24:04AM +0800, Xiao Yang wrote: >>>> The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly >>>> assumes the queue is full when the number of entires 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(-) >>>> >>>> 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)) >>> Please reuse queue_full(). >> Hi Leon, >> >> qp->cur_index and qp->err are introduced for new ibv_wr_* APIs and I am >> not sure if check_qp_queue_full() can be replaced with queue_full(). >> >> Bob, do you have any suggestion? > > diff --git a/providers/rxe/rxe_queue.h b/providers/rxe/rxe_queue.h > index 6de8140c..83eb4a5f 100644 > --- a/providers/rxe/rxe_queue.h > +++ b/providers/rxe/rxe_queue.h > @@ -198,14 +198,11 @@ static inline void advance_qp_cur_index(struct rxe_qp *qp) > static inline int check_qp_queue_full(struct rxe_qp *qp) > { > struct rxe_queue_buf *q = qp->sq.queue; > - uint32_t cons; > - > - cons = atomic_load_explicit(consumer(q), memory_order_acquire); > > if (qp->err) > goto err; > > - if (cons == ((qp->cur_index + 1) % q->index_mask)) > + if (queue_full(q)) > qp->err = ENOSPC; > err: > return qp->err; > (END) > Hi Leon, This change using q->producer_index makes qp->cur_index unused. According to commit 1a894ca10105, qp->cur_index and related functions (advance_qp_cur_index() , check_qp_queue_full()) are introduced for ibv_wr_* APIs on purpose. I am not sure if we can replace qp->cur_index with q->producer_index directly. I think we need Bob to confirm it. By the way, could we just fix the issue here? Best Regards, Xiao Yang >> Best Regards, >> Xiao Yang >>> Thanks >>> >>>> qp->err = ENOSPC; >>>> err: >>>> return qp->err; >>>> -- >>>> 2.25.1 >>>> >>>> >>>>
On 2022/1/11 10:24, Xiao Yang wrote: > The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly > assumes the queue is full when the number of entires 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.wrote Hi Bob, I think the commit message also misled you. The expression "cons == ((qp->cur_index + 1) % q->index_mask)" mistakenly assumes the queue is full when we create an empty two-slots queue (i.e. cur_index = 0 and cons = 0) Best Regards, Xiao Yang
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 the number of entires 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(-)