Chuck Lever <chuck.lever@xxxxxxxxxx> writes: > On Apr 28, 2009, at 5:36 PM, J. Bruce Fields wrote: > >> On Tue, Apr 28, 2009 at 01:24:24PM -0400, Chuck Lever wrote: >>> On Apr 28, 2009, at 12:40 PM, J. Bruce Fields wrote: >>>> On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote: >>>>> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote: >>>>>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote: >>>>>>> Recently, commit ad5b365c fixed a data type alignment issue in >>>>>>> nsm_init_private() by introducing put_unaligned(). We don't >>>>>>> actually _require_ an unaligned access to nsm_private here. >>>>>>> >>>>>>> Instead, we should always use memcpy/memcmp to access the >>>>>>> contents of RPC opaque data types. This permits us to >>>>>>> continue to define these as simple character arrays, as most >>>>>>> legacy RPC code does, and to rely on gcc to pack the fields in >>>>>>> in-core structures optimally without breaking on platforms >>>>>>> that require strict alignment. >>>>>> >>>>>> OK, I'm confused. What structures will get packed differently? >>>>> >>>>> Any struct that has an nsm_private embedded in it, such as struct >>>>> nlm_reboot. >>>> >>>> I don't see how that or any structure is changed by this patch. >>> >>> It's not. Note the phrase above in the description: "permits us to >>> _continue_ to define these" -- meaning, I'm not changing the >>> structures. >> >> Err, but that's not right either, is it?: We don't need to apply this >> patch in order to continue to define the structures as they're >> currently >> defined. >> >> Help! I'm confused! > > This patch is simply a clean up. We don't need to use put_unaligned > in nsm_init_private. There is absolutely nothing special about the > nsm_private data type that would require this. It should be accessed The "special" thing is has not guaranteed alignment. Hence, any access to it must use unaligned-safe methods. > and modified the way all other RPC opaques are, via memset/memcpy. What is so special about put_unaligned() that you insist on replacing it? > Take a look at http://thread.gmane.org/gmane.linux.nfs/25607 and > commit ad5b365c. > > The controversy is over how to define opaques so they are accessible > on both 32- and 64-bit hardware platforms. My first pass at > nsm_init_private worked on 32-bit systems, but broke on 64-bit > systems. An expedient fix for this was to add the put_unaligned in > there so 64-bit systems could access the field no matter how it was > aligned. I argue this is unneeded complexity, and inconsistent with > the way most other RPC opaques are treated in the kernel. > > Andrew Morton proposed making RPC opaques a union of u8, u32 (or > __be32), and u64 -- the u8 would allow us to treat an opaque as a byte > array when needed, the u32 would allow access via XDR quads, and the > u64 would force 64-bit alignment. The issues with this are: > > 1. Defined this way, opaque fields in data structures will force the > encompassing structures to be large enough to honor the alignment > requirements of the fields, and > > 2. Most other RPC opaques are already defined as character arrays, so > we would have to visit all of them to see if there were issues. > > If we insist on accessing opaques only via memset() and memcpy() > problem 1 goes away and we remain compatible with the traditional > definition of an RPC opaque as an array of bytes, on both 64- and 32- > bit systems. I still don't see what problem put_unaligned() poses. Think of it as a more efficient memcpy(). We don't want the code to be larger and slower than necessary, do we? -- Måns Rullgård mans@xxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
