* [PATCH] RDMA/rxe: Remove tasklet call from rxe_cq.c
@ 2023-03-27 21:56 Bob Pearson
2023-03-28 6:46 ` Zhu Yanjun
2023-03-29 11:28 ` Leon Romanovsky
0 siblings, 2 replies; 3+ messages in thread
From: Bob Pearson @ 2023-03-27 21:56 UTC (permalink / raw)
To: jgg, zyjzyj2000, BMT, linux-rdma; +Cc: Bob Pearson
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] RDMA/rxe: Remove tasklet call from rxe_cq.c
2023-03-27 21:56 [PATCH] RDMA/rxe: Remove tasklet call from rxe_cq.c Bob Pearson
@ 2023-03-28 6:46 ` Zhu Yanjun
2023-03-29 11:28 ` Leon Romanovsky
1 sibling, 0 replies; 3+ messages in thread
From: Zhu Yanjun @ 2023-03-28 6:46 UTC (permalink / raw)
To: Bob Pearson; +Cc: jgg, BMT, linux-rdma
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] RDMA/rxe: Remove tasklet call from rxe_cq.c
2023-03-27 21:56 [PATCH] RDMA/rxe: Remove tasklet call from rxe_cq.c Bob Pearson
2023-03-28 6:46 ` Zhu Yanjun
@ 2023-03-29 11:28 ` Leon Romanovsky
1 sibling, 0 replies; 3+ messages in thread
From: Leon Romanovsky @ 2023-03-29 11:28 UTC (permalink / raw)
To: zyjzyj2000, BMT, linux-rdma, Jason Gunthorpe, Bob Pearson
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,
--
Leon Romanovsky <leon@kernel.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-03-29 11:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 21:56 [PATCH] RDMA/rxe: Remove tasklet call from rxe_cq.c Bob Pearson
2023-03-28 6:46 ` Zhu Yanjun
2023-03-29 11:28 ` Leon Romanovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).