On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> > refcount_dec generates a warning when the operation
> > causes the refcount to hit zero. Avoid this by using
> > refcount_dec_and_test.
> >
> > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > Reviewed-by: Adit Ranadive <aditr@xxxxxxxxxx>
> > Reviewed-by: Aditya Sarwade <asarwade@xxxxxxxxxx>
> > Reviewed-by: Jorgen Hansen <jhansen@xxxxxxxxxx>
> > Signed-off-by: Bryan Tan <bryantan@xxxxxxxxxx>
> > drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > index 826ccb8..a2b1a3c 100644
> > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
> > dev->srq_tbl[srq->srq_handle] = NULL;
> > spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
> >
> > - refcount_dec(&srq->refcnt);
> > - wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > + if (!refcount_dec_and_test(&srq->refcnt))
> > + wait_event(srq->wait, !refcount_read(&srq->refcnt));
>
> I really don't like this idiom for using refcount, refcount should
> not be dec'd below 0 even under a dec_and_test..
> Why not just simplify:
>
> wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
>
> ???
>
> Same comment on the other patch switching from atomic to refcount
>
> Jason
The refcount doesn't go below 0 in either case--the problem is that
I didn't realise refcount_dec complains about decrements resulting
in a value of 0 [1]. There are no problems with refcount_dec_and_test
resulting in a value of 0.
About checking for refcnt == 1, I do not know of a safe way to only
wake up when the refcount hits 1. Right now we do that by checking
for 0 and doing a wake_up if the result of refcount_dec_and_test is
0 (see the SRQ event handler in pvrdma_main.c). If there's a way to
accomplish this without another dec_and_test in the destroy path, I
can do so.
Bryan
[1] https://patchwork.kernel.org/patch/9863573/ 3rd paragraph
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
