[PATCH for-next v2] RDMA/rxe: Fix resize_finish() in rxe_queue.c

[PATCH for-next v2] RDMA/rxe: Fix resize_finish() in rxe_queue.c

[Bob Pearson]

Currently in resize_finish() in rxe_queue.c there is a loop which
copies the entries in the original queue into a newly allocated queue.
The termination logic for this loop is incorrect. The call to
queue_next_index() updates cons but has no effect on whether the
queue is empty. So if the queue starts out empty nothing is copied
but if it is not then the loop will run forever. This patch changes
the loop to compare the value of cons to the original producer index.

Fixes: ae6e843fe08d0 ("RDMA/rxe: Add memory barriers to kernel queues")
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
v2
   Fixed typo. Should have replaced all original 'prod' by 'new_prod'
---
 drivers/infiniband/sw/rxe/rxe_queue.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_queue.c b/drivers/infiniband/sw/rxe/rxe_queue.c
index dbd4971039c0..d6dbf5a0058d 100644
--- a/drivers/infiniband/sw/rxe/rxe_queue.c
+++ b/drivers/infiniband/sw/rxe/rxe_queue.c
@@ -112,23 +112,25 @@ static int resize_finish(struct rxe_queue *q, struct rxe_queue *new_q,
 			 unsigned int num_elem)
 {
 	enum queue_type type = q->type;
+	u32 new_prod;
 	u32 prod;
 	u32 cons;
 
 	if (!queue_empty(q, q->type) && (num_elem < queue_count(q, type)))
 		return -EINVAL;
 
-	prod = queue_get_producer(new_q, type);
+	new_prod = queue_get_producer(new_q, type);
+	prod = queue_get_producer(q, type);
 	cons = queue_get_consumer(q, type);
 
-	while (!queue_empty(q, type)) {
-		memcpy(queue_addr_from_index(new_q, prod),
+	while ((prod - cons) & q->index_mask) {
+		memcpy(queue_addr_from_index(new_q, new_prod),
 		       queue_addr_from_index(q, cons), new_q->elem_size);
-		prod = queue_next_index(new_q, prod);
+		new_prod = queue_next_index(new_q, new_prod);
 		cons = queue_next_index(q, cons);
 	}
 
-	new_q->buf->producer_index = prod;
+	new_q->buf->producer_index = new_prod;
 	q->buf->consumer_index = cons;
 
 	/* update private index copies */

base-commit: 2c34bb6dea481fa11048e26ffd1ce7400dbc2105
-- 
2.34.1

Re: [PATCH for-next v2] RDMA/rxe: Fix resize_finish() in rxe_queue.c

[Li Zhijian]

On 26/08/2022 06:14, Bob Pearson wrote:
> Currently in resize_finish() in rxe_queue.c there is a loop which
> copies the entries in the original queue into a newly allocated queue.
> The termination logic for this loop is incorrect. The call to
> queue_next_index() updates cons but has no effect on whether the
> queue is empty. So if the queue starts out empty nothing is copied
> but if it is not then the loop will run forever. This patch changes
> the loop to compare the value of cons to the original producer index.
>
> Fixes: ae6e843fe08d0 ("RDMA/rxe: Add memory barriers to kernel queues")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
> v2
>     Fixed typo. Should have replaced all original 'prod' by 'new_prod'
> ---
>   drivers/infiniband/sw/rxe/rxe_queue.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_queue.c b/drivers/infiniband/sw/rxe/rxe_queue.c
> index dbd4971039c0..d6dbf5a0058d 100644
> --- a/drivers/infiniband/sw/rxe/rxe_queue.c
> +++ b/drivers/infiniband/sw/rxe/rxe_queue.c
> @@ -112,23 +112,25 @@ static int resize_finish(struct rxe_queue *q, struct rxe_queue *new_q,
>   			 unsigned int num_elem)
>   {
>   	enum queue_type type = q->type;
> +	u32 new_prod;
>   	u32 prod;
>   	u32 cons;
>   
>   	if (!queue_empty(q, q->type) && (num_elem < queue_count(q, type)))
>   		return -EINVAL;
>   
> -	prod = queue_get_producer(new_q, type);
> +	new_prod = queue_get_producer(new_q, type);
> +	prod = queue_get_producer(q, type);
>   	cons = queue_get_consumer(q, type);
>   
> -	while (!queue_empty(q, type)) {
> -		memcpy(queue_addr_from_index(new_q, prod),
> +	while ((prod - cons) & q->index_mask) {
The termination condition is correct, but below logic is more readable alternative?

count = queue_count(q, type)
while (count--) {
...
}

Otherwise,
Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>


> +		memcpy(queue_addr_from_index(new_q, new_prod),
>   		       queue_addr_from_index(q, cons), new_q->elem_size);
> -		prod = queue_next_index(new_q, prod);
> +		new_prod = queue_next_index(new_q, new_prod);
>   		cons = queue_next_index(q, cons);
>   	}
>   
> -	new_q->buf->producer_index = prod;
> +	new_q->buf->producer_index = new_prod;
>   	q->buf->consumer_index = cons;
>   
>   	/* update private index copies */
>
> base-commit: 2c34bb6dea481fa11048e26ffd1ce7400dbc2105

Re: [PATCH for-next v2] RDMA/rxe: Fix resize_finish() in rxe_queue.c

[Jason Gunthorpe]

On Thu, Aug 25, 2022 at 05:14:47PM -0500, Bob Pearson wrote:
> Currently in resize_finish() in rxe_queue.c there is a loop which
> copies the entries in the original queue into a newly allocated queue.
> The termination logic for this loop is incorrect. The call to
> queue_next_index() updates cons but has no effect on whether the
> queue is empty. So if the queue starts out empty nothing is copied
> but if it is not then the loop will run forever. This patch changes
> the loop to compare the value of cons to the original producer index.
> 
> Fixes: ae6e843fe08d0 ("RDMA/rxe: Add memory barriers to kernel queues")
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> v2
>    Fixed typo. Should have replaced all original 'prod' by 'new_prod'
> ---
>  drivers/infiniband/sw/rxe/rxe_queue.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Applied to for-next, thanks

Jason