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

[Yanjun Zhu <yanjun.zhu@xxxxxxxxx>]

在 2022/7/15 0:54, Bob Pearson 写道:
> On 7/9/22 23:37, yanjun.zhu@xxxxxxxxx wrote:
> > From: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
> > ---
> >   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.

OK. The reason that I move spin_lock_init() into rxe_qp_init_misc is to 
avoid the error "initialize spin locks before use".

Thanks for sharing your features in HPE. If you want to backport these 
new features into linux upstream, I can

keep spin_lock_init in rxe_init_task for future use.

I will send the latest commit very soon.

And look forward to your feature that extends the tasklet code to 
support tasklets and/or work queues which allow
steering the work to specific cpus in linux upstream.

I am curious about this feautre. And hope I can see it in linux upstream 
very soon ^_^

Zhu Yanjun

> 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