From: "J. Bruce Fields" Subject: Re: [PATCH 19/19] lockd: clean up 64-bit alignment fix in nsm_init_private() Date: Tue, 28 Apr 2009 17:36:38 -0400 Message-ID: <20090428213638.GL23924@fieldses.org> References: <20090423231550.17283.24432.stgit@ingres.1015granger.net> <20090423233340.17283.29580.stgit@ingres.1015granger.net> <20090428163101.GK17891@fieldses.org> <1AF02847-8D16-4387-81CB-F36969CA9883@oracle.com> <20090428164027.GO17891@fieldses.org> <9A6CA37F-0D4D-48C3-AEF8-E47B2836EDCC@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-nfs@vger.kernel.org, Mans Rullgard To: Chuck Lever Return-path: Received: from mail.fieldses.org ([141.211.133.115]:43164 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753727AbZD1Vgi (ORCPT ); Tue, 28 Apr 2009 17:36:38 -0400 In-Reply-To: <9A6CA37F-0D4D-48C3-AEF8-E47B2836EDCC@oracle.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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! > It has been suggested that we use a union or __aligned attribute for RPC > opaques. The problem with that is that it would cause structs like > nlm_reboot to balloon in size; char x[] is aligned on bytes, but a union > of u8, u32, and u64 would be aligned on a double-word boundary. That > would make such structures larger. OK, I agree, so let's not do that. --b. > Legacy RPC code, and any code generated by rpcgen, generally defines an > opaque as a character array. So again, using a union would be > inconsistent with other uses of RPC opaques. > > The point is we want to define and access RPC opaque's consistently, > using memset() and memcpy(), since these are nothing more than byte > arrays. The code in nsm_init_private() was an exception to this, for no > reason. We don't really need special alignment macros in there, as long > as we access the RPC opaque field with the byte sets and copies. > > Would it help if I described this patch as a "clean up" ?