Re: [PATCH] svcrdma: Fix compile warning on 32b platforms

["J. Bruce Fields" <bfields@fieldses.org>]

On Sun, May 25, 2008 at 09:15:58PM -0500, Tom Tucker wrote:
> 
> 
> 
> On 5/25/08 3:21 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Sun, May 25, 2008 at 03:01:12PM -0400, bfields wrote:
> >> On Fri, May 23, 2008 at 10:39:53AM -0500, Tom Tucker wrote:
> >>> 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.
> >> 
> >> 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:
> >>
>  
> >>> +       (void*)(unsigned long)
> >>> +       sge[i].addr, sge[i].length,
> >> 
> >> makes me nervous.
> >
> 
> 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_addr and 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 the dma_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.)

> 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.
>  
> > 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.