svcrdma: Fix compile warning on 32b platforms
Subsystems (3)
PATCH 1/1 svcrdma: Fix compile warning on 32b platforms
The ib_sge is used to store both dma_addr_t and virtual addresses. This causes a warning on 32b platforms when casting ptrs to the u64 addr field of the ib_sge. There is work underway to reduce the memory footprint of the WR context in 2.6.27 the time frame that will remove the overloading of the ib_sge.addr field.
---
Thanks to Bruce and Andrew for pointing this out. I will compile-test both 32b and 64b platforms in the future.
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 3 ++- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2 -1
@@ -231,7 +231,8 @@ static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt,atomic_inc (&xprt->sc_dma_used); ctxt->sge[i].addr = (unsigned long )ib_dma_map_single (xprt->sc_cm_id->device, - (void *)sge[i].addr, sge[i].length, + (void *)(unsigned long ) + sge[i].addr, sge[i].length,DMA_FROM_DEVICE ); ctxt->sge[i].length = sge[i].length; ctxt->sge[i].lkey = sge[i].lkey;
net/sunrpc/xprtrdma/svc_rdma_sendto.c +2 -2
@@ -177,7 +177,7 @@ static int send_write(struct svcxprt_rdma *xprt, struct svc_rqst *rqstp,atomic_inc (&xprt->sc_dma_used); sge[sge_no].addr =ib_dma_map_single (xprt->sc_cm_id->device, - (void *) + (void *)(unsigned long ) xdr_sge[xdr_sge_no].addr + sge_off, sge_bytes,DMA_TO_DEVICE );if (dma_mapping_error (sge[sge_no].addr))
@@ -416,7 +416,7 @@ static int send_reply(struct svcxprt_rdma *rdma,atomic_inc (&rdma->sc_dma_used); ctxt->sge[sge_no].addr =ib_dma_map_single (rdma->sc_cm_id->device, - (void *) + (void *)(unsigned long ) ctxt->sge[sge_no].addr, sge_bytes,DMA_TO_DEVICE ); }
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · Tom Tucker
The
ib_sgeis used to store bothdma_addr_tand virtual addresses. This causes a warning on 32b platforms when casting ptrs to the u64 addr field of theib_sge. There is work underway to reduce the memory footprint of the WR context in 2.6.27 the time frame that will remove the overloading of the ib_sge.addr field.
Naively, this and "svcrdma: Refactor RDMA_WRITE dma mapping logic" still look like a step in the wrong direction; the old code seemed to have a much more clear separation between kernel addresses (void *'s) and on-the-wire addresses (stored in u64's). Stuff like:
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt, · line 235
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2
-0+ (void *)(unsigned long ) + sge[i].addr, sge[i].length,
makes me nervous.
--b.
Quoted 12 lines · bfields
Quoted 5 lines · Tom Tucker
The
ib_sgeis used to store bothdma_addr_tand virtual addresses. This causes a warning on 32b platforms when casting ptrs to the u64 addr field of theib_sge. There is work underway to reduce the memory footprint of the WR context in 2.6.27 the time frame that will remove the overloading of the ib_sge.addr field.Naively, this and "svcrdma: Refactor
RDMA_WRITEdma mapping logic" still look like a step in the wrong direction; the old code seemed to have a much more clear separation between kernel addresses (void *'s) and on-the-wire addresses (stored in u64's). Stuff like:
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt, · line 235
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2
-0+ (void *)(unsigned long ) + sge[i].addr, sge[i].length,makes me nervous.
That aside, are those two patches really bugfixes? We need to keep bugfixes (for 2.6.26) separate from other stuff (for 2.6.27).
--b.
A message that is not in the archive (sent off-list or not yet received): C45F878E.19918%[email protected]
Quoted 25 lines · Tom Tucker
Quoted 14 lines · J. Bruce Fields
Quoted 12 lines · bfields
Quoted 5 lines · Tom Tucker
The
ib_sgeis used to store bothdma_addr_tand virtual addresses. This causes a warning on 32b platforms when casting ptrs to the u64 addr field of theib_sge. There is work underway to reduce the memory footprint of the WR context in 2.6.27 the time frame that will remove the overloading of the ib_sge.addr field.Naively, this and "svcrdma: Refactor
RDMA_WRITEdma mapping logic" still look like a step in the wrong direction; the old code seemed to have a much more clear separation between kernel addresses (void *'s) and on-the-wire addresses (stored in u64's). Stuff like:Quoted 5 lines
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt, · line 235
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2
-0+ (void *)(unsigned long ) + sge[i].addr, sge[i].length,makes me nervous.
Quoted 11 lines
The mapping of data on the wire to addresses in the client is contained in the RPCRDMA header of the incoming request and needs to be translated into an iovec for subsequent submission as WR. To store this mapping, the code uses a temporarily allocated context. As you've noted, pre-fix this context contained only
dma_addrand the WR context contained essentially an empty mapping (ctxt->count==0). The problem is that when the temporary mapping is removed, the SGE are unmapped and this unmap could occur before the WR has actually completed. The fix coded is to have the temporary context contain just the translation and have the WR context contain thedma_addr. This way the unmap occurs when the WR completes, not when the temporary context is put. However, this "solution" introduces the very issue we're discussing.
Oh, OK. From the changelog (and what I saw of the code), it looked like this what just cleanup; I didn't understand that there was an actual bug fixed.
(Or is the actual bugfix in a later patch ("svcrdma: Move the DMA unmap logic to the CQ handler" maybe?) and this is just preparation for that fix? OK, fair enough; but in future maybe a short note to that effect might help--just enough so I understand why this is appropriate for the current release as opposed to something that could be split out and saved for the next merge window.)
Quoted 11 lines
For 2.6.27, I will create a separate data structure for the temporary mapping that does not share the same type as the WR context. This will allow the correct data types to be stored without confusion.
Quoted 3 lines
That aside, are those two patches really bugfixes? We need to keep bugfixes (for 2.6.26) separate from other stuff (for 2.6.27).
I think the changes do in fact fix real bugs, but they are not symptomatic on x86 platforms. It sounds like we should defer to 2.6.27 and I'll fix the design issue above. Sound ok?
If it's a bugfix then we can still slip it in, but for a rare bug in an as-yet fairly new feature, waiting for the next merge window (and a cleaner fix) also sounds OK.
--b.
Quoted 42 lines · J. Bruce Fields
Quoted 25 lines · Tom Tucker
Quoted 14 lines · J. Bruce Fields
Quoted 12 lines · bfields
Quoted 5 lines · Tom Tucker
The
ib_sgeis used to store bothdma_addr_tand virtual addresses. This causes a warning on 32b platforms when casting ptrs to the u64 addr field of theib_sge. There is work underway to reduce the memory footprint of the WR context in 2.6.27 the time frame that will remove the overloading of the ib_sge.addr field.Naively, this and "svcrdma: Refactor
RDMA_WRITEdma mapping logic" still look like a step in the wrong direction; the old code seemed to have a much more clear separation between kernel addresses (void *'s) and on-the-wire addresses (stored in u64's). Stuff like:Quoted 5 lines
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt, · line 235
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2
-0+ (void *)(unsigned long ) + sge[i].addr, sge[i].length,makes me nervous.
Quoted 11 lines
The mapping of data on the wire to addresses in the client is contained in the RPCRDMA header of the incoming request and needs to be translated into an iovec for subsequent submission as WR. To store this mapping, the code uses a temporarily allocated context. As you've noted, pre-fix this context contained only
dma_addrand the WR context contained essentially an empty mapping (ctxt->count==0). The problem is that when the temporary mapping is removed, the SGE are unmapped and this unmap could occur before the WR has actually completed. The fix coded is to have the temporary context contain just the translation and have the WR context contain thedma_addr. This way the unmap occurs when the WR completes, not when the temporary context is put. However, this "solution" introduces the very issue we're discussing.Oh, OK. From the changelog (and what I saw of the code), it looked like this what just cleanup; I didn't understand that there was an actual bug fixed.
Quoted 6 lines
(Or is the actual bugfix in a later patch ("svcrdma: Move the DMA unmap logic to the CQ handler" maybe?) and this is just preparation for that fix? OK, fair enough; but in future maybe a short note to that effect might help--just enough so I understand why this is appropriate for the current release as opposed to something that could be split out and saved for the next merge window.)
It fixes a bug. The splitting out of the DMA unmap fixes _another_ bug where the dma_unmap occurs too late :-\. On the receive side you must unmap before attempting to look at the data.
Quoted 9 lines
Quoted 9 lines
For 2.6.27, I will create a separate data structure for the temporary mapping that does not share the same type as the WR context. This will allow the correct data types to be stored without confusion.
Quoted 3 lines
That aside, are those two patches really bugfixes? We need to keep bugfixes (for 2.6.26) separate from other stuff (for 2.6.27).
Quoted 7 lines
Quoted 3 lines
I think the changes do in fact fix real bugs, but they are not symptomatic on x86 platforms. It sounds like we should defer to 2.6.27 and I'll fix the design issue above. Sound ok?
If it's a bugfix then we can still slip it in, but for a rare bug in an as-yet fairly new feature, waiting for the next merge window (and a cleaner fix) also sounds OK.
Actually Bruce, I've just finished coding the nice, new, and clean (I hope) design. I'll post it after I've tested it some more. Bottom line, your comments and instincts are good. I kind of panicked when I realized how busted the DMA was. I'll post a follow up here shortly... Thanks for your patience.
Tom
Quoted 2 lines
--b.
Quoted 12 lines · J. Bruce Fields
Quoted 5 lines · Tom Tucker
The
ib_sgeis used to store bothdma_addr_tand virtual addresses. This causes a warning on 32b platforms when casting ptrs to the u64 addr field of theib_sge. There is work underway to reduce the memory footprint of the WR context in 2.6.27 the time frame that will remove the overloading of the ib_sge.addr field.Naively, this and "svcrdma: Refactor
RDMA_WRITEdma mapping logic" still look like a step in the wrong direction; the old code seemed to have a much more clear separation between kernel addresses (void *'s) and on-the-wire addresses (stored in u64's). Stuff like:
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c static void rdma_set_ctxt_sge(struct svcxprt_rdma *xprt, · line 235
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +2
-0+ (void *)(unsigned long ) + sge[i].addr, sge[i].length,makes me nervous.
The svc_rdma_op_ctxt is being used for both the context of the NFS_ RPC request (that can map up to 2M) and the RDMA WR (which is bounded by the SGE limit of the device).
What changed is when the dma_map is performed. Originally it was performed when the NFS_ request was mapped to a local SGL. The problem with this is that the temporary context used to keep this mapping is freed (and unmapped) before the WR completes and therefore the memory could be unmapped too early. Since the unmap is a no-op on x86 -- this never caused a problem, however, for IA64 and others -- it is a problem.
What needs to be done is to replace the svc_rdma_op_ctxt with a suitable temporary mapping structure with the appropriate type. I will have this patch soon.
I knew it was broken and was concerned that it would be found in the field, but since we don't hear the screams - it sounds like we should just ditch these interim dma changes and then we won't have code upstream that has this interim typing issue.
If someone does find this problem, I can point them at my tree to give it whirl.
Sound ok?
Tom
Quoted 5 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