From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc33.google.com (mail-oo1-xc33.google.com [IPv6:2607:f8b0:4864:20::c33]) by sourceware.org (Postfix) with ESMTPS id 86A7C3858D35 for ; Tue, 5 Sep 2023 20:48:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 86A7C3858D35 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-oo1-xc33.google.com with SMTP id 006d021491bc7-573449a364fso1919915eaf.1 for ; Tue, 05 Sep 2023 13:48:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693946908; x=1694551708; 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=M3FKuSu8bo5LrLfZFkc3jJ1B5Jttbn1FT1wyua/GwTo=; b=PPeGJIvD/cOHqzH/VaPvB0Ht3543DG9aWXg4DSNa0fV+IsAk8BrRYUKkMIXAq9lIIs QMec1mCpjd7ROwOtCL49+//IJCDp8D/yN/Xs7qOzgx8rV8GvUIBvBu+EITn58rSjNvUt pbxKtA6gmjWOyjRRcSFgze5ARqY2Vk1OYycxLaVC1vNCcYwqYxWOYmU7NUA5KH2n+S+s uz0NT60Wf8zeK7wbYdDPFYiPv/CVECzivHI9USN+/NzRMC6mtubR/6y98R0D8DdqOnhq L3zbLKz/m6Sm0eYJfkTn1pGwl2sMwOJ3i3YkwyZ5cUFwRuoylKvpWFsEujYGEUaSfJGr +d9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693946908; x=1694551708; 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=M3FKuSu8bo5LrLfZFkc3jJ1B5Jttbn1FT1wyua/GwTo=; b=R7rYh5Lp4L4O4SOQpYviWbOyzJM+6hTn3qn5wx2ppquV+gbGZVH9X6qEqZTuVCQZSp K+1mcHAxIj5J3W2D9F4pT1qIO+pml+xCXpE/kj1vQD8OKDnD2SgREoI/CIxKRY3h7D3p QVWt55M7IbB2WnnWyQc297IVFZmAoXpXc51syyWzpu7JqvklPI4aER5Z0xIxmKzqDgkt YiAeaPZx4IeZsZcXh+9VqZ77VRT5RbvGhsK+SnBidaUlwC398M9Dr7DRiWOu5vCDWaP8 Wpa8WDQELXpYay1uDmeufbcXoc4gFFQBXhKLnMWwaIH7apCrffSuCMy/uYvYugAmLbTl SaSA== X-Gm-Message-State: AOJu0Yx3HB53o4DkmlVqjBtUfLnx+V/akyvij6pq8wZgDnFaa5dJg+t9 iJBsqGssOegXiyIThfwRXROp2/buSnHap5t/JfgXB3EB X-Google-Smtp-Source: AGHT+IE7BP7Ra6KN0ChqycO5QFYm2PZ30LJeIK6Lm7t6wL37fbtD2AKnx+dIopMUpWsn3sIemMo2hrPOFBW/cgZvsQY= X-Received: by 2002:a4a:92d5:0:b0:571:281a:ef7 with SMTP id j21-20020a4a92d5000000b00571281a0ef7mr11711824ooh.7.1693946907714; Tue, 05 Sep 2023 13:48:27 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:745a:0:b0:4f0:1250:dd51 with HTTP; Tue, 5 Sep 2023 13:48:27 -0700 (PDT) In-Reply-To: <20230905203421.2127750-1-adhemerval.zanella@linaro.org> References: <20230905203421.2127750-1-adhemerval.zanella@linaro.org> From: Mateusz Guzik Date: Tue, 5 Sep 2023 22:48:27 +0200 Message-ID: Subject: Re: [PATCH] io: Do not implement fstat with fstatat To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Linus Torvalds Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 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 wrote: > AT_EMPTY_PATH is a requirement to implement fstat over fstatat, > however it does not prevent the kernel to read the path argument. > It is not an issue, but it has performance implications on x86_64 > with SMAP enable, since each newfstatat syscall generates a pagefault > even with an empty path. > This does not generate page faults in the usual case. How about: On x86-64 with SMAP-capable CPUs (vast majority today) the kernel is forced to perform expensive user memory access. After that regular lookup is performed which adds even more overhead. I have no commentso n the patch itself. Thanks for quick turn around. > Instead, issue the fstat syscall directly on LFS fstat implementation > (32 bit architectures will still continue to use statx, which is > required to have 64 bit time_t support). it should be even a > small performance gain on non x86_64, since there is no need > to handle the path argument. > > Checked on x86_64-linux-gnu. > --- > sysdeps/unix/sysv/linux/fstat64.c | 37 +++++++++++++++++++++++-- > sysdeps/unix/sysv/linux/fstatat64.c | 12 ++------ > sysdeps/unix/sysv/linux/internal-stat.h | 31 +++++++++++++++++++++ > 3 files changed, 68 insertions(+), 12 deletions(-) > create mode 100644 sysdeps/unix/sysv/linux/internal-stat.h > > diff --git a/sysdeps/unix/sysv/linux/fstat64.c > b/sysdeps/unix/sysv/linux/fstat64.c > index 124384e57f..a7f4935a01 100644 > --- a/sysdeps/unix/sysv/linux/fstat64.c > +++ b/sysdeps/unix/sysv/linux/fstat64.c > @@ -19,20 +19,53 @@ > #define __fstat __redirect___fstat > #define fstat __redirect_fstat > #include > +#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