NFS structure allocation alignment patch

NFS structure allocation alignment patch

[Richard Curnow]

Hi Trond, Marcelo,

Below is a patch against 2.4.21 to tidy up the allocation of two
structures in nfs3_proc_unlink_setup.  We need this change for NFS to
work on the sh64 architecture, which has just been merged into 2.4 in
the last couple of days.  Otherwise, 'res' is 4-byte aligned but not
necessarily 8-byte aligned, but struct nfs_attr contains fields that are
8 bytes wide.  This leads to alignment exceptions on loads and stores
into that structure.

Can you consider this for inclusion before 2.4.22 please?

[BTW this is the same fix we discussed about 3 months ago; I didn't get
around to re-submitting the fix at the time].

Regards,
Richard Curnow

--- fs/nfs/nfs3proc.c   Thu Nov 28 23:53:15 2002
+++ ../sh5linux-16062003/fs/nfs/nfs3proc.c      Mon Jun 30 11:43:14 2003
@@ -300,11 +300,16 @@
 {
        struct nfs3_diropargs   *arg;
        struct nfs_fattr        *res;
+       struct unlinkxdr {
+               struct nfs3_diropargs   arg;
+               struct nfs_fattr        res;
+       } *ptr;
 
-       arg = (struct nfs3_diropargs *)kmalloc(sizeof(*arg)+sizeof(*res), GFP_KERNEL);
-       if (!arg)
+       ptr = (struct unlinkxdr *) kmalloc(sizeof(struct unlinkxdr), GFP_KERNEL);
+       if (!ptr)
                return -ENOMEM;
-       res = (struct nfs_fattr*)(arg + 1);
+       arg = &ptr->arg;
+       res = &ptr->res;
        arg->fh = NFS_FH(dir->d_inode);
        arg->name = name->name;
        arg->len = name->len;

-- 
Richard \\\ SuperH Core+Debug Architect /// .. At home ..
  P.    /// richard.curnow@superh.com  ///  rc@rc0.org.uk
Curnow  \\\ http://www.superh.com/    ///  www.rc0.org.uk

Re: NFS structure allocation alignment patch

[David Woodhouse]

On Mon, 2003-06-30 at 14:52, Richard Curnow wrote:
> Hi Trond, Marcelo,
> 
> Below is a patch against 2.4.21 to tidy up the allocation of two
> structures in nfs3_proc_unlink_setup.  We need this change for NFS to
> work on the sh64 architecture, which has just been merged into 2.4 in
> the last couple of days.  Otherwise, 'res' is 4-byte aligned but not
> necessarily 8-byte aligned, but struct nfs_attr contains fields that are
> 8 bytes wide.  This leads to alignment exceptions on loads and stores
> into that structure.

What's wrong with alignment exceptions? They get fixed up by your
exception handler, surely?

If you assert that it's a performance-critical path and hence we
shouldn't be relying on the exception fixup, that's fine -- but in that
case it's not a correctness fix, it's just an optimisation.

-- 
dwmw2

Re: NFS structure allocation alignment patch

[Richard Curnow]

On Thu, 10 Jul, 2003 at 12:05pm, David Woodhouse wrote:
> On Mon, 2003-06-30 at 14:52, Richard Curnow wrote:
> > Hi Trond, Marcelo,
> > 
> > Below is a patch against 2.4.21 to tidy up the allocation of two
> > structures in nfs3_proc_unlink_setup.  We need this change for NFS to
> > work on the sh64 architecture, which has just been merged into 2.4 in
> > the last couple of days.  Otherwise, 'res' is 4-byte aligned but not
> > necessarily 8-byte aligned, but struct nfs_attr contains fields that are
> > 8 bytes wide.  This leads to alignment exceptions on loads and stores
> > into that structure.
> 
> What's wrong with alignment exceptions? They get fixed up by your
> exception handler, surely?
> 
> If you assert that it's a performance-critical path and hence we
> shouldn't be relying on the exception fixup, that's fine -- but in that
> case it's not a correctness fix, it's just an optimisation.

Apart from this issue, we haven't seen any code- or compiler-related
problems due to misaligned loads and stores occurring.  Indeed, because
gcc takes care to lay out structures to honour load/store alignments, we
deliberately don't 'fix-up' misaligned accesses, rather an oops is
raised since they are almost certainly due to something having gone
wrong, e.g.  a pointer has been overwritten somehow.

I bet someone will wonder how the misalignment hadn't shown up before.
Recall there were 2 structures being allocated with 1 kmalloc call.  The
first (nfs3_diropargs) contains 2 pointers and an unsigned int.  The
second (nfs_fattr) contains amongst other things some __u64's.  On sh64,
the __u64's will be accessed with single 8-byte load/store operations.
Although the SHmedia instruction set fully supports 64-bit addressing,
the current generation implements 32-bit (with sign-extension to 64) so
the toolchains currently store pointers as 32-bit to save memory &
cache.  Hence the 1st structure is only compiled with 4-byte alignment
=> insufficiently aligned 2nd structure in the old code.  I presume all
the other 64-bit architectures already use 64-bit pointers so the
alignment problem didn't happen.  With the patch I sent, the required
alignments are assured for any architecture.

HTH
Richard

-- 
Richard \\\ SuperH Core+Debug Architect /// .. At home ..
  P.    /// richard.curnow@superh.com  ///  rc@rc0.org.uk
Curnow  \\\ http://www.superh.com/    ///  www.rc0.org.uk

Re: NFS structure allocation alignment patch

[David Woodhouse]

On Wed, 2003-07-16 at 16:03, Richard Curnow wrote:
> Apart from this issue, we haven't seen any code- or compiler-related
> problems due to misaligned loads and stores occurring.

That doesn't mean they don't exist. Various parts of the network code
_require_ alignment fixups -- the common case is that it'll be aligned,
and we take the hit only in the _rare_ case of misalignment rather than
using get_unaligned() in the fast path.

Also the flash code does the same in places too. It _did_ use
get_unaligned() for a while but it was removed since it's never valid
for an architecture to _not_ fix up unaligned in-kernel accesses.

> I bet someone will wonder how the misalignment hadn't shown up before.

Because all other architectures implement alignment fixups.

-- 
dwmw2