[v2] Loongarch: adapt for the re-introduction of fstat and newfstatat in 6.11

Message ID 20240820-loong-fstat-v2-1-5a254fef05ce@gmail.com
State Superseded
Headers
Series [v2] Loongarch: adapt for the re-introduction of fstat and newfstatat in 6.11 |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed

Commit Message

Miao Wang via B4 Relay Aug. 20, 2024, 2:12 p.m. UTC
  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>
---
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(+)


---
base-commit: 2eee835eca960c9d4119279804214b7a1ed5d156
change-id: 20240820-loong-fstat-f57f8be48575

Best regards,
  

Comments

Xi Ruoyao Aug. 20, 2024, 3:06 p.m. UTC | #1
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,
  
Xi Ruoyao Aug. 20, 2024, 3:11 p.m. UTC | #2
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.
  
Miao Wang Aug. 20, 2024, 4:01 p.m. UTC | #3
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
  
Xi Ruoyao Aug. 20, 2024, 4:34 p.m. UTC | #4
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).
  
Adhemerval Zanella Netto Aug. 20, 2024, 6:15 p.m. UTC | #5
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...)
  
Adhemerval Zanella Netto Aug. 20, 2024, 6:42 p.m. UTC | #6
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...)
  
Miao Wang Aug. 20, 2024, 8:24 p.m. UTC | #7
> 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.
  
Xi Ruoyao Aug. 21, 2024, 5:21 a.m. UTC | #8
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).
  
Miao Wang Aug. 21, 2024, 6:08 a.m. UTC | #9
> 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
  
Xi Ruoyao Aug. 21, 2024, 6:29 a.m. UTC | #10
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.
  
Miao Wang Aug. 21, 2024, 7:04 a.m. UTC | #11
> 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
  
Xi Ruoyao Aug. 21, 2024, 7:13 a.m. UTC | #12
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.
  
Miao Wang Aug. 21, 2024, 7:30 a.m. UTC | #13
> 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
  
Xi Ruoyao Aug. 21, 2024, 8:26 a.m. UTC | #14
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.
  
Adhemerval Zanella Netto Aug. 21, 2024, 11:55 a.m. UTC | #15
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.
  
Xi Ruoyao Aug. 21, 2024, 12:09 p.m. UTC | #16
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.
  
Adhemerval Zanella Netto Aug. 21, 2024, 12:24 p.m. UTC | #17
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.
  
Miao Wang Aug. 21, 2024, 12:43 p.m. UTC | #18
> 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
  

Patch

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"