From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2c.google.com (mail-oa1-x2c.google.com [IPv6:2001:4860:4864:20::2c]) by sourceware.org (Postfix) with ESMTPS id 191063858D28 for ; Tue, 5 Sep 2023 19:17:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 191063858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x2c.google.com with SMTP id 586e51a60fabf-1d14bb44ad1so2371188fac.1 for ; Tue, 05 Sep 2023 12:17:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1693941424; x=1694546224; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:organization:references:cc:to :from:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=xbBQaLxJtSllUOCV/iz9Bh0qZVNGyLgnwQ72RLTuUxQ=; b=i19BCAx7v0UPAR2X24ebrfiflKtsGPJJm4TR3OUXxvLM9mouXM5wH0iUvQpfW+WINa QkmkZKwG6O9LNMxgvJcsJ2fUR2JRvmYtiz3uU76cxX7DwDkXTAP80W8i021eDChDlu3j km46XipfiB9Q2GPohyeXGVMUcpXeWi62Aaa3ZvS+SPaFpwTzpSMIm+rj5zfysvcLKhAG I/GkTi+MdOAZ0Ro/gxpWOUHj0onwoG2mn5fKxS1gtuR798EjJKT4bgJJtfW0lwNG+YUq JeDAPQGqHtceqSL8ChcXOS/sj/XqfxS/Phze4qWSq1w0iKPnvmluCvkPG3Biq4XJ6cY9 eF3A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693941424; x=1694546224; h=content-transfer-encoding:in-reply-to:organization:references:cc:to :from:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=xbBQaLxJtSllUOCV/iz9Bh0qZVNGyLgnwQ72RLTuUxQ=; b=K2U927m+q1V93FVqlrUCt5dSjuPMcy1iQVartwyvfIc++RfEhN8uwXo+8AQvYN8zbu 1Ebh4ps3t4zSuXh3JEXEb7E2BaOxg8vFcbtkCM8ZN02v4mRbM05ZyM5Byst9Rj50vBRX OYYEklks67qKtLVcZkyXBD/zZAOEENcicRHiBywGhBkGSqB1wwQj07egHpBfi5tXqX7d Ix8fps9Z3GJEC+FmegDYjVD90dP+ZTrlfVw5RnvOc8Y4NSfgHo7+TGk13Q4o45l41EhV PfZ6wUarr7phL/N5aVdCrpDBZSHd/e5XyrEF7hVxphHnpBHloYsm38sj4NPuwNoVHpNP SK8g== X-Gm-Message-State: AOJu0Yzfs9Bs2DOXb/pdm074x1BelX5I54EZNSN0NpB1nEh2vSISjWMc OBOs+OMyqgSTc6t1KMh3X0Wk0Q== X-Google-Smtp-Source: AGHT+IFZE7Qy2TvgL6O8XqvvK+0kJOn/S2bB0NUneSl+48kwX2phRhhydcFHLJFY2WaxiWsS/RCGVw== X-Received: by 2002:a05:6870:6c0d:b0:1d2:4b4c:eb11 with SMTP id na13-20020a0568706c0d00b001d24b4ceb11mr11890296oab.32.1693941423838; Tue, 05 Sep 2023 12:17:03 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c3:fee4:a053:a54:7b70:a25? ([2804:1b3:a7c3:fee4:a053:a54:7b70:a25]) by smtp.gmail.com with ESMTPSA id l23-20020a0568301d7700b006b9a98b9659sm5762750oti.19.2023.09.05.12.17.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 05 Sep 2023 12:17:03 -0700 (PDT) Message-ID: Date: Tue, 5 Sep 2023 16:16:58 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 Subject: Re: fstat(2) penalized by using newfstatat(6, "", buf, AT_EMPTY_PATH) Content-Language: en-US From: Adhemerval Zanella Netto To: Linus Torvalds Cc: Mateusz Guzik , Andreas Schwab , Rich Felker , Mateusz Guzik via Libc-alpha References: <680b330d-6ef3-adc5-9ba6-cf74dd53e422@linaro.org> <6d0e4e9e-ab69-0c73-bb9d-ce344b4a043b@linaro.org> <387426d5-17c8-cf3e-83a1-88b55c9d22a1@linaro.org> Organization: Linaro In-Reply-To: <387426d5-17c8-cf3e-83a1-88b55c9d22a1@linaro.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,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 05/09/23 15:22, Adhemerval Zanella Netto wrote: > > > On 05/09/23 14:45, Linus Torvalds wrote: >> On Tue, 5 Sept 2023 at 10:28, Adhemerval Zanella Netto >> wrote: >>> >>> I think we can still make it in a generic way, stat family would use more >>> syscall (which some filters might complain) but it should be ok: >> >> Ugh. That patch of yours certainly seems to avoid the kernel overhead >> (where that "fetch a byte from user space just to check it is '\0'" is >> much more expensive than it is in user space), but why does glibc do >> that whole "turn fstat() into fstatat(), and then turn it back again"? >> >> It seems just stupid. If the user asked for 'fstat()', just give the >> user 'fstat()'. >> >> Your patch literally adds *complexity*, rather than take it away. >> >> Is this all just because glibc did its own 'struct stat' >> implementation way back when, and you want to have just one place >> where the stat conversion is done? >> >> Can't you just special case *that* case instead? It's long been >> irrelevant. Afaik, on 32-bit x86, you just want to use the 'stat64' >> system call family, and on 64-bit x86 you just use the regular 'stat' >> family (yes, called 'newfstatat' for that one system call for internal >> historial reasons), and there is no conversion needed. >> >> IOW, all those wrappers just for 'struct stat' conversion are pure >> overhead and silly garbage. Why do they exist? And if there is still >> some other reason for them to exist, can't you make that be a >> 'sysdeps' file of its own, and not penalize all the sane cases with >> 'copy stat structures around'? > > The old glibc stat code was a complete mess that was added prior symbol > versioning, with multiple implementations, along with a static linkage > that defines the current version (there is no need to describe it > further). The consolidation also provide a much simpler way to support > y2038. > > On x86_64 and some other 64 bits ABIs where glibc exported struct stat > is the same as the kernel, there is no conversion whatsoever. It is > only required on some ABI where due historical mistakes the exported > user struct does not match kernel (sparc64, mips64) and on 32 bits > where the no-LFS support is just routed to LFS interface to simplify > things. > > So the fstat to fstatat approach is a way to simplify the code and put > all the required syscall logic only at fstatat implementation. By adding > the 'special' case back on fstat.c it would require to replicate all this > logic plus when to use statx instead (FSTATAT_USE_STATX), which is far > from ideal. It is doable, but not really simpler... Below it is the stat logic implemented directly on stat64.c code. It avoids the "flag == AT_EMPTY_PATH ..." check, but it only handles LFS calls where statx is not required (so 32 bit architectures will still continue to use statx, but I don't think this would be a problem). It not really simpler than add the logic on fstatat64.c, but it should be ok. 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