> -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma-owner@xxxxxxxxxxxxxxx] On Behalf Of Jason Gunthorpe > Sent: Tuesday, July 14, 2015 3:30 PM > To: 'Christoph Hellwig' > Cc: Steve Wise; 'Sagi Grimberg'; 'Steve Wise'; 'Tom Talpey'; 'Doug Ledford'; sagig@xxxxxxxxxxxx; ogerlitz@xxxxxxxxxxxx; > roid@xxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; eli@xxxxxxxxxxxx; target-devel@xxxxxxxxxxxxxxx; linux-nfs@xxxxxxxxxxxxxxx; > trond.myklebust@xxxxxxxxxxxxxxx; bfields@xxxxxxxxxxxx; 'Oren Duer' > Subject: Re: [PATCH V3 1/5] RDMA/core: Transport-independent access flags > > On Tue, Jul 14, 2015 at 12:55:11PM -0700, 'Christoph Hellwig' wrote: > > On Tue, Jul 14, 2015 at 02:32:31PM -0500, Steve Wise wrote: > > > You mean "should not", yea? > > > > > > Ok. I'll check for iWARP. But don't tell me to remove the transport-specific hacks in this series when I post it! ;) > > > > Just curious if there are any holes in this little scheme to deal with > > the lkey mess: > > > > (1) make sure all drivers that currently do not set > > IB_DEVICE_LOCAL_DMA_LKEY but which can safely use ib_get_dma_mr > > call it underneath at device setup time, and tear it down before > > removal. > > Yes, I'd like this. > > local_dma_lkey appears to be global, it works with any PD. > > ib_get_dma_mr is tied to a PD, so it cannot replace local_dma_lkey at > the struct device level. > > ib_alloc_pd is the in-kernel entry point, the UAPI calls > device->alloc_pd directly.. So how about the below patch as a starting > >point? > > (Steve the goal of step #1 would be to remove ib_get_dma_mr from ULPs > Follow on patches would be to convert all ULPs to use this API change.) > > In the long run this also makes it easier to develop helper wrappers > around posting because a local_dma_lkey is now always accessible to > the core code. > I'm not seeing the benefit of adding pd->local_dma_lkey? pd->device->local_dma_lkey is there for core and ULP use, and we could have old drivers that don't currently have support for local_dma_lkey allocate their own private pd/dma_mr (via their private functions for doing this) with only LOCAL_WRITE access flags, and export that lkey as the device->local_dma_lkey. Wouldn't that be simpler? > > (2) now ULD can check for IB_DEVICE_LOCAL_DMA_LKEY and use local_dma_lkey > > in that case, or will have to perform a per-IO MR with LOCAL and > > REMOTE flags if not > > If we do the above all ULPs simply use pd->local_dma_lkey and we drop > correct uses of ib_get_dma_mr completely ? > > > (3) remove insecure remote uses of ib_get_dma_mr from ULDs > > (4) remove ib_get_dma_mr from the public API > > Sure would be nice! > > > This should help to sort out the lkey side of the memory registrations, > > and isn't too hard. For rkeys I'd love to go with something like Sagis > > proposal as a first step, and then we can see if we can refine it in > > another iteration. > > Agree. I'd love to see FMR fit under that, but even if we cannot, it > is appears to be a sane way to advance indirect MR.. > > Jason > > >From 5a9799bf487d822daae5a8b8f3b197f1dda1db45 Mon Sep 17 00:00:00 2001 > From: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> > Date: Tue, 14 Jul 2015 14:27:37 -0600 > Subject: [PATCH] IB/core: Guarantee that a local_dma_lkey is available > > Every single ULP requires a local_dma_lkey to do anything with > a QP, so lets us ensure one exists for every PD created. > > If the driver can supply a global local_dma_lkey then use that, otherwise > ask the driver to create a local use all physical memory MR associated > with the new PD. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> > --- > drivers/infiniband/core/verbs.c | 40 +++++++++++++++++++++++++++++++++++----- > include/rdma/ib_verbs.h | 2 ++ > 2 files changed, 37 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index bac3fb406a74..1ddf06314f36 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -213,24 +213,54 @@ EXPORT_SYMBOL(rdma_port_get_link_layer); > > /* Protection domains */ > > +/* Return a pd for in-kernel use that has a local_dma_lkey which provides > + local access to all physical memory. */ > struct ib_pd *ib_alloc_pd(struct ib_device *device) > { > struct ib_pd *pd; > + struct ib_device_attr devattr; > + int rc; > + > + rc = ib_query_device(device, &devattr); > + if (rc) > + return ERR_PTR(rc); > > pd = device->alloc_pd(device, NULL, NULL); > + if (IS_ERR(pd)) > + return pd; > + > + pd->device = device; > + pd->uobject = NULL; > + pd->local_mr = NULL; > + atomic_set(&pd->usecnt, 0); > + > + if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY) > + pd->local_dma_lkey = device->local_dma_lkey; > + else { > + struct ib_mr *mr; > + > + mr = ib_get_dma_mr(pd, IB_ACCESS_LOCAL_WRITE); > + if (IS_ERR(mr)) { > + ib_dealloc_pd(pd); > + return (struct ib_pd *)mr; > + } > > - if (!IS_ERR(pd)) { > - pd->device = device; > - pd->uobject = NULL; > - atomic_set(&pd->usecnt, 0); > + pd->local_mr = mr; > + pd->local_dma_lkey = pd->local_mr->lkey; > } > - > return pd; > } > + > EXPORT_SYMBOL(ib_alloc_pd); > > int ib_dealloc_pd(struct ib_pd *pd) > { > + if (pd->local_mr) { > + if (ib_dereg_mr(pd->local_mr)) > + return -EBUSY; > + pd->local_mr = NULL; > + } > + > if (atomic_read(&pd->usecnt)) > return -EBUSY; > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 986fddb08579..cfda95d7b067 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1255,6 +1255,8 @@ struct ib_pd { > struct ib_device *device; > struct ib_uobject *uobject; > atomic_t usecnt; /* count all resources */ > + struct ib_mr *local_mr; > + u32 local_dma_lkey; > }; > > struct ib_xrcd { > -- > 2.1.4 > > -- > 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 -- 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
