On Wed, Jan 09, 2019 at 07:48:42AM -0500, Zhu Yanjun wrote:
> It is a common way to use bitops to indicate the state. So replace spin
> lock with bitops.
>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxxx>
> drivers/infiniband/sw/rxe/rxe_cq.c | 14 +++-----------
> drivers/infiniband/sw/rxe/rxe_verbs.h | 4 +++-
> 2 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_cq.c b/drivers/infiniband/sw/rxe/rxe_cq.c
> index a57276f..7b34d04 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_cq.c
> @@ -69,14 +69,9 @@ int rxe_cq_chk_attr(struct rxe_dev *rxe, struct rxe_cq *cq,
> static void rxe_send_complete(unsigned long data)
> {
> struct rxe_cq *cq = (struct rxe_cq *)data;
> - unsigned long flags;
>
> - spin_lock_irqsave(&cq->cq_lock, flags);
> - if (cq->is_dying) {
> - spin_unlock_irqrestore(&cq->cq_lock, flags);
> + if (test_bit(RXE_CQ_IS_DYING, &cq->is_dying))
> return;
> - }
> - spin_unlock_irqrestore(&cq->cq_lock, flags);
>
> cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
> }
This code just looks completely wrong.
The only purpose of the CQ_IS_DYING is to be set before doing
rxe_drop_ref(cq), which drives a kref that causes the cq memory to be
recycled..
So what is protecting the CQ memory during the above comp_handler?
Nothing it seems.
I guess the 'is_dying' was an incorrect attempt to create safety.
You should send a patch to delete is_dying and fix the
krefing/lifetime to avoid this problem instead of noodling at the edge
like this. Maybe the tasklet needs to be flushed instead of is_dying?
'is dying' is alomst always a dangerous idea..
Jason
