> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Wednesday, August 08, 2018 10:54 PM
> On Wed, Aug 08, 2018 at 06:50:17PM +0000, Kalderon, Michal wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > > Sent: Tuesday, August 07, 2018 5:28 PM On Tue, Aug 07, 2018 at
> > > 10:02:03AM +0000, Kalderon, Michal wrote:
> > > > > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> > > > > owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
> > > > >
> > > > > On Sun, Jul 29, 2018 at 01:16:10PM +0300, Yuval Bason wrote:
> > > > >
> > > > > > diff --git a/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> > > > > > b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> > > > > > index 26dc374..505fa36 100644
> > > > > > +++ b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> > > > > > @@ -491,7 +491,7 @@ int qedr_iw_connect(struct iw_cm_id
> > > > > > *cm_id,
> > > > > struct iw_cm_conn_param *conn_param)
> > > > > > int rc = 0;
> > > > > > int i;
> > > > > >
> > > > > > - qp = idr_find(&dev->qpidr, conn_param->qpn);
> > > > > > + qp = idr_find(&dev->qpidr.idr, conn_param->qpn);
> > > > > >
> > > > > > laddr = (struct sockaddr_in *)&cm_id->m_local_addr;
> > > > > > raddr = (struct sockaddr_in *)&cm_id->m_remote_addr; @@
> > > > > > -679,7
> > > > > > +679,7 @@ int qedr_iw_accept(struct iw_cm_id *cm_id, struct
> > > > > > iw_cm_conn_param *conn_param)
> > > > >
> > > > > [..]
> > > > >
> > > > > > @@ -2279,8 +2275,9 @@ int qedr_destroy_qp(struct ib_qp *ibqp)
> > > > > >
> > > > > > qedr_free_qp_resources(dev, qp);
> > > > > >
> > > > > > - if (atomic_dec_and_test(&qp->refcnt)) {
> > > > > > - qedr_idr_remove(dev, qp->qp_id);
> > > > > > + if (atomic_dec_and_test(&qp->refcnt) &&
> > > > > > + rdma_protocol_iwarp(&dev->ibdev, 1)) {
> > > > > > + qedr_idr_remove(dev, &dev->qpidr, qp->qp_id);
> > > > > > kfree(qp);
> > > > >
> > > > > So what prevents kfree from racing with the idr_find above?
> > > > > Looks like noting.
> > > >
> > > > Hi Jason,
> > >
> > > > This is the QP idr, and the use case here is for iWARP rdma
> > > > connection flow. The flows here are part of the rdma-cm for iWARP
> > > > and the races here are dealt with by locking and ref-counting in the
> rdma-cm.
> > >
> > > How?
>
> > Specifically for the kfree race with idr_find the description below
> > explains how it can't occur. But for the race of the idr being
> > modified by adding/removing a different QP and performing idr_find, I
> > agree, we need an rcu_read_lock around the idr_find.
>
> This needs more than rcu_read_lock.
>
> > The idr_find above that you're referring to won't race with the
> > kfree(qp) because qedr_idr_remove & kfree(qp) will be called only when
> > the qp->refcnt reaches zero
>
> This pattern is to be implemented using a 'kref', it needs to change, and then
> it will become clearer what needs to be done..
Right, this is something you mentioned in one of the first patch version:
https://www.spinics.net/lists/linux-rdma/msg67029.html and we have already started
looking at it. We will look into the entire iWARP CM flow in qedr with the qpidr and the refcnt
together. This will of course be a separate patch as it is not related to SRQ at all.
Thanks for your comments
Michal
>
> 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
