| Message ID | 20220710043709.707649-1-yanjun.zhu@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCHv2,1/1] RDMA/rxe: Fix qp error handler | expand |
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
在 2022/7/15 0:54, Bob Pearson 写道: > 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. 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
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; }