Message ID | 20230905203421.2127750-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | io: Do not implement fstat with fstatat | expand |
On 9/5/23, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > AT_EMPTY_PATH is a requirement to implement fstat over fstatat, > however it does not prevent the kernel to read the path argument. > It is not an issue, but it has performance implications on x86_64 > with SMAP enable, since each newfstatat syscall generates a pagefault > even with an empty path. > This does not generate page faults in the usual case. How about: On x86-64 with SMAP-capable CPUs (vast majority today) the kernel is forced to perform expensive user memory access. After that regular lookup is performed which adds even more overhead. I have no commentso n the patch itself. Thanks for quick turn around. > Instead, issue the fstat syscall directly on LFS fstat implementation > (32 bit architectures will still continue to use statx, which is > required to have 64 bit time_t support). it should be even a > small performance gain on non x86_64, since there is no need > to handle the path argument. > > Checked on x86_64-linux-gnu. > --- > sysdeps/unix/sysv/linux/fstat64.c | 37 +++++++++++++++++++++++-- > sysdeps/unix/sysv/linux/fstatat64.c | 12 ++------ > sysdeps/unix/sysv/linux/internal-stat.h | 31 +++++++++++++++++++++ > 3 files changed, 68 insertions(+), 12 deletions(-) > create mode 100644 sysdeps/unix/sysv/linux/internal-stat.h > > diff --git a/sysdeps/unix/sysv/linux/fstat64.c > b/sysdeps/unix/sysv/linux/fstat64.c > index 124384e57f..a7f4935a01 100644 > --- a/sysdeps/unix/sysv/linux/fstat64.c > +++ b/sysdeps/unix/sysv/linux/fstat64.c > @@ -19,20 +19,53 @@ > #define __fstat __redirect___fstat > #define fstat __redirect_fstat > #include <sys/stat.h> > +#undef __fstat > +#undef fstat > #include <fcntl.h> > -#include <kernel_stat.h> > -#include <stat_t64_cp.h> > +#include <internal-stat.h> > #include <errno.h> > > int > __fstat64_time64 (int fd, struct __stat64_t64 *buf) > { > +#if !FSTATAT_USE_STATX > +# if XSTAT_IS_XSTAT64 > +# ifdef __NR_newfstatat > + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and > + x86_64. */ > + return INLINE_SYSCALL_CALL (fstat, fd, buf); > +# elif defined __NR_fstatat64 > +# if STAT64_IS_KERNEL_STAT64 > + /* 64-bit kABI outlier, e.g. alpha */ > + return INLINE_SYSCALL_CALL (fstat64, fd, buf); > +# else > + /* 64-bit kABI outlier, e.g. sparc64. */ > + struct kernel_stat64 kst64; > + int r = INLINE_SYSCALL_CALL (fstat64, fd, &kst64); > + if (r == 0) > + __cp_stat64_kstat64 (buf, &kst64); > + return r; > +# endif /* STAT64_IS_KERNEL_STAT64 */ > +# endif > +# else /* XSTAT_IS_XSTAT64 */ > + /* 64-bit kabi outlier, e.g. mips64 and mips64-n32. */ > + struct kernel_stat kst; > + int r = INLINE_SYSCALL_CALL (fstat, fd, &kst); > + if (r == 0) > + __cp_kstat_stat64_t64 (&kst, buf); > + return r; > +# endif > +#else /* !FSTATAT_USE_STATX */ > + /* All kABIs with non-LFS support and with old 32-bit time_t support > + e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, powerpc32, > + and sparc32. */ > if (fd < 0) > { > __set_errno (EBADF); > return -1; > } > return __fstatat64_time64 (fd, "", buf, AT_EMPTY_PATH); > +#endif > } > #if __TIMESIZE != 64 > hidden_def (__fstat64_time64) > diff --git a/sysdeps/unix/sysv/linux/fstatat64.c > b/sysdeps/unix/sysv/linux/fstatat64.c > index 3509d3ca6d..127c6ff601 100644 > --- a/sysdeps/unix/sysv/linux/fstatat64.c > +++ b/sysdeps/unix/sysv/linux/fstatat64.c > @@ -21,12 +21,10 @@ > #include <sys/stat.h> > #include <fcntl.h> > #include <string.h> > -#include <kernel_stat.h> > #include <sysdep.h> > #include <time.h> > -#include <kstat_cp.h> > -#include <stat_t64_cp.h> > #include <sys/sysmacros.h> > +#include <internal-stat.h> > > #if __TIMESIZE == 64 \ > && (__WORDSIZE == 32 \ > @@ -40,11 +38,7 @@ _Static_assert (sizeof (__blkcnt_t) == sizeof > (__blkcnt64_t), > "__blkcnt_t and __blkcnt64_t must match"); > #endif > > -#if (__WORDSIZE == 32 \ > - && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ > - || defined STAT_HAS_TIME32 \ > - || (!defined __NR_newfstatat && !defined __NR_fstatat64) > -# define FSTATAT_USE_STATX 1 > +#if FSTATAT_USE_STATX > > static inline int > fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 > *buf, > @@ -79,8 +73,6 @@ fstatat64_time64_statx (int fd, const char *file, struct > __stat64_t64 *buf, > > return r; > } > -#else > -# define FSTATAT_USE_STATX 0 > #endif > > /* Only statx supports 64-bit timestamps for 32-bit architectures with > diff --git a/sysdeps/unix/sysv/linux/internal-stat.h > b/sysdeps/unix/sysv/linux/internal-stat.h > new file mode 100644 > index 0000000000..e3b0569853 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/internal-stat.h > @@ -0,0 +1,31 @@ > +/* Internal stat definitions. > + Copyright (C) 2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <sysdep.h> > +#include <stat_t64_cp.h> > +#include <kernel_stat.h> > +#include <kstat_cp.h> > + > +#if (__WORDSIZE == 32 \ > + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ > + || defined STAT_HAS_TIME32 \ > + || (!defined __NR_newfstatat && !defined __NR_fstatat64) > +# define FSTATAT_USE_STATX 1 > +#else > +# define FSTATAT_USE_STATX 0 > +#endif > -- > 2.34.1 > >
On 05/09/23 17:48, Mateusz Guzik wrote: > On 9/5/23, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >> AT_EMPTY_PATH is a requirement to implement fstat over fstatat, >> however it does not prevent the kernel to read the path argument. >> It is not an issue, but it has performance implications on x86_64 >> with SMAP enable, since each newfstatat syscall generates a pagefault >> even with an empty path. >> > > This does not generate page faults in the usual case. > > How about: > > On x86-64 with SMAP-capable CPUs (vast majority today) the kernel is > forced to perform expensive user memory access. After that regular > lookup is performed which adds even more overhead. Alright, I have changed to: AT_EMPTY_PATH is a requirement to implement fstat over fstatat, however it does not prevent the kernel to read the path argument. It is not an issue, but on x86-64 with SMAP-capable CPUs the kernel is forced to perform expensive user memory access. After that regular lookup is performed which adds even more overhead. > > I have no commentso n the patch itself. > > Thanks for quick turn around. > > >> Instead, issue the fstat syscall directly on LFS fstat implementation >> (32 bit architectures will still continue to use statx, which is >> required to have 64 bit time_t support). it should be even a >> small performance gain on non x86_64, since there is no need >> to handle the path argument. >> >> Checked on x86_64-linux-gnu. >> --- >> sysdeps/unix/sysv/linux/fstat64.c | 37 +++++++++++++++++++++++-- >> sysdeps/unix/sysv/linux/fstatat64.c | 12 ++------ >> sysdeps/unix/sysv/linux/internal-stat.h | 31 +++++++++++++++++++++ >> 3 files changed, 68 insertions(+), 12 deletions(-) >> create mode 100644 sysdeps/unix/sysv/linux/internal-stat.h >> >> diff --git a/sysdeps/unix/sysv/linux/fstat64.c >> b/sysdeps/unix/sysv/linux/fstat64.c >> index 124384e57f..a7f4935a01 100644 >> --- a/sysdeps/unix/sysv/linux/fstat64.c >> +++ b/sysdeps/unix/sysv/linux/fstat64.c >> @@ -19,20 +19,53 @@ >> #define __fstat __redirect___fstat >> #define fstat __redirect_fstat >> #include <sys/stat.h> >> +#undef __fstat >> +#undef fstat >> #include <fcntl.h> >> -#include <kernel_stat.h> >> -#include <stat_t64_cp.h> >> +#include <internal-stat.h> >> #include <errno.h> >> >> int >> __fstat64_time64 (int fd, struct __stat64_t64 *buf) >> { >> +#if !FSTATAT_USE_STATX >> +# if XSTAT_IS_XSTAT64 >> +# ifdef __NR_newfstatat >> + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and >> + x86_64. */ >> + return INLINE_SYSCALL_CALL (fstat, fd, buf); >> +# elif defined __NR_fstatat64 >> +# if STAT64_IS_KERNEL_STAT64 >> + /* 64-bit kABI outlier, e.g. alpha */ >> + return INLINE_SYSCALL_CALL (fstat64, fd, buf); >> +# else >> + /* 64-bit kABI outlier, e.g. sparc64. */ >> + struct kernel_stat64 kst64; >> + int r = INLINE_SYSCALL_CALL (fstat64, fd, &kst64); >> + if (r == 0) >> + __cp_stat64_kstat64 (buf, &kst64); >> + return r; >> +# endif /* STAT64_IS_KERNEL_STAT64 */ >> +# endif >> +# else /* XSTAT_IS_XSTAT64 */ >> + /* 64-bit kabi outlier, e.g. mips64 and mips64-n32. */ >> + struct kernel_stat kst; >> + int r = INLINE_SYSCALL_CALL (fstat, fd, &kst); >> + if (r == 0) >> + __cp_kstat_stat64_t64 (&kst, buf); >> + return r; >> +# endif >> +#else /* !FSTATAT_USE_STATX */ >> + /* All kABIs with non-LFS support and with old 32-bit time_t support >> + e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, powerpc32, >> + and sparc32. */ >> if (fd < 0) >> { >> __set_errno (EBADF); >> return -1; >> } >> return __fstatat64_time64 (fd, "", buf, AT_EMPTY_PATH); >> +#endif >> } >> #if __TIMESIZE != 64 >> hidden_def (__fstat64_time64) >> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c >> b/sysdeps/unix/sysv/linux/fstatat64.c >> index 3509d3ca6d..127c6ff601 100644 >> --- a/sysdeps/unix/sysv/linux/fstatat64.c >> +++ b/sysdeps/unix/sysv/linux/fstatat64.c >> @@ -21,12 +21,10 @@ >> #include <sys/stat.h> >> #include <fcntl.h> >> #include <string.h> >> -#include <kernel_stat.h> >> #include <sysdep.h> >> #include <time.h> >> -#include <kstat_cp.h> >> -#include <stat_t64_cp.h> >> #include <sys/sysmacros.h> >> +#include <internal-stat.h> >> >> #if __TIMESIZE == 64 \ >> && (__WORDSIZE == 32 \ >> @@ -40,11 +38,7 @@ _Static_assert (sizeof (__blkcnt_t) == sizeof >> (__blkcnt64_t), >> "__blkcnt_t and __blkcnt64_t must match"); >> #endif >> >> -#if (__WORDSIZE == 32 \ >> - && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ >> - || defined STAT_HAS_TIME32 \ >> - || (!defined __NR_newfstatat && !defined __NR_fstatat64) >> -# define FSTATAT_USE_STATX 1 >> +#if FSTATAT_USE_STATX >> >> static inline int >> fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 >> *buf, >> @@ -79,8 +73,6 @@ fstatat64_time64_statx (int fd, const char *file, struct >> __stat64_t64 *buf, >> >> return r; >> } >> -#else >> -# define FSTATAT_USE_STATX 0 >> #endif >> >> /* Only statx supports 64-bit timestamps for 32-bit architectures with >> diff --git a/sysdeps/unix/sysv/linux/internal-stat.h >> b/sysdeps/unix/sysv/linux/internal-stat.h >> new file mode 100644 >> index 0000000000..e3b0569853 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/internal-stat.h >> @@ -0,0 +1,31 @@ >> +/* Internal stat definitions. >> + Copyright (C) 2023 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + <https://www.gnu.org/licenses/>. */ >> + >> +#include <sysdep.h> >> +#include <stat_t64_cp.h> >> +#include <kernel_stat.h> >> +#include <kstat_cp.h> >> + >> +#if (__WORDSIZE == 32 \ >> + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ >> + || defined STAT_HAS_TIME32 \ >> + || (!defined __NR_newfstatat && !defined __NR_fstatat64) >> +# define FSTATAT_USE_STATX 1 >> +#else >> +# define FSTATAT_USE_STATX 0 >> +#endif >> -- >> 2.34.1 >> >> > >
On 9/5/23, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > > On 05/09/23 17:48, Mateusz Guzik wrote: >> On 9/5/23, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >>> AT_EMPTY_PATH is a requirement to implement fstat over fstatat, >>> however it does not prevent the kernel to read the path argument. >>> It is not an issue, but it has performance implications on x86_64 >>> with SMAP enable, since each newfstatat syscall generates a pagefault >>> even with an empty path. >>> >> >> This does not generate page faults in the usual case. >> >> How about: >> >> On x86-64 with SMAP-capable CPUs (vast majority today) the kernel is >> forced to perform expensive user memory access. After that regular >> lookup is performed which adds even more overhead. > > Alright, I have changed to: > > AT_EMPTY_PATH is a requirement to implement fstat over fstatat, > however it does not prevent the kernel to read the path argument. > It is not an issue, but on x86-64 with SMAP-capable CPUs the kernel is > forced to perform expensive user memory access. After that regular > lookup is performed which adds even more overhead. > LGTM >> >> I have no commentso n the patch itself. >> >> Thanks for quick turn around. >> >> >>> Instead, issue the fstat syscall directly on LFS fstat implementation >>> (32 bit architectures will still continue to use statx, which is >>> required to have 64 bit time_t support). it should be even a >>> small performance gain on non x86_64, since there is no need >>> to handle the path argument. >>> >>> Checked on x86_64-linux-gnu. >>> --- >>> sysdeps/unix/sysv/linux/fstat64.c | 37 +++++++++++++++++++++++-- >>> sysdeps/unix/sysv/linux/fstatat64.c | 12 ++------ >>> sysdeps/unix/sysv/linux/internal-stat.h | 31 +++++++++++++++++++++ >>> 3 files changed, 68 insertions(+), 12 deletions(-) >>> create mode 100644 sysdeps/unix/sysv/linux/internal-stat.h >>> >>> diff --git a/sysdeps/unix/sysv/linux/fstat64.c >>> b/sysdeps/unix/sysv/linux/fstat64.c >>> index 124384e57f..a7f4935a01 100644 >>> --- a/sysdeps/unix/sysv/linux/fstat64.c >>> +++ b/sysdeps/unix/sysv/linux/fstat64.c >>> @@ -19,20 +19,53 @@ >>> #define __fstat __redirect___fstat >>> #define fstat __redirect_fstat >>> #include <sys/stat.h> >>> +#undef __fstat >>> +#undef fstat >>> #include <fcntl.h> >>> -#include <kernel_stat.h> >>> -#include <stat_t64_cp.h> >>> +#include <internal-stat.h> >>> #include <errno.h> >>> >>> int >>> __fstat64_time64 (int fd, struct __stat64_t64 *buf) >>> { >>> +#if !FSTATAT_USE_STATX >>> +# if XSTAT_IS_XSTAT64 >>> +# ifdef __NR_newfstatat >>> + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and >>> + x86_64. */ >>> + return INLINE_SYSCALL_CALL (fstat, fd, buf); >>> +# elif defined __NR_fstatat64 >>> +# if STAT64_IS_KERNEL_STAT64 >>> + /* 64-bit kABI outlier, e.g. alpha */ >>> + return INLINE_SYSCALL_CALL (fstat64, fd, buf); >>> +# else >>> + /* 64-bit kABI outlier, e.g. sparc64. */ >>> + struct kernel_stat64 kst64; >>> + int r = INLINE_SYSCALL_CALL (fstat64, fd, &kst64); >>> + if (r == 0) >>> + __cp_stat64_kstat64 (buf, &kst64); >>> + return r; >>> +# endif /* STAT64_IS_KERNEL_STAT64 */ >>> +# endif >>> +# else /* XSTAT_IS_XSTAT64 */ >>> + /* 64-bit kabi outlier, e.g. mips64 and mips64-n32. */ >>> + struct kernel_stat kst; >>> + int r = INLINE_SYSCALL_CALL (fstat, fd, &kst); >>> + if (r == 0) >>> + __cp_kstat_stat64_t64 (&kst, buf); >>> + return r; >>> +# endif >>> +#else /* !FSTATAT_USE_STATX */ >>> + /* All kABIs with non-LFS support and with old 32-bit time_t support >>> + e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, >>> powerpc32, >>> + and sparc32. */ >>> if (fd < 0) >>> { >>> __set_errno (EBADF); >>> return -1; >>> } >>> return __fstatat64_time64 (fd, "", buf, AT_EMPTY_PATH); >>> +#endif >>> } >>> #if __TIMESIZE != 64 >>> hidden_def (__fstat64_time64) >>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c >>> b/sysdeps/unix/sysv/linux/fstatat64.c >>> index 3509d3ca6d..127c6ff601 100644 >>> --- a/sysdeps/unix/sysv/linux/fstatat64.c >>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c >>> @@ -21,12 +21,10 @@ >>> #include <sys/stat.h> >>> #include <fcntl.h> >>> #include <string.h> >>> -#include <kernel_stat.h> >>> #include <sysdep.h> >>> #include <time.h> >>> -#include <kstat_cp.h> >>> -#include <stat_t64_cp.h> >>> #include <sys/sysmacros.h> >>> +#include <internal-stat.h> >>> >>> #if __TIMESIZE == 64 \ >>> && (__WORDSIZE == 32 \ >>> @@ -40,11 +38,7 @@ _Static_assert (sizeof (__blkcnt_t) == sizeof >>> (__blkcnt64_t), >>> "__blkcnt_t and __blkcnt64_t must match"); >>> #endif >>> >>> -#if (__WORDSIZE == 32 \ >>> - && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ >>> - || defined STAT_HAS_TIME32 \ >>> - || (!defined __NR_newfstatat && !defined __NR_fstatat64) >>> -# define FSTATAT_USE_STATX 1 >>> +#if FSTATAT_USE_STATX >>> >>> static inline int >>> fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 >>> *buf, >>> @@ -79,8 +73,6 @@ fstatat64_time64_statx (int fd, const char *file, >>> struct >>> __stat64_t64 *buf, >>> >>> return r; >>> } >>> -#else >>> -# define FSTATAT_USE_STATX 0 >>> #endif >>> >>> /* Only statx supports 64-bit timestamps for 32-bit architectures with >>> diff --git a/sysdeps/unix/sysv/linux/internal-stat.h >>> b/sysdeps/unix/sysv/linux/internal-stat.h >>> new file mode 100644 >>> index 0000000000..e3b0569853 >>> --- /dev/null >>> +++ b/sysdeps/unix/sysv/linux/internal-stat.h >>> @@ -0,0 +1,31 @@ >>> +/* Internal stat definitions. >>> + Copyright (C) 2023 Free Software Foundation, Inc. >>> + This file is part of the GNU C Library. >>> + >>> + The GNU C Library is free software; you can redistribute it and/or >>> + modify it under the terms of the GNU Lesser General Public >>> + License as published by the Free Software Foundation; either >>> + version 2.1 of the License, or (at your option) any later version. >>> + >>> + The GNU C Library is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + Lesser General Public License for more details. >>> + >>> + You should have received a copy of the GNU Lesser General Public >>> + License along with the GNU C Library; if not, see >>> + <https://www.gnu.org/licenses/>. */ >>> + >>> +#include <sysdep.h> >>> +#include <stat_t64_cp.h> >>> +#include <kernel_stat.h> >>> +#include <kstat_cp.h> >>> + >>> +#if (__WORDSIZE == 32 \ >>> + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ >>> + || defined STAT_HAS_TIME32 \ >>> + || (!defined __NR_newfstatat && !defined __NR_fstatat64) >>> +# define FSTATAT_USE_STATX 1 >>> +#else >>> +# define FSTATAT_USE_STATX 0 >>> +#endif >>> -- >>> 2.34.1 >>> >>> >> >> >
* Adhemerval Zanella via Libc-alpha: > diff --git a/sysdeps/unix/sysv/linux/fstat64.c b/sysdeps/unix/sysv/linux/fstat64.c > index 124384e57f..a7f4935a01 100644 > --- a/sysdeps/unix/sysv/linux/fstat64.c > +++ b/sysdeps/unix/sysv/linux/fstat64.c > int > __fstat64_time64 (int fd, struct __stat64_t64 *buf) > { > +#if !FSTATAT_USE_STATX > +# if XSTAT_IS_XSTAT64 > +# ifdef __NR_newfstatat > + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and > + x86_64. */ > + return INLINE_SYSCALL_CALL (fstat, fd, buf); It's a bit suspicious to check for __NR_newfstatat an then use fstat. Could you add a comment to explain this? Surprisingly it compiles on all architectures. Thanks, Florian
On 06/09/23 17:48, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/sysdeps/unix/sysv/linux/fstat64.c b/sysdeps/unix/sysv/linux/fstat64.c >> index 124384e57f..a7f4935a01 100644 >> --- a/sysdeps/unix/sysv/linux/fstat64.c >> +++ b/sysdeps/unix/sysv/linux/fstat64.c > >> int >> __fstat64_time64 (int fd, struct __stat64_t64 *buf) >> { >> +#if !FSTATAT_USE_STATX >> +# if XSTAT_IS_XSTAT64 >> +# ifdef __NR_newfstatat >> + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and >> + x86_64. */ >> + return INLINE_SYSCALL_CALL (fstat, fd, buf); > > It's a bit suspicious to check for __NR_newfstatat an then use fstat. > Could you add a comment to explain this? > > Surprisingly it compiles on all architectures. I used the same fstatat logic, but using __NR_fstat should be fine. It build fine because for the affected architectures, newfstatat was added for archs that already supported fstat. > > Thanks, > Florian >
On Mon, 11 Sept 2023 at 06:11, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > I used the same fstatat logic, but using __NR_fstat should be fine. I think you should keep using the same logic as in fstatat(). Using "#ifdef __NR_newfstatat" basically checks for "not stat64". So, for example, x86-64 (and x64) have __NR_newfstatat, but 32-bit i386 has __NR_fstatat64. Now, maybe the other tests effectively already capture this (ie I suspect FSTATAT_USE_STATX may already be the thing that makes 32-bit i386 different), but I do think it's actually better the way it was. ... except maybe a comment somewhere? And maybe it might be good to actually make this "struct stat64" vs "struct stat" more obvious. Linus
On 11/09/23 16:32, Linus Torvalds wrote: > On Mon, 11 Sept 2023 at 06:11, Adhemerval Zanella Netto > <adhemerval.zanella@linaro.org> wrote: >> >> I used the same fstatat logic, but using __NR_fstat should be fine. > > I think you should keep using the same logic as in fstatat(). > > Using "#ifdef __NR_newfstatat" basically checks for "not stat64". > > So, for example, x86-64 (and x64) have __NR_newfstatat, but 32-bit > i386 has __NR_fstatat64. > > Now, maybe the other tests effectively already capture this (ie I > suspect FSTATAT_USE_STATX may already be the thing that makes 32-bit > i386 different), but I do think it's actually better the way it was. The FSTATAT_USE_STATX already handles it, and there is a explicit comment later at !FSTATAT_USE_STATX for which ABIs are affected regarding glibc support. So either way (check __NR_newfstat and __NR_fstat) should be ok. > > ... except maybe a comment somewhere? And maybe it might be good to > actually make this "struct stat64" vs "struct stat" more obvious. > > Linus
* Adhemerval Zanella Netto: > On 11/09/23 16:32, Linus Torvalds wrote: >> On Mon, 11 Sept 2023 at 06:11, Adhemerval Zanella Netto >> <adhemerval.zanella@linaro.org> wrote: >>> >>> I used the same fstatat logic, but using __NR_fstat should be fine. >> >> I think you should keep using the same logic as in fstatat(). >> >> Using "#ifdef __NR_newfstatat" basically checks for "not stat64". >> >> So, for example, x86-64 (and x64) have __NR_newfstatat, but 32-bit >> i386 has __NR_fstatat64. >> >> Now, maybe the other tests effectively already capture this (ie I >> suspect FSTATAT_USE_STATX may already be the thing that makes 32-bit >> i386 different), but I do think it's actually better the way it was. > > The FSTATAT_USE_STATX already handles it, and there is a explicit comment > later at !FSTATAT_USE_STATX for which ABIs are affected regarding glibc > support. So either way (check __NR_newfstat and __NR_fstat) should be > ok. If __NR_newfstatat is clear to the subject matter experts, I won't object to it. But please add a comment. Thanks, Florian
On 12/09/23 02:31, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 11/09/23 16:32, Linus Torvalds wrote: >>> On Mon, 11 Sept 2023 at 06:11, Adhemerval Zanella Netto >>> <adhemerval.zanella@linaro.org> wrote: >>>> >>>> I used the same fstatat logic, but using __NR_fstat should be fine. >>> >>> I think you should keep using the same logic as in fstatat(). >>> >>> Using "#ifdef __NR_newfstatat" basically checks for "not stat64". >>> >>> So, for example, x86-64 (and x64) have __NR_newfstatat, but 32-bit >>> i386 has __NR_fstatat64. >>> >>> Now, maybe the other tests effectively already capture this (ie I >>> suspect FSTATAT_USE_STATX may already be the thing that makes 32-bit >>> i386 different), but I do think it's actually better the way it was. >> >> The FSTATAT_USE_STATX already handles it, and there is a explicit comment >> later at !FSTATAT_USE_STATX for which ABIs are affected regarding glibc >> support. So either way (check __NR_newfstat and __NR_fstat) should be >> ok. > > If __NR_newfstatat is clear to the subject matter experts, I won't > object to it. But please add a comment. I sent a newer version that checks for __NR_stat instead, with comments to which ABI selects what [1]. [1] https://patchwork.sourceware.org/project/glibc/patch/20230911132548.1981093-1-adhemerval.zanella@linaro.org/
diff --git a/sysdeps/unix/sysv/linux/fstat64.c b/sysdeps/unix/sysv/linux/fstat64.c index 124384e57f..a7f4935a01 100644 --- a/sysdeps/unix/sysv/linux/fstat64.c +++ b/sysdeps/unix/sysv/linux/fstat64.c @@ -19,20 +19,53 @@ #define __fstat __redirect___fstat #define fstat __redirect_fstat #include <sys/stat.h> +#undef __fstat +#undef fstat #include <fcntl.h> -#include <kernel_stat.h> -#include <stat_t64_cp.h> +#include <internal-stat.h> #include <errno.h> int __fstat64_time64 (int fd, struct __stat64_t64 *buf) { +#if !FSTATAT_USE_STATX +# if XSTAT_IS_XSTAT64 +# ifdef __NR_newfstatat + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and + x86_64. */ + return INLINE_SYSCALL_CALL (fstat, fd, buf); +# elif defined __NR_fstatat64 +# if STAT64_IS_KERNEL_STAT64 + /* 64-bit kABI outlier, e.g. alpha */ + return INLINE_SYSCALL_CALL (fstat64, fd, buf); +# else + /* 64-bit kABI outlier, e.g. sparc64. */ + struct kernel_stat64 kst64; + int r = INLINE_SYSCALL_CALL (fstat64, fd, &kst64); + if (r == 0) + __cp_stat64_kstat64 (buf, &kst64); + return r; +# endif /* STAT64_IS_KERNEL_STAT64 */ +# endif +# else /* XSTAT_IS_XSTAT64 */ + /* 64-bit kabi outlier, e.g. mips64 and mips64-n32. */ + struct kernel_stat kst; + int r = INLINE_SYSCALL_CALL (fstat, fd, &kst); + if (r == 0) + __cp_kstat_stat64_t64 (&kst, buf); + return r; +# endif +#else /* !FSTATAT_USE_STATX */ + /* All kABIs with non-LFS support and with old 32-bit time_t support + e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, powerpc32, + and sparc32. */ if (fd < 0) { __set_errno (EBADF); return -1; } return __fstatat64_time64 (fd, "", buf, AT_EMPTY_PATH); +#endif } #if __TIMESIZE != 64 hidden_def (__fstat64_time64) diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c index 3509d3ca6d..127c6ff601 100644 --- a/sysdeps/unix/sysv/linux/fstatat64.c +++ b/sysdeps/unix/sysv/linux/fstatat64.c @@ -21,12 +21,10 @@ #include <sys/stat.h> #include <fcntl.h> #include <string.h> -#include <kernel_stat.h> #include <sysdep.h> #include <time.h> -#include <kstat_cp.h> -#include <stat_t64_cp.h> #include <sys/sysmacros.h> +#include <internal-stat.h> #if __TIMESIZE == 64 \ && (__WORDSIZE == 32 \ @@ -40,11 +38,7 @@ _Static_assert (sizeof (__blkcnt_t) == sizeof (__blkcnt64_t), "__blkcnt_t and __blkcnt64_t must match"); #endif -#if (__WORDSIZE == 32 \ - && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ - || defined STAT_HAS_TIME32 \ - || (!defined __NR_newfstatat && !defined __NR_fstatat64) -# define FSTATAT_USE_STATX 1 +#if FSTATAT_USE_STATX static inline int fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, @@ -79,8 +73,6 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, return r; } -#else -# define FSTATAT_USE_STATX 0 #endif /* Only statx supports 64-bit timestamps for 32-bit architectures with diff --git a/sysdeps/unix/sysv/linux/internal-stat.h b/sysdeps/unix/sysv/linux/internal-stat.h new file mode 100644 index 0000000000..e3b0569853 --- /dev/null +++ b/sysdeps/unix/sysv/linux/internal-stat.h @@ -0,0 +1,31 @@ +/* Internal stat definitions. + Copyright (C) 2023 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <sysdep.h> +#include <stat_t64_cp.h> +#include <kernel_stat.h> +#include <kstat_cp.h> + +#if (__WORDSIZE == 32 \ + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ + || defined STAT_HAS_TIME32 \ + || (!defined __NR_newfstatat && !defined __NR_fstatat64) +# define FSTATAT_USE_STATX 1 +#else +# define FSTATAT_USE_STATX 0 +#endif