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. /// [email protected] /// [email protected]
Curnow \\\ http://www.superh.com/ /// http://www.rc0.org.uk
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
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. /// [email protected] /// [email protected]
Curnow \\\ http://www.superh.com/ /// http://www.rc0.org.uk
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