| Message ID | 20190620161240.22738-21-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Removing struct page from P2PDMA | expand |
On Thu, Jun 20, 2019 at 10:12:32AM -0600, Logan Gunthorpe wrote: > Introduce rdma_rw_ctx_dma_init() and rdma_rw_ctx_dma_destroy() which > peform the same operation as rdma_rw_ctx_init() and > rdma_rw_ctx_destroy() respectively except they operate on a DMA > address and length instead of an SGL. > > This will be used for struct page-less P2PDMA, but there's also > been opinions expressed to migrate away from SGLs and struct > pages in the RDMA APIs and this will likely fit with that > effort. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > drivers/infiniband/core/rw.c | 74 ++++++++++++++++++++++++++++++------ > include/rdma/rw.h | 6 +++ > 2 files changed, 69 insertions(+), 11 deletions(-) > > diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c > index 32ca8429eaae..cefa6b930bc8 100644 > +++ b/drivers/infiniband/core/rw.c > @@ -319,6 +319,39 @@ int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, > } > EXPORT_SYMBOL(rdma_rw_ctx_init); > > +/** > + * rdma_rw_ctx_dma_init - initialize a RDMA READ/WRITE context from a > + * DMA address instead of SGL > + * @ctx: context to initialize > + * @qp: queue pair to operate on > + * @port_num: port num to which the connection is bound > + * @addr: DMA address to READ/WRITE from/to > + * @len: length of memory to operate on > + * @remote_addr:remote address to read/write (relative to @rkey) > + * @rkey: remote key to operate on > + * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ > + * > + * Returns the number of WQEs that will be needed on the workqueue if > + * successful, or a negative error code. > + */ > +int rdma_rw_ctx_dma_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, > + u8 port_num, dma_addr_t addr, u32 len, u64 remote_addr, > + u32 rkey, enum dma_data_direction dir) Why not keep the same basic signature here but replace the scatterlist with the dma vec ? > +{ > + struct scatterlist sg; > + > + sg_dma_address(&sg) = addr; > + sg_dma_len(&sg) = len; This needs to fail if the driver is one of the few that require struct page to work.. Really want I want to do is to have this new 'dma vec' pushed through the RDMA APIs so we know that if a driver is using the dma vec interface it is struct page free. This is not so hard to do, as most drivers are already struct page free, but is pretty much blocked on needing some way to go from the block layer SGL world to the dma vec world that does not hurt storage performance. I am hoping that the biovec dma mapping that CH has talked about will give the missing pieces. FWIW, rdma is one of the places that is largely struct page free, and has few problems to natively handle a 'dma vec' from top to bottom, so I do like this approach. Someone would have to look carefully at siw, rxe and hfi/qib to see how they could continue to work with a dma vec, as they do actually seem to need to kmap the data they are transferring. However, I thought they were using custom dma ops these days, so maybe they just encode a struct page in their dma vec and reject p2p entirely? Jason
On 2019-06-20 10:49 a.m., Jason Gunthorpe wrote: > On Thu, Jun 20, 2019 at 10:12:32AM -0600, Logan Gunthorpe wrote: >> Introduce rdma_rw_ctx_dma_init() and rdma_rw_ctx_dma_destroy() which >> peform the same operation as rdma_rw_ctx_init() and >> rdma_rw_ctx_destroy() respectively except they operate on a DMA >> address and length instead of an SGL. >> >> This will be used for struct page-less P2PDMA, but there's also >> been opinions expressed to migrate away from SGLs and struct >> pages in the RDMA APIs and this will likely fit with that >> effort. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> drivers/infiniband/core/rw.c | 74 ++++++++++++++++++++++++++++++------ >> include/rdma/rw.h | 6 +++ >> 2 files changed, 69 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c >> index 32ca8429eaae..cefa6b930bc8 100644 >> +++ b/drivers/infiniband/core/rw.c >> @@ -319,6 +319,39 @@ int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, >> } >> EXPORT_SYMBOL(rdma_rw_ctx_init); >> >> +/** >> + * rdma_rw_ctx_dma_init - initialize a RDMA READ/WRITE context from a >> + * DMA address instead of SGL >> + * @ctx: context to initialize >> + * @qp: queue pair to operate on >> + * @port_num: port num to which the connection is bound >> + * @addr: DMA address to READ/WRITE from/to >> + * @len: length of memory to operate on >> + * @remote_addr:remote address to read/write (relative to @rkey) >> + * @rkey: remote key to operate on >> + * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ >> + * >> + * Returns the number of WQEs that will be needed on the workqueue if >> + * successful, or a negative error code. >> + */ >> +int rdma_rw_ctx_dma_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, >> + u8 port_num, dma_addr_t addr, u32 len, u64 remote_addr, >> + u32 rkey, enum dma_data_direction dir) > > Why not keep the same basic signature here but replace the scatterlist > with the dma vec ? Could do. At the moment, I had no need for dma_vec in this interface. >> +{ >> + struct scatterlist sg; >> + >> + sg_dma_address(&sg) = addr; >> + sg_dma_len(&sg) = len; > > This needs to fail if the driver is one of the few that require > struct page to work.. Yes, right. Currently P2PDMA checks for the use of dma_virt_ops. And that probably should also be done here. But is that sufficient? You're probably right that it'll take an audit of the RDMA tree to sort that out. > Really want I want to do is to have this new 'dma vec' pushed through > the RDMA APIs so we know that if a driver is using the dma vec > interface it is struct page free. Yeah, I know you were talking about heading this way during LSF/MM and is partly what inspired this series. However, largely, my focus for this RFC was the block layer to see this is an acceptable approach -- I just kind of hacked RDMA for now. > This is not so hard to do, as most drivers are already struct page > free, but is pretty much blocked on needing some way to go from the > block layer SGL world to the dma vec world that does not hurt storage > performance. Maybe I can end up helping with that if it helps push the ideas here through. (And assuming people think it's an acceptable approach for the block-layer side of things). Thanks, Logan
On Thu, Jun 20, 2019 at 10:59:44AM -0600, Logan Gunthorpe wrote: > > > On 2019-06-20 10:49 a.m., Jason Gunthorpe wrote: > > On Thu, Jun 20, 2019 at 10:12:32AM -0600, Logan Gunthorpe wrote: > >> Introduce rdma_rw_ctx_dma_init() and rdma_rw_ctx_dma_destroy() which > >> peform the same operation as rdma_rw_ctx_init() and > >> rdma_rw_ctx_destroy() respectively except they operate on a DMA > >> address and length instead of an SGL. > >> > >> This will be used for struct page-less P2PDMA, but there's also > >> been opinions expressed to migrate away from SGLs and struct > >> pages in the RDMA APIs and this will likely fit with that > >> effort. > >> > >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > >> drivers/infiniband/core/rw.c | 74 ++++++++++++++++++++++++++++++------ > >> include/rdma/rw.h | 6 +++ > >> 2 files changed, 69 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c > >> index 32ca8429eaae..cefa6b930bc8 100644 > >> +++ b/drivers/infiniband/core/rw.c > >> @@ -319,6 +319,39 @@ int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, > >> } > >> EXPORT_SYMBOL(rdma_rw_ctx_init); > >> > >> +/** > >> + * rdma_rw_ctx_dma_init - initialize a RDMA READ/WRITE context from a > >> + * DMA address instead of SGL > >> + * @ctx: context to initialize > >> + * @qp: queue pair to operate on > >> + * @port_num: port num to which the connection is bound > >> + * @addr: DMA address to READ/WRITE from/to > >> + * @len: length of memory to operate on > >> + * @remote_addr:remote address to read/write (relative to @rkey) > >> + * @rkey: remote key to operate on > >> + * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ > >> + * > >> + * Returns the number of WQEs that will be needed on the workqueue if > >> + * successful, or a negative error code. > >> + */ > >> +int rdma_rw_ctx_dma_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, > >> + u8 port_num, dma_addr_t addr, u32 len, u64 remote_addr, > >> + u32 rkey, enum dma_data_direction dir) > > > > Why not keep the same basic signature here but replace the scatterlist > > with the dma vec ? > > Could do. At the moment, I had no need for dma_vec in this interface. I think that is because you only did nvme not srp/iser :) > >> +{ > >> + struct scatterlist sg; > >> + > >> + sg_dma_address(&sg) = addr; > >> + sg_dma_len(&sg) = len; > > > > This needs to fail if the driver is one of the few that require > > struct page to work.. > > Yes, right. Currently P2PDMA checks for the use of dma_virt_ops. And > that probably should also be done here. But is that sufficient? You're > probably right that it'll take an audit of the RDMA tree to sort that out. For this purpose I'd be fine if you added a flag to the struct ib_device_ops that is set on drivers that we know are OK.. We can make that list bigger over time. > > This is not so hard to do, as most drivers are already struct page > > free, but is pretty much blocked on needing some way to go from the > > block layer SGL world to the dma vec world that does not hurt storage > > performance. > > Maybe I can end up helping with that if it helps push the ideas here > through. (And assuming people think it's an acceptable approach for the > block-layer side of things). Let us hope for a clear decision then Jason
On 2019-06-20 11:11 a.m., Jason Gunthorpe wrote: > On Thu, Jun 20, 2019 at 10:59:44AM -0600, Logan Gunthorpe wrote: >> >> >> On 2019-06-20 10:49 a.m., Jason Gunthorpe wrote: >>> On Thu, Jun 20, 2019 at 10:12:32AM -0600, Logan Gunthorpe wrote: >>>> Introduce rdma_rw_ctx_dma_init() and rdma_rw_ctx_dma_destroy() which >>>> peform the same operation as rdma_rw_ctx_init() and >>>> rdma_rw_ctx_destroy() respectively except they operate on a DMA >>>> address and length instead of an SGL. >>>> >>>> This will be used for struct page-less P2PDMA, but there's also >>>> been opinions expressed to migrate away from SGLs and struct >>>> pages in the RDMA APIs and this will likely fit with that >>>> effort. >>>> >>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>> drivers/infiniband/core/rw.c | 74 ++++++++++++++++++++++++++++++------ >>>> include/rdma/rw.h | 6 +++ >>>> 2 files changed, 69 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c >>>> index 32ca8429eaae..cefa6b930bc8 100644 >>>> +++ b/drivers/infiniband/core/rw.c >>>> @@ -319,6 +319,39 @@ int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, >>>> } >>>> EXPORT_SYMBOL(rdma_rw_ctx_init); >>>> >>>> +/** >>>> + * rdma_rw_ctx_dma_init - initialize a RDMA READ/WRITE context from a >>>> + * DMA address instead of SGL >>>> + * @ctx: context to initialize >>>> + * @qp: queue pair to operate on >>>> + * @port_num: port num to which the connection is bound >>>> + * @addr: DMA address to READ/WRITE from/to >>>> + * @len: length of memory to operate on >>>> + * @remote_addr:remote address to read/write (relative to @rkey) >>>> + * @rkey: remote key to operate on >>>> + * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ >>>> + * >>>> + * Returns the number of WQEs that will be needed on the workqueue if >>>> + * successful, or a negative error code. >>>> + */ >>>> +int rdma_rw_ctx_dma_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, >>>> + u8 port_num, dma_addr_t addr, u32 len, u64 remote_addr, >>>> + u32 rkey, enum dma_data_direction dir) >>> >>> Why not keep the same basic signature here but replace the scatterlist >>> with the dma vec ? >> >> Could do. At the moment, I had no need for dma_vec in this interface. > > I think that is because you only did nvme not srp/iser :) I'm not sure that's true at least for the P2P case. With P2P we are able to allocate one continuous region of memory for each transaction. It would be quite weird to allocate multiple regions for a single transaction. >>>> +{ >>>> + struct scatterlist sg; >>>> + >>>> + sg_dma_address(&sg) = addr; >>>> + sg_dma_len(&sg) = len; >>> >>> This needs to fail if the driver is one of the few that require >>> struct page to work.. >> >> Yes, right. Currently P2PDMA checks for the use of dma_virt_ops. And >> that probably should also be done here. But is that sufficient? You're >> probably right that it'll take an audit of the RDMA tree to sort that out. > > For this purpose I'd be fine if you added a flag to the struct > ib_device_ops that is set on drivers that we know are OK.. We can make > that list bigger over time. Ok, that would mirror what we did for the block layer. I'll look at doing something like that in the near future. Thanks, Logan
diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c index 32ca8429eaae..cefa6b930bc8 100644 --- a/drivers/infiniband/core/rw.c +++ b/drivers/infiniband/core/rw.c @@ -319,6 +319,39 @@ int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, } EXPORT_SYMBOL(rdma_rw_ctx_init); +/** + * rdma_rw_ctx_dma_init - initialize a RDMA READ/WRITE context from a + * DMA address instead of SGL + * @ctx: context to initialize + * @qp: queue pair to operate on + * @port_num: port num to which the connection is bound + * @addr: DMA address to READ/WRITE from/to + * @len: length of memory to operate on + * @remote_addr:remote address to read/write (relative to @rkey) + * @rkey: remote key to operate on + * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ + * + * Returns the number of WQEs that will be needed on the workqueue if + * successful, or a negative error code. + */ +int rdma_rw_ctx_dma_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, + u8 port_num, dma_addr_t addr, u32 len, u64 remote_addr, + u32 rkey, enum dma_data_direction dir) +{ + struct scatterlist sg; + + sg_dma_address(&sg) = addr; + sg_dma_len(&sg) = len; + + if (rdma_rw_io_needs_mr(qp->device, port_num, dir, 1)) + return rdma_rw_init_mr_wrs(ctx, qp, port_num, &sg, 1, 0, + remote_addr, rkey, dir); + else + return rdma_rw_init_single_wr(ctx, qp, &sg, 0, remote_addr, + rkey, dir); +} +EXPORT_SYMBOL(rdma_rw_ctx_dma_init); + /** * rdma_rw_ctx_signature_init - initialize a RW context with signature offload * @ctx: context to initialize @@ -566,17 +599,7 @@ int rdma_rw_ctx_post(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, } EXPORT_SYMBOL(rdma_rw_ctx_post); -/** - * rdma_rw_ctx_destroy - release all resources allocated by rdma_rw_ctx_init - * @ctx: context to release - * @qp: queue pair to operate on - * @port_num: port num to which the connection is bound - * @sg: scatterlist that was used for the READ/WRITE - * @sg_cnt: number of entries in @sg - * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ - */ -void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, - struct scatterlist *sg, u32 sg_cnt, enum dma_data_direction dir) +static void __rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp) { int i; @@ -596,6 +619,21 @@ void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, BUG(); break; } +} + +/** + * rdma_rw_ctx_destroy - release all resources allocated by rdma_rw_ctx_init + * @ctx: context to release + * @qp: queue pair to operate on + * @port_num: port num to which the connection is bound + * @sg: scatterlist that was used for the READ/WRITE + * @sg_cnt: number of entries in @sg + * @dir: %DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ + */ +void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, + struct scatterlist *sg, u32 sg_cnt, enum dma_data_direction dir) +{ + __rdma_rw_ctx_destroy(ctx, qp); /* P2PDMA contexts do not need to be unmapped */ if (!is_pci_p2pdma_page(sg_page(sg))) @@ -603,6 +641,20 @@ void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, } EXPORT_SYMBOL(rdma_rw_ctx_destroy); +/** + * rdma_rw_ctx_dma_destroy - release all resources allocated by + * rdma_rw_ctx_dma_init + * @ctx: context to release + * @qp: queue pair to operate on + * @port_num: port num to which the connection is bound + */ +void rdma_rw_ctx_dma_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, + u8 port_num) +{ + __rdma_rw_ctx_destroy(ctx, qp); +} +EXPORT_SYMBOL(rdma_rw_ctx_dma_destroy); + /** * rdma_rw_ctx_destroy_signature - release all resources allocated by * rdma_rw_ctx_init_signature diff --git a/include/rdma/rw.h b/include/rdma/rw.h index 494f79ca3e62..e47f8053af6e 100644 --- a/include/rdma/rw.h +++ b/include/rdma/rw.h @@ -58,6 +58,12 @@ void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, struct scatterlist *sg, u32 sg_cnt, enum dma_data_direction dir); +int rdma_rw_ctx_dma_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, + u8 port_num, dma_addr_t addr, u32 len, u64 remote_addr, + u32 rkey, enum dma_data_direction dir); +void rdma_rw_ctx_dma_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, + u8 port_num); + int rdma_rw_ctx_signature_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num, struct scatterlist *sg, u32 sg_cnt, struct scatterlist *prot_sg, u32 prot_sg_cnt,
Introduce rdma_rw_ctx_dma_init() and rdma_rw_ctx_dma_destroy() which peform the same operation as rdma_rw_ctx_init() and rdma_rw_ctx_destroy() respectively except they operate on a DMA address and length instead of an SGL. This will be used for struct page-less P2PDMA, but there's also been opinions expressed to migrate away from SGLs and struct pages in the RDMA APIs and this will likely fit with that effort. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/infiniband/core/rw.c | 74 ++++++++++++++++++++++++++++++------ include/rdma/rw.h | 6 +++ 2 files changed, 69 insertions(+), 11 deletions(-)