On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote:
> 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.
Thats what I ment with my remark.. Sorry for the confusion
> 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.
What is wrong with this:
wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
?
refcount == 1 means the caller is the last owner of the refcount,
presumably you have already taken care to ensure it cannot be inc'd
again (or the code is already not locked right)
Jason
--
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
