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

[rdma-core PATCH] 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 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(-)

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: [rdma-core PATCH] providers/rxe: Replace '%' with '&' in check_qp_queue_full()

[yangx.jy]

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;

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

[Bob Pearson]

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

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

[Haakon Bugge]

> 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

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

[Jason Gunthorpe]

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