| Message ID | 20230327215643.10410-1-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | RDMA/rxe: Remove tasklet call from rxe_cq.c | expand |
On Tue, Mar 28, 2023 at 5:58 AM Bob Pearson <rpearsonhpe@gmail.com> wrote: > > Remove the tasklet call in rxe_cq.c and also the is_dying in the > cq struct. There is no reason for the rxe driver to defer the call > to the cq completion handler by scheduling a tasklet. rxe_cq_post() > is not called in a hard irq context. > > The rxe driver currently is incorrect because the tasklet call is > made without protecting the cq pointer with a reference from having > the underlying memory freed before the deferred routine is called. > Executing the comp_handler inline fixes this problem. > > Fixes: 8700e3e7c485 ("Soft RoCE driver") > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> Thanks Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com> Zhu Yanjun > --- > drivers/infiniband/sw/rxe/rxe_cq.c | 32 +++------------------------ > drivers/infiniband/sw/rxe/rxe_verbs.c | 2 -- > drivers/infiniband/sw/rxe/rxe_verbs.h | 2 -- > 3 files changed, 3 insertions(+), 33 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_cq.c b/drivers/infiniband/sw/rxe/rxe_cq.c > index 66a13c935d50..20ff0c0c4605 100644 > --- a/drivers/infiniband/sw/rxe/rxe_cq.c > +++ b/drivers/infiniband/sw/rxe/rxe_cq.c > @@ -39,21 +39,6 @@ int rxe_cq_chk_attr(struct rxe_dev *rxe, struct rxe_cq *cq, > return -EINVAL; > } > > -static void rxe_send_complete(struct tasklet_struct *t) > -{ > - struct rxe_cq *cq = from_tasklet(cq, t, comp_task); > - unsigned long flags; > - > - spin_lock_irqsave(&cq->cq_lock, flags); > - if (cq->is_dying) { > - spin_unlock_irqrestore(&cq->cq_lock, flags); > - return; > - } > - spin_unlock_irqrestore(&cq->cq_lock, flags); > - > - cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context); > -} > - > int rxe_cq_from_init(struct rxe_dev *rxe, struct rxe_cq *cq, int cqe, > int comp_vector, struct ib_udata *udata, > struct rxe_create_cq_resp __user *uresp) > @@ -79,10 +64,6 @@ int rxe_cq_from_init(struct rxe_dev *rxe, struct rxe_cq *cq, int cqe, > > cq->is_user = uresp; > > - cq->is_dying = false; > - > - tasklet_setup(&cq->comp_task, rxe_send_complete); > - > spin_lock_init(&cq->cq_lock); > cq->ibcq.cqe = cqe; > return 0; > @@ -103,6 +84,7 @@ int rxe_cq_resize_queue(struct rxe_cq *cq, int cqe, > return err; > } > > +/* caller holds reference to cq */ > int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited) > { > struct ib_event ev; > @@ -136,21 +118,13 @@ int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited) > if ((cq->notify == IB_CQ_NEXT_COMP) || > (cq->notify == IB_CQ_SOLICITED && solicited)) { > cq->notify = 0; > - tasklet_schedule(&cq->comp_task); > + > + cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context); > } > > return 0; > } > > -void rxe_cq_disable(struct rxe_cq *cq) > -{ > - unsigned long flags; > - > - spin_lock_irqsave(&cq->cq_lock, flags); > - cq->is_dying = true; > - spin_unlock_irqrestore(&cq->cq_lock, flags); > -} > - > void rxe_cq_cleanup(struct rxe_pool_elem *elem) > { > struct rxe_cq *cq = container_of(elem, typeof(*cq), elem); > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c > index 84b53c070fc5..090d5bfb1e18 100644 > --- a/drivers/infiniband/sw/rxe/rxe_verbs.c > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c > @@ -1178,8 +1178,6 @@ static int rxe_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata) > goto err_out; > } > > - rxe_cq_disable(cq); > - > err = rxe_cleanup(cq); > if (err) > rxe_err_cq(cq, "cleanup failed, err = %d", err); > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h > index 84994a474e9a..dab6fa305bf2 100644 > --- a/drivers/infiniband/sw/rxe/rxe_verbs.h > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h > @@ -63,9 +63,7 @@ struct rxe_cq { > struct rxe_queue *queue; > spinlock_t cq_lock; > u8 notify; > - bool is_dying; > bool is_user; > - struct tasklet_struct comp_task; > atomic_t num_wq; > }; > > -- > 2.37.2 >
On Mon, 27 Mar 2023 16:56:44 -0500, Bob Pearson wrote: > Remove the tasklet call in rxe_cq.c and also the is_dying in the > cq struct. There is no reason for the rxe driver to defer the call > to the cq completion handler by scheduling a tasklet. rxe_cq_post() > is not called in a hard irq context. > > The rxe driver currently is incorrect because the tasklet call is > made without protecting the cq pointer with a reference from having > the underlying memory freed before the deferred routine is called. > Executing the comp_handler inline fixes this problem. > > [...] Applied, thanks! [1/1] RDMA/rxe: Remove tasklet call from rxe_cq.c https://git.kernel.org/rdma/rdma/c/78b26a335310a0 Best regards,
diff --git a/drivers/infiniband/sw/rxe/rxe_cq.c b/drivers/infiniband/sw/rxe/rxe_cq.c index 66a13c935d50..20ff0c0c4605 100644 --- a/drivers/infiniband/sw/rxe/rxe_cq.c +++ b/drivers/infiniband/sw/rxe/rxe_cq.c @@ -39,21 +39,6 @@ int rxe_cq_chk_attr(struct rxe_dev *rxe, struct rxe_cq *cq, return -EINVAL; } -static void rxe_send_complete(struct tasklet_struct *t) -{ - struct rxe_cq *cq = from_tasklet(cq, t, comp_task); - unsigned long flags; - - spin_lock_irqsave(&cq->cq_lock, flags); - if (cq->is_dying) { - spin_unlock_irqrestore(&cq->cq_lock, flags); - return; - } - spin_unlock_irqrestore(&cq->cq_lock, flags); - - cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context); -} - int rxe_cq_from_init(struct rxe_dev *rxe, struct rxe_cq *cq, int cqe, int comp_vector, struct ib_udata *udata, struct rxe_create_cq_resp __user *uresp) @@ -79,10 +64,6 @@ int rxe_cq_from_init(struct rxe_dev *rxe, struct rxe_cq *cq, int cqe, cq->is_user = uresp; - cq->is_dying = false; - - tasklet_setup(&cq->comp_task, rxe_send_complete); - spin_lock_init(&cq->cq_lock); cq->ibcq.cqe = cqe; return 0; @@ -103,6 +84,7 @@ int rxe_cq_resize_queue(struct rxe_cq *cq, int cqe, return err; } +/* caller holds reference to cq */ int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited) { struct ib_event ev; @@ -136,21 +118,13 @@ int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited) if ((cq->notify == IB_CQ_NEXT_COMP) || (cq->notify == IB_CQ_SOLICITED && solicited)) { cq->notify = 0; - tasklet_schedule(&cq->comp_task); + + cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context); } return 0; } -void rxe_cq_disable(struct rxe_cq *cq) -{ - unsigned long flags; - - spin_lock_irqsave(&cq->cq_lock, flags); - cq->is_dying = true; - spin_unlock_irqrestore(&cq->cq_lock, flags); -} - void rxe_cq_cleanup(struct rxe_pool_elem *elem) { struct rxe_cq *cq = container_of(elem, typeof(*cq), elem); diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c index 84b53c070fc5..090d5bfb1e18 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.c +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c @@ -1178,8 +1178,6 @@ static int rxe_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata) goto err_out; } - rxe_cq_disable(cq); - err = rxe_cleanup(cq); if (err) rxe_err_cq(cq, "cleanup failed, err = %d", err); diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h index 84994a474e9a..dab6fa305bf2 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.h +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h @@ -63,9 +63,7 @@ struct rxe_cq { struct rxe_queue *queue; spinlock_t cq_lock; u8 notify; - bool is_dying; bool is_user; - struct tasklet_struct comp_task; atomic_t num_wq; };
Remove the tasklet call in rxe_cq.c and also the is_dying in the cq struct. There is no reason for the rxe driver to defer the call to the cq completion handler by scheduling a tasklet. rxe_cq_post() is not called in a hard irq context. The rxe driver currently is incorrect because the tasklet call is made without protecting the cq pointer with a reference from having the underlying memory freed before the deferred routine is called. Executing the comp_handler inline fixes this problem. Fixes: 8700e3e7c485 ("Soft RoCE driver") Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- drivers/infiniband/sw/rxe/rxe_cq.c | 32 +++------------------------ drivers/infiniband/sw/rxe/rxe_verbs.c | 2 -- drivers/infiniband/sw/rxe/rxe_verbs.h | 2 -- 3 files changed, 3 insertions(+), 33 deletions(-)