[3/3] xfs: remove __arch_pack
diff mbox

Message ID 1466754767-10657-4-git-send-email-hch@lst.de
State Accepted
Headers show

Commit Message

Christoph Hellwig June 24, 2016, 7:52 a.m. UTC
Instead we always declare struct xfs_dir2_sf_hdr as packed.  That's
the expected layout, and while most major architectures do the packing
by default the new structure size and offset checker showed that not
only the ARM old ABI got this wrong, but various minor embedded
architectures did as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_da_format.h | 2 +-
 fs/xfs/xfs_linux.h            | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

Comments

Eric Sandeen June 24, 2016, 2:55 p.m. UTC | #1
On 6/24/16 2:52 AM, Christoph Hellwig wrote:
> Instead we always declare struct xfs_dir2_sf_hdr as packed.  That's
> the expected layout, and while most major architectures do the packing
> by default the new structure size and offset checker showed that not
> only the ARM old ABI got this wrong, but various minor embedded
> architectures did as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_da_format.h | 2 +-
>  fs/xfs/xfs_linux.h            | 7 -------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index f877bb1..685f23b 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -229,7 +229,7 @@ typedef struct xfs_dir2_sf_hdr {
>  	__uint8_t		count;		/* count of entries */
>  	__uint8_t		i8count;	/* count of 8-byte inode #s */
>  	__uint8_t		parent[8];	/* parent dir inode number */
> -} __arch_pack xfs_dir2_sf_hdr_t;
> +} __packed xfs_dir2_sf_hdr_t;

The reason I did this in the first place was a vague notion that unconditional
packing was harmful.

http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

"However, it's actively harmful to add the attribute to a structure that's
already going to be laid out with no padding."
...
"gcc gets scared about unaligned accesses and generates six times as much code
(96 bytes vs. 16 bytes)! sparc64 goes similarly crazy, bloating from 12 bytes
to 52 bytes"

I don't know if that's (still) correct or not, but that was the reason
for the selective __pack application way back when.  Might be worth
investigating?

-Eric

>  typedef struct xfs_dir2_sf_entry {
>  	__u8			namelen;	/* actual name length */
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index a8192dc..b8d64d5 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -328,13 +328,6 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
>  	return x;
>  }
>  
> -/* ARM old ABI has some weird alignment/padding */
> -#if defined(__arm__) && !defined(__ARM_EABI__)
> -#define __arch_pack __attribute__((packed))
> -#else
> -#define __arch_pack
> -#endif
> -
>  #define ASSERT_ALWAYS(expr)	\
>  	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
>  
>
Dave Chinner July 18, 2016, 5:37 a.m. UTC | #2
On Fri, Jun 24, 2016 at 09:55:37AM -0500, Eric Sandeen wrote:
> On 6/24/16 2:52 AM, Christoph Hellwig wrote:
> > Instead we always declare struct xfs_dir2_sf_hdr as packed.  That's
> > the expected layout, and while most major architectures do the packing
> > by default the new structure size and offset checker showed that not
> > only the ARM old ABI got this wrong, but various minor embedded
> > architectures did as well.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/libxfs/xfs_da_format.h | 2 +-
> >  fs/xfs/xfs_linux.h            | 7 -------
> >  2 files changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> > index f877bb1..685f23b 100644
> > --- a/fs/xfs/libxfs/xfs_da_format.h
> > +++ b/fs/xfs/libxfs/xfs_da_format.h
> > @@ -229,7 +229,7 @@ typedef struct xfs_dir2_sf_hdr {
> >  	__uint8_t		count;		/* count of entries */
> >  	__uint8_t		i8count;	/* count of 8-byte inode #s */
> >  	__uint8_t		parent[8];	/* parent dir inode number */
> > -} __arch_pack xfs_dir2_sf_hdr_t;
> > +} __packed xfs_dir2_sf_hdr_t;
> 
> The reason I did this in the first place was a vague notion that unconditional
> packing was harmful.
> 
> http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/
> 
> "However, it's actively harmful to add the attribute to a structure that's
> already going to be laid out with no padding."
> ...
> "gcc gets scared about unaligned accesses and generates six times as much code
> (96 bytes vs. 16 bytes)! sparc64 goes similarly crazy, bloating from 12 bytes
> to 52 bytes"
> 
> I don't know if that's (still) correct or not, but that was the reason
> for the selective __pack application way back when.  Might be worth
> investigating?

Christoph? The first two ptches are fine, but more info is needed
for this one...

Cheers,

Dave.
Christoph Hellwig July 19, 2016, 8:52 a.m. UTC | #3
On Mon, Jul 18, 2016 at 03:37:46PM +1000, Dave Chinner wrote:
> > The reason I did this in the first place was a vague notion that unconditional
> > packing was harmful.
> > 
> > http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/
> > 
> > "However, it's actively harmful to add the attribute to a structure that's
> > already going to be laid out with no padding."
> > ...
> > "gcc gets scared about unaligned accesses and generates six times as much code
> > (96 bytes vs. 16 bytes)! sparc64 goes similarly crazy, bloating from 12 bytes
> > to 52 bytes"
> > 
> > I don't know if that's (still) correct or not, but that was the reason
> > for the selective __pack application way back when.  Might be worth
> > investigating?
> 
> Christoph? The first two ptches are fine, but more info is needed
> for this one...

I don't have a sparc64 compiler to test unfortunately.  But I can confirm
that on x86-64 xfs.o is bit to bit identical with or without the patch.
Dave Chinner July 19, 2016, 11:46 p.m. UTC | #4
On Tue, Jul 19, 2016 at 10:52:08AM +0200, Christoph Hellwig wrote:
> 
> On Mon, Jul 18, 2016 at 03:37:46PM +1000, Dave Chinner wrote:
> > > The reason I did this in the first place was a vague notion that unconditional
> > > packing was harmful.
> > > 
> > > http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/
> > > 
> > > "However, it's actively harmful to add the attribute to a structure that's
> > > already going to be laid out with no padding."
> > > ...
> > > "gcc gets scared about unaligned accesses and generates six times as much code
> > > (96 bytes vs. 16 bytes)! sparc64 goes similarly crazy, bloating from 12 bytes
> > > to 52 bytes"
> > > 
> > > I don't know if that's (still) correct or not, but that was the reason
> > > for the selective __pack application way back when.  Might be worth
> > > investigating?
> > 
> > Christoph? The first two ptches are fine, but more info is needed
> > for this one...
> 
> I don't have a sparc64 compiler to test unfortunately.  But I can confirm
> that on x86-64 xfs.o is bit to bit identical with or without the patch.

OK, that's probably good enough to go with for now. Thanks for
following up, Christoph.

Cheers,

Dave.

Patch
diff mbox

diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index f877bb1..685f23b 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -229,7 +229,7 @@  typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	__uint8_t		parent[8];	/* parent dir inode number */
-} __arch_pack xfs_dir2_sf_hdr_t;
+} __packed xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__u8			namelen;	/* actual name length */
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index a8192dc..b8d64d5 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -328,13 +328,6 @@  static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
 	return x;
 }
 
-/* ARM old ABI has some weird alignment/padding */
-#if defined(__arm__) && !defined(__ARM_EABI__)
-#define __arch_pack __attribute__((packed))
-#else
-#define __arch_pack
-#endif
-
 #define ASSERT_ALWAYS(expr)	\
 	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))