Re: [PATCHv2 1/1] RDMA/rxe: Fix qp error handler

[Bob Pearson <rpearsonhpe@gmail.com>]

On 7/9/22 23:37, yanjun.zhu@linux.dev wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> About 7 spin locks in qp creation needs to be initialized. Now these
> spin locks are initialized in the function rxe_qp_init_misc. This
> will avoid the error "initialize spin locks before use".
> 
> Reported-by: syzbot+833061116fa28df97f3b@syzkaller.appspotmail.com
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe_qp.c   | 12 ++++++++----
>  drivers/infiniband/sw/rxe/rxe_task.c |  1 -
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index 8355a5b1cb60..259d8bb15116 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -172,6 +172,14 @@ static void rxe_qp_init_misc(struct rxe_dev *rxe, struct rxe_qp *qp,
>  
>  	spin_lock_init(&qp->state_lock);
>  
> +	spin_lock_init(&qp->req.task.state_lock);
> +	spin_lock_init(&qp->resp.task.state_lock);
> +	spin_lock_init(&qp->comp.task.state_lock);
> +
> +	spin_lock_init(&qp->sq.sq_lock);
> +	spin_lock_init(&qp->rq.producer_lock);
> +	spin_lock_init(&qp->rq.consumer_lock);
> +
>  	atomic_set(&qp->ssn, 0);
>  	atomic_set(&qp->skb_out, 0);
>  }
> @@ -231,7 +239,6 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
>  	qp->req.opcode		= -1;
>  	qp->comp.opcode		= -1;
>  
> -	spin_lock_init(&qp->sq.sq_lock);
>  	skb_queue_head_init(&qp->req_pkts);
>  
>  	rxe_init_task(rxe, &qp->req.task, qp,
> @@ -282,9 +289,6 @@ static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
>  		}
>  	}
>  
> -	spin_lock_init(&qp->rq.producer_lock);
> -	spin_lock_init(&qp->rq.consumer_lock);
> -
>  	skb_queue_head_init(&qp->resp_pkts);
>  
>  	rxe_init_task(rxe, &qp->resp.task, qp,
> diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
> index 0c4db5bb17d7..77c691570673 100644
> --- a/drivers/infiniband/sw/rxe/rxe_task.c
> +++ b/drivers/infiniband/sw/rxe/rxe_task.c
> @@ -98,7 +98,6 @@ int rxe_init_task(void *obj, struct rxe_task *task,
>  	tasklet_setup(&task->tasklet, rxe_do_task);
>  
>  	task->state = TASK_STATE_START;
> -	spin_lock_init(&task->state_lock);
>  
>  	return 0;
>  }

Zhu,

The task.state_lock spinlocks are an implementation detail of the tasklet code. Seems strange to
move the spin_lock_init() calls up into the qp code for these. This breaks encapsulation. We (HPE)
have a patch coming that extends the tasklet code to support tasklets and/or work queues which allow
steering the work to specific cpus. This gives a significant performance boost for IO intensive
work flows.

The only other issue with this patch is that for xrc QPs, which we don't support yet, the QPs only
have one side implemented and there won't be a reason to do unneeded work. Not a big issue though.

Bob