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

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

[yangx.jy]

CC: linux-rdma mail list

On 2022/1/14 4:51, Bob Pearson wrote:
> 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
Hi Bob,

Thanks for your detailed example.
> 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.
The issue happens here, let's look at the "writing a new value" step.
We think queue is full again when prod = 0 and cons = 1, but this 
expression thinks the queue is not full.
cons == ((qp->cur_index + 1) % q->index_mask))
<1> == <0 + 1>     % <1>

In addition, this expression thinks the queue is full wrongly when we 
create an empty two-slots queue (i.e. prod = 0 and cons = 0).
cons == ((qp->cur_index + 1) % q->index_mask))
<0> == <0 + 1>     % <1>

The following steps can expose the issue:
-------------------------------------------
attr_ex.cap.max_send_wr = attr_ex.cap.max_recv_wr = 1;
...
ret = rdma_create_qp_ex(id, &attr_ex);
...
ibv_wr_start(qpx);
ibv_wr_rdma_atomic_write(qpx,...);  --> check_qp_queue_full() returns 
ENOSPC wrongly.
ibv_wr_complete(qpx);
-------------------------------------------
Please see my example[1] for details.

[1] 
https://github.com/yangx-jy/rdma-core/commit/257c6672dc147e0881525e469a49d933f1bc9183

I think the commit message misled you.

Best Regards,
Xiao Yang
> Please do not make this change.
>
> Bob

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

[Bob Pearson]

On 1/18/22 03:02, yangx.jy@fujitsu.com wrote:
> CC: linux-rdma mail list
> 
> On 2022/1/14 4:51, Bob Pearson wrote:
>> 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
> Hi Bob,
> 
> Thanks for your detailed example.
>> prod = cons = 0 and is empty and is*not*  full
full = (cons == ((prod + 1) % 2)) = (0 == 1) = false
empty = (prod == cons) == (0 == 0) = true
>>
>> Adding one entry gives
>>
>> slot[0] = value, prod = 1, cons = 0 and is full and not empty
full = (cons == ((prod + 1) % 2)) = (0 == 0) = true
empty = (prod == cons) == (0 == 1) = false
>>
>> After reading this value
>>
>> slot[0] = old value, prod = cons = 1 and queue is empty again.
full = (cons == ((prod + 1) % 2)) = (1 == 0) = false
empty = (prod == cons) = (1 == 1) = true
>>
>> Writing a new value
>>
>> slot[1] = new value, slot[0] = old value, prod = 0 and cons = 1 and queue is full again.
full = (cons == ((prod + 1) % 2)) = (1 == (0 + 1) % 2) = true
empty = (prod == cons) = (0 == 1) = false

and x % 2^N is the same as x & (2^N - 1) or x & index_mask. 
>>
>> The expression full = (cons == ((qp->cur_index + 1) % q->index_mask)) is correct.

Actually you are correct. If you look at queue_full() it uses
full = (cons == (prod + 1) & q->index_mask) and queue_empty() uses
empty = (prod == cons) *but*
check_qp_queue_full() uses
full = (cons == ((qp->cur_index + 1) % q->index_mask)) with % instead of &.
I was so used to looking at the first two I missed the error in the last one.

% should be & like the others.

Bob

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

[yangx.jy]

On 2022/1/20 13:08, Bob Pearson wrote:
> On 1/18/22 03:02, yangx.jy@fujitsu.com wrote:
>> CC: linux-rdma mail list
>>
>> On 2022/1/14 4:51, Bob Pearson wrote:
>>> 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
>> Hi Bob,
>>
>> Thanks for your detailed example.
>>> prod = cons = 0 and is empty and is*not*  full
> full = (cons == ((prod + 1) % 2)) = (0 == 1) = false
> empty = (prod == cons) == (0 == 0) = true
>>> Adding one entry gives
>>>
>>> slot[0] = value, prod = 1, cons = 0 and is full and not empty
> full = (cons == ((prod + 1) % 2)) = (0 == 0) = true
> empty = (prod == cons) == (0 == 1) = false
>>> After reading this value
>>>
>>> slot[0] = old value, prod = cons = 1 and queue is empty again.
> full = (cons == ((prod + 1) % 2)) = (1 == 0) = false
> empty = (prod == cons) = (1 == 1) = true
>>> Writing a new value
>>>
>>> slot[1] = new value, slot[0] = old value, prod = 0 and cons = 1 and queue is full again.
> full = (cons == ((prod + 1) % 2)) = (1 == (0 + 1) % 2) = true
> empty = (prod == cons) = (0 == 1) = false
>
> and x % 2^N is the same as x&  (2^N - 1) or x&  index_mask.
Hi Bob,

Agreed.  either x % 2^N or x & (2^N - 1) is fine, but we use the wrong x 
% (2^N - 1) here.

>>> The expression full = (cons == ((qp->cur_index + 1) % q->index_mask)) is correct.
> Actually you are correct. If you look at queue_full() it uses
> full = (cons == (prod + 1)&  q->index_mask) and queue_empty() uses
> empty = (prod == cons) *but*
> check_qp_queue_full() uses
> full = (cons == ((qp->cur_index + 1) % q->index_mask)) with % instead of&.
> I was so used to looking at the first two I missed the error in the last one.
>
> % should be&  like the others.
I will update the commit message and send v2 patch.

Thanks lot.

Best Regards,
Xiao Yang
> Bob
>
>