From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) by sourceware.org (Postfix) with ESMTPS id 5C0873856DC1 for ; Thu, 23 Jun 2022 04:15:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5C0873856DC1 Received: by mail-pl1-x632.google.com with SMTP id l6so8934374plg.11 for ; Wed, 22 Jun 2022 21:15:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6N9ylMuyEN9Dt9NGTtgLxwM3+BL/u+6Qqvb8jX1BAIc=; b=YlMb1UgxuXNL09rNhqWuMLlBLkFAWOr8RjcjROCMKM46fE8xY9OwviXbNabV0rMLm+ KgJBNgRPCU7hkTPB+yN0fUhj6zQKn1u8fRPsSLNGYFEXhl5xvNXgqi0rmLMeBqrEOu+5 dL3r97jMTNc/W/rKPZcoTFJyxbMyjOb0rOF9s/eft0wwKWgEuuRWE0QygPWruf57W15b Uoqan9zjZJYeEJyrgS+Wi3xwOThOKu157wOaWnPeJyN0UfZxKtOlG1sJoqs01qNJSO3q 4161JC6feVaOM0Lu5+VmUOG8ZbIxFdPGNUysem2QWC1D6hmyQngJFZq1hReIXutB8UNh 2iHw== X-Gm-Message-State: AJIora849pdBERW4OtYTqe86fuyoUrytNqhkJtRDBjNft8lAt9nrDezb iCI6HQJFLVX3lgZ6nbxkStk3iq/8vIrRFQ== X-Google-Smtp-Source: AGRyM1uVpriyMxyjxurVTmgvYfUkSRg9/R2WqoIKXQ9Yks1EKyTP6ifL99V66evf6FsnFE0DF9HisA== X-Received: by 2002:a17:90b:1bc8:b0:1ec:881d:86ad with SMTP id oa8-20020a17090b1bc800b001ec881d86admr1936503pjb.4.1655957746240; Wed, 22 Jun 2022 21:15:46 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:7eb6:9fa:5199:e1da]) by smtp.gmail.com with ESMTPSA id jj4-20020a170903048400b001678898ad06sm7622923plb.47.2022.06.22.21.15.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jun 2022 21:15:45 -0700 (PDT) Date: Wed, 22 Jun 2022 21:15:43 -0700 From: Fangrui Song To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Wilco Dijkstra Subject: Re: [PATCH v3 1/4] misc: Optimize internal usage of __libc_single_threaded Message-ID: <20220623041543.dfkbmoaqlgnkfr7h@google.com> References: <20220621203225.714328-1-adhemerval.zanella@linaro.org> <20220621203225.714328-2-adhemerval.zanella@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20220621203225.714328-2-adhemerval.zanella@linaro.org> X-Spam-Status: No, score=-27.4 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_FILL_THIS_FORM_SHORT, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Jun 2022 04:15:49 -0000 On 2022-06-21, Adhemerval Zanella wrote: >By adding an internal alias to avoid the GOT indirection. >On some architecture, __libc_single_thread may be accessed through >copy relocations and thus it requires to update also the copies >default copy. > >This is done by adding a new internal macro, >libc_hidden_data_{proto,def}, which has an addition argument that >specifies the alias name (instead of default __GI_ one). > >Checked on x86_64-linux-gnu and i686-linux-gnu. >--- > elf/libc_early_init.c | 2 +- > include/libc-symbols.h | 22 ++++++++++++++++++++++ > include/sys/single_threaded.h | 11 +++++++++++ > misc/single_threaded.c | 2 ++ > nptl/pthread_create.c | 5 ++++- > posix/fork.c | 2 +- > sysdeps/nptl/setxid.h | 2 +- > 7 files changed, 42 insertions(+), 4 deletions(-) > >diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c >index 3c4a19cf6b..affc46fefc 100644 >--- a/elf/libc_early_init.c >+++ b/elf/libc_early_init.c >@@ -38,7 +38,7 @@ __libc_early_init (_Bool initial) > __libc_single_threaded = initial; > > #ifdef SHARED >- __libc_initial = initial; >+ __libc_single_threaded_internal = __libc_initial = initial; > #endif > > __pthread_early_init (); >diff --git a/include/libc-symbols.h b/include/libc-symbols.h >index 4bb3d8c7ba..4fde839471 100644 >--- a/include/libc-symbols.h >+++ b/include/libc-symbols.h >@@ -534,11 +534,15 @@ for linking") > __attribute__ ((visibility ("hidden"), ##attrs)) > # define hidden_proto(name, attrs...) \ > __hidden_proto (name, , __GI_##name, ##attrs) >+# define hidden_proto_alias(name, alias, attrs...) \ >+ __hidden_proto_alias (name, , alias, ##attrs) > # define hidden_tls_proto(name, attrs...) \ > __hidden_proto (name, __thread, __GI_##name, ##attrs) > # define __hidden_proto(name, thread, internal, attrs...) \ > extern thread __typeof (name) name __asm__ (__hidden_asmname (#internal)) \ > __hidden_proto_hiddenattr (attrs); >+# define __hidden_proto_alias(name, thread, internal, attrs...) \ >+ extern thread __typeof (name) internal __hidden_proto_hiddenattr (attrs); > # define __hidden_asmname(name) \ > __hidden_asmname1 (__USER_LABEL_PREFIX__, name) > # define __hidden_asmname1(prefix, name) __hidden_asmname2(prefix, name) >@@ -554,7 +558,10 @@ for linking") > # define hidden_ver(local, name) __hidden_ver1(local, __GI_##name, name); > # define hidden_data_ver(local, name) hidden_ver(local, name) > # define hidden_def(name) __hidden_ver1(__GI_##name, name, name); >+# define hidden_def_alias(name, internal) \ >+ strong_alias (name, internal) > # define hidden_data_def(name) hidden_def(name) >+# define hidden_data_def_alias(name, alias) hidden_def_alias(name, alias) > # define hidden_tls_def(name) \ > __hidden_ver2 (__thread, __GI_##name, name, name); > # define hidden_weak(name) \ >@@ -581,9 +588,11 @@ for linking") > hidden_proto doesn't make sense for assembly but the equivalent > is to call via the HIDDEN_JUMPTARGET macro instead of JUMPTARGET. */ > # define hidden_def(name) strong_alias (name, __GI_##name) >+# define hidden_def_alias(name, alias) strong_alias (name, alias) > # define hidden_weak(name) hidden_def (name) > # define hidden_ver(local, name) strong_alias (local, __GI_##name) > # define hidden_data_def(name) strong_data_alias (name, __GI_##name) >+# define hidden_data_def_alias(name, alias) strong_data_alias (name, alias) > # define hidden_tls_def(name) hidden_data_def (name) > # define hidden_data_weak(name) hidden_data_def (name) > # define hidden_data_ver(local, name) strong_data_alias (local, __GI_##name) >@@ -598,12 +607,17 @@ for linking") > __attribute__ ((visibility ("hidden"), ##attrs)) > # define hidden_proto(name, attrs...) \ > __hidden_proto (name, , name, ##attrs) >+# define hidden_proto_alias(name, alias, attrs...) \ >+ __hidden_proto_alias (name, , alias, ##attrs) > # define hidden_tls_proto(name, attrs...) \ > __hidden_proto (name, __thread, name, ##attrs) > # define __hidden_proto(name, thread, internal, attrs...) \ > extern thread __typeof (name) name __hidden_proto_hiddenattr (attrs); >+# define __hidden_proto_alias(name, thread, internal, attrs...) \ >+ extern thread __typeof (name) internal __hidden_proto_hiddenattr (attrs); > # else > # define hidden_proto(name, attrs...) >+# define hidden_proto_alias(name, alias, attrs...) > # define hidden_tls_proto(name, attrs...) > # endif > # else >@@ -611,9 +625,11 @@ for linking") > # endif /* Not __ASSEMBLER__ */ > # define hidden_weak(name) > # define hidden_def(name) >+# define hidden_def_alias(name, alias) > # define hidden_ver(local, name) > # define hidden_data_weak(name) > # define hidden_data_def(name) >+# define hidden_data_def_alias(name, alias) > # define hidden_tls_def(name) > # define hidden_data_ver(local, name) > # define hidden_nolink(name, lib, version) >@@ -621,22 +637,28 @@ for linking") > > #if IS_IN (libc) > # define libc_hidden_proto(name, attrs...) hidden_proto (name, ##attrs) >+# define libc_hidden_proto_alias(name, alias, attrs...) \ >+ hidden_proto_alias (name, alias, ##attrs) > # define libc_hidden_tls_proto(name, attrs...) hidden_tls_proto (name, ##attrs) > # define libc_hidden_def(name) hidden_def (name) >+# define libc_hidden_def_alias(name, alias) hidden_def_alias (name, alias) > # define libc_hidden_weak(name) hidden_weak (name) > # define libc_hidden_nolink_sunrpc(name, version) hidden_nolink (name, libc, version) > # define libc_hidden_ver(local, name) hidden_ver (local, name) > # define libc_hidden_data_def(name) hidden_data_def (name) >+# define libc_hidden_data_def_alias(name, alias) hidden_data_def_alias (name, alias) > # define libc_hidden_tls_def(name) hidden_tls_def (name) > # define libc_hidden_data_weak(name) hidden_data_weak (name) > # define libc_hidden_data_ver(local, name) hidden_data_ver (local, name) > #else > # define libc_hidden_proto(name, attrs...) >+# define libc_hidden_proto_alias(name, alias, attrs...) > # define libc_hidden_tls_proto(name, attrs...) > # define libc_hidden_def(name) > # define libc_hidden_weak(name) > # define libc_hidden_ver(local, name) > # define libc_hidden_data_def(name) >+# define libc_hidden_data_def_alias(name, alias) > # define libc_hidden_tls_def(name) > # define libc_hidden_data_weak(name) > # define libc_hidden_data_ver(local, name) include/libc-symbols.h is very difficult to read. I can only use the preprocessed output to understand that this patch works as intended. I wish that we can simplify the file a bit :-) >diff --git a/include/sys/single_threaded.h b/include/sys/single_threaded.h >index 18f6972482..2015742be0 100644 >--- a/include/sys/single_threaded.h >+++ b/include/sys/single_threaded.h >@@ -1 +1,12 @@ > #include >+ >+#ifndef _ISOMAC >+ >+libc_hidden_proto_alias (__libc_single_threaded, >+ __libc_single_threaded_internal); >+ >+#if !defined SHARED || !IS_IN(libc) >+# define __libc_single_threaded_internal __libc_single_threaded >+#endif >+ >+#endif Does this file need a header guard now? >diff --git a/misc/single_threaded.c b/misc/single_threaded.c >index 96ada9137b..9b0746a69c 100644 >--- a/misc/single_threaded.c >+++ b/misc/single_threaded.c >@@ -25,3 +25,5 @@ char __libc_single_threaded; > #else > char __libc_single_threaded = 1; > #endif >+libc_hidden_data_def_alias (__libc_single_threaded, >+ __libc_single_threaded_internal) >diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c >index e7a099acb7..5b98e053d6 100644 >--- a/nptl/pthread_create.c >+++ b/nptl/pthread_create.c >@@ -624,9 +624,12 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > > /* Avoid a data race in the multi-threaded case, and call the > deferred initialization only once. */ >- if (__libc_single_threaded) >+ if (__libc_single_threaded_internal) > { > late_init (); >+ __libc_single_threaded_internal = 0; >+ /* __libc_single_threaded can be accessed through copy relocations, so >+ it requires to update the external copy. */ > __libc_single_threaded = 0; > } LGTM. I verify that __libc_single_threaded has a GLOB_DAT. Reviewed-by: Fangrui Song >diff --git a/posix/fork.c b/posix/fork.c >index e1be3422ea..987916a175 100644 >--- a/posix/fork.c >+++ b/posix/fork.c >@@ -45,7 +45,7 @@ __libc_fork (void) > requirement for fork (Austin Group tracker issue #62) this is > best effort to make is async-signal-safe at least for single-thread > case. */ >- bool multiple_threads = __libc_single_threaded == 0; >+ bool multiple_threads = __libc_single_threaded_internal == 0; > uint64_t lastrun; > > lastrun = __run_prefork_handlers (multiple_threads); >diff --git a/sysdeps/nptl/setxid.h b/sysdeps/nptl/setxid.h >index b821d6bc07..b87cad7b18 100644 >--- a/sysdeps/nptl/setxid.h >+++ b/sysdeps/nptl/setxid.h >@@ -29,7 +29,7 @@ > #define INLINE_SETXID_SYSCALL(name, nr, args...) \ > ({ \ > int __result; \ >- if (!__libc_single_threaded) \ >+ if (!__libc_single_threaded_internal) \ > { \ > struct xid_command __cmd; \ > __cmd.syscall_no = __NR_##name; \ >-- >2.34.1 >