[PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[Xiao Yang]

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))
 		qp->err = ENOSPC;
 err:
 	return qp->err;
-- 
2.25.1

RE: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[Devesh Sharma]

> -----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
> 
> 

Re: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[yangx.jy]

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
>>
>>

RE: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[Devesh Sharma]

> -----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
> >>
> >>

Re: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[Leon Romanovsky]

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
> 
> 
> 

Re: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[yangx.jy]

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
>>
>>
>>

Re: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[Leon Romanovsky]

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
> >>
> >>
> >>

Re: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[yangx.jy]

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
>>>>
>>>>
>>>>

Re: [PATCH rdma-core RESEND] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[yangx.jy]

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