From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28509 invoked by alias); 10 Oct 2017 19:08:52 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 28500 invoked by uid 89); 10 Oct 2017 19:08:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.2 spammy=ah, HTo:D*se, perfect X-HELO: mail-qt0-f180.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=gIBe5ngtU5FlJ9RInJ+HialY3f497ksLzC2G6bZU7oE=; b=AMgfFPYiRfbDbqpHnitSwyRYkxDWAppagtMeD5uBpRUd6xpdv8uvv3aPbavB8c6dOI t9dmmFoLl/vJESwvujg7nmXYrAlV059gRjJSbgYG2cp8PVEapeWp7NGaVp/uKPmgJudX ywoMjaLKajTD1u6BmVh9JHJohtqvCU+eDM6myoEosu2h0D9SZubD7pU8IlvdHpYvDO2K DU29uKezRdPAm+riqouaBbobfqO++2T/mdUE2Qq2Q9V8SC0kGf6WHd6wCZCsckA1NYix g175nXNwZ6ZspCnMX8qS8HI+lIp6a686gVmy10E5K8CYGsf7bfCSROf9ehy1Z4QL5YHk XTWA== X-Gm-Message-State: AMCzsaU8D9l2AK9rJ98owaDvptBAGSdD2bFPhkzabxlVL2RUDLayWCy8 P/yK46KQIt2LRIx54SS28sAbeINMVRQ= X-Google-Smtp-Source: AOwi7QC3gLjEhuPVX89yWTUmndNnnhRAhGWgfQG3lTcl7cN89lECtcijNMDdltJf+l9MiTgXwyAqBA== X-Received: by 10.55.15.65 with SMTP id z62mr16183238qkg.2.1507662528564; Tue, 10 Oct 2017 12:08:48 -0700 (PDT) Subject: Re: [PATCH v2] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062] To: Florian Weimer , Arjun Shankar , libc-alpha@sourceware.org References: <20171002143148.GA41930@aloka.lostca.se> <940c9be0-37ae-ebf7-1013-e6e6a4991028@redhat.com> <08eed690-d5eb-ce13-0c61-15a93e4bc8a7@redhat.com> <1b058316-b54d-165d-5876-af1327d0cfde@redhat.com> From: Carlos O'Donell Message-ID: Date: Tue, 10 Oct 2017 19:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1b058316-b54d-165d-5876-af1327d0cfde@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-10/txt/msg00428.txt.bz2 On 10/05/2017 11:38 PM, Florian Weimer wrote: > On 10/06/2017 01:05 AM, Carlos O'Donell wrote: >> Is there any reason we need this atomic load? >> >>> + gconv_path_elem_local = atomic_load_relaxed >>> (&__gconv_path_elem); >> ^^^^^^^^^^^^^^^^^^^ > > Oh, this follows just the general rule that if atomics are used, all > accesses should employ atomics. This follows from the general > principle of C11 compatibility, where an atomic type would default to > seq-cst access (which we don't want here). I see this was not > mentioned in the Concurrency wiki page. I've fixed that. Ah! Yes, that makes perfect sense. The assumption is we would switch the type of the variable to an atomic type, and then the unadorned accesses have to be seq-cst, which we don't need here. Thanks for updating the wiki. I'd forgotten entirely about this. > I did not notice that there was a __libc_once guard. So it would be > better to remove the locking from __gconv_get_path and document that > (including that this serializes initialization of the gconv cache via > __gconv_load_cache). Yes, that's right. Arjun, I suggest you rework the patch like this: - Remove locking from __gconv_get_path, and add comments to that function access is serialized by __libc_once in the caller. - Add comment to __gconv_read_conf that it must be serialized by the __libc_once in the caller. -- Cheers, Carlos.