| Message ID | 20240820-loong-fstat-v2-1-5a254fef05ce@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Loongarch: adapt for the re-introduction of fstat and newfstatat in 6.11 | expand |
On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: > From: Miao Wang <shankerwangmiao@gmail.com> > > In Linux 6.11, fstat and newfstatat are added back. We need to include > this change in kernel-features.h to avoid producing libraries > incompatible with previous linux versions with new headers. > > The definition of the two syscalls will be removed when the targeted > kernel version is below 6.11 in loongarch/sysdep.h. > > Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> You should resolve the previous review comments before sending a new version. If you don't agree with 6.10.6 then fine to leave the code unchanged but you need to explain the reason in the commit message. And to me "I think the backport is questionable" isn't a good reason (i.e. we need the reason it's questionable, not just "it's questionable"). And before you start to say "downstream distros may think the backport is questionable and revert it," no. Glibc does not support non-Linus trees. If you must patch the kernel in some way Glibc cannot expect it's your own job to fix up your Glibc fork as well. > --- > Kernel 6.11 adds back fstat and newfstatat in commit 7697a0fe0154 > ("LoongArch: Define __ARCH_WANT_NEW_STAT in unistd.h"). With kernel > headers from 6.11, make update-syscall-lists will generate the > following > diffs: > > > diff --git a/sysdeps/unix/sysv/linux/loongarch/arch-syscall.h > > b/sysdeps/unix/sysv/linux/loongarch/arch-syscall.h > > index 8bb82448a7..7e732256fd 100644 > > --- a/sysdeps/unix/sysv/linux/loongarch/arch-syscall.h > > +++ b/sysdeps/unix/sysv/linux/loongarch/arch-syscall.h > > @@ -59,6 +59,7 @@ > > #define __NR_fsmount 432 > > #define __NR_fsopen 430 > > #define __NR_fspick 433 > > +#define __NR_fstat 80 > > #define __NR_fstatfs 44 > > #define __NR_fsync 82 > > #define __NR_ftruncate 46 > > @@ -166,6 +167,7 @@ > > #define __NR_munmap 215 > > #define __NR_name_to_handle_at 264 > > #define __NR_nanosleep 101 > > +#define __NR_newfstatat 79 > > #define __NR_nfsservctl 42 > > #define __NR_open_by_handle_at 265 > > #define __NR_open_tree 428 > > With that changed, the compiled libraries will unexpectedly include > calls to these two syscalls, and be incompatible with previous kernel > versions. > > This patch addresses this issue by removing the two definitions if the > targeted kernel version is below 6.11. I have tested this patch along > with the change to arch-syscall.h with or without the configuration > option --enable-kernel=6.11. > --- > Changes in v2: > - Improve commit message for better patch handling > - Link to v1: > https://sourceware.org/pipermail/libc-alpha/2024-August/159295.html > --- > .../unix/sysv/linux/loongarch/kernel-features.h | 27 > ++++++++++++++++++++++ > sysdeps/unix/sysv/linux/loongarch/sysdep.h | 5 ++++ > 2 files changed, 32 insertions(+) > > diff --git a/sysdeps/unix/sysv/linux/loongarch/kernel-features.h > b/sysdeps/unix/sysv/linux/loongarch/kernel-features.h > new file mode 100644 > index 0000000000..c0e7ccb5de > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/loongarch/kernel-features.h > @@ -0,0 +1,27 @@ > +/* Set flags signalling availability of kernel features based on > given > + kernel version number. Loongarch version. > + Copyright (C) 2024-2024 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_next <kernel-features.h> > + > +#define __ASSUME_LOONGARCH_NEWSTAT 1 > + > +/* No support for fstat or newfstatat before 6.11. */ > +#if __LINUX_KERNEL_VERSION < 0x060b00 > +# undef __ASSUME_LOONGARCH_NEWSTAT > +#endif > diff --git a/sysdeps/unix/sysv/linux/loongarch/sysdep.h > b/sysdeps/unix/sysv/linux/loongarch/sysdep.h > index eb0ba790da..1fdf18197f 100644 > --- a/sysdeps/unix/sysv/linux/loongarch/sysdep.h > +++ b/sysdeps/unix/sysv/linux/loongarch/sysdep.h > @@ -109,6 +109,11 @@ > #undef SYS_ify > #define SYS_ify(syscall_name) __NR_##syscall_name > > +#ifndef __ASSUME_LOONGARCH_NEWSTAT > +#undef __NR_fstat > +#undef __NR_newfstatat > +#endif > + > #ifndef __ASSEMBLER__ > > #define VDSO_NAME "LINUX_5.10" > > --- > base-commit: 2eee835eca960c9d4119279804214b7a1ed5d156 > change-id: 20240820-loong-fstat-f57f8be48575 > > Best regards,
On Tue, 2024-08-20 at 23:06 +0800, Xi Ruoyao wrote: > On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: > > From: Miao Wang <shankerwangmiao@gmail.com> > > > > In Linux 6.11, fstat and newfstatat are added back. We need to include > > this change in kernel-features.h to avoid producing libraries > > incompatible with previous linux versions with new headers. > > > > The definition of the two syscalls will be removed when the targeted > > kernel version is below 6.11 in loongarch/sysdep.h. > > > > Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> > > You should resolve the previous review comments before sending a new > version. If you don't agree with 6.10.6 then fine to leave the code > unchanged but you need to explain the reason in the commit message. And > to me "I think the backport is questionable" isn't a good reason (i.e. > we need the reason it's questionable, not just "it's questionable"). Actually "the reason it's *wrong*," not just "questionable." Everything can be questionable.
Hi, > 2024年8月20日 23:11，Xi Ruoyao <xry111@xry111.site> 写道： > > On Tue, 2024-08-20 at 23:06 +0800, Xi Ruoyao wrote: >> On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: >>> From: Miao Wang <shankerwangmiao@gmail.com> >>> >>> In Linux 6.11, fstat and newfstatat are added back. We need to include >>> this change in kernel-features.h to avoid producing libraries >>> incompatible with previous linux versions with new headers. >>> >>> The definition of the two syscalls will be removed when the targeted >>> kernel version is below 6.11 in loongarch/sysdep.h. >>> >>> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> >> >> You should resolve the previous review comments before sending a new >> version. If you don't agree with 6.10.6 then fine to leave the code >> unchanged but you need to explain the reason in the commit message. And >> to me "I think the backport is questionable" isn't a good reason (i.e. >> we need the reason it's questionable, not just "it's questionable"). > > Actually "the reason it's *wrong*," not just "questionable." Everything > can be questionable. I don't think the choice between 6.10.6 or 6.11 should be a major problem. Because the macro __LINUX_KERNEL_VERSION here, is not the kernel glibc is running with, nor the kernel headers glibc is compiled with, but actually a compile option specified by the user via the configure command line, i.e. --enable-kernel, which will be, if absent, in the context of loongarch, 5.19. I tend to choose 6.11, instead of 6.10.6, based on the following reasons: 1. Most of other kernel version comparing statements contains only the minor version. Actually, there is only one occurrence of comparing with the patch version, if I did not miss anything. 2. As you've pointed before, I quote, "non-Linus forks of the kernel aren't supported", end of quote, 6.10.6 is not in Linus' kernel tree, but 6.11 is. 3. Backporting makes the mapping between version numbers and features in a non-linear way. If version numbers in the stable tree should also be considered, saying kernel versions >= 6.10.6 supports fstat and newfstatat, it would be unfair to ignore versions >= 6.6.47 and < 6.7, versions >= 6.1.106 and < 6.2. So I would prefer using 6.11, i.e. the version in the Torvalds' tree. I'll leave 6.11 unchanged in my patch. Since this patch will not get merged before someone refreshes all the syscall definitions with the 6.11 kernel, I expect further discussions can make this clearer. Cheers, Miao Wang > > -- > Xi Ruoyao <xry111@xry111.site> > School of Aerospace Science and Technology, Xidian University
On Wed, 2024-08-21 at 00:01 +0800, Miao Wang wrote: > Hi, > > > 2024年8月20日 23:11，Xi Ruoyao <xry111@xry111.site> 写道： > > > > On Tue, 2024-08-20 at 23:06 +0800, Xi Ruoyao wrote: > > > On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: > > > > From: Miao Wang <shankerwangmiao@gmail.com> > > > > > > > > In Linux 6.11, fstat and newfstatat are added back. We need to include > > > > this change in kernel-features.h to avoid producing libraries > > > > incompatible with previous linux versions with new headers. > > > > > > > > The definition of the two syscalls will be removed when the targeted > > > > kernel version is below 6.11 in loongarch/sysdep.h. > > > > > > > > Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> > > > > > > You should resolve the previous review comments before sending a new > > > version. If you don't agree with 6.10.6 then fine to leave the code > > > unchanged but you need to explain the reason in the commit message. And > > > to me "I think the backport is questionable" isn't a good reason (i.e. > > > we need the reason it's questionable, not just "it's questionable"). > > > > Actually "the reason it's *wrong*," not just "questionable." Everything > > can be questionable. > > I don't think the choice between 6.10.6 or 6.11 should be a major problem. > Because the macro __LINUX_KERNEL_VERSION here, is not the kernel glibc is > running with, nor the kernel headers glibc is compiled with, but actually > a compile option specified by the user via the configure command line, i.e. > --enable-kernel, which will be, if absent, in the context of loongarch, > 5.19. There is --enable-kernel=current, which means the kernel glibc is running with. And the user cannot randomly specify --enable-kernel, i.e. (s)he cannot run a 6.10.6 kernel but specify --enable-kernel=6.11. The value of -- enable-kernel is both checked at the configure time (against the UAPI headers) and runtime (against the running kernel). Thus a user running 6.10.6 would have no way to take the advantage of fstat/newfstatat if we hard code 6.11 here: (s)he cannot use --enable-kernel=6.11 because even if a newer copy of the UAPI headers is used, anything linked to the glibc build will just refuse to start. Note that the fast path of statx (added in kernel commit 0ef625bba6fb) isn't backported, so using fstat/newfstatat is the only way the user can avoid the performance impact of statx on 6.10.x. Thus IMO we should allow the user to do so if (s)he has a 6.10.6 or newer 6.10.x. For 6.1.106 and 6.6.47 unfortunately it's not possible, but for 6.10.6 we can do our best. > I tend to choose 6.11, instead of 6.10.6, based on the following reasons: > > 1. Most of other kernel version comparing statements contains only the minor > version. Actually, there is only one occurrence of comparing with the patch > version, if I did not miss anything. > > 2. As you've pointed before, I quote, "non-Linus forks of the kernel aren't > supported", end of quote, 6.10.6 is not in Linus' kernel tree, but 6.11 is. But there's one existing example to specify a point release: /* The ARM kernel before 3.14.3 may or may not support futex_atomic_cmpxchg_inatomic, depending on kernel configuration. */ #if __LINUX_KERNEL_VERSION < 0x030E03 # undef __ASSUME_SET_ROBUST_LIST #endif > 3. Backporting makes the mapping between version numbers and features in a > non-linear way. If version numbers in the stable tree should also be > considered, saying kernel versions >= 6.10.6 supports fstat and newfstatat, > it would be unfair to ignore versions >= 6.6.47 and < 6.7, versions >= > 6.1.106 and < 6.2. So I would prefer using 6.11, i.e. the version in the > Torvalds' tree. Yes, so we cannot use 06062f or 06016a here. The initial reason of the backport was "supporting some rouge proprietary programs not obeying the upstream ABI and some badly designed sandboxes." Thus it's favoring bad behavior and that's already much unfair. Allowing a vanilla Linux 6.10.6/Glibc-2.41 system to take the performance advantage of fstat/newfstatat will at least make it slightly fairer (i.e. the bad guys still eat the meat, but the good guys can at least have some soup).
On 20/08/24 13:34, Xi Ruoyao wrote: > On Wed, 2024-08-21 at 00:01 +0800, Miao Wang wrote: >> Hi, >> >>> 2024年8月20日 23:11，Xi Ruoyao <xry111@xry111.site> 写道： >>> >>> On Tue, 2024-08-20 at 23:06 +0800, Xi Ruoyao wrote: >>>> On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: >>>>> From: Miao Wang <shankerwangmiao@gmail.com> >>>>> >>>>> In Linux 6.11, fstat and newfstatat are added back. We need to include >>>>> this change in kernel-features.h to avoid producing libraries >>>>> incompatible with previous linux versions with new headers. >>>>> >>>>> The definition of the two syscalls will be removed when the targeted >>>>> kernel version is below 6.11 in loongarch/sysdep.h. >>>>> >>>>> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> >>>> >>>> You should resolve the previous review comments before sending a new >>>> version. If you don't agree with 6.10.6 then fine to leave the code >>>> unchanged but you need to explain the reason in the commit message. And >>>> to me "I think the backport is questionable" isn't a good reason (i.e. >>>> we need the reason it's questionable, not just "it's questionable"). >>> >>> Actually "the reason it's *wrong*," not just "questionable." Everything >>> can be questionable. >> >> I don't think the choice between 6.10.6 or 6.11 should be a major problem. >> Because the macro __LINUX_KERNEL_VERSION here, is not the kernel glibc is >> running with, nor the kernel headers glibc is compiled with, but actually >> a compile option specified by the user via the configure command line, i.e. >> --enable-kernel, which will be, if absent, in the context of loongarch, >> 5.19. > > There is --enable-kernel=current, which means the kernel glibc is > running with. > > And the user cannot randomly specify --enable-kernel, i.e. (s)he cannot > run a 6.10.6 kernel but specify --enable-kernel=6.11. The value of -- > enable-kernel is both checked at the configure time (against the UAPI > headers) and runtime (against the running kernel). Thus a user running > 6.10.6 would have no way to take the advantage of fstat/newfstatat if we > hard code 6.11 here: (s)he cannot use --enable-kernel=6.11 because even > if a newer copy of the UAPI headers is used, anything linked to the > glibc build will just refuse to start. It is not checked at runtime anymore (b46d250656), so the user can indeed configure with --enable-kernel=X.Y and run on a Z.K with Z>=X and/or Y>=K. > > Note that the fast path of statx (added in kernel commit 0ef625bba6fb) > isn't backported, so using fstat/newfstatat is the only way the user can > avoid the performance impact of statx on 6.10.x. Thus IMO we should > allow the user to do so if (s)he has a 6.10.6 or newer 6.10.x. For > 6.1.106 and 6.6.47 unfortunately it's not possible, but for 6.10.6 we > can do our best. So the issue is performance? I had the impression that it a due some LSM failing to filter statx correctly. > >> I tend to choose 6.11, instead of 6.10.6, based on the following reasons: >> >> 1. Most of other kernel version comparing statements contains only the minor >> version. Actually, there is only one occurrence of comparing with the patch >> version, if I did not miss anything. >> >> 2. As you've pointed before, I quote, "non-Linus forks of the kernel aren't >> supported", end of quote, 6.10.6 is not in Linus' kernel tree, but 6.11 is. > > But there's one existing example to specify a point release: > > /* The ARM kernel before 3.14.3 may or may not support > futex_atomic_cmpxchg_inatomic, depending on kernel > configuration. */ > #if __LINUX_KERNEL_VERSION < 0x030E03 > # undef __ASSUME_SET_ROBUST_LIST > #endif > >> 3. Backporting makes the mapping between version numbers and features in a >> non-linear way. If version numbers in the stable tree should also be >> considered, saying kernel versions >= 6.10.6 supports fstat and newfstatat, >> it would be unfair to ignore versions >= 6.6.47 and < 6.7, versions >= >> 6.1.106 and < 6.2. So I would prefer using 6.11, i.e. the version in the >> Torvalds' tree. > > Yes, so we cannot use 06062f or 06016a here. > > The initial reason of the backport was "supporting some rouge > proprietary programs not obeying the upstream ABI and some badly > designed sandboxes." Thus it's favoring bad behavior and that's already > much unfair. Allowing a vanilla Linux 6.10.6/Glibc-2.41 system to take > the performance advantage of fstat/newfstatat will at least make it > slightly fairer (i.e. the bad guys still eat the meat, but the good guys > can at least have some soup). Sigh, I though we were over the Linux stat mess but it seems it keep coming back to bite us. The main problem is assuming this kernel patch would be backported is we will never know if it actually has the backport. To properly support it the correct way would be something like: - sysdeps/unix/sysv/linux/loongarch/sysdep.h: #undef __NR_fstat #undef __NR_newfstatat - sysdeps/unix/sysv/linux/loongarch/kernel-features.h #if __LINUX_KERNEL_VERSION > 0x060b00 # define __ASSUME_LOONGARCH_NEWSTATAT #endif - sysdeps/unix/sysv/linux/loongarch/fstatat64.c int __fstatat64 (int fd, const char *file, struct stat64 *buf, int flags) { int r; #ifdef __ASSUME_LOONGARCH_NEWSTATAT r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); #else static int newfstat_supported = 1; if (atomic_load_relaxed (&newfstat_supported) == 1) { r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); if (r == 0) return r; else if (r != -ENOSYS) return INLINE_SYSCALL_ERROR_RETURN_VALUE (r); atomic_store_relaxed (&newfstat_supported, false); } r = fstatat64_time64_statx (fd, file, buf, flag); #endif return INTERNAL_SYSCALL_ERROR_P (r) ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r) : 0; } So you will need to either use la64v1.1 or --enable-kernel=6.11 to avoid the 'dbar 0x700' on every stat call. (And then we start to have all the arch-specific stat implementation mess all over again...)
On 20/08/24 15:15, Adhemerval Zanella Netto wrote: > > > On 20/08/24 13:34, Xi Ruoyao wrote: >> On Wed, 2024-08-21 at 00:01 +0800, Miao Wang wrote: >>> Hi, >>> >>>> 2024年8月20日 23:11，Xi Ruoyao <xry111@xry111.site> 写道： >>>> >>>> On Tue, 2024-08-20 at 23:06 +0800, Xi Ruoyao wrote: >>>>> On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: >>>>>> From: Miao Wang <shankerwangmiao@gmail.com> >>>>>> >>>>>> In Linux 6.11, fstat and newfstatat are added back. We need to include >>>>>> this change in kernel-features.h to avoid producing libraries >>>>>> incompatible with previous linux versions with new headers. >>>>>> >>>>>> The definition of the two syscalls will be removed when the targeted >>>>>> kernel version is below 6.11 in loongarch/sysdep.h. >>>>>> >>>>>> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> >>>>> >>>>> You should resolve the previous review comments before sending a new >>>>> version. If you don't agree with 6.10.6 then fine to leave the code >>>>> unchanged but you need to explain the reason in the commit message. And >>>>> to me "I think the backport is questionable" isn't a good reason (i.e. >>>>> we need the reason it's questionable, not just "it's questionable"). >>>> >>>> Actually "the reason it's *wrong*," not just "questionable." Everything >>>> can be questionable. >>> >>> I don't think the choice between 6.10.6 or 6.11 should be a major problem. >>> Because the macro __LINUX_KERNEL_VERSION here, is not the kernel glibc is >>> running with, nor the kernel headers glibc is compiled with, but actually >>> a compile option specified by the user via the configure command line, i.e. >>> --enable-kernel, which will be, if absent, in the context of loongarch, >>> 5.19. >> >> There is --enable-kernel=current, which means the kernel glibc is >> running with. >> >> And the user cannot randomly specify --enable-kernel, i.e. (s)he cannot >> run a 6.10.6 kernel but specify --enable-kernel=6.11. The value of -- >> enable-kernel is both checked at the configure time (against the UAPI >> headers) and runtime (against the running kernel). Thus a user running >> 6.10.6 would have no way to take the advantage of fstat/newfstatat if we >> hard code 6.11 here: (s)he cannot use --enable-kernel=6.11 because even >> if a newer copy of the UAPI headers is used, anything linked to the >> glibc build will just refuse to start. > > It is not checked at runtime anymore (b46d250656), so the user can indeed > configure with --enable-kernel=X.Y and run on a Z.K with Z>=X and/or Y>=K. > >> >> Note that the fast path of statx (added in kernel commit 0ef625bba6fb) >> isn't backported, so using fstat/newfstatat is the only way the user can >> avoid the performance impact of statx on 6.10.x. Thus IMO we should >> allow the user to do so if (s)he has a 6.10.6 or newer 6.10.x. For >> 6.1.106 and 6.6.47 unfortunately it's not possible, but for 6.10.6 we >> can do our best. > > So the issue is performance? I had the impression that it a due some LSM > failing to filter statx correctly. > >> >>> I tend to choose 6.11, instead of 6.10.6, based on the following reasons: >>> >>> 1. Most of other kernel version comparing statements contains only the minor >>> version. Actually, there is only one occurrence of comparing with the patch >>> version, if I did not miss anything. >>> >>> 2. As you've pointed before, I quote, "non-Linus forks of the kernel aren't >>> supported", end of quote, 6.10.6 is not in Linus' kernel tree, but 6.11 is. >> >> But there's one existing example to specify a point release: >> >> /* The ARM kernel before 3.14.3 may or may not support >> futex_atomic_cmpxchg_inatomic, depending on kernel >> configuration. */ >> #if __LINUX_KERNEL_VERSION < 0x030E03 >> # undef __ASSUME_SET_ROBUST_LIST >> #endif >> >>> 3. Backporting makes the mapping between version numbers and features in a >>> non-linear way. If version numbers in the stable tree should also be >>> considered, saying kernel versions >= 6.10.6 supports fstat and newfstatat, >>> it would be unfair to ignore versions >= 6.6.47 and < 6.7, versions >= >>> 6.1.106 and < 6.2. So I would prefer using 6.11, i.e. the version in the >>> Torvalds' tree. >> >> Yes, so we cannot use 06062f or 06016a here. >> >> The initial reason of the backport was "supporting some rouge >> proprietary programs not obeying the upstream ABI and some badly >> designed sandboxes." Thus it's favoring bad behavior and that's already >> much unfair. Allowing a vanilla Linux 6.10.6/Glibc-2.41 system to take >> the performance advantage of fstat/newfstatat will at least make it >> slightly fairer (i.e. the bad guys still eat the meat, but the good guys >> can at least have some soup). > > Sigh, I though we were over the Linux stat mess but it seems it keep > coming back to bite us. > > The main problem is assuming this kernel patch would be backported is > we will never know if it actually has the backport. To properly support > it the correct way would be something like: > > - sysdeps/unix/sysv/linux/loongarch/sysdep.h: > > #undef __NR_fstat > #undef __NR_newfstatat Nvm this part, there is no need to undefine the new syscalls. > > - sysdeps/unix/sysv/linux/loongarch/kernel-features.h > > #if __LINUX_KERNEL_VERSION > 0x060b00 > # define __ASSUME_LOONGARCH_NEWSTATAT > #endif > > - sysdeps/unix/sysv/linux/loongarch/fstatat64.c > > int > __fstatat64 (int fd, const char *file, struct stat64 *buf, int flags) > { > int r; > #ifdef __ASSUME_LOONGARCH_NEWSTATAT > r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); > #else > static int newfstat_supported = 1; > if (atomic_load_relaxed (&newfstat_supported) == 1) > { > r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); > if (r == 0) > return r; > else if (r != -ENOSYS) > return INLINE_SYSCALL_ERROR_RETURN_VALUE (r); > atomic_store_relaxed (&newfstat_supported, false); > } > r = fstatat64_time64_statx (fd, file, buf, flag); > #endif > return INTERNAL_SYSCALL_ERROR_P (r) > ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r) > : 0; > } > > > So you will need to either use la64v1.1 or --enable-kernel=6.11 to > avoid the 'dbar 0x700' on every stat call. > > (And then we start to have all the arch-specific stat implementation > mess all over again...)
> 2024年8月21日 02:42，Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> 写道： > > > > On 20/08/24 15:15, Adhemerval Zanella Netto wrote: >> >> >> On 20/08/24 13:34, Xi Ruoyao wrote: >>> On Wed, 2024-08-21 at 00:01 +0800, Miao Wang wrote: >>>> Hi, >>>> >>>>> 2024年8月20日 23:11，Xi Ruoyao <xry111@xry111.site> 写道： >>>>> >>>>> On Tue, 2024-08-20 at 23:06 +0800, Xi Ruoyao wrote: >>>>>> On Tue, 2024-08-20 at 22:12 +0800, Miao Wang via B4 Relay wrote: >>>>>>> From: Miao Wang <shankerwangmiao@gmail.com> >>>>>>> >>>>>>> In Linux 6.11, fstat and newfstatat are added back. We need to include >>>>>>> this change in kernel-features.h to avoid producing libraries >>>>>>> incompatible with previous linux versions with new headers. >>>>>>> >>>>>>> The definition of the two syscalls will be removed when the targeted >>>>>>> kernel version is below 6.11 in loongarch/sysdep.h. >>>>>>> >>>>>>> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> >>>>>> >>>>>> You should resolve the previous review comments before sending a new >>>>>> version. If you don't agree with 6.10.6 then fine to leave the code >>>>>> unchanged but you need to explain the reason in the commit message. And >>>>>> to me "I think the backport is questionable" isn't a good reason (i.e. >>>>>> we need the reason it's questionable, not just "it's questionable"). >>>>> >>>>> Actually "the reason it's *wrong*," not just "questionable." Everything >>>>> can be questionable. >>>> >>>> I don't think the choice between 6.10.6 or 6.11 should be a major problem. >>>> Because the macro __LINUX_KERNEL_VERSION here, is not the kernel glibc is >>>> running with, nor the kernel headers glibc is compiled with, but actually >>>> a compile option specified by the user via the configure command line, i.e. >>>> --enable-kernel, which will be, if absent, in the context of loongarch, >>>> 5.19. >>> >>> There is --enable-kernel=current, which means the kernel glibc is >>> running with. >>> >>> And the user cannot randomly specify --enable-kernel, i.e. (s)he cannot >>> run a 6.10.6 kernel but specify --enable-kernel=6.11. The value of -- >>> enable-kernel is both checked at the configure time (against the UAPI >>> headers) and runtime (against the running kernel). Thus a user running >>> 6.10.6 would have no way to take the advantage of fstat/newfstatat if we >>> hard code 6.11 here: (s)he cannot use --enable-kernel=6.11 because even >>> if a newer copy of the UAPI headers is used, anything linked to the >>> glibc build will just refuse to start. >> >> It is not checked at runtime anymore (b46d250656), so the user can indeed >> configure with --enable-kernel=X.Y and run on a Z.K with Z>=X and/or Y>=K. >> >>> >>> Note that the fast path of statx (added in kernel commit 0ef625bba6fb) >>> isn't backported, so using fstat/newfstatat is the only way the user can >>> avoid the performance impact of statx on 6.10.x. Thus IMO we should >>> allow the user to do so if (s)he has a 6.10.6 or newer 6.10.x. For >>> 6.1.106 and 6.6.47 unfortunately it's not possible, but for 6.10.6 we >>> can do our best. >> >> So the issue is performance? I had the impression that it a due some LSM >> failing to filter statx correctly. No, this reason for introducing them back is simply for sandboxing, since there is no way for a seccomp sandbox to only allow stat() for opening fds without fstat. To solve this, there are two approaches, the first is allowing NULL pointers instead of a pointer of an empty strings to be passed in when using statx(fd, NULL, AT_EMPTY_PATH), which also solves the performance issue caused by checking whether the string is empty; the second is adding back fstat and newfstatat. Both are now implemented. My patch is not trying to solve the performance issue, but to solving the correctness, since without my patch, if glibc is compiled with 6.11 kernel header, the binary cannot work on older kernels, because the detection of the existence of the fstat/newfstatat syscalls relies on the corresponding syscall number macro. My approach is relatively short, but the added syscalls will not be utilized in glibc compiled targeted at old kernels. >> >>> >>>> I tend to choose 6.11, instead of 6.10.6, based on the following reasons: >>>> >>>> 1. Most of other kernel version comparing statements contains only the minor >>>> version. Actually, there is only one occurrence of comparing with the patch >>>> version, if I did not miss anything. >>>> >>>> 2. As you've pointed before, I quote, "non-Linus forks of the kernel aren't >>>> supported", end of quote, 6.10.6 is not in Linus' kernel tree, but 6.11 is. >>> >>> But there's one existing example to specify a point release: >>> >>> /* The ARM kernel before 3.14.3 may or may not support >>> futex_atomic_cmpxchg_inatomic, depending on kernel >>> configuration. */ >>> #if __LINUX_KERNEL_VERSION < 0x030E03 >>> # undef __ASSUME_SET_ROBUST_LIST >>> #endif >>> >>>> 3. Backporting makes the mapping between version numbers and features in a >>>> non-linear way. If version numbers in the stable tree should also be >>>> considered, saying kernel versions >= 6.10.6 supports fstat and newfstatat, >>>> it would be unfair to ignore versions >= 6.6.47 and < 6.7, versions >= >>>> 6.1.106 and < 6.2. So I would prefer using 6.11, i.e. the version in the >>>> Torvalds' tree. >>> >>> Yes, so we cannot use 06062f or 06016a here. >>> >>> The initial reason of the backport was "supporting some rouge >>> proprietary programs not obeying the upstream ABI and some badly >>> designed sandboxes." Thus it's favoring bad behavior and that's already >>> much unfair. Allowing a vanilla Linux 6.10.6/Glibc-2.41 system to take >>> the performance advantage of fstat/newfstatat will at least make it >>> slightly fairer (i.e. the bad guys still eat the meat, but the good guys >>> can at least have some soup). >> >> Sigh, I though we were over the Linux stat mess but it seems it keep >> coming back to bite us. I chose to implement in that way just because I can see there is so many ifdefs in the implementation of those stat related functions. >> >> The main problem is assuming this kernel patch would be backported is >> we will never know if it actually has the backport. To properly support >> it the correct way would be something like: >> >> - sysdeps/unix/sysv/linux/loongarch/sysdep.h: >> >> #undef __NR_fstat >> #undef __NR_newfstatat > > Nvm this part, there is no need to undefine the new syscalls. > >> >> - sysdeps/unix/sysv/linux/loongarch/kernel-features.h >> >> #if __LINUX_KERNEL_VERSION > 0x060b00 >> # define __ASSUME_LOONGARCH_NEWSTATAT >> #endif >> >> - sysdeps/unix/sysv/linux/loongarch/fstatat64.c >> >> int >> __fstatat64 (int fd, const char *file, struct stat64 *buf, int flags) >> { >> int r; >> #ifdef __ASSUME_LOONGARCH_NEWSTATAT >> r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); >> #else >> static int newfstat_supported = 1; >> if (atomic_load_relaxed (&newfstat_supported) == 1) >> { >> r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); >> if (r == 0) >> return r; >> else if (r != -ENOSYS) >> return INLINE_SYSCALL_ERROR_RETURN_VALUE (r); >> atomic_store_relaxed (&newfstat_supported, false); >> } >> r = fstatat64_time64_statx (fd, file, buf, flag); >> #endif >> return INTERNAL_SYSCALL_ERROR_P (r) >> ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r) >> : 0; >> } >> >> >> So you will need to either use la64v1.1 or --enable-kernel=6.11 to >> avoid the 'dbar 0x700' on every stat call. >> >> (And then we start to have all the arch-specific stat implementation >> mess all over again...) The arch-specific mess is because fstat and newfstatat are existing syscalls on other architectures, but are "new features" on loongarch. What if we choose a unified implementation of fstatat64 in the following way: 1. if kernel version >= 6.11 -> directly call statx(fd, NULL, AT_EMPTY_PATH) 2. or otherwise -> try to call statx(fd, NULL, AT_EMPTY_PATH), if statx is available if fails -> call fstat(fd) instead, if __NR_fstat is defined -> or call statx(fd, "", AT_EMPTY_PATH), if __NR_fstat is not defined So on all architectures, we shall prefer to using statx with the fast kernel path. The drawback is that fstat will not be tried if statx(fd, NULL, AT_EMPTY_PATH) fails.
On Wed, 2024-08-21 at 04:24 +0800, Miao Wang wrote: /* snip */ > > > So the issue is performance? I had the impression that it a due some LSM > > > failing to filter statx correctly. > > No, this reason for introducing them back is simply for sandboxing, since there > is no way for a seccomp sandbox to only allow stat() for opening fds without > fstat. To solve this, there are two approaches, the first is allowing NULL > pointers instead of a pointer of an empty strings to be passed in when using > statx(fd, NULL, AT_EMPTY_PATH), which also solves the performance issue caused > by checking whether the string is empty; the second is adding back fstat and > newfstatat. Both are now implemented. > > My patch is not trying to solve the performance issue, But it will solve the performance issue for 6.10.6+ if we allow them. And it's the only way to solve it for >= 6.10.6, < 6.11 because the fast statx path isn't backported. > but to solving the correctness If we just need to solve the correctness the easiest way is to unconditionally undef __NR_newfstatat and __NR_fstat and prevent they were never added. Doing so is still correct. Then why we still want to use them? Because using them indeed provide a better performance (even on 6.11+: using statx there's still a necessity to convert the struct statx to struct stat in the user space because these data structures are not compatible). > since without my patch, if glibc is compiled with 6.11 kernel > header It's "after the glibc arch-syscall.h is updated with the new kernel header," not just "glibc is compiled with 6.11 kernel header." When people simply compile glibc (i.e. w/o developing it) they shouldn't update arch-syscall.h. The script to perform the update is not wired to the building system, it's a script for developers. > the binary cannot work on older kernels, because the detection of the > existence of the fstat/newfstatat syscalls relies on the corresponding syscall > number macro. > > My approach is relatively short, but the added syscalls will not be utilized in > glibc compiled targeted at old kernels. So will changing 060b00 to 060a06 make the patch much more complex? /* snip */ > > > > > > > - sysdeps/unix/sysv/linux/loongarch/kernel-features.h > > > > > > #if __LINUX_KERNEL_VERSION > 0x060b00 > > > # define __ASSUME_LOONGARCH_NEWSTATAT > > > #endif > > > > > > - sysdeps/unix/sysv/linux/loongarch/fstatat64.c > > > > > > int > > > __fstatat64 (int fd, const char *file, struct stat64 *buf, int flags) > > > { > > > int r; > > > #ifdef __ASSUME_LOONGARCH_NEWSTATAT > > > r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); > > > #else > > > static int newfstat_supported = 1; > > > if (atomic_load_relaxed (&newfstat_supported) == 1) > > > { > > > r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); > > > if (r == 0) > > > return r; > > > else if (r != -ENOSYS) > > > return INLINE_SYSCALL_ERROR_RETURN_VALUE (r); > > > atomic_store_relaxed (&newfstat_supported, false); > > > } > > > r = fstatat64_time64_statx (fd, file, buf, flag); > > > #endif > > > return INTERNAL_SYSCALL_ERROR_P (r) > > > ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r) > > > : 0; > > > } > > > > > > > > > So you will need to either use la64v1.1 or --enable-kernel=6.11 to > > > avoid the 'dbar 0x700' on every stat call. dbar 0x700 is a nop on la64v1.1, thus on la64v1.1 removing it is only a code size micro-optimization. But on la64v1.0 (and --enable-kernel < 6.11) it's really needed. Technically dbar 0x700 is only needed when we get an "uninitialized" state and we are not sure if it's really uninitialized or a load-load hazard. If newfstat_supported is a tri-state (-1 uninitialized, 0 unsupported, 1 supported) we can have something like... pcalau12i $t0, %pc_hi20(newfstat_supported) ld.d $t1, $t0, %pc_lo12(newfstat_supported) blt $t1, $r0, 1f dbar 0x700 ld.d $t1, $t0, %pc_lo12(newfstat_supported) blt $t1, $r0, 1f # try fstat # if supported, store 1 to newfstat_supported and return, # otherwise store 0 to newfstat_supported and jump to 2 1: beqz $t1, 2f # do fstat and return 2: # do statx and return I.e. dbar 0x700 is only executed once. But there seems no way make C emit the first "ld.d $t1, $t0, %pc_lo12(newfstat_supported)" w/o invoking an undefined behavior (data race). > > > (And then we start to have all the arch-specific stat implementation > > > mess all over again...) > > The arch-specific mess is because fstat and newfstatat are existing syscalls > on other architectures, but are "new features" on loongarch. > > What if we choose a unified implementation of fstatat64 in the following way: > > 1. if kernel version >= 6.11 > -> directly call statx(fd, NULL, AT_EMPTY_PATH) > 2. or otherwise > -> try to call statx(fd, NULL, AT_EMPTY_PATH), if statx is available > if fails statx(fd, NULL, AT_EMPTY_PATH) is still slower than fstat because glibc needs to convert struct statx to struct stat after the syscall. Thus on kernel version >= 6.11 we should just use fstat for fstat (unless it's a 32-bit platform where fstat suffers y2038). > -> call fstat(fd) instead, if __NR_fstat is defined > -> or call statx(fd, "", AT_EMPTY_PATH), if __NR_fstat is not defined > > So on all architectures, we shall prefer to using statx with the fast kernel > path. The drawback is that fstat will not be tried if statx(fd, NULL, > AT_EMPTY_PATH) fails. This drawback is not acceptable because it will cause a severe performance regression on all architectures with Linux kernel < 6.11. See commit 551101e8240b which was applied to fix such a regression (it's avoiding fstatat for stat, but statx for fstat suffers the same issue on even more kernel versions, and with even an additional cost to convert the data structure).
> 2024年8月21日 13:21，Xi Ruoyao <xry111@xry111.site> 写道： > > On Wed, 2024-08-21 at 04:24 +0800, Miao Wang wrote: > > /* snip */ > >>>> So the issue is performance? I had the impression that it a due some LSM >>>> failing to filter statx correctly. >> >> No, this reason for introducing them back is simply for sandboxing, since there >> is no way for a seccomp sandbox to only allow stat() for opening fds without >> fstat. To solve this, there are two approaches, the first is allowing NULL >> pointers instead of a pointer of an empty strings to be passed in when using >> statx(fd, NULL, AT_EMPTY_PATH), which also solves the performance issue caused >> by checking whether the string is empty; the second is adding back fstat and >> newfstatat. Both are now implemented. >> >> My patch is not trying to solve the performance issue, > > But it will solve the performance issue for 6.10.6+ if we allow them. > And it's the only way to solve it for >= 6.10.6, < 6.11 because the fast > statx path isn't backported. > >> but to solving the correctness > > If we just need to solve the correctness the easiest way is to > unconditionally undef __NR_newfstatat and __NR_fstat and prevent they > were never added. Doing so is still correct. Then why we still want to > use them? Because using them indeed provide a better performance (even > on 6.11+: using statx there's still a necessity to convert the struct > statx to struct stat in the user space because these data structures are > not compatible). > >> since without my patch, if glibc is compiled with 6.11 kernel >> header > > It's "after the glibc arch-syscall.h is updated with the new kernel > header," not just "glibc is compiled with 6.11 kernel header." When > people simply compile glibc (i.e. w/o developing it) they shouldn't > update arch-syscall.h. The script to perform the update is not wired to > the building system, it's a script for developers. I fully understand that. I just used a simpler but less precise way to express, expecting understood in this context. > >> the binary cannot work on older kernels, because the detection of the >> existence of the fstat/newfstatat syscalls relies on the corresponding syscall >> number macro. >> >> My approach is relatively short, but the added syscalls will not be utilized in >> glibc compiled targeted at old kernels. > > So will changing 060b00 to 060a06 make the patch much more complex? Please refrain from comparing 6.11 and 6.10.6 anymore, since I don't think it is a major problem. You have given a good reason for using 6.10.6, for benefitting as many users as possible, which I can accept. To clarify, The purpose of my saying "short" and "not trying to solve the performance issue" is to compare my patch to the approach proposed by Adhemerval. I believe the current topic would be how to fully utilize the fstat syscall which might be provided by the kernel on glibc compiled targeted with previous kernel versions, so that we can further improve the performance, while keeping the code clean. > > /* snip */ > >>>>> >>>> - sysdeps/unix/sysv/linux/loongarch/kernel-features.h >>>> >>>> #if __LINUX_KERNEL_VERSION > 0x060b00 >>>> # define __ASSUME_LOONGARCH_NEWSTATAT >>>> #endif >>>> >>>> - sysdeps/unix/sysv/linux/loongarch/fstatat64.c >>>> >>>> int >>>> __fstatat64 (int fd, const char *file, struct stat64 *buf, int flags) >>>> { >>>> int r; >>>> #ifdef __ASSUME_LOONGARCH_NEWSTATAT >>>> r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); >>>> #else >>>> static int newfstat_supported = 1; >>>> if (atomic_load_relaxed (&newfstat_supported) == 1) >>>> { >>>> r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); >>>> if (r == 0) >>>> return r; >>>> else if (r != -ENOSYS) >>>> return INLINE_SYSCALL_ERROR_RETURN_VALUE (r); >>>> atomic_store_relaxed (&newfstat_supported, false); >>>> } >>>> r = fstatat64_time64_statx (fd, file, buf, flag); >>>> #endif >>>> return INTERNAL_SYSCALL_ERROR_P (r) >>>> ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r) >>>> : 0; >>>> } >>>> >>>> >>>> So you will need to either use la64v1.1 or --enable-kernel=6.11 to >>>> avoid the 'dbar 0x700' on every stat call. > > dbar 0x700 is a nop on la64v1.1, thus on la64v1.1 removing it is only a > code size micro-optimization. But on la64v1.0 (and --enable-kernel < > 6.11) it's really needed. > > Technically dbar 0x700 is only needed when we get an "uninitialized" > state and we are not sure if it's really uninitialized or a load-load > hazard. If newfstat_supported is a tri-state (-1 uninitialized, 0 > unsupported, 1 supported) we can have something like... > > pcalau12i $t0, %pc_hi20(newfstat_supported) > ld.d $t1, $t0, %pc_lo12(newfstat_supported) > blt $t1, $r0, 1f > dbar 0x700 > ld.d $t1, $t0, %pc_lo12(newfstat_supported) > blt $t1, $r0, 1f > # try fstat > # if supported, store 1 to newfstat_supported and return, > # otherwise store 0 to newfstat_supported and jump to 2 > 1: > beqz $t1, 2f > # do fstat and return > 2: > # do statx and return > > I.e. dbar 0x700 is only executed once. But there seems no way make C > emit the first "ld.d $t1, $t0, %pc_lo12(newfstat_supported)" w/o > invoking an undefined behavior (data race). > >>>> (And then we start to have all the arch-specific stat implementation >>>> mess all over again...) >> >> The arch-specific mess is because fstat and newfstatat are existing syscalls >> on other architectures, but are "new features" on loongarch. >> >> What if we choose a unified implementation of fstatat64 in the following way: >> >> 1. if kernel version >= 6.11 >> -> directly call statx(fd, NULL, AT_EMPTY_PATH) >> 2. or otherwise >> -> try to call statx(fd, NULL, AT_EMPTY_PATH), if statx is available >> if fails > > statx(fd, NULL, AT_EMPTY_PATH) is still slower than fstat because glibc > needs to convert struct statx to struct stat after the syscall. Thus on > kernel version >= 6.11 we should just use fstat for fstat (unless it's a > 32-bit platform where fstat suffers y2038). > >> -> call fstat(fd) instead, if __NR_fstat is defined >> -> or call statx(fd, "", AT_EMPTY_PATH), if __NR_fstat is not defined >> >> So on all architectures, we shall prefer to using statx with the fast kernel >> path. The drawback is that fstat will not be tried if statx(fd, NULL, >> AT_EMPTY_PATH) fails. > > This drawback is not acceptable because it will cause a severe > performance regression on all architectures with Linux kernel < 6.11. I don't think the regression will be huge, since statx(fd, NULL, AT_EMPTY_PATH) will go through the fast path. If the fast path is not available, we choose (with a flag, maybe permanently afterwards) fstat. The only cost will be the conversion afterwards. Our benefit will be a unified implementation. Since statx(fd, NULL, AT_EMPTY_PATH) is a new feature for all the architectures, but loongarch is the only one where fstat is missing, and thus we will always maintaining a code path which is only used by loongarch. By preferring statx, we may jump out of the mess caused by fstat. We should evaluate whether the trade is worth itself, or we can always come up with other approaches which can both fully utilize the available features provided by the kernel and avoid introducing another layer of mess into the stat functions. > See commit 551101e8240b which was applied to fix such a regression (it's > avoiding fstatat for stat, but statx for fstat suffers the same issue on > even more kernel versions, and with even an additional cost to convert > the data structure). > > -- > Xi Ruoyao <xry111@xry111.site> > School of Aerospace Science and Technology, Xidian University
On Wed, 2024-08-21 at 14:08 +0800, Miao Wang wrote: > > > What if we choose a unified implementation of fstatat64 in the following way: > > > > > > 1. if kernel version >= 6.11 > > > -> directly call statx(fd, NULL, AT_EMPTY_PATH) > > > 2. or otherwise > > > -> try to call statx(fd, NULL, AT_EMPTY_PATH), if statx is available > > > if fails > > > > statx(fd, NULL, AT_EMPTY_PATH) is still slower than fstat because glibc > > needs to convert struct statx to struct stat after the syscall. Thus on > > kernel version >= 6.11 we should just use fstat for fstat (unless it's a > > 32-bit platform where fstat suffers y2038). > > > > > -> call fstat(fd) instead, if __NR_fstat is defined > > > -> or call statx(fd, "", AT_EMPTY_PATH), if __NR_fstat is not defined > > > > > > So on all architectures, we shall prefer to using statx with the fast kernel > > > path. The drawback is that fstat will not be tried if statx(fd, NULL, > > > AT_EMPTY_PATH) fails. > > > > This drawback is not acceptable because it will cause a severe > > performance regression on all architectures with Linux kernel < 6.11. > I don't think the regression will be huge, since statx(fd, NULL, AT_EMPTY_PATH) > will go through the fast path. If the fast path is not available, we choose > (with a flag, maybe permanently afterwards) fstat. This contradicts with what you said: "The drawback is that fstat will not be tried if statx(fd, NULL, AT_EMPTY_PATH) fails." Then on Linux < 6.11 statx(fd, NULL, AT_EMPTY_PATH) definitely fails, and then fstat won't be tried and we end up with stupidly slow statx(fd, "", AT_EMPTY_PATH). > The only cost will be the conversion afterwards. Our benefit will be a > unified implementation. This is not what Linus wants. Linus believes "if the user wants fstat, just use fstat." And this is the reason he pushed the resurrection of fstat. Linus cannot be both correct and incorrect at the same time. Thus we can just consider two possibilities: - If Linus is correct, we should just call fstat instead of statx for when the user wants fstat. - If Linus is incorrect, we should just consider the resurrection of fstat an error and never use it. In either case, we should not "try statx first then fall back to fstat." > Since statx(fd, NULL, AT_EMPTY_PATH) is a new feature for all the architectures, but > loongarch is the only one where fstat is missing And it will always be the only one (besides 32-bit platforms where Glibc always prefer statx for y2038), per Linus. So it's reasonable to treat LoongArch specially here.
> 2024年8月21日 14:29，Xi Ruoyao <xry111@xry111.site> 写道： > > On Wed, 2024-08-21 at 14:08 +0800, Miao Wang wrote: >>>> What if we choose a unified implementation of fstatat64 in the following way: >>>> >>>> 1. if kernel version >= 6.11 >>>> -> directly call statx(fd, NULL, AT_EMPTY_PATH) >>>> 2. or otherwise >>>> -> try to call statx(fd, NULL, AT_EMPTY_PATH), if statx is available >>>> if fails >>> >>> statx(fd, NULL, AT_EMPTY_PATH) is still slower than fstat because glibc >>> needs to convert struct statx to struct stat after the syscall. Thus on >>> kernel version >= 6.11 we should just use fstat for fstat (unless it's a >>> 32-bit platform where fstat suffers y2038). >>> >>>> -> call fstat(fd) instead, if __NR_fstat is defined >>>> -> or call statx(fd, "", AT_EMPTY_PATH), if __NR_fstat is not defined >>>> >>>> So on all architectures, we shall prefer to using statx with the fast kernel >>>> path. The drawback is that fstat will not be tried if statx(fd, NULL, >>>> AT_EMPTY_PATH) fails. >>> >>> This drawback is not acceptable because it will cause a severe >>> performance regression on all architectures with Linux kernel < 6.11. > >> I don't think the regression will be huge, since statx(fd, NULL, AT_EMPTY_PATH) >> will go through the fast path. If the fast path is not available, we choose >> (with a flag, maybe permanently afterwards) fstat. > > This contradicts with what you said: > > "The drawback is that fstat will not be tried if statx(fd, NULL, > AT_EMPTY_PATH) fails." > > Then on Linux < 6.11 statx(fd, NULL, AT_EMPTY_PATH) definitely fails, > and then fstat won't be tried and we end up with stupidly slow statx(fd, > "", AT_EMPTY_PATH). No, it is not contradictory. By saying "fstat will not be tried if statx(NULL) fails.", I mean when glibc is compiled targeted previous kernel versions, it will happen on loongarch kernel whose version is below 6.11, and the behavior remains the same before the introduction of fstat and statx(NULL). By saying "we choose fstat if the fast path is not available", I mean on other architectures where fstat is initially available, faster fstat is still utilized if statx(NULL) is unavailable, with the behavior remaining the same. > >> The only cost will be the conversion afterwards. Our benefit will be a >> unified implementation. > > This is not what Linus wants. > > Linus believes "if the user wants fstat, just use fstat." And this is > the reason he pushed the resurrection of fstat. Linus cannot be both > correct and incorrect at the same time. Thus we can just consider two > possibilities: > > - If Linus is correct, we should just call fstat instead of statx for > when the user wants fstat. > - If Linus is incorrect, we should just consider the resurrection of > fstat an error and never use it. > > In either case, we should not "try statx first then fall back to fstat." So in my proposal, falling back to fstat after trying statx happens on other architectures where fstat is available, and on loongarch if glibc is targeted to kernel version below 6.11 and greater than or equal to 6.10.6. Please note that in my original proposal, the condition of "calling fstat(fd) instead" is that __NR_fstat is defined, instead of fstat is probed to be existing. The reason why I want to avoid probing the existence of fstat on runtime is because all other architectures have fstat, and it will be expected according to the latest change happens to the generic syscall table, i.e. scripts/syscall.tbl, future architectures will possibly also include fstat. Introducing a code path specially for loongarch may increase the maintenance burden, since Adhemerval wanted to avoid arch-specific stat implementations. > >> Since statx(fd, NULL, AT_EMPTY_PATH) is a new feature for all the architectures, but >> loongarch is the only one where fstat is missing > > And it will always be the only one (besides 32-bit platforms where Glibc > always prefer statx for y2038), per Linus. So it's reasonable to treat > LoongArch specially here. > > -- > Xi Ruoyao <xry111@xry111.site> > School of Aerospace Science and Technology, Xidian University
On Wed, 2024-08-21 at 15:04 +0800, Miao Wang wrote: > So in my proposal, falling back to fstat after trying statx happens on other > architectures where fstat is available, and on loongarch if glibc is targeted > to kernel version below 6.11 and greater than or equal to 6.10.6. Please note > that in my original proposal, the condition of "calling fstat(fd) instead" is > that __NR_fstat is defined, instead of fstat is probed to be existing. > > The reason why I want to avoid probing the existence of fstat on runtime is > because all other architectures have fstat, and it will be expected according to the > latest change happens to the generic syscall table, i.e. scripts/syscall.tbl, > future architectures will possibly also include fstat. Introducing a code path > specially for loongarch may increase the maintenance burden, since Adhemerval > wanted to avoid arch-specific stat implementations. I mostly agree with your original proposal but the 6.11 vs 6.10.6 debate. I don't think it's worthy to do a runtime probe just for making 6.1.x or 6.6.x faster.
> 2024年8月21日 15:13，Xi Ruoyao <xry111@xry111.site> 写道： > > On Wed, 2024-08-21 at 15:04 +0800, Miao Wang wrote: >> So in my proposal, falling back to fstat after trying statx happens on other >> architectures where fstat is available, and on loongarch if glibc is targeted >> to kernel version below 6.11 and greater than or equal to 6.10.6. Please note >> that in my original proposal, the condition of "calling fstat(fd) instead" is >> that __NR_fstat is defined, instead of fstat is probed to be existing. >> >> The reason why I want to avoid probing the existence of fstat on runtime is >> because all other architectures have fstat, and it will be expected according to the >> latest change happens to the generic syscall table, i.e. scripts/syscall.tbl, >> future architectures will possibly also include fstat. Introducing a code path >> specially for loongarch may increase the maintenance burden, since Adhemerval >> wanted to avoid arch-specific stat implementations. > > I mostly agree with your original proposal but the 6.11 vs 6.10.6 > debate. I don't think it's worthy to do a runtime probe just for making > 6.1.x or 6.6.x faster. In my previous proposal, the key point is to do a runtime probe for the availability of statx(fd, NULL, AT_EMPTY_PATH), instead of fstat, since this can benefit all the architectures on newer kernels. The cost would be the conversion afterwards, while the benefit would be a unified implementation. I wonder if you are happy with that. I think we still have time for further discussion on how to utilize the new features provided by the kernel. After all we have the patch for the correctness (with 6.11 changed to 6.10.6, as you have requested) as the baseline. > -- > Xi Ruoyao <xry111@xry111.site> > School of Aerospace Science and Technology, Xidian University
On Wed, 2024-08-21 at 15:30 +0800, Miao Wang wrote: > > > 2024年8月21日 15:13，Xi Ruoyao <xry111@xry111.site> 写道： > > > > On Wed, 2024-08-21 at 15:04 +0800, Miao Wang wrote: > > > So in my proposal, falling back to fstat after trying statx happens on other > > > architectures where fstat is available, and on loongarch if glibc is targeted > > > to kernel version below 6.11 and greater than or equal to 6.10.6. Please note > > > that in my original proposal, the condition of "calling fstat(fd) instead" is > > > that __NR_fstat is defined, instead of fstat is probed to be existing. > > > > > > The reason why I want to avoid probing the existence of fstat on runtime is > > > because all other architectures have fstat, and it will be expected according to the > > > latest change happens to the generic syscall table, i.e. scripts/syscall.tbl, > > > future architectures will possibly also include fstat. Introducing a code path > > > specially for loongarch may increase the maintenance burden, since Adhemerval > > > wanted to avoid arch-specific stat implementations. > > > > I mostly agree with your original proposal but the 6.11 vs 6.10.6 > > debate. I don't think it's worthy to do a runtime probe just for making > > 6.1.x or 6.6.x faster. > > In my previous proposal, the key point is to do a runtime probe for the > availability of statx(fd, NULL, AT_EMPTY_PATH), instead of fstat, since > this can benefit all the architectures on newer kernels. The cost would be the > conversion afterwards, while the benefit would be a unified implementation. > I wonder if you are happy with that. It penalizes non-LoongArch 64-bit platforms. They have worked happily but the change will make them slower. So why not just (on top of the original correctness patch): if not (32-bit or (undefined __NR_newfstatat and undefined __NR_fstatat64) ... ... else + r = statx(..., NULL, ...) + if kernel < 6.11 and r == -EFAULT + r = statx(..., "", ...) + endif - r = statx(..., "", ...) if kernel < 4.11 and r == -ENOSYS r = fstat(...) // y2037 issue will happen endif endif Then all 32-bit platforms are benefited, and 64-bit platforms are not penalized? > I think we still have time for further discussion on how to utilize the new > features provided by the kernel. After all we have the patch for the > correctness (with 6.11 changed to 6.10.6, as you have requested) as the > baseline.
On 21/08/24 05:26, Xi Ruoyao wrote: > On Wed, 2024-08-21 at 15:30 +0800, Miao Wang wrote: >> >>> 2024年8月21日 15:13，Xi Ruoyao <xry111@xry111.site> 写道： >>> >>> On Wed, 2024-08-21 at 15:04 +0800, Miao Wang wrote: >>>> So in my proposal, falling back to fstat after trying statx happens on other >>>> architectures where fstat is available, and on loongarch if glibc is targeted >>>> to kernel version below 6.11 and greater than or equal to 6.10.6. Please note >>>> that in my original proposal, the condition of "calling fstat(fd) instead" is >>>> that __NR_fstat is defined, instead of fstat is probed to be existing. >>>> >>>> The reason why I want to avoid probing the existence of fstat on runtime is >>>> because all other architectures have fstat, and it will be expected according to the >>>> latest change happens to the generic syscall table, i.e. scripts/syscall.tbl, >>>> future architectures will possibly also include fstat. Introducing a code path >>>> specially for loongarch may increase the maintenance burden, since Adhemerval >>>> wanted to avoid arch-specific stat implementations. >>> >>> I mostly agree with your original proposal but the 6.11 vs 6.10.6 >>> debate. I don't think it's worthy to do a runtime probe just for making >>> 6.1.x or 6.6.x faster. >> >> In my previous proposal, the key point is to do a runtime probe for the >> availability of statx(fd, NULL, AT_EMPTY_PATH), instead of fstat, since >> this can benefit all the architectures on newer kernels. The cost would be the >> conversion afterwards, while the benefit would be a unified implementation. >> I wonder if you are happy with that. > > It penalizes non-LoongArch 64-bit platforms. They have worked happily > but the change will make them slower. > > So why not just (on top of the original correctness patch): > > if not (32-bit or (undefined __NR_newfstatat and undefined __NR_fstatat64) > ... ... > else > + r = statx(..., NULL, ...) > + if kernel < 6.11 and r == -EFAULT > + r = statx(..., "", ...) > + endif > - r = statx(..., "", ...) > if kernel < 4.11 and r == -ENOSYS > r = fstat(...) // y2037 issue will happen > endif > endif > > Then all 32-bit platforms are benefited, and 64-bit platforms are not > penalized? Just a note that 'io: Do not implement fstat with fstatat' (551101e8240b75) was added mainly to avoid a arch-specific hardening that was making the syscall slower (x86-64 with SMAP-capable CPUs). Ideally I think if kernel provides a composable interface, it also should allow userland to use without the need to be aware of such pitfalls. Also, I think changing fstat64/fstatat64 to use statx instead of the direct syscall is no go (it is a gratuitous performance degradation). I would like to avoid a arch-specific implementation, but it seems that loongarch now assumes a reverse logic: 1. possible stat with 64 bit time_t support and statx as fallback. Where current code assumes: 1. statx with 64 bit time_t support and old stat support as fallback 2. stat with 64 bit time_t support. And since this should *not* be the future generic support (I assume new ABI kernel will just provide __NR_fstat/__NR_newfstatat or whatever instead of just __NR_statx) I think a arch-specific implementation for loongarch would be better. So it is up to you if you want a runtime detection for fstat/newfstatat or if you just do something like: - sysdeps/unix/sysv/linux/loongarch/kernel-features.h #if __LINUX_KERNEL_VERSION > 0x060b00. // or whatever if you decide # define __ASSUME_LOONGARCH_NEWSTATAT #endif - sysdeps/unix/sysv/linux/loongarch/fstatat64.c int __fstatat64 (int fd, const char *file, struct stat64 *buf, int flags) { int r; r = INTERNAL_SYSCALL_CALL (newfstatat, fd, file, buf, flag); #infdef __ASSUME_LOONGARCH_NEWSTATAT if (r == 0) return r; else if (r != -ENOSYS) return INLINE_SYSCALL_ERROR_RETURN_VALUE (r); r = fstatat64_time64_statx (fd, file, buf, flag); #endif return INTERNAL_SYSCALL_ERROR_P (r) ? INLINE_SYSCALL_ERROR_RETURN_VALUE (-r) : 0; } In any case, I don't think changing the generic code to fit the loongarch assumptions is a good idea.
On Wed, 2024-08-21 at 08:55 -0300, Adhemerval Zanella Netto wrote: > Ideally I think if kernel > provides a composable interface, it also should allow userland to use > without the need to be aware of such pitfalls. That's why now statx(..., NULL, AT_EMPTY_PATH, ...), with NULL instead of "" is now allowed with Linux 6.11+. By using NULL instead of "" there's no such a pitfall. And with Linux 6.11+ even statx(..., "", AT_EMPTY_PATH, ...) is faster. Per the kernel developer: statx with AT_EMPTY_PATH paired with "" or NULL argument as appropriate issued on Sapphire Rapids (ops/s): statx(..., "", AT_EMPTY_PATH, ...) on 6.10: 4231237 statx(..., "", AT_EMPTY_PATH, ...) on 6.11: 5944063 (+40%) statx(..., NULL, AT_EMPTY_PATH, ...) on 6.11: 6601619 (+11%/+56%) It's a different topic from this LoongArch specific change. It'll benefit all 32-bit platforms where we have to use statx for fstat to solve y2038. > So it is up to you if you want a runtime detection for fstat/newfstatat My purposed change to the generic 32-bit code (using NULL instead of "" if possible calling statx for fstat) is a different topic, not really related to LoongArch. To me for LoongArch a configuration-time decision is enough, I really do **not** want a runtime detection just benefiting 6.1.x and 6.6.x but with the cost of some maintenance churn.
On 21/08/24 09:09, Xi Ruoyao wrote: > On Wed, 2024-08-21 at 08:55 -0300, Adhemerval Zanella Netto wrote: > >> Ideally I think if kernel >> provides a composable interface, it also should allow userland to use >> without the need to be aware of such pitfalls. > > That's why now statx(..., NULL, AT_EMPTY_PATH, ...), with NULL instead > of "" is now allowed with Linux 6.11+. By using NULL instead of "" > there's no such a pitfall. And with Linux 6.11+ even statx(..., "", > AT_EMPTY_PATH, ...) is faster. Per the kernel developer: > > statx with AT_EMPTY_PATH paired with "" or NULL argument as > appropriate issued on Sapphire Rapids (ops/s): > statx(..., "", AT_EMPTY_PATH, ...) on 6.10: 4231237 > statx(..., "", AT_EMPTY_PATH, ...) on 6.11: 5944063 (+40%) > statx(..., NULL, AT_EMPTY_PATH, ...) on 6.11: 6601619 (+11%/+56%) > > It's a different topic from this LoongArch specific change. It'll > benefit all 32-bit platforms where we have to use statx for fstat to > solve y2038. > This would be a different patch then, something like: * sysdeps/unix/sysv/linux/kernel-features.h #if __LINUX_KERNEL_VERSION >= 0x060B00 # define __STATX_EMPTY_PATHNAME_ARG NULL #else # define __STATX_EMPTY_PATHNAME_ARG "" #endif And replace the pathname argument with on fstat.c/fstat64.c. >> So it is up to you if you want a runtime detection for fstat/newfstatat > > My purposed change to the generic 32-bit code (using NULL instead of "" > if possible calling statx for fstat) is a different topic, not really > related to LoongArch. To me for LoongArch a configuration-time decision > is enough, I really do **not** want a runtime detection just benefiting > 6.1.x and 6.6.x but with the cost of some maintenance churn. Fair enough.
> 2024年8月21日 20:24，Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> 写道： > > > > On 21/08/24 09:09, Xi Ruoyao wrote: >> On Wed, 2024-08-21 at 08:55 -0300, Adhemerval Zanella Netto wrote: >> >>> Ideally I think if kernel >>> provides a composable interface, it also should allow userland to use >>> without the need to be aware of such pitfalls. >> >>> Also, I think changing fstat64/fstatat64 to use statx instead of the >>> direct syscall is no go (it is a gratuitous performance degradation). >>> I would like to avoid a arch-specific implementation, but it seems that >>> loongarch now assumes a reverse logic: >>> >>> 1. possible stat with 64 bit time_t support and statx as fallback. >>> >>> Where current code assumes: >>> >>> 1. statx with 64 bit time_t support and old stat support as fallback >>> 2. stat with 64 bit time_t support. Yes, you have pointed out the key problem. As a result, We all agree that introducing dynamic detection for the existence of fstat to the generic fstat implementation would add another layer of mess. >> >> That's why now statx(..., NULL, AT_EMPTY_PATH, ...), with NULL instead >> of "" is now allowed with Linux 6.11+. By using NULL instead of "" >> there's no such a pitfall. And with Linux 6.11+ even statx(..., "", >> AT_EMPTY_PATH, ...) is faster. Per the kernel developer: >> >> statx with AT_EMPTY_PATH paired with "" or NULL argument as >> appropriate issued on Sapphire Rapids (ops/s): >> statx(..., "", AT_EMPTY_PATH, ...) on 6.10: 4231237 >> statx(..., "", AT_EMPTY_PATH, ...) on 6.11: 5944063 (+40%) >> statx(..., NULL, AT_EMPTY_PATH, ...) on 6.11: 6601619 (+11%/+56%) >> >> It's a different topic from this LoongArch specific change. It'll >> benefit all 32-bit platforms where we have to use statx for fstat to >> solve y2038. >> > > This would be a different patch then, something like: > > * sysdeps/unix/sysv/linux/kernel-features.h > > #if __LINUX_KERNEL_VERSION >= 0x060B00 > # define __STATX_EMPTY_PATHNAME_ARG NULL > #else > # define __STATX_EMPTY_PATHNAME_ARG "" > #endif > > And replace the pathname argument with on fstat.c/fstat64.c. I'm in favor of Ruoyao's suggestion, enabling runtime detection of this feature, so that other 32-bit platforms and loongarch will benefit from this. I'll later send out the patches for your review. Cheers, Miao Wang
diff --git a/sysdeps/unix/sysv/linux/loongarch/kernel-features.h b/sysdeps/unix/sysv/linux/loongarch/kernel-features.h new file mode 100644 index 0000000000..c0e7ccb5de --- /dev/null +++ b/sysdeps/unix/sysv/linux/loongarch/kernel-features.h @@ -0,0 +1,27 @@ +/* Set flags signalling availability of kernel features based on given + kernel version number. Loongarch version. + Copyright (C) 2024-2024 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_next <kernel-features.h> + +#define __ASSUME_LOONGARCH_NEWSTAT 1 + +/* No support for fstat or newfstatat before 6.11. */ +#if __LINUX_KERNEL_VERSION < 0x060b00 +# undef __ASSUME_LOONGARCH_NEWSTAT +#endif diff --git a/sysdeps/unix/sysv/linux/loongarch/sysdep.h b/sysdeps/unix/sysv/linux/loongarch/sysdep.h index eb0ba790da..1fdf18197f 100644 --- a/sysdeps/unix/sysv/linux/loongarch/sysdep.h +++ b/sysdeps/unix/sysv/linux/loongarch/sysdep.h @@ -109,6 +109,11 @@ #undef SYS_ify #define SYS_ify(syscall_name) __NR_##syscall_name +#ifndef __ASSUME_LOONGARCH_NEWSTAT +#undef __NR_fstat +#undef __NR_newfstatat +#endif + #ifndef __ASSEMBLER__ #define VDSO_NAME "LINUX_5.10"