> On 13 Jan 2022, at 21:51, Bob Pearson <rpearsonhpe@xxxxxxxxx> wrote:
>
> On 1/10/22 20:22, yangx.jy@xxxxxxxxxxx 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@xxxxxxxxxxx>
>>> ---
>>> 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
