From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2f.google.com (mail-oa1-x2f.google.com [IPv6:2001:4860:4864:20::2f]) by sourceware.org (Postfix) with ESMTPS id 91DF13857725 for ; Tue, 5 Sep 2023 21:03:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 91DF13857725 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oa1-x2f.google.com with SMTP id 586e51a60fabf-1cca7cf6e01so1745134fac.0 for ; Tue, 05 Sep 2023 14:03:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693947822; x=1694552622; darn=sourceware.org; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=79xIRu3izyk6bBJ6cHq6eqZNWP9G+fNWsnzwD1LdE9U=; b=gA0Xvn9BnsYRVY0K/R0lm3+fNVdPY2LPvwPad4GUxWqNJtgOQuWu+6UOIQcaZ2bj+u IYFJXMX5Dg1CqCVzHGKqUEhPxj886RlJGpshoAHGEzvgtpOp+e2XBQ+2+TnjSL/JxOUa +dV6seqDyN5+VGToIvfbdKBtDUwKG3xVBFQj4gtIx+o6uC0Y3JYcc4sC7OqmNV3iTrxz wda2sGb5yJEhQDObw5lWl+gC2wBYi+HuZNF+B3PaIqgmVFT/RVOBi+Y3hHE8FMCwAzat Es/AeJZKIrAju7vRhm1G2LiK+fB9Owa6tPClwfTUDnY/XiMKGEtul9dKnpn+r1g3RMgC 4RYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693947822; x=1694552622; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=79xIRu3izyk6bBJ6cHq6eqZNWP9G+fNWsnzwD1LdE9U=; b=JxfFa7BS4qpEBgHRT+ufJyxv13ARelrWp0o9Bc2kniwW4XxTBDdQj69DHzXLc8LkHF KStMMA0kRGXFr3L+JnzxehR6L72WFNGqWEhlyYovljpsRPE61PidsADcEFjWY7LYUYjW Qp/VXQrCcd1Rz7p3SfYLd1pyq+D0gx7DqJCw28FsgKO7F1urNJu0OZL2kHkxeGD4s8OG FHKUXZOZ7heBZ0Eu1g6zflPOgc5vTxmquei1IaQR6cpI8s6UO3SMzEkESDMYgstxGGBH 2r5v6I6LETMkW2Q+tD9kGCgjkXkkJJk8mm7bAGHgGXAm8mHzSjvlI2r1askzxS8fn0p5 HFtg== X-Gm-Message-State: AOJu0Yy+nTqbO07jvAPu/vLBvbE16bhDxeLzAQ1RR1h7juIMMOCzOfkX DdVtF5IwVjBvT5eWyG7dWClW8ebwT4sTM6ubawtUjPvh X-Google-Smtp-Source: AGHT+IHmJkR5M+AcdU64+0JywBnIrKnjirQyFt8vu3XDGn6JEmx3qeCTLmqav8FRKTYpLpVnLIMw5+BbU9YPU4ph9hE= X-Received: by 2002:a05:6870:9d8c:b0:1d4:d016:229f with SMTP id pv12-20020a0568709d8c00b001d4d016229fmr7924454oab.10.1693947821651; Tue, 05 Sep 2023 14:03:41 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:745a:0:b0:4f0:1250:dd51 with HTTP; Tue, 5 Sep 2023 14:03:41 -0700 (PDT) In-Reply-To: <5b4cb21e-5bb0-5015-4c4b-f4fb4b9d2892@linaro.org> References: <20230905203421.2127750-1-adhemerval.zanella@linaro.org> <5b4cb21e-5bb0-5015-4c4b-f4fb4b9d2892@linaro.org> From: Mateusz Guzik Date: Tue, 5 Sep 2023 23:03:41 +0200 Message-ID: Subject: Re: [PATCH] io: Do not implement fstat with fstatat To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org, Linus Torvalds Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 9/5/23, Adhemerval Zanella Netto wrote: > > > On 05/09/23 17:48, Mateusz Guzik wrote: >> On 9/5/23, Adhemerval Zanella wrote: >>> AT_EMPTY_PATH is a requirement to implement fstat over fstatat, >>> however it does not prevent the kernel to read the path argument. >>> It is not an issue, but it has performance implications on x86_64 >>> with SMAP enable, since each newfstatat syscall generates a pagefault >>> even with an empty path. >>> >> >> This does not generate page faults in the usual case. >> >> How about: >> >> On x86-64 with SMAP-capable CPUs (vast majority today) the kernel is >> forced to perform expensive user memory access. After that regular >> lookup is performed which adds even more overhead. > > Alright, I have changed to: > > AT_EMPTY_PATH is a requirement to implement fstat over fstatat, > however it does not prevent the kernel to read the path argument. > It is not an issue, but on x86-64 with SMAP-capable CPUs the kernel is > forced to perform expensive user memory access. After that regular > lookup is performed which adds even more overhead. > LGTM >> >> I have no commentso n the patch itself. >> >> Thanks for quick turn around. >> >> >>> Instead, issue the fstat syscall directly on LFS fstat implementation >>> (32 bit architectures will still continue to use statx, which is >>> required to have 64 bit time_t support). it should be even a >>> small performance gain on non x86_64, since there is no need >>> to handle the path argument. >>> >>> Checked on x86_64-linux-gnu. >>> --- >>> sysdeps/unix/sysv/linux/fstat64.c | 37 +++++++++++++++++++++++-- >>> sysdeps/unix/sysv/linux/fstatat64.c | 12 ++------ >>> sysdeps/unix/sysv/linux/internal-stat.h | 31 +++++++++++++++++++++ >>> 3 files changed, 68 insertions(+), 12 deletions(-) >>> create mode 100644 sysdeps/unix/sysv/linux/internal-stat.h >>> >>> diff --git a/sysdeps/unix/sysv/linux/fstat64.c >>> b/sysdeps/unix/sysv/linux/fstat64.c >>> index 124384e57f..a7f4935a01 100644 >>> --- a/sysdeps/unix/sysv/linux/fstat64.c >>> +++ b/sysdeps/unix/sysv/linux/fstat64.c >>> @@ -19,20 +19,53 @@ >>> #define __fstat __redirect___fstat >>> #define fstat __redirect_fstat >>> #include >>> +#undef __fstat >>> +#undef fstat >>> #include >>> -#include >>> -#include >>> +#include >>> #include >>> >>> int >>> __fstat64_time64 (int fd, struct __stat64_t64 *buf) >>> { >>> +#if !FSTATAT_USE_STATX >>> +# if XSTAT_IS_XSTAT64 >>> +# ifdef __NR_newfstatat >>> + /* 64-bit kABI, e.g. aarch64, ia64, powerpc64*, s390x, riscv64, and >>> + x86_64. */ >>> + return INLINE_SYSCALL_CALL (fstat, fd, buf); >>> +# elif defined __NR_fstatat64 >>> +# if STAT64_IS_KERNEL_STAT64 >>> + /* 64-bit kABI outlier, e.g. alpha */ >>> + return INLINE_SYSCALL_CALL (fstat64, fd, buf); >>> +# else >>> + /* 64-bit kABI outlier, e.g. sparc64. */ >>> + struct kernel_stat64 kst64; >>> + int r = INLINE_SYSCALL_CALL (fstat64, fd, &kst64); >>> + if (r == 0) >>> + __cp_stat64_kstat64 (buf, &kst64); >>> + return r; >>> +# endif /* STAT64_IS_KERNEL_STAT64 */ >>> +# endif >>> +# else /* XSTAT_IS_XSTAT64 */ >>> + /* 64-bit kabi outlier, e.g. mips64 and mips64-n32. */ >>> + struct kernel_stat kst; >>> + int r = INLINE_SYSCALL_CALL (fstat, fd, &kst); >>> + if (r == 0) >>> + __cp_kstat_stat64_t64 (&kst, buf); >>> + return r; >>> +# endif >>> +#else /* !FSTATAT_USE_STATX */ >>> + /* All kABIs with non-LFS support and with old 32-bit time_t support >>> + e.g. arm, csky, i386, hppa, m68k, microblaze, nios2, sh, >>> powerpc32, >>> + and sparc32. */ >>> if (fd < 0) >>> { >>> __set_errno (EBADF); >>> return -1; >>> } >>> return __fstatat64_time64 (fd, "", buf, AT_EMPTY_PATH); >>> +#endif >>> } >>> #if __TIMESIZE != 64 >>> hidden_def (__fstat64_time64) >>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c >>> b/sysdeps/unix/sysv/linux/fstatat64.c >>> index 3509d3ca6d..127c6ff601 100644 >>> --- a/sysdeps/unix/sysv/linux/fstatat64.c >>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c >>> @@ -21,12 +21,10 @@ >>> #include >>> #include >>> #include >>> -#include >>> #include >>> #include >>> -#include >>> -#include >>> #include >>> +#include >>> >>> #if __TIMESIZE == 64 \ >>> && (__WORDSIZE == 32 \ >>> @@ -40,11 +38,7 @@ _Static_assert (sizeof (__blkcnt_t) == sizeof >>> (__blkcnt64_t), >>> "__blkcnt_t and __blkcnt64_t must match"); >>> #endif >>> >>> -#if (__WORDSIZE == 32 \ >>> - && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ >>> - || defined STAT_HAS_TIME32 \ >>> - || (!defined __NR_newfstatat && !defined __NR_fstatat64) >>> -# define FSTATAT_USE_STATX 1 >>> +#if FSTATAT_USE_STATX >>> >>> static inline int >>> fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 >>> *buf, >>> @@ -79,8 +73,6 @@ fstatat64_time64_statx (int fd, const char *file, >>> struct >>> __stat64_t64 *buf, >>> >>> return r; >>> } >>> -#else >>> -# define FSTATAT_USE_STATX 0 >>> #endif >>> >>> /* Only statx supports 64-bit timestamps for 32-bit architectures with >>> diff --git a/sysdeps/unix/sysv/linux/internal-stat.h >>> b/sysdeps/unix/sysv/linux/internal-stat.h >>> new file mode 100644 >>> index 0000000000..e3b0569853 >>> --- /dev/null >>> +++ b/sysdeps/unix/sysv/linux/internal-stat.h >>> @@ -0,0 +1,31 @@ >>> +/* Internal stat definitions. >>> + Copyright (C) 2023 Free Software Foundation, Inc. >>> + This file is part of the GNU C Library. >>> + >>> + The GNU C Library is free software; you can redistribute it and/or >>> + modify it under the terms of the GNU Lesser General Public >>> + License as published by the Free Software Foundation; either >>> + version 2.1 of the License, or (at your option) any later version. >>> + >>> + The GNU C Library is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + Lesser General Public License for more details. >>> + >>> + You should have received a copy of the GNU Lesser General Public >>> + License along with the GNU C Library; if not, see >>> + . */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#if (__WORDSIZE == 32 \ >>> + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \ >>> + || defined STAT_HAS_TIME32 \ >>> + || (!defined __NR_newfstatat && !defined __NR_fstatat64) >>> +# define FSTATAT_USE_STATX 1 >>> +#else >>> +# define FSTATAT_USE_STATX 0 >>> +#endif >>> -- >>> 2.34.1 >>> >>> >> >> > -- Mateusz Guzik