| Message ID | 3fdcf67f-2e90-4c61-92da-a8f7743cf54a@CMEXHTCAS2.ad.emulex.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 7/17/2014 9:01 AM, Devesh Sharma wrote: > xprtrdma tries to destroy pd after destruction of cm_id. However, > pd should be deallocated before destruction of cm_id. Why? I think you really mean that the pd dealloc needs to be done before the module deref that you added in the first patch. But let us see what Roland says about how device removal is supposed to be handled when kernel applications are using the device... > Signed-off-by: Devesh Sharma <devesh.sharma@emulex.com> > --- > net/sunrpc/xprtrdma/verbs.c | 22 +++++++++++++--------- > 1 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c > index b00e55e..096e94b 100644 > --- a/net/sunrpc/xprtrdma/verbs.c > +++ b/net/sunrpc/xprtrdma/verbs.c > @@ -544,7 +544,7 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg) > if (rc) { > dprintk("RPC: %s: ib_query_device failed %d\n", > __func__, rc); > - goto out2; > + goto out3; > } > > if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY) { > @@ -602,14 +602,14 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg) > "phys register failed with %lX\n", > __func__, PTR_ERR(ia->ri_bind_mem)); > rc = -ENOMEM; > - goto out2; > + goto out3; > } > break; > default: > printk(KERN_ERR "RPC: Unsupported memory " > "registration mode: %d\n", memreg); > rc = -ENOMEM; > - goto out2; > + goto out3; > } > dprintk("RPC: %s: memory registration strategy is %d\n", > __func__, memreg); > @@ -619,6 +619,9 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg) > > rwlock_init(&ia->ri_qplock); > return 0; > +out3: > + ib_dealloc_pd(ia->ri_pd); > + ia->ri_pd = NULL; > out2: > module_put(ia->ri_id->device->owner); > rdma_destroy_id(ia->ri_id); > @@ -643,19 +646,20 @@ rpcrdma_ia_close(struct rpcrdma_ia *ia) > dprintk("RPC: %s: ib_dereg_mr returned %i\n", > __func__, rc); > } > + > if (ia->ri_id != NULL && !IS_ERR(ia->ri_id)) { > if (ia->ri_id->qp) > rdma_destroy_qp(ia->ri_id); > + > + if (ia->ri_pd != NULL && !IS_ERR(ia->ri_pd)) { > + rc = ib_dealloc_pd(ia->ri_pd); > + dprintk("RPC: %s: ib_dealloc_pd returned %i\n", > + __func__, rc); > + } > module_put(ia->ri_id->device->owner); > rdma_destroy_id(ia->ri_id); > ia->ri_id = NULL; > } > - > - if (ia->ri_pd != NULL && !IS_ERR(ia->ri_pd)) { > - rc = ib_dealloc_pd(ia->ri_pd); > - dprintk("RPC: %s: ib_dealloc_pd returned %i\n", > - __func__, rc); > - } > } > > /* -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Steve Wise [mailto:swise@opengridcomputing.com] > Sent: Thursday, July 17, 2014 8:36 PM > To: Devesh Sharma; linux-rdma@vger.kernel.org > Cc: chuck.lever@oracle.com > Subject: Re: [for-next 2/2] xprtrdma: fix deallocation sequence of pd > > On 7/17/2014 9:01 AM, Devesh Sharma wrote: > > xprtrdma tries to destroy pd after destruction of cm_id. However, pd > > should be deallocated before destruction of cm_id. > > Why? > > I think you really mean that the pd dealloc needs to be done before the > module deref that you added in the first patch. But let us see what Roland > says about how device removal is supposed to be handled when kernel > applications are using the device... Partially Yes, PD de-allocation should not be done after taking out the module reference. In the allocation Sequence PD is allocated after cm_id allocation hence should be de-allocated before cm-id destroy. > > > > > Signed-off-by: Devesh Sharma <devesh.sharma@emulex.com> > > --- > > net/sunrpc/xprtrdma/verbs.c | 22 +++++++++++++--------- > > 1 files changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c > > index b00e55e..096e94b 100644 > > --- a/net/sunrpc/xprtrdma/verbs.c > > +++ b/net/sunrpc/xprtrdma/verbs.c > > @@ -544,7 +544,7 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct > sockaddr *addr, int memreg) > > if (rc) { > > dprintk("RPC: %s: ib_query_device failed %d\n", > > __func__, rc); > > - goto out2; > > + goto out3; > > } > > > > if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY) { @@ > > -602,14 +602,14 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct > sockaddr *addr, int memreg) > > "phys register failed with %lX\n", > > __func__, PTR_ERR(ia->ri_bind_mem)); > > rc = -ENOMEM; > > - goto out2; > > + goto out3; > > } > > break; > > default: > > printk(KERN_ERR "RPC: Unsupported memory " > > "registration mode: %d\n", memreg); > > rc = -ENOMEM; > > - goto out2; > > + goto out3; > > } > > dprintk("RPC: %s: memory registration strategy is %d\n", > > __func__, memreg); > > @@ -619,6 +619,9 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct > > sockaddr *addr, int memreg) > > > > rwlock_init(&ia->ri_qplock); > > return 0; > > +out3: > > + ib_dealloc_pd(ia->ri_pd); > > + ia->ri_pd = NULL; > > out2: > > module_put(ia->ri_id->device->owner); > > rdma_destroy_id(ia->ri_id); > > @@ -643,19 +646,20 @@ rpcrdma_ia_close(struct rpcrdma_ia *ia) > > dprintk("RPC: %s: ib_dereg_mr returned %i\n", > > __func__, rc); > > } > > + > > if (ia->ri_id != NULL && !IS_ERR(ia->ri_id)) { > > if (ia->ri_id->qp) > > rdma_destroy_qp(ia->ri_id); > > + > > + if (ia->ri_pd != NULL && !IS_ERR(ia->ri_pd)) { > > + rc = ib_dealloc_pd(ia->ri_pd); > > + dprintk("RPC: %s: ib_dealloc_pd returned %i\n", > > + __func__, rc); > > + } > > module_put(ia->ri_id->device->owner); > > rdma_destroy_id(ia->ri_id); > > ia->ri_id = NULL; > > } > > - > > - if (ia->ri_pd != NULL && !IS_ERR(ia->ri_pd)) { > > - rc = ib_dealloc_pd(ia->ri_pd); > > - dprintk("RPC: %s: ib_dealloc_pd returned %i\n", > > - __func__, rc); > > - } > > } > > > > /* -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c index b00e55e..096e94b 100644 --- a/net/sunrpc/xprtrdma/verbs.c +++ b/net/sunrpc/xprtrdma/verbs.c @@ -544,7 +544,7 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg) if (rc) { dprintk("RPC: %s: ib_query_device failed %d\n", __func__, rc); - goto out2; + goto out3; } if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY) { @@ -602,14 +602,14 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg) "phys register failed with %lX\n", __func__, PTR_ERR(ia->ri_bind_mem)); rc = -ENOMEM; - goto out2; + goto out3; } break; default: printk(KERN_ERR "RPC: Unsupported memory " "registration mode: %d\n", memreg); rc = -ENOMEM; - goto out2; + goto out3; } dprintk("RPC: %s: memory registration strategy is %d\n", __func__, memreg); @@ -619,6 +619,9 @@ rpcrdma_ia_open(struct rpcrdma_xprt *xprt, struct sockaddr *addr, int memreg) rwlock_init(&ia->ri_qplock); return 0; +out3: + ib_dealloc_pd(ia->ri_pd); + ia->ri_pd = NULL; out2: module_put(ia->ri_id->device->owner); rdma_destroy_id(ia->ri_id); @@ -643,19 +646,20 @@ rpcrdma_ia_close(struct rpcrdma_ia *ia) dprintk("RPC: %s: ib_dereg_mr returned %i\n", __func__, rc); } + if (ia->ri_id != NULL && !IS_ERR(ia->ri_id)) { if (ia->ri_id->qp) rdma_destroy_qp(ia->ri_id); + + if (ia->ri_pd != NULL && !IS_ERR(ia->ri_pd)) { + rc = ib_dealloc_pd(ia->ri_pd); + dprintk("RPC: %s: ib_dealloc_pd returned %i\n", + __func__, rc); + } module_put(ia->ri_id->device->owner); rdma_destroy_id(ia->ri_id); ia->ri_id = NULL; } - - if (ia->ri_pd != NULL && !IS_ERR(ia->ri_pd)) { - rc = ib_dealloc_pd(ia->ri_pd); - dprintk("RPC: %s: ib_dealloc_pd returned %i\n", - __func__, rc); - } } /*
xprtrdma tries to destroy pd after destruction of cm_id. However, pd should be deallocated before destruction of cm_id. Signed-off-by: Devesh Sharma <devesh.sharma@emulex.com> --- net/sunrpc/xprtrdma/verbs.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-)