svcrdma: WR context management bug fixes and cleanup
Subsystems (2)
Quoted 17 lines · Tom Tucker
Bruce:
This set of patches fixes bugs related to DMA mapping and unmapping and in the process redesigns and cleans up the way that WR contexts are managed.
In addition to the bug fixes, there are three major changes:
- The data structure that maps client side memory to server side memory has been separated from the WR context. This makes for cleaner type checking and a smaller memory footprint for the RDMA transport.
- The mapping and unmapping of DMA is performed independently from the allocation and deallocation of the WR context. This is to allow the unmapping to occur early on recv where the unmapping of the DMA needs to be done prior to looking at the data and therefore before deallocating the WR context.
Quoted 6 lines
- The WR context cache now uses a
kmem_cacheand is shared across all transport instances. This allows contexts to be shared between mounts and idle mounts don't consume context memory.This patchset is intended for 2.6.27 and is based on 2.6.26-rc4. These patches are also available at the following git url:
OK, thanks. Bug me if I haven't commented or applied these by next week.
Is there anything else remaining for 2.6.26 (as opposed to .27)?
--b.
Quoted 68 lines
git://git.linux-nfs.org/projects/tomtucker/xprt-switch-2.6.git[PATCH 01/11] svcrdma: Add a type for keeping NFS request mapping
include/linux/sunrpc/svc_rdma.h | 27 +++++++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma.c | 23 +++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 26 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 0 deletions(-)[PATCH 02/11] svcrdma: Use rpc reply map for
RDMA_WRITEprocessingnet/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)[PATCH 03/11] svcrdma: Use reply and chunk map for
RDMA_READprocessinginclude/linux/sunrpc/svc_rdma.h | 1 + net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 83 ++++++++++++++----------------- 2 files changed, 39 insertions(+), 45 deletions(-)[PATCH 04/11] svcrdma: Move the DMA unmap logic to the CQ handler
net/sunrpc/xprtrdma/svc_rdma_transport.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)[PATCH 05/11] svcrdma: Add dma map count and
WARN_ONinclude/linux/sunrpc/svc_rdma.h | 1 + net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 1 + net/sunrpc/xprtrdma/svc_rdma_sendto.c | 3 +++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +++++ 4 files changed, 10 insertions(+), 0 deletions(-)[PATCH 06/11] svcrdma: Remove unneeded spin locks from
__svc_rdma_freenet/sunrpc/xprtrdma/svc_rdma_transport.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)[PATCH 07/11] svcrdma: Remove unused wait q from
svcrdma_xprtstructureinclude/linux/sunrpc/svc_rdma.h | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)[PATCH 08/11] svcrdma: Limit ORD based on client's advertised IRD
net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-)[PATCH 09/11] svcrdma: Add
flush_scheduled_workto module exit functionnet/sunrpc/xprtrdma/svc_rdma.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)[PATCH 10/11] svcrdma: Create a kmem cache for the WR contexts
net/sunrpc/xprtrdma/svc_rdma.c | 24 ++++++++++++++++++++---- 1 files changed, 20 insertions(+), 4 deletions(-)[PATCH 11/11] svcrdma: Change WR context get/put to use the kmem cache
include/linux/sunrpc/svc_rdma.h | 6 -- net/sunrpc/xprtrdma/svc_rdma_transport.c | 121 +++--------------------------- 2 files changed, 12 insertions(+), 115 deletions(-)Signed-off-by Tom Tucker <[email protected]>
Quoted 22 lines · J. Bruce Fields
Quoted 21 lines · Tom Tucker
Bruce:
This set of patches fixes bugs related to DMA mapping and unmapping and in the process redesigns and cleans up the way that WR contexts are managed.
In addition to the bug fixes, there are three major changes:
- The data structure that maps client side memory to server side memory has been separated from the WR context. This makes for cleaner type checking and a smaller memory footprint for the RDMA transport.
- The mapping and unmapping of DMA is performed independently from the allocation and deallocation of the WR context. This is to allow the unmapping to occur early on recv where the unmapping of the DMA needs to be done prior to looking at the data and therefore before deallocating the WR context.
- The WR context cache now uses a
kmem_cacheand is shared across all transport instances. This allows contexts to be shared between mounts and idle mounts don't consume context memory.
Quoted 7 lines
Quoted 2 lines
This patchset is intended for 2.6.27 and is based on 2.6.26-rc4. These patches are also available at the following git url:
OK, thanks. Bug me if I haven't commented or applied these by next week.
Is there anything else remaining for 2.6.26 (as opposed to .27)?
I'm inclined to say "no". But I'd like to hear if anyone out there is dead in the water due to a bug in the 2.6.26 version.
Tom
Quoted 71 lines
--b.
Quoted 68 lines
git://git.linux-nfs.org/projects/tomtucker/xprt-switch-2.6.git[PATCH 01/11] svcrdma: Add a type for keeping NFS request mapping
include/linux/sunrpc/svc_rdma.h | 27 +++++++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma.c | 23 +++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 26 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 0 deletions(-)[PATCH 02/11] svcrdma: Use rpc reply map for
RDMA_WRITEprocessingnet/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)[PATCH 03/11] svcrdma: Use reply and chunk map for
RDMA_READprocessinginclude/linux/sunrpc/svc_rdma.h | 1 + net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 83 ++++++++++++++----------------- 2 files changed, 39 insertions(+), 45 deletions(-)[PATCH 04/11] svcrdma: Move the DMA unmap logic to the CQ handler
net/sunrpc/xprtrdma/svc_rdma_transport.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)[PATCH 05/11] svcrdma: Add dma map count and
WARN_ONinclude/linux/sunrpc/svc_rdma.h | 1 + net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 1 + net/sunrpc/xprtrdma/svc_rdma_sendto.c | 3 +++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +++++ 4 files changed, 10 insertions(+), 0 deletions(-)[PATCH 06/11] svcrdma: Remove unneeded spin locks from
__svc_rdma_freenet/sunrpc/xprtrdma/svc_rdma_transport.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)[PATCH 07/11] svcrdma: Remove unused wait q from
svcrdma_xprtstructureinclude/linux/sunrpc/svc_rdma.h | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)[PATCH 08/11] svcrdma: Limit ORD based on client's advertised IRD
net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-)[PATCH 09/11] svcrdma: Add
flush_scheduled_workto module exit functionnet/sunrpc/xprtrdma/svc_rdma.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)[PATCH 10/11] svcrdma: Create a kmem cache for the WR contexts
net/sunrpc/xprtrdma/svc_rdma.c | 24 ++++++++++++++++++++---- 1 files changed, 20 insertions(+), 4 deletions(-)[PATCH 11/11] svcrdma: Change WR context get/put to use the kmem cache
include/linux/sunrpc/svc_rdma.h | 6 -- net/sunrpc/xprtrdma/svc_rdma_transport.c | 121 +++--------------------------- 2 files changed, 12 insertions(+), 115 deletions(-)Signed-off-by Tom Tucker <[email protected]>
Quoted 2 lines
I'm inclined to say "no". But I'd like to hear if anyone out there is dead in the water due to a bug in the 2.6.26 version.
To be honest I haven't had a chance to do much NFS/RDMA testing with the current 2.6.26 tree. But last I looked there were no major regressions at least (still open issues I think, but definitely better than 2.6.25)
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
In reply to a message not in the archiveQuoted 61 lines · Tom Tucker
Create a new data structure to hold the remote client address space to local server address space mapping.
Signed-off-by Tom Tucker <[email protected]>--- include/linux/sunrpc/svc_rdma.h | 27 +++++++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma.c | 23 +++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 26 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 0 deletions(-)include/linux/sunrpc/svc_rdma.h +27
-0@@ -86,6 +86,31 @@ struct svc_rdma_op_ctxt {struct page *pages[RPCSVC_MAXPAGES ]; }; +/* +* NFS_ requests are mapped on the client side by the chunk lists in +* the RPCRDMA header. During the fetching of the RPC from the client +* and the writing of the reply to the client, the memory in the +* client and the memory in the server must be mapped as contiguous +* vaddr/len for access by the hardware. These data strucures keep +* these mappings. +* +* For an RDMA_WRITE, the 'sge' maps the RPC REPLY. For RDMA_READ, the +* 'sge' in the svc_rdma_req_map maps the server side RPC reply and the +* 'ch' field maps the read-list of the RPCRDMA header to the 'sge' +* mapping of the reply. +*/ +struct svc_rdma_chunk_sge { +int start;/* sge no for this chunk */ +int count;/* sge count for this chunk */ +}; +struct svc_rdma_req_map { +unsigned long count; +union { +struct kvec sge[RPCSVC_MAXPAGES ]; +struct svc_rdma_chunk_sge ch[RPCSVC_MAXPAGES ]; + }; +}; +#define RDMACTXT_F_LAST_CTXT 2struct svcxprt_rdma {@@ -173,6 +198,8 @@ extern int svc_rdma_post_recv(struct svcxprt_rdma *);extern int svc_rdma_create_listen (struct svc_serv *,int ,struct sockaddr *);extern struct svc_rdma_op_ctxt *svc_rdma_get_context (struct svcxprt_rdma *);extern void svc_rdma_put_context (struct svc_rdma_op_ctxt *,int ); +extern struct svc_rdma_req_map *svc_rdma_get_req_map (void ); +extern void svc_rdma_put_req_map (struct svc_rdma_req_map *);extern void svc_sq_reap (struct svcxprt_rdma *);extern void svc_rq_reap (struct svcxprt_rdma *);extern struct svc_xprt_class svc_rdma_class;net/sunrpc/xprtrdma/svc_rdma.c +0
-0
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 73
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-0@@ -69,6 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod; +/* Temporary NFS request map cache */ +struct kmem_cache *svc_rdma_map_cachep =NULL ;
No need to initialize globals to NULL.
net/sunrpc/xprtrdma/svc_rdma.c void svc_rdma_cleanup(void) · line 248
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma.c +3
-0+/* * This function implements reading and resetting an atomic_t stat * variable through read/write to a proc file. Any write to the file @@ -241,6 +244,8 @@ void svc_rdma_cleanup(void) svcrdma_table_header =NULL ; }svc_unreg_xprt_class (&svc_rdma_class); +if (svc_rdma_map_cachep) +kmem_cache_destroy (svc_rdma_map_cachep);
By design this can't be NULL (and the code presumably would have oopsed much earlier if it were mistakenly allowed to be) so I'm inclined not to bother checking....
Quoted 15 lines
net/sunrpc/xprtrdma/svc_rdma.c +8
-0}int svc_rdma_init (void )@@ -255,9 +260,27 @@ int svc_rdma_init(void) svcrdma_table_header =register_sysctl_table (svcrdma_root_table); +/* Create the temporary map cache */ + svc_rdma_map_cachep =kmem_cache_create ("svc_rdma_map_cache" , +sizeof (struct svc_rdma_req_map ), +0 , +SLAB_HWCACHE_ALIGN , +NULL ); +if (!svc_rdma_map_cachep) { +printk (KERN_INFO "Could not allocate map cache.\n" );
net/sunrpc/xprtrdma/svc_rdma.c int svc_rdma_init(void) · line 280
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma.c +7
-0+ goto err; + } +/* Register RDMA with the SVC transport switch */ svc_reg_xprt_class (&svc_rdma_class);return 0 ; + + err: +if (svcrdma_table_header) { +unregister_sysctl_table (svcrdma_table_header);
unregister_sysctl_table() already handles the NULL case, so you could skip the if (svcrdma_table_header).
--b.
Quoted 43 lines
net/sunrpc/xprtrdma/svc_rdma.c +3
-0+ svcrdma_table_header =NULL ; + } +return -ENOMEM ; }MODULE_AUTHOR ("Tom Tucker <[email protected]>" );MODULE_DESCRIPTION ("SVC RDMA Transport" );net/sunrpc/xprtrdma/svc_rdma_transport.c +26
-0@@ -173,6 +173,32 @@ void svc_rdma_put_context(struct svc_rdma_op_ctxt *ctxt, int free_pages)atomic_dec (&xprt->sc_ctxt_used); } +/* Temporary NFS request map cache. Created in svc_rdma.c */ +extern struct kmem_cache *svc_rdma_map_cachep; + +/* +* Temporary NFS req mappings are shared across all transport +* instances. These are short lived and should be bounded by the number +* of concurrent server threads * depth of the SQ. +*/ +struct svc_rdma_req_map *svc_rdma_get_req_map (void ) +{ +struct svc_rdma_req_map *map; +while (1 ) { + map =kmem_cache_alloc (svc_rdma_map_cachep,GFP_KERNEL ); +if (map) +break ; +schedule_timeout_uninterruptible (msecs_to_jiffies (500 )); + } + map->count =0 ; +return map; +} + +void svc_rdma_put_req_map (struct svc_rdma_req_map *map) +{ +kmem_cache_free (svc_rdma_map_cachep, map); +} +/* ib_cq event handler */ static void cq_event_handler (struct ib_event *event,void *context) {
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to a message not in the archiveQuoted 30 lines · Tom Tucker
Use the new
svc_rdma_req_mapdata type for mapping the client side memory to the server side memory. Move the DMA mapping to the context pointed to by each WR individually so that it is unmapped after the WR completes.Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)net/sunrpc/xprtrdma/svc_rdma_sendto.c +3
-4@@ -64,10 +64,9 @@* SGE[sge_count-1] data from xdr->tail. * */ -static struct ib_sge *xdr_to_sge (struct svcxprt_rdma *xprt, -struct xdr_buf *xdr, -struct ib_sge *sge, -int *sge_count) +static void xdr_to_sge (struct svcxprt_rdma *xprt, +struct xdr_buf *xdr, +struct svc_rdma_req_map *vec) {/* Max we need is the length of the XDR / pagesize + one for * head + one for tail + one for RPCRDMA header
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 83
Quoted 1 line
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0@@ -84,14 +83,10 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, sge_no =1 ;
Should the arrays in the svc_rdma_req_map actually be size RPC_SVC_MAXPAGES+1 to allow for the RPCRDMA header?
Also, the only caller of xdr_to_sge() appears to be svc_rdma_sendto(), which is called from svc_sendto() immediately after setting:
xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len + xb->page_len + xb->tail[0].iov_len;
So I think xdr_to_sge() is doing a bunch of unnecessary work to handle the case where xdr->len could be less than that sum?
Quoted 116 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +19
-36/* Head SGE */ - sge[sge_no].addr =ib_dma_map_single (xprt->sc_cm_id->device, - xdr->head[0 ].iov_base, - xdr->head[0 ].iov_len, -DMA_TO_DEVICE ); + vec->sge[sge_no].iov_base = xdr->head[0 ].iov_base; sge_bytes =min_t (u32, byte_count, xdr->head[0 ].iov_len); byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++;/* pages SGE */ @@ -99,16 +94,13 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, page_bytes = xdr->page_len; page_off = xdr->page_base;while (byte_count && page_bytes) { + vec->sge[sge_no].iov_base = +page_address (xdr->pages[page_no]) + page_off; sge_bytes =min_t (u32, byte_count, (PAGE_SIZE -page_off)); - sge[sge_no].addr = -ib_dma_map_page (xprt->sc_cm_id->device, - xdr->pages[page_no], page_off, - sge_bytes,DMA_TO_DEVICE ); sge_bytes =min (sge_bytes, page_bytes); byte_count -= sge_bytes; page_bytes -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; page_no++;@@ -117,23 +109,17 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,/* Tail SGE */ if (byte_count && xdr->tail[0 ].iov_len) { - sge[sge_no].addr = -ib_dma_map_single (xprt->sc_cm_id->device, - xdr->tail[0 ].iov_base, - xdr->tail[0 ].iov_len, -DMA_TO_DEVICE ); + vec->sge[sge_no].iov_base = xdr->tail[0 ].iov_base; sge_bytes =min_t (u32, byte_count, xdr->tail[0 ].iov_len); byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; }BUG_ON (sge_no > sge_max);BUG_ON (byte_count !=0 ); - *sge_count = sge_no; -return sge; + vec->count = sge_no; }@@ -143,9 +129,8 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,static int send_write (struct svcxprt_rdma *xprt,struct svc_rqst *rqstp,u32 rmr,u64 to,u32 xdr_off,int write_len, -struct ib_sge *xdr_sge,int sge_count) +struct svc_rdma_req_map *vec) { -struct svc_rdma_op_ctxt *tmp_sge_ctxt;struct ib_send_wr write_wr;struct ib_sge *sge;int xdr_sge_no;@@ -156,23 +141,22 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,struct svc_rdma_op_ctxt *ctxt;int ret =0 ; -BUG_ON (sge_count >RPCSVC_MAXPAGES ); +BUG_ON (vec->count >RPCSVC_MAXPAGES );dprintk ("svcrdma: RDMA_WRITE rmr=%x, to=%llx, xdr_off=%d, " -"write_len=%d, xdr_sge=%p, sge_count=%d\n" , +"write_len=%d, vec->sge=%p, vec->count=%lu\n" , rmr, (unsigned long long )to, xdr_off, - write_len, xdr_sge, sge_count); + write_len, vec->sge, vec->count); ctxt =svc_rdma_get_context (xprt); - ctxt->count =0 ; - tmp_sge_ctxt =svc_rdma_get_context (xprt); - sge = tmp_sge_ctxt->sge; + ctxt->direction =DMA_TO_DEVICE ; + sge = ctxt->sge;/* Find the SGE associated with xdr_off */ -for (bc = xdr_off, xdr_sge_no =1 ; bc && xdr_sge_no < sge_count; +for (bc = xdr_off, xdr_sge_no =1 ; bc && xdr_sge_no < vec->count; xdr_sge_no++) { -if (xdr_sge[xdr_sge_no].length > bc) +if (vec->sge[xdr_sge_no].iov_len > bc)break ; - bc -= xdr_sge[xdr_sge_no].length; + bc -= vec->sge[xdr_sge_no].iov_len; } sge_off = bc;@@ -180,21 +164,27 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, sge_no =0 ;/* Copy the remaining SGE */ -while (bc !=0 && xdr_sge_no < sge_count) { - sge[sge_no].addr = xdr_sge[xdr_sge_no].addr + sge_off; - sge[sge_no].lkey = xdr_sge[xdr_sge_no].lkey; +while (bc !=0 && xdr_sge_no < vec->count) { + sge[sge_no].lkey = xprt->sc_phys_mr->lkey; sge_bytes = min((size_t)bc, - (size_t )(xdr_sge[xdr_sge_no].length-sge_off));
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, · line 178
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +8
-1+ (size_t)(vec->sge[xdr_sge_no].iov_len-sge_off)); sge[sge_no].length = sge_bytes; - + sge[sge_no].addr = +ib_dma_map_single (xprt->sc_cm_id->device, + (void *) + vec->sge[xdr_sge_no].iov_base + sge_off, + sge_bytes,DMA_TO_DEVICE ); +if (dma_mapping_error (sge[sge_no].addr)) +return -EINVAL ;
We leak a svc_rdma_op_ctxt on error here.
--b.
Quoted 183 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +28
-23sge_off =0 ; sge_no++; + ctxt->count++; xdr_sge_no++; bc -= sge_bytes; }BUG_ON (bc !=0 ); -BUG_ON (xdr_sge_no > sge_count); +BUG_ON (xdr_sge_no > vec->count);/* Prepare WRITE WR */ memset (&write_wr,0 ,sizeof write_wr);@@ -210,11 +200,10 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,/* Post It */ atomic_inc (&rdma_stat_write);if (svc_rdma_send (xprt, &write_wr)) { -svc_rdma_put_context (ctxt,1 ); +svc_rdma_put_context (ctxt,0 );/* Fatal error, close transport */ ret = -EIO ; } -svc_rdma_put_context (tmp_sge_ctxt,0 );return ret; }@@ -222,8 +211,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt,struct rpcrdma_msg *rdma_argp, struct rpcrdma_msg *rdma_resp, struct svc_rqst *rqstp, - struct ib_sge *sge, - int sge_count) + struct svc_rdma_req_map *vec) { u32 xfer_len = rqstp->rq_res.page_len + rqstp->rq_res.tail[0].iov_len;int write_len;@@ -269,8 +257,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt, rs_offset + chunk_off, xdr_off, this_write, - sge, - sge_count); + vec);if (ret) {dprintk ("svcrdma: RDMA_WRITE failed, ret=%d\n" , ret);@@ -292,8 +279,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt,struct rpcrdma_msg *rdma_argp, struct rpcrdma_msg *rdma_resp, struct svc_rqst *rqstp, - struct ib_sge *sge, - int sge_count) + struct svc_rdma_req_map *vec) { u32 xfer_len = rqstp->rq_res.len;int write_len;@@ -341,8 +327,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt, rs_offset + chunk_off, xdr_off, this_write, - sge, - sge_count); + vec);if (ret) {dprintk ("svcrdma: RDMA_WRITE failed, ret=%d\n" , ret);@@ -380,7 +365,7 @@ static int send_reply(struct svcxprt_rdma *rdma,struct page *page, struct rpcrdma_msg *rdma_resp, struct svc_rdma_op_ctxt *ctxt, - int sge_count, + struct svc_rdma_req_map *vec, int byte_count) { struct ib_send_wr send_wr;@@ -413,10 +398,15 @@ static int send_reply(struct svcxprt_rdma *rdma, ctxt->sge[0 ].lkey = rdma->sc_phys_mr->lkey;/* Determine how many of our SGE are to be transmitted */ -for (sge_no =1 ; byte_count && sge_no < sge_count; sge_no++) { - sge_bytes =min ((size_t )ctxt->sge[sge_no].length, - (size_t )byte_count); +for (sge_no =1 ; byte_count && sge_no < vec->count; sge_no++) { + sge_bytes =min_t (size_t, vec->sge[sge_no].iov_len, byte_count); byte_count -= sge_bytes; + ctxt->sge[sge_no].addr = +ib_dma_map_single (rdma->sc_cm_id->device, + vec->sge[sge_no].iov_base, + sge_bytes,DMA_TO_DEVICE ); + ctxt->sge[sge_no].length = sge_bytes; + ctxt->sge[sge_no].lkey = rdma->sc_phys_mr->lkey; }BUG_ON (byte_count !=0 );@@ -428,8 +418,10 @@ static int send_reply(struct svcxprt_rdma *rdma, ctxt->pages[page_no+1 ] = rqstp->rq_respages[page_no]; ctxt->count++; rqstp->rq_respages[page_no] =NULL ; +/* If there are more pages than SGE, terminate SGE list */ +if (page_no+1 >= sge_no) + ctxt->sge[page_no+1 ].length =0 ; } -BUG_ON (sge_no > rdma->sc_max_sge);memset (&send_wr,0 ,sizeof send_wr); ctxt->wr_op =IB_WR_SEND ;@@ -473,20 +465,20 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)enum rpcrdma_proc reply_type;int ret;int inline_bytes; -struct ib_sge *sge; -int sge_count =0 ;struct page *res_page;struct svc_rdma_op_ctxt *ctxt; +struct svc_rdma_req_map *vec;dprintk ("svcrdma: sending response for rqstp=%p\n" , rqstp);/* Get the RDMA request header. */ rdma_argp =xdr_start (&rqstp->rq_arg); -/* Build an SGE for the XDR */ +/* Build an req vec for the XDR */ ctxt =svc_rdma_get_context (rdma); ctxt->direction =DMA_TO_DEVICE ; - sge =xdr_to_sge (rdma, &rqstp->rq_res, ctxt->sge, &sge_count); + vec =svc_rdma_get_req_map (); +xdr_to_sge (rdma, &rqstp->rq_res, vec); inline_bytes = rqstp->rq_res.len;@@ -503,7 +495,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)/* Send any write-chunk data and build resp write-list */ ret =send_write_chunks (rdma, rdma_argp, rdma_resp, - rqstp, sge, sge_count); + rqstp, vec);if (ret <0 ) {printk (KERN_ERR "svcrdma: failed to send write chunks, rc=%d\n" , ret);@@ -513,7 +505,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)/* Send any reply-list data and update resp reply-list */ ret =send_reply_chunks (rdma, rdma_argp, rdma_resp, - rqstp, sge, sge_count); + rqstp, vec);if (ret <0 ) {printk (KERN_ERR "svcrdma: failed to send reply chunks, rc=%d\n" , ret);@@ -521,11 +513,13 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) } inline_bytes -= ret; - ret =send_reply (rdma, rqstp, res_page, rdma_resp, ctxt, sge_count, + ret =send_reply (rdma, rqstp, res_page, rdma_resp, ctxt, vec, inline_bytes); +svc_rdma_put_req_map (vec);dprintk ("svcrdma: send_reply returns %d\n" , ret);return ret; error: +svc_rdma_put_req_map (vec);svc_rdma_put_context (ctxt,0 );put_page (res_page);return ret;net/sunrpc/xprtrdma/svc_rdma_transport.c +4
-1@@ -387,10 +387,13 @@ static void sq_cq_reap(struct svcxprt_rdma *xprt)switch (ctxt->wr_op) {case IB_WR_SEND : -case IB_WR_RDMA_WRITE :svc_rdma_put_context (ctxt,1 );break ; +case IB_WR_RDMA_WRITE : +svc_rdma_put_context (ctxt,0 ); +break ; +case IB_WR_RDMA_READ :if (test_bit (RDMACTXT_F_LAST_CTXT , &ctxt->flags)) {struct svc_rdma_op_ctxt *read_hdr = ctxt->read_hdr;
Re: [PATCH 10/11] svcrdma: Create a kmem cache for the WR contexts
In reply to a message not in the archiveQuoted 14 lines · Tom Tucker
Create a kmem cache to hold WR contexts. Next we will convert the WR context get and put services to use this kmem cache.
Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma.c | 24 ++++++++++++++++++++---- 1 files changed, 20 insertions(+), 4 deletions(-)net/sunrpc/xprtrdma/svc_rdma.c +0
-0
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 74
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-1@@ -69,8 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod; -/* Temporary NFS request map cache */ +/* Temporary NFS request map and context caches */ struct kmem_cache *svc_rdma_map_cachep =NULL ; +struct kmem_cache *svc_rdma_ctxt_cachep =NULL ;
No need to initialize this one either.
net/sunrpc/xprtrdma/svc_rdma.c void svc_rdma_cleanup(void) · line 252
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-0/* * This function implements reading and resetting an atomic_t stat @@ -247,6 +248,8 @@ void svc_rdma_cleanup(void)svc_unreg_xprt_class (&svc_rdma_class);if (svc_rdma_map_cachep)kmem_cache_destroy (svc_rdma_map_cachep); +if (svc_rdma_ctxt_cachep) +kmem_cache_destroy (svc_rdma_ctxt_cachep);
And the conditional is probably unnecessary again.
--b.
Quoted 34 lines
net/sunrpc/xprtrdma/svc_rdma.c +16
-3}int svc_rdma_init (void )@@ -269,14 +272,27 @@ int svc_rdma_init(void) NULL);if (!svc_rdma_map_cachep) {printk (KERN_INFO "Could not allocate map cache.\n" ); - goto err; + goto err0; + } + +/* Create the temporary context cache */ + svc_rdma_ctxt_cachep = +kmem_cache_create ("svc_rdma_ctxt_cache" , +sizeof (struct svc_rdma_op_ctxt ), +0 , +SLAB_HWCACHE_ALIGN , +NULL ); +if (!svc_rdma_ctxt_cachep) { +printk (KERN_INFO "Could not allocate WR ctxt cache.\n" ); + goto err1; }/* Register RDMA with the SVC transport switch */ svc_reg_xprt_class (&svc_rdma_class);return 0 ; - - err: + err1: +kmem_cache_destroy (svc_rdma_map_cachep); + err0:if (svcrdma_table_header) {unregister_sysctl_table (svcrdma_table_header); svcrdma_table_header =NULL ;
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to J. Bruce FieldsQuoted 29 lines · J. Bruce Fields
Quoted 27 lines · Tom Tucker
Use the new
svc_rdma_req_mapdata type for mapping the client side memory to the server side memory. Move the DMA mapping to the context pointed to by each WR individually so that it is unmapped after the WR completes.Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)net/sunrpc/xprtrdma/svc_rdma_sendto.c +3
-4@@ -64,10 +64,9 @@* SGE[sge_count-1] data from xdr->tail. * */ -static struct ib_sge *xdr_to_sge (struct svcxprt_rdma *xprt, -struct xdr_buf *xdr, -struct ib_sge *sge, -int *sge_count) +static void xdr_to_sge (struct svcxprt_rdma *xprt, +struct xdr_buf *xdr, +struct svc_rdma_req_map *vec) {
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 84
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0/* Max we need is the length of the XDR / pagesize + one for * head + one for tail + one for RPCRDMA header @@ -84,14 +83,10 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, sge_no =1 ;Should the arrays in the
svc_rdma_req_mapactually be size RPC_SVC_MAXPAGES+1 to allow for the RPCRDMA header?
Yes, I think this is a bug. I'm going to clamp max data in a different way in an upcomeing patch. So this will change in a subsequent patch.
Quoted 12 lines
Also, the only caller of
xdr_to_sge()appears to besvc_rdma_sendto(), which is called fromsvc_sendto()immediately after setting:xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len + xb->page_len + xb->tail[0].iov_len;So I think
xdr_to_sge()is doing a bunch of unnecessary work to handle the case wherexdr->lencould be less than that sum?
Ok... check me below please.
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 88
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-4/* Head SGE */ - sge[sge_no].addr =ib_dma_map_single (xprt->sc_cm_id->device, - xdr->head[0 ].iov_base, - xdr->head[0 ].iov_len, -DMA_TO_DEVICE ); + vec->sge[sge_no].iov_base = xdr->head[0 ].iov_base; sge_bytes =min_t (u32, byte_count, xdr->head[0 ].iov_len);
This doesn't need to be min_t. It could be xdr->head[0].iov_len.
Quoted 6 lines
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-2byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++;
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 100
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +2
-0/* pages SGE */ @@ -99,16 +94,13 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, page_bytes = xdr->page_len; page_off = xdr->page_base;while (byte_count && page_bytes) { + vec->sge[sge_no].iov_base = +page_address (xdr->pages[page_no]) + page_off; sge_bytes =min_t (u32, byte_count, (PAGE_SIZE -page_off));
This is still needed if the xdr terminates with a portion of a page and there is not tail.
Quoted 11 lines
Quoted 11 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-6- sge[sge_no].addr = -ib_dma_map_page (xprt->sc_cm_id->device, - xdr->pages[page_no], page_off, - sge_bytes,DMA_TO_DEVICE ); sge_bytes =min (sge_bytes, page_bytes); byte_count -= sge_bytes; page_bytes -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes;
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 112
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0sge_no++;net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0page_no++;@@ -117,23 +109,17 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,/* Tail SGE */ if (byte_count && xdr->tail[0 ].iov_len) {
This is defensive. It could just be byte_count.
If byte_count != xdr->tail[0].iov_len, the BUG_ON below will let us know.
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 115
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-5- sge[sge_no].addr = -ib_dma_map_single (xprt->sc_cm_id->device, - xdr->tail[0 ].iov_base, - xdr->tail[0 ].iov_len, -DMA_TO_DEVICE ); + vec->sge[sge_no].iov_base = xdr->tail[0 ].iov_base; sge_bytes =min_t (u32, byte_count, xdr->tail[0 ].iov_len);
The min_t isn't needed. It could just be byte_count.
Quoted 75 lines
Quoted 75 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +16
-20byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; }BUG_ON (sge_no > sge_max);BUG_ON (byte_count !=0 ); - *sge_count = sge_no; -return sge; + vec->count = sge_no; }@@ -143,9 +129,8 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,static int send_write (struct svcxprt_rdma *xprt,struct svc_rqst *rqstp,u32 rmr,u64 to,u32 xdr_off,int write_len, -struct ib_sge *xdr_sge,int sge_count) +struct svc_rdma_req_map *vec) { -struct svc_rdma_op_ctxt *tmp_sge_ctxt;struct ib_send_wr write_wr;struct ib_sge *sge;int xdr_sge_no;@@ -156,23 +141,22 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,struct svc_rdma_op_ctxt *ctxt;int ret =0 ; -BUG_ON (sge_count >RPCSVC_MAXPAGES ); +BUG_ON (vec->count >RPCSVC_MAXPAGES );dprintk ("svcrdma: RDMA_WRITE rmr=%x, to=%llx, xdr_off=%d, " -"write_len=%d, xdr_sge=%p, sge_count=%d\n" , +"write_len=%d, vec->sge=%p, vec->count=%lu\n" , rmr, (unsigned long long )to, xdr_off, - write_len, xdr_sge, sge_count); + write_len, vec->sge, vec->count); ctxt =svc_rdma_get_context (xprt); - ctxt->count =0 ; - tmp_sge_ctxt =svc_rdma_get_context (xprt); - sge = tmp_sge_ctxt->sge; + ctxt->direction =DMA_TO_DEVICE ; + sge = ctxt->sge;/* Find the SGE associated with xdr_off */ -for (bc = xdr_off, xdr_sge_no =1 ; bc && xdr_sge_no < sge_count; +for (bc = xdr_off, xdr_sge_no =1 ; bc && xdr_sge_no < vec->count; xdr_sge_no++) { -if (xdr_sge[xdr_sge_no].length > bc) +if (vec->sge[xdr_sge_no].iov_len > bc)break ; - bc -= xdr_sge[xdr_sge_no].length; + bc -= vec->sge[xdr_sge_no].iov_len; } sge_off = bc;@@ -180,21 +164,27 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, sge_no =0 ;/* Copy the remaining SGE */ -while (bc !=0 && xdr_sge_no < sge_count) { - sge[sge_no].addr = xdr_sge[xdr_sge_no].addr + sge_off; - sge[sge_no].lkey = xdr_sge[xdr_sge_no].lkey; +while (bc !=0 && xdr_sge_no < vec->count) { + sge[sge_no].lkey = xprt->sc_phys_mr->lkey; sge_bytes =min ((size_t )bc, - (size_t )(xdr_sge[xdr_sge_no].length-sge_off)); + (size_t )(vec->sge[xdr_sge_no].iov_len-sge_off)); sge[sge_no].length = sge_bytes; - + sge[sge_no].addr = + ib_dma_map_single(xprt->sc_cm_id->device,
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, · line 179
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +5
-0+ (void *) + vec->sge[xdr_sge_no].iov_base + sge_off, + sge_bytes, DMA_TO_DEVICE); +if (dma_mapping_error (sge[sge_no].addr)) +return -EINVAL ;We leak a
svc_rdma_op_ctxton error here.
Ok.
Quoted 192 lines
--b.
Quoted 174 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +2
-1sge_off =0 ; sge_no++; + ctxt->count++; xdr_sge_no++; bc -= sge_bytes; }BUG_ON (bc !=0 ); -BUG_ON (xdr_sge_no > sge_count); +BUG_ON (xdr_sge_no > vec->count);/* Prepare WRITE WR */ net/sunrpc/xprtrdma/svc_rdma_sendto.c +26
-22memset (&write_wr,0 ,sizeof write_wr);@@ -210,11 +200,10 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,/* Post It */ atomic_inc (&rdma_stat_write);if (svc_rdma_send (xprt, &write_wr)) { -svc_rdma_put_context (ctxt,1 ); +svc_rdma_put_context (ctxt,0 );/* Fatal error, close transport */ ret = -EIO ; } -svc_rdma_put_context (tmp_sge_ctxt,0 );return ret; }@@ -222,8 +211,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt,struct rpcrdma_msg *rdma_argp, struct rpcrdma_msg *rdma_resp, struct svc_rqst *rqstp, - struct ib_sge *sge, - int sge_count) + struct svc_rdma_req_map *vec) { u32 xfer_len = rqstp->rq_res.page_len + rqstp->rq_res.tail[0].iov_len;int write_len;@@ -269,8 +257,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt, rs_offset + chunk_off, xdr_off, this_write, - sge, - sge_count); + vec);if (ret) {dprintk ("svcrdma: RDMA_WRITE failed, ret=%d\n" , ret);@@ -292,8 +279,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt,struct rpcrdma_msg *rdma_argp, struct rpcrdma_msg *rdma_resp, struct svc_rqst *rqstp, - struct ib_sge *sge, - int sge_count) + struct svc_rdma_req_map *vec) { u32 xfer_len = rqstp->rq_res.len;int write_len;@@ -341,8 +327,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt, rs_offset + chunk_off, xdr_off, this_write, - sge, - sge_count); + vec);if (ret) {dprintk ("svcrdma: RDMA_WRITE failed, ret=%d\n" , ret);@@ -380,7 +365,7 @@ static int send_reply(struct svcxprt_rdma *rdma,struct page *page, struct rpcrdma_msg *rdma_resp, struct svc_rdma_op_ctxt *ctxt, - int sge_count, + struct svc_rdma_req_map *vec, int byte_count) { struct ib_send_wr send_wr;@@ -413,10 +398,15 @@ static int send_reply(struct svcxprt_rdma *rdma, ctxt->sge[0 ].lkey = rdma->sc_phys_mr->lkey;/* Determine how many of our SGE are to be transmitted */ -for (sge_no =1 ; byte_count && sge_no < sge_count; sge_no++) { - sge_bytes =min ((size_t )ctxt->sge[sge_no].length, - (size_t )byte_count); +for (sge_no =1 ; byte_count && sge_no < vec->count; sge_no++) { + sge_bytes =min_t (size_t, vec->sge[sge_no].iov_len, byte_count); byte_count -= sge_bytes; + ctxt->sge[sge_no].addr = +ib_dma_map_single (rdma->sc_cm_id->device, + vec->sge[sge_no].iov_base, + sge_bytes,DMA_TO_DEVICE ); + ctxt->sge[sge_no].length = sge_bytes; + ctxt->sge[sge_no].lkey = rdma->sc_phys_mr->lkey; }BUG_ON (byte_count !=0 );@@ -428,8 +418,10 @@ static int send_reply(struct svcxprt_rdma *rdma, ctxt->pages[page_no+1 ] = rqstp->rq_respages[page_no]; ctxt->count++; rqstp->rq_respages[page_no] =NULL ; +/* If there are more pages than SGE, terminate SGE list */ +if (page_no+1 >= sge_no) + ctxt->sge[page_no+1 ].length =0 ; } -BUG_ON (sge_no > rdma->sc_max_sge);memset (&send_wr,0 ,sizeof send_wr); ctxt->wr_op =IB_WR_SEND ;@@ -473,20 +465,20 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)enum rpcrdma_proc reply_type;int ret;int inline_bytes; -struct ib_sge *sge; -int sge_count =0 ;struct page *res_page;struct svc_rdma_op_ctxt *ctxt; +struct svc_rdma_req_map *vec;dprintk ("svcrdma: sending response for rqstp=%p\n" , rqstp);/* Get the RDMA request header. */ rdma_argp =xdr_start (&rqstp->rq_arg); -/* Build an SGE for the XDR */ +/* Build an req vec for the XDR */ ctxt =svc_rdma_get_context (rdma); ctxt->direction =DMA_TO_DEVICE ; - sge =xdr_to_sge (rdma, &rqstp->rq_res, ctxt->sge, &sge_count); + vec =svc_rdma_get_req_map (); +xdr_to_sge (rdma, &rqstp->rq_res, vec); inline_bytes = rqstp->rq_res.len;@@ -503,7 +495,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)/* Send any write-chunk data and build resp write-list */ ret =send_write_chunks (rdma, rdma_argp, rdma_resp, - rqstp, sge, sge_count); + rqstp, vec);if (ret <0 ) {printk (KERN_ERR "svcrdma: failed to send write chunks, rc=%d\n" , ret);@@ -513,7 +505,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)/* Send any reply-list data and update resp reply-list */ ret =send_reply_chunks (rdma, rdma_argp, rdma_resp, - rqstp, sge, sge_count); + rqstp, vec);if (ret <0 ) {printk (KERN_ERR "svcrdma: failed to send reply chunks, rc=%d\n" , ret);@@ -521,11 +513,13 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) } inline_bytes -= ret; - ret =send_reply (rdma, rqstp, res_page, rdma_resp, ctxt, sge_count, + ret =send_reply (rdma, rqstp, res_page, rdma_resp, ctxt, vec, inline_bytes); +svc_rdma_put_req_map (vec);dprintk ("svcrdma: send_reply returns %d\n" , ret);return ret; error: +svc_rdma_put_req_map (vec);svc_rdma_put_context (ctxt,0 );put_page (res_page);return ret;net/sunrpc/xprtrdma/svc_rdma_transport.c +0
-1@@ -387,10 +387,13 @@ static void sq_cq_reap(struct svcxprt_rdma *xprt)switch (ctxt->wr_op) {case IB_WR_SEND : -case IB_WR_RDMA_WRITE :net/sunrpc/xprtrdma/svc_rdma_transport.c static void sq_cq_reap(struct svcxprt_rdma *xprt) · line 399
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +4
-0svc_rdma_put_context (ctxt,1 );break ; +case IB_WR_RDMA_WRITE : +svc_rdma_put_context (ctxt,0 ); +break ; +case IB_WR_RDMA_READ :if (test_bit (RDMACTXT_F_LAST_CTXT , &ctxt->flags)) {struct svc_rdma_op_ctxt *read_hdr = ctxt->read_hdr;--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
In reply to J. Bruce FieldsQuoted 66 lines · J. Bruce Fields
Quoted 64 lines · Tom Tucker
Create a new data structure to hold the remote client address space to local server address space mapping.
Signed-off-by Tom Tucker <[email protected]>--- include/linux/sunrpc/svc_rdma.h | 27 +++++++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma.c | 23 +++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 26 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 0 deletions(-)include/linux/sunrpc/svc_rdma.h +27
-0@@ -86,6 +86,31 @@ struct svc_rdma_op_ctxt {struct page *pages[RPCSVC_MAXPAGES ]; }; +/* +* NFS_ requests are mapped on the client side by the chunk lists in +* the RPCRDMA header. During the fetching of the RPC from the client +* and the writing of the reply to the client, the memory in the +* client and the memory in the server must be mapped as contiguous +* vaddr/len for access by the hardware. These data strucures keep +* these mappings. +* +* For an RDMA_WRITE, the 'sge' maps the RPC REPLY. For RDMA_READ, the +* 'sge' in the svc_rdma_req_map maps the server side RPC reply and the +* 'ch' field maps the read-list of the RPCRDMA header to the 'sge' +* mapping of the reply. +*/ +struct svc_rdma_chunk_sge { +int start;/* sge no for this chunk */ +int count;/* sge count for this chunk */ +}; +struct svc_rdma_req_map { +unsigned long count; +union { +struct kvec sge[RPCSVC_MAXPAGES ]; +struct svc_rdma_chunk_sge ch[RPCSVC_MAXPAGES ]; + }; +}; +#define RDMACTXT_F_LAST_CTXT 2struct svcxprt_rdma {@@ -173,6 +198,8 @@ extern int svc_rdma_post_recv(struct svcxprt_rdma *);extern int svc_rdma_create_listen (struct svc_serv *,int ,struct sockaddr *);extern struct svc_rdma_op_ctxt *svc_rdma_get_context (struct svcxprt_rdma *);extern void svc_rdma_put_context (struct svc_rdma_op_ctxt *,int ); +extern struct svc_rdma_req_map *svc_rdma_get_req_map (void ); +extern void svc_rdma_put_req_map (struct svc_rdma_req_map *);extern void svc_sq_reap (struct svcxprt_rdma *);extern void svc_rq_reap (struct svcxprt_rdma *);extern struct svc_xprt_class svc_rdma_class;net/sunrpc/xprtrdma/svc_rdma.c +0
-0@@ -69,6 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod;
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 74
Quoted 3 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-0+/* Temporary NFS request map cache */ +struct kmem_cache *svc_rdma_map_cachep =NULL ;No need to initialize globals to NULL.
I thought only static objects were initialized per the C standard. Or are you saying that this particular global doesn't need to be initialized because of the way it is used?
Quoted 7 lines
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma.c +1
-0+/* * This function implements reading and resetting an atomic_t stat * variable through read/write to a proc file. Any write to the file
net/sunrpc/xprtrdma/svc_rdma.c +0
-0@@ -241,6 +244,8 @@ void svc_rdma_cleanup(void) svcrdma_table_header =NULL ; }
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma.c void svc_rdma_cleanup(void) · line 249
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-0svc_unreg_xprt_class (&svc_rdma_class); +if (svc_rdma_map_cachep) +kmem_cache_destroy (svc_rdma_map_cachep);By design this can't be NULL (and the code presumably would have oopsed much earlier if it were mistakenly allowed to be) so I'm inclined not to bother checking....
Ok.
Quoted 21 lines
Quoted 21 lines
net/sunrpc/xprtrdma/svc_rdma.c +0
-0}int
svc_rdma_init(void)net/sunrpc/xprtrdma/svc_rdma.c +11
-0@@ -255,9 +260,27 @@ int svc_rdma_init(void) svcrdma_table_header =register_sysctl_table (svcrdma_root_table); +/* Create the temporary map cache */ + svc_rdma_map_cachep =kmem_cache_create ("svc_rdma_map_cache" , +sizeof (struct svc_rdma_req_map ), +0 , +SLAB_HWCACHE_ALIGN , +NULL ); +if (!svc_rdma_map_cachep) { +printk (KERN_INFO "Could not allocate map cache.\n" ); + goto err; + } +/* Register RDMA with the SVC transport switch */ svc_reg_xprt_class (&svc_rdma_class);return 0 ;
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma.c int svc_rdma_init(void) · line 281
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma.c +4
-0+ + err: +if (svcrdma_table_header) { +unregister_sysctl_table (svcrdma_table_header);
unregister_sysctl_table()already handles the NULL case, so you could skip the if (svcrdma_table_header).
Ok.
Quoted 47 lines
--b.
Quoted 44 lines
net/sunrpc/xprtrdma/svc_rdma.c +3
-0+ svcrdma_table_header =NULL ; + } +return -ENOMEM ; }MODULE_AUTHOR ("Tom Tucker <[email protected]>" );
MODULE_DESCRIPTION("SVC RDMA Transport");net/sunrpc/xprtrdma/svc_rdma_transport.c +26
-0@@ -173,6 +173,32 @@ void svc_rdma_put_context(struct svc_rdma_op_ctxt *ctxt, int free_pages)atomic_dec (&xprt->sc_ctxt_used); } +/* Temporary NFS request map cache. Created in svc_rdma.c */ +extern struct kmem_cache *svc_rdma_map_cachep; + +/* +* Temporary NFS req mappings are shared across all transport +* instances. These are short lived and should be bounded by the number +* of concurrent server threads * depth of the SQ. +*/ +struct svc_rdma_req_map *svc_rdma_get_req_map (void ) +{ +struct svc_rdma_req_map *map; +while (1 ) { + map =kmem_cache_alloc (svc_rdma_map_cachep,GFP_KERNEL ); +if (map) +break ; +schedule_timeout_uninterruptible (msecs_to_jiffies (500 )); + } + map->count =0 ; +return map; +} + +void svc_rdma_put_req_map (struct svc_rdma_req_map *map) +{ +kmem_cache_free (svc_rdma_map_cachep, map); +} +/* ib_cq event handler */ static void cq_event_handler (struct ib_event *event,void *context) {
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to J. Bruce FieldsQuoted 29 lines · J. Bruce Fields
Quoted 27 lines · Tom Tucker
Use the new
svc_rdma_req_mapdata type for mapping the client side memory to the server side memory. Move the DMA mapping to the context pointed to by each WR individually so that it is unmapped after the WR completes.Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)net/sunrpc/xprtrdma/svc_rdma_sendto.c +3
-4@@ -64,10 +64,9 @@* SGE[sge_count-1] data from xdr->tail. * */ -static struct ib_sge *xdr_to_sge (struct svcxprt_rdma *xprt, -struct xdr_buf *xdr, -struct ib_sge *sge, -int *sge_count) +static void xdr_to_sge (struct svcxprt_rdma *xprt, +struct xdr_buf *xdr, +struct svc_rdma_req_map *vec) {
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 84
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0/* Max we need is the length of the XDR / pagesize + one for * head + one for tail + one for RPCRDMA header @@ -84,14 +83,10 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, sge_no =1 ;Should the arrays in the
svc_rdma_req_mapactually be size RPC_SVC_MAXPAGES+1 to allow for the RPCRDMA header?
Actually, upon further inspection, this is not a bug. The RPCSVC_MAXPAGES is used to size the rqstp page array. This array includes a page for the request hdr, reply hdr and one extra page for alignment. For our use case, the reply hdr page covers the RDMA header.
However, I think this payload length should be clamped in a different way. It seems silly to allocate these things to handle big-data mounts that are rarely used in practice. I think it's better to have a transport max that is more reasonable by default and a module parameter that is configurable if you're looking to do big-data mounts.
Thoughts?
Quoted 334 lines
Also, the only caller of
xdr_to_sge()appears to besvc_rdma_sendto(), which is called fromsvc_sendto()immediately after setting:xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len + xb->page_len + xb->tail[0].iov_len;So I think
xdr_to_sge()is doing a bunch of unnecessary work to handle the case wherexdr->lencould be less than that sum?Quoted 117 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +20
-36/* Head SGE */ - sge[sge_no].addr =ib_dma_map_single (xprt->sc_cm_id->device, - xdr->head[0 ].iov_base, - xdr->head[0 ].iov_len, -DMA_TO_DEVICE ); + vec->sge[sge_no].iov_base = xdr->head[0 ].iov_base; sge_bytes =min_t (u32, byte_count, xdr->head[0 ].iov_len); byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++;/* pages SGE */ @@ -99,16 +94,13 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, page_bytes = xdr->page_len; page_off = xdr->page_base;while (byte_count && page_bytes) { + vec->sge[sge_no].iov_base = +page_address (xdr->pages[page_no]) + page_off; sge_bytes =min_t (u32, byte_count, (PAGE_SIZE -page_off)); - sge[sge_no].addr = -ib_dma_map_page (xprt->sc_cm_id->device, - xdr->pages[page_no], page_off, - sge_bytes,DMA_TO_DEVICE ); sge_bytes =min (sge_bytes, page_bytes); byte_count -= sge_bytes; page_bytes -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; page_no++;@@ -117,23 +109,17 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,/* Tail SGE */ if (byte_count && xdr->tail[0 ].iov_len) { - sge[sge_no].addr = -ib_dma_map_single (xprt->sc_cm_id->device, - xdr->tail[0 ].iov_base, - xdr->tail[0 ].iov_len, -DMA_TO_DEVICE ); + vec->sge[sge_no].iov_base = xdr->tail[0 ].iov_base; sge_bytes =min_t (u32, byte_count, xdr->tail[0 ].iov_len); byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; }BUG_ON (sge_no > sge_max);BUG_ON (byte_count !=0 ); - *sge_count = sge_no; -return sge; + vec->count = sge_no; }@@ -143,9 +129,8 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt,static int send_write (struct svcxprt_rdma *xprt,struct svc_rqst *rqstp,u32 rmr,u64 to,u32 xdr_off,int write_len, -struct ib_sge *xdr_sge,int sge_count) +struct svc_rdma_req_map *vec) { -struct svc_rdma_op_ctxt *tmp_sge_ctxt;struct ib_send_wr write_wr;struct ib_sge *sge;int xdr_sge_no;@@ -156,23 +141,22 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,struct svc_rdma_op_ctxt *ctxt;int ret =0 ; -BUG_ON (sge_count >RPCSVC_MAXPAGES ); +BUG_ON (vec->count >RPCSVC_MAXPAGES );dprintk ("svcrdma: RDMA_WRITE rmr=%x, to=%llx, xdr_off=%d, " -"write_len=%d, xdr_sge=%p, sge_count=%d\n" , +"write_len=%d, vec->sge=%p, vec->count=%lu\n" , rmr, (unsigned long long )to, xdr_off, - write_len, xdr_sge, sge_count); + write_len, vec->sge, vec->count); ctxt =svc_rdma_get_context (xprt); - ctxt->count =0 ; - tmp_sge_ctxt =svc_rdma_get_context (xprt); - sge = tmp_sge_ctxt->sge; + ctxt->direction =DMA_TO_DEVICE ; + sge = ctxt->sge;/* Find the SGE associated with xdr_off */ -for (bc = xdr_off, xdr_sge_no =1 ; bc && xdr_sge_no < sge_count; +for (bc = xdr_off, xdr_sge_no =1 ; bc && xdr_sge_no < vec->count; xdr_sge_no++) { -if (xdr_sge[xdr_sge_no].length > bc) +if (vec->sge[xdr_sge_no].iov_len > bc)break ; - bc -= xdr_sge[xdr_sge_no].length; + bc -= vec->sge[xdr_sge_no].iov_len; } sge_off = bc;@@ -180,21 +164,27 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, sge_no =0 ;/* Copy the remaining SGE */ -while (bc !=0 && xdr_sge_no < sge_count) { - sge[sge_no].addr = xdr_sge[xdr_sge_no].addr + sge_off; - sge[sge_no].lkey = xdr_sge[xdr_sge_no].lkey; +while (bc !=0 && xdr_sge_no < vec->count) { + sge[sge_no].lkey = xprt->sc_phys_mr->lkey; sge_bytes =min ((size_t )bc, - (size_t )(xdr_sge[xdr_sge_no].length-sge_off)); + (size_t )(vec->sge[xdr_sge_no].iov_len-sge_off));net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp, · line 179
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +7
-1sge[sge_no].length = sge_bytes; - + sge[sge_no].addr = +ib_dma_map_single (xprt->sc_cm_id->device, + (void *) + vec->sge[xdr_sge_no].iov_base + sge_off, + sge_bytes,DMA_TO_DEVICE ); +if (dma_mapping_error (sge[sge_no].addr)) +return -EINVAL ;We leak a
svc_rdma_op_ctxton error here.--b.
Quoted 174 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +2
-1sge_off =0 ; sge_no++; + ctxt->count++; xdr_sge_no++; bc -= sge_bytes; }BUG_ON (bc !=0 ); -BUG_ON (xdr_sge_no > sge_count); +BUG_ON (xdr_sge_no > vec->count);/* Prepare WRITE WR */ net/sunrpc/xprtrdma/svc_rdma_sendto.c +26
-22memset (&write_wr,0 ,sizeof write_wr);@@ -210,11 +200,10 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,/* Post It */ atomic_inc (&rdma_stat_write);if (svc_rdma_send (xprt, &write_wr)) { -svc_rdma_put_context (ctxt,1 ); +svc_rdma_put_context (ctxt,0 );/* Fatal error, close transport */ ret = -EIO ; } -svc_rdma_put_context (tmp_sge_ctxt,0 );return ret; }@@ -222,8 +211,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt,struct rpcrdma_msg *rdma_argp, struct rpcrdma_msg *rdma_resp, struct svc_rqst *rqstp, - struct ib_sge *sge, - int sge_count) + struct svc_rdma_req_map *vec) { u32 xfer_len = rqstp->rq_res.page_len + rqstp->rq_res.tail[0].iov_len;int write_len;@@ -269,8 +257,7 @@ static int send_write_chunks(struct svcxprt_rdma *xprt, rs_offset + chunk_off, xdr_off, this_write, - sge, - sge_count); + vec);if (ret) {dprintk ("svcrdma: RDMA_WRITE failed, ret=%d\n" , ret);@@ -292,8 +279,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt,struct rpcrdma_msg *rdma_argp, struct rpcrdma_msg *rdma_resp, struct svc_rqst *rqstp, - struct ib_sge *sge, - int sge_count) + struct svc_rdma_req_map *vec) { u32 xfer_len = rqstp->rq_res.len;int write_len;@@ -341,8 +327,7 @@ static int send_reply_chunks(struct svcxprt_rdma *xprt, rs_offset + chunk_off, xdr_off, this_write, - sge, - sge_count); + vec);if (ret) {dprintk ("svcrdma: RDMA_WRITE failed, ret=%d\n" , ret);@@ -380,7 +365,7 @@ static int send_reply(struct svcxprt_rdma *rdma,struct page *page, struct rpcrdma_msg *rdma_resp, struct svc_rdma_op_ctxt *ctxt, - int sge_count, + struct svc_rdma_req_map *vec, int byte_count) { struct ib_send_wr send_wr;@@ -413,10 +398,15 @@ static int send_reply(struct svcxprt_rdma *rdma, ctxt->sge[0 ].lkey = rdma->sc_phys_mr->lkey;/* Determine how many of our SGE are to be transmitted */ -for (sge_no =1 ; byte_count && sge_no < sge_count; sge_no++) { - sge_bytes =min ((size_t )ctxt->sge[sge_no].length, - (size_t )byte_count); +for (sge_no =1 ; byte_count && sge_no < vec->count; sge_no++) { + sge_bytes =min_t (size_t, vec->sge[sge_no].iov_len, byte_count); byte_count -= sge_bytes; + ctxt->sge[sge_no].addr = +ib_dma_map_single (rdma->sc_cm_id->device, + vec->sge[sge_no].iov_base, + sge_bytes,DMA_TO_DEVICE ); + ctxt->sge[sge_no].length = sge_bytes; + ctxt->sge[sge_no].lkey = rdma->sc_phys_mr->lkey; }BUG_ON (byte_count !=0 );@@ -428,8 +418,10 @@ static int send_reply(struct svcxprt_rdma *rdma, ctxt->pages[page_no+1 ] = rqstp->rq_respages[page_no]; ctxt->count++; rqstp->rq_respages[page_no] =NULL ; +/* If there are more pages than SGE, terminate SGE list */ +if (page_no+1 >= sge_no) + ctxt->sge[page_no+1 ].length =0 ; } -BUG_ON (sge_no > rdma->sc_max_sge);memset (&send_wr,0 ,sizeof send_wr); ctxt->wr_op =IB_WR_SEND ;@@ -473,20 +465,20 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)enum rpcrdma_proc reply_type;int ret;int inline_bytes; -struct ib_sge *sge; -int sge_count =0 ;struct page *res_page;struct svc_rdma_op_ctxt *ctxt; +struct svc_rdma_req_map *vec;dprintk ("svcrdma: sending response for rqstp=%p\n" , rqstp);/* Get the RDMA request header. */ rdma_argp =xdr_start (&rqstp->rq_arg); -/* Build an SGE for the XDR */ +/* Build an req vec for the XDR */ ctxt =svc_rdma_get_context (rdma); ctxt->direction =DMA_TO_DEVICE ; - sge =xdr_to_sge (rdma, &rqstp->rq_res, ctxt->sge, &sge_count); + vec =svc_rdma_get_req_map (); +xdr_to_sge (rdma, &rqstp->rq_res, vec); inline_bytes = rqstp->rq_res.len;@@ -503,7 +495,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)/* Send any write-chunk data and build resp write-list */ ret =send_write_chunks (rdma, rdma_argp, rdma_resp, - rqstp, sge, sge_count); + rqstp, vec);if (ret <0 ) {printk (KERN_ERR "svcrdma: failed to send write chunks, rc=%d\n" , ret);@@ -513,7 +505,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)/* Send any reply-list data and update resp reply-list */ ret =send_reply_chunks (rdma, rdma_argp, rdma_resp, - rqstp, sge, sge_count); + rqstp, vec);if (ret <0 ) {printk (KERN_ERR "svcrdma: failed to send reply chunks, rc=%d\n" , ret);@@ -521,11 +513,13 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) } inline_bytes -= ret; - ret =send_reply (rdma, rqstp, res_page, rdma_resp, ctxt, sge_count, + ret =send_reply (rdma, rqstp, res_page, rdma_resp, ctxt, vec, inline_bytes); +svc_rdma_put_req_map (vec);dprintk ("svcrdma: send_reply returns %d\n" , ret);return ret; error: +svc_rdma_put_req_map (vec);svc_rdma_put_context (ctxt,0 );put_page (res_page);return ret;net/sunrpc/xprtrdma/svc_rdma_transport.c +0
-1@@ -387,10 +387,13 @@ static void sq_cq_reap(struct svcxprt_rdma *xprt)switch (ctxt->wr_op) {case IB_WR_SEND : -case IB_WR_RDMA_WRITE :net/sunrpc/xprtrdma/svc_rdma_transport.c static void sq_cq_reap(struct svcxprt_rdma *xprt) · line 399
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +4
-0svc_rdma_put_context (ctxt,1 );break ; +case IB_WR_RDMA_WRITE : +svc_rdma_put_context (ctxt,0 ); +break ; +case IB_WR_RDMA_READ :if (test_bit (RDMACTXT_F_LAST_CTXT , &ctxt->flags)) {struct svc_rdma_op_ctxt *read_hdr = ctxt->read_hdr;--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] svcrdma: Create a kmem cache for the WR contexts
In reply to J. Bruce FieldsQuoted 19 lines · J. Bruce Fields
Quoted 17 lines · Tom Tucker
Create a kmem cache to hold WR contexts. Next we will convert the WR context get and put services to use this kmem cache.
Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma.c | 24 ++++++++++++++++++++---- 1 files changed, 20 insertions(+), 4 deletions(-)net/sunrpc/xprtrdma/svc_rdma.c +0
-0@@ -69,8 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod;
Quoted 9 lines
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 75
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-1-/* Temporary NFS request map cache */ +/* Temporary NFS request map and context caches */ struct kmem_cache *svc_rdma_map_cachep =NULL ; +struct kmem_cache *svc_rdma_ctxt_cachep =NULL ;No need to initialize this one either.
Given the way it's used, agreed.
Quoted 14 lines
net/sunrpc/xprtrdma/svc_rdma.c void svc_rdma_cleanup(void) · line 253
Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma.c +2
-0/* * This function implements reading and resetting an atomic_t stat @@ -247,6 +248,8 @@ void svc_rdma_cleanup(void)svc_unreg_xprt_class (&svc_rdma_class);if (svc_rdma_map_cachep)kmem_cache_destroy (svc_rdma_map_cachep); +if (svc_rdma_ctxt_cachep) +kmem_cache_destroy (svc_rdma_ctxt_cachep);And the conditional is probably unnecessary again.
ok.
Quoted 38 lines
--b.
Quoted 35 lines
net/sunrpc/xprtrdma/svc_rdma.c +0
-0}int
svc_rdma_init(void)net/sunrpc/xprtrdma/svc_rdma.c +16
-3@@ -269,14 +272,27 @@ int svc_rdma_init(void) NULL);if (!svc_rdma_map_cachep) {printk (KERN_INFO "Could not allocate map cache.\n" ); - goto err; + goto err0; + } + +/* Create the temporary context cache */ + svc_rdma_ctxt_cachep = +kmem_cache_create ("svc_rdma_ctxt_cache" , +sizeof (struct svc_rdma_op_ctxt ), +0 , +SLAB_HWCACHE_ALIGN , +NULL ); +if (!svc_rdma_ctxt_cachep) { +printk (KERN_INFO "Could not allocate WR ctxt cache.\n" ); + goto err1; }/* Register RDMA with the SVC transport switch */ svc_reg_xprt_class (&svc_rdma_class);return 0 ; - - err: + err1: +kmem_cache_destroy (svc_rdma_map_cachep); + err0:if (svcrdma_table_header) {unregister_sysctl_table (svcrdma_table_header); svcrdma_table_header =NULL ;
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to Tom TuckerQuoted 13 lines · Tom Tucker
Quoted 11 lines · J. Bruce Fields
Also, the only caller of
xdr_to_sge()appears to besvc_rdma_sendto(), which is called fromsvc_sendto()immediately after setting:xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len + xb->page_len + xb->tail[0].iov_len;So I think
xdr_to_sge()is doing a bunch of unnecessary work to handle the case wherexdr->lencould be less than that sum?Ok... check me below please.
Quoted 10 lines
Quoted 8 lines
Review
Quoted 8 lines
/* Head SGE */
(quoted diff) +1
-4- sge[sge_no].addr = ib_dma_map_single(xprt->sc_cm_id->device, - xdr->head[0].iov_base, - xdr->head[0].iov_len, - DMA_TO_DEVICE); + vec->sge[sge_no].iov_base = xdr->head[0].iov_base; sge_bytes = min_t(u32, byte_count, xdr->head[0].iov_len);This doesn't need to be
min_t. It could be xdr->head[0].iov_len.
Yes. The variable byte_count shouldn't be needed at all, so I'd start by eliminating all references to byte_count.
Quoted 17 lines
Quoted 15 lines
Quoted 8 lines
(quoted diff) +1
-2byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; /* pages SGE */Review static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 100
Quoted 7 lines
(quoted diff) +2
-0@@ -99,16 +94,13 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, page_bytes = xdr->page_len; page_off = xdr->page_base; while (byte_count && page_bytes) { + vec->sge[sge_no].iov_base = + page_address(xdr->pages[page_no]) + page_off; sge_bytes = min_t(u32, byte_count, (PAGE_SIZE-page_off));This is still needed if the xdr terminates with a portion of a page and there is not tail.
Yes, but the condition could just be (page_bytes).
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
(quoted diff) +0
-6- sge[sge_no].addr = - ib_dma_map_page(xprt->sc_cm_id->device, - xdr->pages[page_no], page_off, - sge_bytes, DMA_TO_DEVICE); sge_bytes = min(sge_bytes, page_bytes); byte_count -= sge_bytes; page_bytes -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey;
Quoted 8 lines
Quoted 7 lines
Review static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 111
Quoted 7 lines
(quoted diff) +1
-0+ vec->sge[sge_no].iov_len = sge_bytes; sge_no++; page_no++;@@ -117,23 +109,17 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, /* Tail SGE */ if (byte_count && xdr->tail[0].iov_len) {This is defensive. It could just be
byte_count.
I agree, though I'd just make it (xdr->tail[0].iov_len) instead of just (byte_count).
Quoted 10 lines
If
byte_count!= xdr->tail[0].iov_len, theBUG_ONbelow will let us know.Quoted 8 lines
Review static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 114
Quoted 8 lines
(quoted diff) +1
-5- sge[sge_no].addr = - ib_dma_map_single(xprt->sc_cm_id->device, - xdr->tail[0].iov_base, - xdr->tail[0].iov_len, - DMA_TO_DEVICE); + vec->sge[sge_no].iov_base = xdr->tail[0].iov_base; sge_bytes = min_t(u32, byte_count, xdr->tail[0].iov_len);The
min_tisn't needed. It could just bebyte_count.
Right, but could also just be tail length, as above.
--b.
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
In reply to Tom TuckerQuoted 71 lines · Tom Tucker
Quoted 60 lines · J. Bruce Fields
Quoted 58 lines · Tom Tucker
Create a new data structure to hold the remote client address space to local server address space mapping.
Signed-off-by Tom Tucker <[email protected]>--- include/linux/sunrpc/svc_rdma.h | 27 +++++++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma.c | 23 +++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 26 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 0 deletions(-)include/linux/sunrpc/svc_rdma.h +26
-0@@ -86,6 +86,31 @@ struct svc_rdma_op_ctxt {struct page *pages[RPCSVC_MAXPAGES ]; }; +/* +* NFS_ requests are mapped on the client side by the chunk lists in +* the RPCRDMA header. During the fetching of the RPC from the client +* and the writing of the reply to the client, the memory in the +* client and the memory in the server must be mapped as contiguous +* vaddr/len for access by the hardware. These data strucures keep +* these mappings. +* +* For an RDMA_WRITE, the 'sge' maps the RPC REPLY. For RDMA_READ, the +* 'sge' in the svc_rdma_req_map maps the server side RPC reply and the +* 'ch' field maps the read-list of the RPCRDMA header to the 'sge' +* mapping of the reply. +*/ +struct svc_rdma_chunk_sge { + int start; /* sge no for this chunk */ +int count;/* sge count for this chunk */ +}; +struct svc_rdma_req_map { +unsigned long count; +union { +struct kvec sge[RPCSVC_MAXPAGES ]; +struct svc_rdma_chunk_sge ch[RPCSVC_MAXPAGES ]; + }; +}; +#define RDMACTXT_F_LAST_CTXT 2 struct svcxprt_rdma {@@ -173,6 +198,8 @@ extern int svc_rdma_post_recv(struct svcxprt_rdma *);extern int svc_rdma_create_listen (struct svc_serv *,int ,struct sockaddr *);extern struct svc_rdma_op_ctxt *svc_rdma_get_context (struct svcxprt_rdma *);extern void svc_rdma_put_context (struct svc_rdma_op_ctxt *,int ); +extern struct svc_rdma_req_map *svc_rdma_get_req_map (void ); +extern void svc_rdma_put_req_map (struct svc_rdma_req_map *);extern void svc_sq_reap (struct svcxprt_rdma *);extern void svc_rq_reap (struct svcxprt_rdma *);extern struct svc_xprt_class svc_rdma_class;net/sunrpc/xprtrdma/svc_rdma.c +0
-0Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 73
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma.c +1
-0@@ -69,6 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod; +/* Temporary NFS request map cache */ +struct kmem_cache *svc_rdma_map_cachep = NULL;No need to initialize globals to NULL.
Quoted 3 lines
I thought only static objects were initialized per the C standard. Or are you saying that this particular
global doesn't need to be initialized because of the way it is used?
I don't know if the initialization is mandated by the standards or whether it's just gcc behavior, but I know I get a complaint every time somebody finds one of those....
That may partly just be a preference for conciseness, but I think it may also allow gcc to put the thing in a different section and save some space in the on-disk kernel--I don't know.
--b.
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to Tom TuckerQuoted 40 lines · Tom Tucker
Quoted 29 lines · J. Bruce Fields
Quoted 27 lines · Tom Tucker
Use the new
svc_rdma_req_mapdata type for mapping the client side memory to the server side memory. Move the DMA mapping to the context pointed to by each WR individually so that it is unmapped after the WR completes.Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)net/sunrpc/xprtrdma/svc_rdma_sendto.c +3
-4@@ -64,10 +64,9 @@* SGE[sge_count-1] data from xdr->tail. * */ -static struct ib_sge *xdr_to_sge (struct svcxprt_rdma *xprt, -struct xdr_buf *xdr, -struct ib_sge *sge, -int *sge_count) +static void xdr_to_sge (struct svcxprt_rdma *xprt, +struct xdr_buf *xdr, +struct svc_rdma_req_map *vec) {Quoted 10 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 84
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0/* Max we need is the length of the XDR / pagesize + one for * head + one for tail + one for RPCRDMA header @@ -84,14 +83,10 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, sge_no =1 ;Should the arrays in the
svc_rdma_req_mapactually be size RPC_SVC_MAXPAGES+1 to allow for the RPCRDMA header?
Quoted 5 lines
Actually, upon further inspection, this is not a bug. The
RPCSVC_MAXPAGESis used to size the rqstp
page array. This array includes a page for the request hdr, reply hdr and one extra page for alignment. For
our use case, the reply hdr page covers the RDMA header.
OK, that makes sense. (Let's just fix the comment above so I don't forget and ask the same question again next time.)
Quoted 9 lines
However, I think this payload length should be clamped in a different way. It seems silly to allocate these
things to handle big-data mounts that are rarely used in practice. I think it's better to have a transport max
that is more reasonable by default and a module parameter that is configurable if you're looking to do big-data
mounts.Thoughts?
This is decided in fs/nfsd/nfssvc.c:nfsd_create_serv(), based on the machine's ram size, and can be overridden with the nfsd filesystem's maxblksize parameter.
I'm open to suggestions, but my feeling is that it's hard to educate people about how to use additional sysctl's, and that it's reasonable to waste some memory to get good out-of-the-box performance on streaming IO.
See also recent patches from Olga and Dean for similar discussion (about WAN tcp performance in their case, since the size of these buffers affects the size of the tcp window we advertise).
--b.
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to J. Bruce FieldsQuoted 52 lines · J. Bruce Fields
Quoted 16 lines · Tom Tucker
Quoted 12 lines · J. Bruce Fields
Also, the only caller of
xdr_to_sge()appears to besvc_rdma_sendto(), which is called fromsvc_sendto()immediately after setting:xb = &rqstp->rq_res; xb->len = xb->head[0].iov_len + xb->page_len + xb->tail[0].iov_len;So I think
xdr_to_sge()is doing a bunch of unnecessary work to handle the case wherexdr->lencould be less than that sum?Ok... check me below please.
Quoted 11 lines
Quoted 9 lines
Review
Quoted 9 lines
/* Head SGE */
(quoted diff) +1
-4- sge[sge_no].addr = ib_dma_map_single(xprt->sc_cm_id->device, - xdr->head[0].iov_base, - xdr->head[0].iov_len, - DMA_TO_DEVICE); + vec->sge[sge_no].iov_base = xdr->head[0].iov_base; sge_bytes = min_t(u32, byte_count, xdr->head[0].iov_len);This doesn't need to be
min_t. It could be xdr->head[0].iov_len.Yes. The variable
byte_countshouldn't be needed at all, so I'd start by eliminating all references tobyte_count.Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +1
-2byte_count -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes; sge_no++; /* pages SGE */@@ -99,16 +94,13 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, page_bytes = xdr->page_len;Quoted 9 lines
Quoted 7 lines
Review static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 101
Quoted 7 lines
(quoted diff) +2
-0page_off = xdr->page_base; while (byte_count && page_bytes) { + vec->sge[sge_no].iov_base = + page_address(xdr->pages[page_no]) + page_off; sge_bytes = min_t(u32, byte_count, (PAGE_SIZE-page_off));This is still needed if the xdr terminates with a portion of a page and there is not tail.
Quoted 3 lines
Yes, but the condition could just be (
page_bytes).
Right.
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
(quoted diff) +1
-6- sge[sge_no].addr = - ib_dma_map_page(xprt->sc_cm_id->device, - xdr->pages[page_no], page_off, - sge_bytes, DMA_TO_DEVICE); sge_bytes = min(sge_bytes, page_bytes); byte_count -= sge_bytes; page_bytes -= sge_bytes; - sge[sge_no].length = sge_bytes; - sge[sge_no].lkey = xprt->sc_phys_mr->lkey; + vec->sge[sge_no].iov_len = sge_bytes;Quoted 9 lines
Quoted 7 lines
Review static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 112
Quoted 7 lines
(quoted diff) +0
-0sge_no++;(quoted diff) +0
-0page_no++;@@ -117,23 +109,17 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, /* Tail SGE */ if (byte_count && xdr->tail[0].iov_len) {This is defensive. It could just be
byte_count.
Quoted 4 lines
I agree, though I'd just make it (xdr->tail[0].iov_len) instead of just (
byte_count).
Agreed.
Quoted 14 lines
Quoted 13 lines
If
byte_count!= xdr->tail[0].iov_len, theBUG_ONbelow will let us know.Quoted 9 lines
Review static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 116
Quoted 9 lines
(quoted diff) +1
-5- sge[sge_no].addr = - ib_dma_map_single(xprt->sc_cm_id->device, - xdr->tail[0].iov_base, - xdr->tail[0].iov_len, - DMA_TO_DEVICE); + vec->sge[sge_no].iov_base = xdr->tail[0].iov_base; sge_bytes = min_t(u32, byte_count, xdr->tail[0].iov_len);The
min_tisn't needed. It could just bebyte_count.
Quoted 3 lines
Right, but could also just be tail length, as above.
Ok, page # confirmed...
Quoted 6 lines
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
In reply to J. Bruce FieldsQuoted 76 lines · J. Bruce Fields
Quoted 74 lines · Tom Tucker
Quoted 72 lines · J. Bruce Fields
Quoted 59 lines · Tom Tucker
Create a new data structure to hold the remote client address space to local server address space mapping.
Signed-off-by Tom Tucker <[email protected]>--- include/linux/sunrpc/svc_rdma.h | 27 +++++++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma.c | 23 +++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_transport.c | 26 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 0 deletions(-)include/linux/sunrpc/svc_rdma.h +26
-0@@ -86,6 +86,31 @@ struct svc_rdma_op_ctxt {struct page *pages[RPCSVC_MAXPAGES ]; }; +/* +* NFS_ requests are mapped on the client side by the chunk lists in +* the RPCRDMA header. During the fetching of the RPC from the client +* and the writing of the reply to the client, the memory in the +* client and the memory in the server must be mapped as contiguous +* vaddr/len for access by the hardware. These data strucures keep +* these mappings. +* +* For an RDMA_WRITE, the 'sge' maps the RPC REPLY. For RDMA_READ, the +* 'sge' in the svc_rdma_req_map maps the server side RPC reply and the +* 'ch' field maps the read-list of the RPCRDMA header to the 'sge' +* mapping of the reply. +*/ +struct svc_rdma_chunk_sge { + int start; /* sge no for this chunk */ +int count;/* sge count for this chunk */ +}; +struct svc_rdma_req_map { +unsigned long count; +union { +struct kvec sge[RPCSVC_MAXPAGES ]; +struct svc_rdma_chunk_sge ch[RPCSVC_MAXPAGES ]; + }; +}; +#define RDMACTXT_F_LAST_CTXT 2 struct svcxprt_rdma {@@ -173,6 +198,8 @@ extern int svc_rdma_post_recv(struct svcxprt_rdma *);extern int svc_rdma_create_listen (struct svc_serv *,int ,struct sockaddr *);extern struct svc_rdma_op_ctxt *svc_rdma_get_context (struct svcxprt_rdma *);extern void svc_rdma_put_context (struct svc_rdma_op_ctxt *,int ); +extern struct svc_rdma_req_map *svc_rdma_get_req_map (void ); +extern void svc_rdma_put_req_map (struct svc_rdma_req_map *);extern void svc_sq_reap (struct svcxprt_rdma *);extern void svc_rq_reap (struct svcxprt_rdma *);extern struct svc_xprt_class svc_rdma_class;net/sunrpc/xprtrdma/svc_rdma.c +0
-0net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 74
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma.c +1
-0@@ -69,6 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod; +/* Temporary NFS request map cache */ +struct kmem_cache *svc_rdma_map_cachep = NULL;No need to initialize globals to NULL.
Quoted 9 lines
Quoted 3 lines
I thought only static objects were initialized per the C standard. Or are you saying that this particular global doesn't need to be initialized because of the way it is used?
I don't know if the initialization is mandated by the standards or whether it's just gcc behavior, but I know I get a complaint every time somebody finds one of those....
In this case, the initialization is unnecessary, so it can be safely dumped.
Quoted 5 lines
That may partly just be a preference for conciseness, but I think it may also allow gcc to put the thing in a different section and save some space in the on-disk kernel--I don't know.
Right -- un-initialized data goes in a different section, the .bss section in particular. Since the .bss section is not "initialized", there are no values (zeroes in this case) sitting in the object file ready to be mapped into whatever location becomes .bss. By contrast, the .data section contains initialized data and the initial values are sitting in the object file.
So my question here is a little subtler:
A. Do we discard _all_ zero initializations of non-static globals because we can safely assume that .bss is initialzed (not a fan of this), or
B. Don't be an idiot and initialize objects unnecessarily because it bloats the kernel object file?
An idiot voting for B,
Tom
Quoted 6 lines
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/11] svcrdma: Use RPC reply map for RDMA_WRITE processing
In reply to J. Bruce FieldsQuoted 45 lines · J. Bruce Fields
Quoted 43 lines · Tom Tucker
Quoted 41 lines · J. Bruce Fields
Quoted 30 lines · Tom Tucker
Use the new
svc_rdma_req_mapdata type for mapping the client side memory to the server side memory. Move the DMA mapping to the context pointed to by each WR individually so that it is unmapped after the WR completes.Signed-off-by Tom Tucker <[email protected]>--- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 122 ++++++++++++++---------------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 +- 2 files changed, 62 insertions(+), 65 deletions(-)net/sunrpc/xprtrdma/svc_rdma_sendto.c +3
-4@@ -64,10 +64,9 @@* SGE[sge_count-1] data from xdr->tail. * */ -static struct ib_sge *xdr_to_sge (struct svcxprt_rdma *xprt, -struct xdr_buf *xdr, -struct ib_sge *sge, -int *sge_count) +static void xdr_to_sge (struct svcxprt_rdma *xprt, +struct xdr_buf *xdr, +struct svc_rdma_req_map *vec) {/* Max we need is the length of the XDR / pagesize + one for * head + one for tail + one for RPCRDMA header net/sunrpc/xprtrdma/svc_rdma_sendto.c static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, · line 85
Quoted 3 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +0
-0@@ -84,14 +83,10 @@ static struct ib_sge *xdr_to_sge(struct svcxprt_rdma *xprt, sge_no =1 ;Should the arrays in the
svc_rdma_req_mapactually be size RPC_SVC_MAXPAGES+1 to allow for the RPCRDMA header?
Quoted 9 lines
Quoted 4 lines
Actually, upon further inspection, this is not a bug. The
RPCSVC_MAXPAGESis used to size the rqstp page array. This array includes a page for the request hdr, reply hdr and one extra page for alignment. For our use case, the reply hdr page covers the RDMA header.OK, that makes sense. (Let's just fix the comment above so I don't forget and ask the same question again next time.)
Ok, done.
Quoted 23 lines
Quoted 7 lines
However, I think this payload length should be clamped in a different way. It seems silly to allocate these things to handle big-data mounts that are rarely used in practice. I think it's better to have a transport max that is more reasonable by default and a module parameter that is configurable if you're looking to do big-data mounts.
Thoughts?
This is decided in fs/nfsd/nfssvc.c:nfsd_create_serv(), based on the machine's ram size, and can be overridden with the nfsd filesystem's maxblksize parameter.
I'm open to suggestions, but my feeling is that it's hard to educate people about how to use additional sysctl's, and that it's reasonable to waste some memory to get good out-of-the-box performance on streaming IO.
See also recent patches from Olga and Dean for similar discussion (about WAN tcp performance in their case, since the size of these buffers affects the size of the tcp window we advertise).
--b.
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
In reply to Tom TuckerQuoted 46 lines · Tom Tucker
Quoted 18 lines · J. Bruce Fields
Quoted 16 lines · Tom Tucker
Quoted 14 lines · J. Bruce Fields
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 73
Quoted 10 lines · Tom Tucker
net/sunrpc/xprtrdma/svc_rdma.c +1
-0@@ -69,6 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod; +/* Temporary NFS request map cache */ +struct kmem_cache *svc_rdma_map_cachep = NULL;No need to initialize globals to NULL.
Quoted 10 lines
Quoted 4 lines
I thought only static objects were initialized per the C standard. Or
are you saying that this particular
global doesn't need to be initialized because of the way it is used?I don't know if the initialization is mandated by the standards or whether it's just gcc behavior, but I know I get a complaint every time somebody finds one of those....
In this case, the initialization is unnecessary, so it can be safely dumped.
Quoted 5 lines
That may partly just be a preference for conciseness, but I think it may also allow gcc to put the thing in a different section and save some space in the on-disk kernel--I don't know.
Right -- un-initialized data goes in a different section, the .bss section in particular. Since the .bss section is not "initialized", there are no values (zeroes in this case) sitting in the object file ready to be mapped into whatever location becomes .bss. By contrast, the .data section contains initialized data and the initial values are sitting in the object file.
So my question here is a little subtler:
Quoted 8 lines
A. Do we discard _all_ zero initializations of non-static globals because we can safely assume that .bss is initialzed (not a fan of this), or
B. Don't be an idiot and initialize objects unnecessarily because it bloats the kernel object file?
An idiot voting for B,
My understanding is that it's A--e.g. checkpatch.pl has a dumb regex that just checks for these assignments and whines about them however they're used.
Google doesn't find me any more detailed discussion of the policy.
--b.
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
Quoted 20 lines · J. Bruce Fields
Quoted 19 lines · Tom Tucker
Quoted 18 lines · J. Bruce Fields
Quoted 16 lines · Tom Tucker
Quoted 14 lines · J. Bruce Fields
net/sunrpc/xprtrdma/svc_rdma.c atomic_t rdma_stat_rq_prod; · line 73
Quoted 10 lines · Tom Tucker
net/sunrpc/xprtrdma/svc_rdma.c +1
-0@@ -69,6 +69,9 @@ atomic_t rdma_stat_rq_prod;atomic_t rdma_stat_sq_poll;atomic_t rdma_stat_sq_prod; +/* Temporary NFS request map cache */ +struct kmem_cache *svc_rdma_map_cachep = NULL;No need to initialize globals to NULL.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
I thought only static objects were initialized per the C standard. Or
are you saying that this particular
global doesn't need to be initialized because of the way it is used?I don't know if the initialization is mandated by the standards or whether it's just gcc behavior, but I know I get a complaint every time somebody finds one of those....
The implicit initialization to zero was defined back then by K&R:
http://www.cs.utah.edu/~phister/K_n_R/chapter4.html#s4.9
"In the absence of explicit initialization, external and static variables are guaranteed to be initialized to zero"
AFAIK this convention was kept by the C standards ever since.
Quoted 16 lines
Quoted 16 lines
Quoted 2 lines
In this case, the initialization is unnecessary, so it can be safely dumped.
Quoted 5 lines
That may partly just be a preference for conciseness, but I think it may also allow gcc to put the thing in a different section and save some space in the on-disk kernel--I don't know.
Right -- un-initialized data goes in a different section, the .bss section in particular. Since the .bss section is not "initialized", there are no values (zeroes in this case) sitting in the object file ready to be mapped into whatever location becomes .bss. By contrast, the .data section contains initialized data and the initial values are sitting in the object file.
Quoted 5 lines
Quoted 5 lines
So my question here is a little subtler:
A. Do we discard _all_ zero initializations of non-static globals because we can safely assume that .bss is initialzed (not a fan of this), or
Also static locals... What matters is that the variable has static allocation, its scope is irrelevant to the initialization issue.
Benny
Quoted 17 lines
Quoted 5 lines
B. Don't be an idiot and initialize objects unnecessarily because it bloats the kernel object file?
An idiot voting for B,
My understanding is that it's A--e.g. checkpatch.pl has a dumb regex that just checks for these assignments and whines about them however they're used.
Google doesn't find me any more detailed discussion of the policy.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] svcrdma: Add a type for keeping NFS RPC mapping
In reply to J. Bruce FieldsQuoted 5 lines · J. Bruce Fields
My understanding is that it's A--e.g. checkpatch.pl has a dumb regex that just checks for these assignments and whines about them however they're used.
Google doesn't find me any more detailed discussion of the policy.
We had that discussion about 10 years ago.
What followed was a general purge of all zero initialisations of global and static variables in the kernel, and the official policy is that initialising those variables is wrong, and should _not_ be done.
The reason is that BSS is automatically zeroed out, and so adding an initialiser causes unnecessary code bloat and slows down the boot process.
Trond