From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x234.google.com (mail-oi1-x234.google.com [IPv6:2607:f8b0:4864:20::234]) by sourceware.org (Postfix) with ESMTPS id 2D8BC3857361 for ; Fri, 23 Sep 2022 13:26:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2D8BC3857361 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-oi1-x234.google.com with SMTP id n83so16333131oif.11 for ; Fri, 23 Sep 2022 06:26:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date; bh=GBm8PRv3PCA94J2Agg00zGXD8byLAcQsxVfOjpPRdtE=; b=qslr9p6CdIVpRX1S48JECeBwHPV82+f7j4+05bvlrik9CBNUMvWAFzRa0Sd5GBLPrn wskymeloIIo1pKwimbGUGG0rh47uVcIgJtLM3llsqfnoyHASQoynW3AXZXX5+z4tA7BB mKCMMv4lOmsNciUIf7eOxpjrs1sgZejE2lzkQ0tIVhibEI9Ybit8kFbCXuNDeTI2PVEg v+xTTp9CzrqyVTsONtKOSsEDtSgUJqSuQSiqbqhucwqLrYptSaYhtxFTQbUzOPfNhKKz LYJNjb2XvuGyWP2Iz1+IDHRTSEfgOrmnrvljk+9J2gl5a9UwZ3iofMNpdWuJJdcPfrJw jCAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=GBm8PRv3PCA94J2Agg00zGXD8byLAcQsxVfOjpPRdtE=; b=v9srMCGYpmE/T7K1yiehjitdlL2qvoWmIxK9oTztK5vm4b0jri7xVfTHVGJX9QnLsC 8PX+Rm97f2KJAUV5Vpydiem+3zrsBnq5DPSGBYCDOEEa6Zz+F9JSDde7DvXZe6PKXWcc rLuUpeFWbZmIwO/tgoVwHcepQ0QSlNF4ic+gXOjkCGs3IorSolB4F08a10hHxKMYAKRz N6HgVOg9sMgpnoo08Bgmwffc5ilpxXMpWDw7l8Q0l8syAvyOaj7vJpcJdylN2P7WlpqX H+y0ZIIBYsiTusxp6nQI58batpGJSeNZxUoQDZ/9zKGYZrJ6GK9bZBzP4H2SbInwFeDU Hxyg== X-Gm-Message-State: ACrzQf1KJaaqOfNEaZa5vx9PIvCl2Nk9UIKgHFMSQr9BSsi+le8CE0Jm yxWpGgBx3RB7P0Sij1ZdTqlZFylGQ+QhJ6Za X-Google-Smtp-Source: AMsMyM7/Xsku/+FZMsB0xzWuXXX0kUpY1h1Y7e4RfLGkTlJGhkYrkI9xGyPt0J6RHBT77k8Mmx+FwA== X-Received: by 2002:aca:1117:0:b0:350:42c:55ba with SMTP id 23-20020aca1117000000b00350042c55bamr4052725oir.113.1663939607396; Fri, 23 Sep 2022 06:26:47 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c1:c266:202e:f71c:c0e7:6b4e? ([2804:1b3:a7c1:c266:202e:f71c:c0e7:6b4e]) by smtp.gmail.com with ESMTPSA id s190-20020a4a51c7000000b004764a441aa5sm1337479ooa.27.2022.09.23.06.26.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 23 Sep 2022 06:26:47 -0700 (PDT) Message-ID: <3b62647d-6c17-d6b6-6829-6e8ba7cd059c@linaro.org> Date: Fri, 23 Sep 2022 10:26:44 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH] Use C11 atomics instead of atomic_increment(_val) Content-Language: en-US To: Wilco Dijkstra , 'GNU C Library' References: From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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 22/09/22 10:27, Wilco Dijkstra wrote: > Hi Adhemerval, > > In general you only need acquire/release for locks that synchronize access to > shared data which does not use atomic accesses. If there is no such shared > data, there is no need for acquire/release. Similarly only using acquire and never > release (as the old atomics did) makes no sense - you need a release atomic > to synchronize with. Alright, I agree that default to acquire MO as the most atomic does today does not make much sense. > >> I am not sure if relaxed MO is correct here, shouldn't it synchronize with the >> __nptl_setxid_sighandler ? > > The counter cntr in nptl/nptl_setxid.c is just a simple atomic counter to keep > track of the number of outstanding signals that were sent to threads. There is > no data it is trying to synchronize with, so release/acquire semantics do not > make sense here. Ack. > >> Not sure if this is correct for hurd, __pthread_total is used on __pthread_exit >> to call exit although for i686 atomic_decrement_and_test will be used and it >> has strong MO. > > The same is true for pthread_total counter, it is used to call exit when the last > thread stops. Note while the increment is done optimistically (thread creation > can fail, and then it is decremented again), the decrement resulting in a call to > exit can only happen once even if multiple threads are finishing concurrently > (and that happen once is what is required). Ack. > > Cheers, > Wilco > The patch looks ok then. Reviewed-by: Adhemerval Zanella > > >> diff --git a/htl/pt-create.c b/htl/pt-create.c >> index ce52ed9f52210a4e4c7a049ebee817ec9ccfeeb1..14f02cd2b8a19e8581a170dfba2b948ef8304203 100644 >> --- a/htl/pt-create.c >> +++ b/htl/pt-create.c >> @@ -228,7 +228,7 @@ __pthread_create_internal (struct __pthread **thread, >>        the number of threads from within the new thread isn't an option >>        since this thread might return and call `pthread_exit' before the >>        new thread runs.  */ >> -  atomic_increment (&__pthread_total); >> +  atomic_fetch_add_relaxed (&__pthread_total, 1); >>   >>     /* Store a pointer to this thread in the thread ID lookup table.  We >>        could use __thread_setid, however, we only lock for reading as no > > Not sure if this is correct for hurd, __pthread_total is used on __pthread_exit > to call exit although for i686 atomic_decrement_and_test will be used and it > has strong MO. > > >> diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c >> index aa863c7ea8122ea01d1aa4cffe101bbb7c11270c..3b7e2d434abe8a15145349d1a08a4e706061c74d 100644 >> --- a/nptl/nptl_setxid.c >> +++ b/nptl/nptl_setxid.c >> @@ -163,7 +163,7 @@ setxid_signal_thread (struct xid_command *cmdp, struct pthread *t) >>     /* If this failed, it must have had not started yet or else exited.  */ >>     if (!INTERNAL_SYSCALL_ERROR_P (val)) >>       { >> -      atomic_increment (&cmdp->cntr); >> +      atomic_fetch_add_relaxed (&cmdp->cntr, 1); >>         return 1; >>       } >>     else > > I am not sure if relaxed MO is correct here, shouldn't it synchronize with the > __nptl_setxid_sighandler ?