[PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[Guan Xuetao]

From: Guan Xuetao <guanxuetao@mprc.pku.edu.cn>

 This patch modifies stat.h in include/asm-generic to be applicable to more architectures.
 STAT64_HAS_BROKEN_ST_INO is defined in most architecture's asm/stat.h, and it need 
 32-bit __st_ino member to be defined in different position of 64-bit st_ino member.
 STAT64_PAD_BEFORE_ST_SIZE is the pad before st_size member, with default value 8 bytes.
 STAT64_PAD_BEFORE_ST_BLOCKS is the pad before st_blocks member, with default value
 4 bytes to align the following member to 64-bit.

Signed-off-by: Guan Xuetao <guanxuetao@mprc.pku.edu.cn>
---
 include/asm-generic/stat.h |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/stat.h b/include/asm-generic/stat.h
index bd8cad2..82d1108 100644
--- a/include/asm-generic/stat.h
+++ b/include/asm-generic/stat.h
@@ -45,18 +45,36 @@ struct stat {
 
 /* This matches struct stat64 in glibc2.1. Only used for 32 bit. */
 #if __BITS_PER_LONG != 64 || defined(__ARCH_WANT_STAT64)
+
+#ifndef STAT64_PAD_BEFORE_ST_SIZE
+#define STAT64_PAD_BEFORE_ST_SIZE	8
+#endif
+
+#ifndef STAT64_PAD_BEFORE_ST_BLOCKS
+#define STAT64_PAD_BEFORE_ST_BLOCKS	4
+#endif
+
 struct stat64 {
 	unsigned long long st_dev;	/* Device.  */
+#ifndef STAT64_HAS_BROKEN_ST_INO
 	unsigned long long st_ino;	/* File serial number.  */
+#else /* STAT64_HAS_BROKEN_ST_INO */
+	unsigned int	__unused1;	/* aligned to 64-bit. */
+	unsigned int	__st_ino;	/* File serial number.  */
+#endif /* STAT64_HAS_BROKEN_ST_INO */
 	unsigned int	st_mode;	/* File mode.  */
 	unsigned int	st_nlink;	/* Link count.  */
 	unsigned int	st_uid;		/* User ID of the file's owner.  */
 	unsigned int	st_gid;		/* Group ID of the file's group. */
 	unsigned long long st_rdev;	/* Device number, if device.  */
-	unsigned long long __pad1;
+#if STAT64_PAD_BEFORE_ST_SIZE != 0
+	unsigned char	__unused2[STAT64_PAD_BEFORE_ST_SIZE];
+#endif
 	long long	st_size;	/* Size of file, in bytes.  */
 	int		st_blksize;	/* Optimal block size for I/O.  */
-	int		__pad2;
+#if STAT64_PAD_BEFORE_ST_BLOCKS != 0
+	unsigned char	__unused3[STAT64_PAD_BEFORE_ST_BLOCKS];
+#endif
 	long long	st_blocks;	/* Number 512-byte blocks allocated. */
 	int		st_atime;	/* Time of last access.  */
 	unsigned int	st_atime_nsec;
@@ -64,8 +82,12 @@ struct stat64 {
 	unsigned int	st_mtime_nsec;
 	int		st_ctime;	/* Time of last status change.  */
 	unsigned int	st_ctime_nsec;
+#ifndef STAT64_HAS_BROKEN_ST_INO
 	unsigned int	__unused4;
 	unsigned int	__unused5;
+#else /* STAT64_HAS_BROKEN_ST_INO */
+	unsigned long long st_ino;	/* File serial number.  */
+#endif /* STAT64_HAS_BROKEN_ST_INO */
 };
 #endif

Re: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[Arnd Bergmann]

On Saturday 08 January 2011, Guan Xuetao wrote:
> From: Guan Xuetao <guanxuetao@mprc.pku.edu.cn>
> 
>  This patch modifies stat.h in include/asm-generic to be applicable to more architectures.
>  STAT64_HAS_BROKEN_ST_INO is defined in most architecture's asm/stat.h, and it need 
>  32-bit __st_ino member to be defined in different position of 64-bit st_ino member.
>  STAT64_PAD_BEFORE_ST_SIZE is the pad before st_size member, with default value 8 bytes.
>  STAT64_PAD_BEFORE_ST_BLOCKS is the pad before st_blocks member, with default value
>  4 bytes to align the following member to 64-bit.

I'd prefer not to apply this patch. It makes the generic header
significantly more complex, and I can't see a significant benefit.

The existing architectures would all still have to define the macros
you test and also keep defining stuff like __old_kernel_stat, while
risking to introduce bugs while changing to the common header. We've
done similar tricks in other places, where the differences between
architectures are smaller, but this one doesn't seem worth it
unless we can get to the point where we can define struct stat in
linux/stat.h for everyone and only do the macros for the architectures
that need it.

	Arnd

RE: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[Guan Xuetao]

IMO, asm-generic headers should be used in more architectures as far as possible.
The patch of stat.h could be split into two parts to discuss.
Firstly,  STAT64_HAS_BROKEN_ST_INO is defined in most architecture's asm/stat.h,
and it should be considered as the part in asm-generic/stat.h.
Secondly, STAT64_PAD_BEFORE_* are misunderstanding definitions, and perhaps
it should use STAT64_ST_SIZE_NEED_ALIGN_64.
Indeed, the macros are used for compatibility, but most architectures could make full use of
asm-generic headers, and new architectures could just follow the default values.

Guan Xuetao

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Sunday, January 09, 2011 8:28 AM
> To: Guan Xuetao
> Cc: linux-arch@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures
> 
> On Saturday 08 January 2011, Guan Xuetao wrote:
> > From: Guan Xuetao <guanxuetao@mprc.pku.edu.cn>
> >
> >  This patch modifies stat.h in include/asm-generic to be applicable to more architectures.
> >  STAT64_HAS_BROKEN_ST_INO is defined in most architecture's asm/stat.h, and it need
> >  32-bit __st_ino member to be defined in different position of 64-bit st_ino member.
> >  STAT64_PAD_BEFORE_ST_SIZE is the pad before st_size member, with default value 8 bytes.
> >  STAT64_PAD_BEFORE_ST_BLOCKS is the pad before st_blocks member, with default value
> >  4 bytes to align the following member to 64-bit.
> 
> I'd prefer not to apply this patch. It makes the generic header
> significantly more complex, and I can't see a significant benefit.
> 
> The existing architectures would all still have to define the macros
> you test and also keep defining stuff like __old_kernel_stat, while
> risking to introduce bugs while changing to the common header. We've
> done similar tricks in other places, where the differences between
> architectures are smaller, but this one doesn't seem worth it
> unless we can get to the point where we can define struct stat in
> linux/stat.h for everyone and only do the macros for the architectures
> that need it.
> 
> 	Arnd

Re: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[Arnd Bergmann]

On Monday 10 January 2011, Guan Xuetao wrote:
> IMO, asm-generic headers should be used in more architectures as far as possible.

They serve two purposes:

1. To be used by other architectures (with or without being modified by arch
   specific defines)
2. As an example implementation to read by people understanding the kernel or
   porting to a new architecture.

Your patch helps the first cause, but it's counterproductive for the second.

Also, if we wanted to make all these headers fully generic, we would no longer
need an asm-generic directory for the first use case, because then everything
in them could be put into the include/linux/*.h headers directly. This has
happened for a number of files in the past, but it's impractical for others.

> The patch of stat.h could be split into two parts to discuss.
> Firstly,  STAT64_HAS_BROKEN_ST_INO is defined in most architecture's asm/stat.h,
> and it should be considered as the part in asm-generic/stat.h.

This one is purely historical. The definition of st_ino was too short
initially, so the field was kept in place by renamed to __st_ino
instead. The __st_ino should however never be used by any application
you compile now, only by the kernel in order to remain compatible with
existing application binaries. No new architecture should need to see
this, because there are no preexisting binaries that use this field.

I intentionally left out all the backwards-compatibility fields from
the asm-generic headers that I wrote.

> Secondly, STAT64_PAD_BEFORE_* are misunderstanding definitions, and perhaps
> it should use STAT64_ST_SIZE_NEED_ALIGN_64.

It's much more complicated ;-)

The padding after st_rdev is usually from glibc trying to reserve a large
amount of space for future extensions of rdev. Padding around st_blocks
and st_size is usually for making it possible to extend those values
to full 64 bit when they are not already, but some architectures use
the padding just to provide natural alignment of the words.

Almost every architecture has a slightly different definition of
stat64, and most of them got at least one aspect slightly wrong.

The mistake that I made in the asm-generic version was that it uses 32 bit
st_*time, which should really be 64 bit in order to avoid the year 2038
overflow bug.

> Indeed, the macros are used for compatibility, but most architectures could make full use of
> asm-generic headers, and new architectures could just follow the default values.

I totally agree with the idea in generical (for other headers), but in case of the
stat.h file, it always gets more complicated than you think at first. 
David Howells worked on a new 'struct xstat' for some time, if anything we should
just do that and keep struct stat hidden in the dark corners of the kernel.

	Arnd

RE: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[Guan Xuetao]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, January 11, 2011 12:41 PM
> To: Guan Xuetao; David Howells
> Cc: linux-arch@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures
> 
> On Monday 10 January 2011, Guan Xuetao wrote:
> > IMO, asm-generic headers should be used in more architectures as far as possible.
> 
> They serve two purposes:
> 
> 1. To be used by other architectures (with or without being modified by arch
>    specific defines)
> 2. As an example implementation to read by people understanding the kernel or
>    porting to a new architecture.
> 
> Your patch helps the first cause, but it's counterproductive for the second.
> 
> Also, if we wanted to make all these headers fully generic, we would no longer
> need an asm-generic directory for the first use case, because then everything
> in them could be put into the include/linux/*.h headers directly. This has
> happened for a number of files in the past, but it's impractical for others.
> 
> > The patch of stat.h could be split into two parts to discuss.
> > Firstly,  STAT64_HAS_BROKEN_ST_INO is defined in most architecture's asm/stat.h,
> > and it should be considered as the part in asm-generic/stat.h.
> 
> This one is purely historical. The definition of st_ino was too short
> initially, so the field was kept in place by renamed to __st_ino
> instead. The __st_ino should however never be used by any application
> you compile now, only by the kernel in order to remain compatible with
> existing application binaries. No new architecture should need to see
> this, because there are no preexisting binaries that use this field.
> 
> I intentionally left out all the backwards-compatibility fields from
> the asm-generic headers that I wrote.
> 
> > Secondly, STAT64_PAD_BEFORE_* are misunderstanding definitions, and perhaps
> > it should use STAT64_ST_SIZE_NEED_ALIGN_64.
> 
> It's much more complicated ;-)
> 
> The padding after st_rdev is usually from glibc trying to reserve a large
> amount of space for future extensions of rdev. Padding around st_blocks
> and st_size is usually for making it possible to extend those values
> to full 64 bit when they are not already, but some architectures use
> the padding just to provide natural alignment of the words.
> 
> Almost every architecture has a slightly different definition of
> stat64, and most of them got at least one aspect slightly wrong.
> 
> The mistake that I made in the asm-generic version was that it uses 32 bit
> st_*time, which should really be 64 bit in order to avoid the year 2038
> overflow bug.
> 
> > Indeed, the macros are used for compatibility, but most architectures could make full use of
> > asm-generic headers, and new architectures could just follow the default values.
> 
> I totally agree with the idea in generical (for other headers), but in case of the
> stat.h file, it always gets more complicated than you think at first.
> David Howells worked on a new 'struct xstat' for some time, if anything we should
> just do that and keep struct stat hidden in the dark corners of the kernel.
> 
> 	Arnd

Ok, I see.
Thanks.

Guan Xuetao

Re: [PATCH] asm-generic headers: modify stat.h in include/asm-generic to be applicable to more architectures

[Chris Metcalf]

On 1/10/2011 8:58 AM, Guan Xuetao wrote:
> IMO, asm-generic headers should be used in more architectures as far as possible.
> The patch of stat.h could be split into two parts to discuss.
> [...]
> Secondly, STAT64_PAD_BEFORE_* are misunderstanding definitions, and perhaps
> it should use STAT64_ST_SIZE_NEED_ALIGN_64.
> Indeed, the macros are used for compatibility, but most architectures could make full use of
> asm-generic headers, and new architectures could just follow the default values.

Other than the issue with 32-bit time_t (as mentioned by Arnd elsewhere in
this thread) the Tilera architecture happily uses the existing
<asm-generic/stat.h>.  I have a proposed "generic kernel ABI" version of
<bits/stat.h> for glibc which includes the appended code.  This is a little
peculiar but essentially allows all the permutations of 64-bit, old-school
32-bit, and 32-bit with __USE_FILE_OFFSET64 to work correctly, taking into
account the endianness of the platform, and allows glibc to have equivalent
definitions of its "kernel stat" and "user stat" structures, thus avoiding
having to do a field-by-field copy.

[...]
#if defined(__USE_FILE_OFFSET64)
# define __field64(type,type64,name,n) type64 name
#elif __WORDSIZE == 64
# define __field64(type,type64,name,n) type name
#elif __BYTE_ORDER == __LITTLE_ENDIAN
# define __field64(type,type64,name,n) \
  type name __attribute__((aligned(8))); int __pad##n
#else
# define __field64(type,type64,name,n) \
  int __pad##n __attribute__((aligned(8))); type name
#endif

struct stat
  {
    __dev_t st_dev;             /* Device.  */
    __field64(__ino_t, __ino64_t, st_ino, 0);  /* File serial number. */
    __mode_t st_mode;           /* File mode.  */
    __nlink_t st_nlink;         /* Link count.  */
    __uid_t st_uid;             /* User ID of the file's owner. */
    __gid_t st_gid;             /* Group ID of the file's group.*/
    __dev_t st_rdev;            /* Device number, if device.  */
    __dev_t __pad1;
    __field64(__off_t, __off64_t, st_size, 2);  /* Size of file. */
    __blksize_t st_blksize;     /* Optimal block size for I/O.  */
    int __pad3;
    __field64(__blkcnt_t, __blkcnt64_t, st_blocks, 4);  /* Blocks */
#ifdef __USE_MISC
    /* Nanosecond resolution timestamps are stored in a format
       equivalent to 'struct timespec'.  This is the type used
       whenever possible but the Unix namespace rules do not allow the
       identifier 'timespec' to appear in the <sys/stat.h> header.
       Therefore we have to handle the use of this header in strictly
       standard-compliant sources special.  */
    struct timespec st_atim;            /* Time of last access.  */
    struct timespec st_mtim;            /* Time of last modification.  */
    struct timespec st_ctim;            /* Time of last status change.  */
# define st_atime st_atim.tv_sec        /* Backward compatibility.  */
# define st_mtime st_mtim.tv_sec
# define st_ctime st_ctim.tv_sec
#else
    __time_t st_atime;                  /* Time of last access.  */
    unsigned long int st_atimensec;     /* Nscecs of last access.  */
    __time_t st_mtime;                  /* Time of last modification.  */
    unsigned long int st_mtimensec;     /* Nsecs of last modification.  */
    __time_t st_ctime;                  /* Time of last status change.  */
    unsigned long int st_ctimensec;     /* Nsecs of last status change.  */
#endif
    int __unused[2];
  };

#undef __field64
[...]

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com