| Message ID | 1547038122-22236-1-git-send-email-yanjun.zhu@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [1/1] IB: rxe: replace spin lock with bitops in rxe cq | expand |
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. Umm, the spin locks also ensure other threads have exited their critical sections, this is not an equivalent behavior if the spinlock is not removed.. Did you check is it OK? Jason
On 2019/1/9 23:34, Jason Gunthorpe wrote: > 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. > Umm, the spin locks also ensure other threads have exited their > critical sections, this is not an equivalent behavior if the spinlock > is not removed.. > > Did you check is it OK? Sure. I also notice this before this patch. And I analyzed these critical sections. It seems that this patch will not make difference on this. If I am missing something, please feel free to let me know. And I made tests with this patch for 2 days. All the tests including rds-ping, rds-stress, rping and ibv_rc_pingpong can work well. If any problem occurs with this patch, I will fix it.:-) Zhu Yanjun > > Jason
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@oracle.com> > 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
diff --git a/drivers/infiniband/sw/rxe/rxe_cq.c b/drivers/infiniband/sw/rxe/rxe_cq.c index a57276f..7b34d04 100644 --- a/drivers/infiniband/sw/rxe/rxe_cq.c +++ 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); } @@ -105,7 +100,7 @@ int rxe_cq_from_init(struct rxe_dev *rxe, struct rxe_cq *cq, int cqe, if (uresp) cq->is_user = 1; - cq->is_dying = false; + clear_bit(RXE_CQ_IS_DYING, &cq->is_dying); tasklet_init(&cq->comp_task, rxe_send_complete, (unsigned long)cq); @@ -169,11 +164,8 @@ int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited) 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); + set_bit(RXE_CQ_IS_DYING, &cq->is_dying); } void rxe_cq_cleanup(struct rxe_pool_entry *arg) diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h index 74e0480..ba3f726 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.h +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h @@ -84,13 +84,15 @@ struct rxe_cqe { }; }; +#define RXE_CQ_IS_DYING 0 + struct rxe_cq { struct rxe_pool_entry pelem; struct ib_cq ibcq; struct rxe_queue *queue; spinlock_t cq_lock; u8 notify; - bool is_dying; + unsigned long is_dying; int is_user; struct tasklet_struct comp_task; };
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@oracle.com> --- drivers/infiniband/sw/rxe/rxe_cq.c | 14 +++----------- drivers/infiniband/sw/rxe/rxe_verbs.h | 4 +++- 2 files changed, 6 insertions(+), 12 deletions(-)