From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31778 invoked by alias); 4 Jan 2018 18:33:51 -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 31734 invoked by uid 89); 4 Jan 2018 18:33:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Message-ID: <1515090825.9480.295.camel@redhat.com> Subject: Re: [PATCH] Implement libc_once_retry for atomic initialization with allocation From: Torvald Riegel To: Florian Weimer Cc: GNU C Library , "Dmitry V. Levin" Date: Thu, 04 Jan 2018 18:33:00 -0000 In-Reply-To: <2d12ba41-e3ff-70db-484e-19d9805c5e53@redhat.com> References: <6e4b6b22-3f38-f798-f2e0-eb22311ed6b5@redhat.com> <1515086144.9480.286.camel@redhat.com> <2d12ba41-e3ff-70db-484e-19d9805c5e53@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-01/txt/msg00140.txt.bz2 On Thu, 2018-01-04 at 18:44 +0100, Florian Weimer wrote: > On 01/04/2018 06:15 PM, Torvald Riegel wrote: > > On Thu, 2018-01-04 at 16:09 +0100, Florian Weimer wrote: > >> Subject: [PATCH] Implement libc_once_retry for atomic initialization with allocation > > > > Can you find a different name for it? It has similarities to a generic > > libc_once, but the pattern is more special. > > libc_concurrent_init? Maybe sth like libc_allocate_and_init_once, or just libc_init_once? One can do without allocation if setting DEALLOCATE to an empty function, but the parameters really suggest that this is about allocation. > > Do you need it often enough to make it worth breaking it out? > > I think it's worthwhile to separate the concurrency review from the > application logic. That can be helpful, but making a generic helper for special cases (if it's indeed one...) also has consts. > >> diff --git a/include/atomic.h b/include/atomic.h > >> index 6af07dba58..91e176b040 100644 > >> --- a/include/atomic.h > >> > >> +++ b/include/atomic.h > > > > I don't think this should go into atomic.h because this is where we > > really handle the basics of synchronization. This is a synchronization > > helper, and doesn't need to be touched by someone porting glibc to > > another arch. > > What would be a good place instead? A new header file? I think so. I'm not too keen on fine-granular headers usually, but atomics and what we build on top of them is worth separating IMO. > >> +/* Perform an acquire MO load on *PLACE. If *PLACE is not NULL, > >> > >> + return *PLACE. Otherwise, call ALLOCATE (CLOSURE). If that > >> > >> + returns NULL, return NULL. Otherwise, atomically replace *PLACE > >> > >> + with PTR, the result of the ALLOCATE call (with acquire-release > > > > That doesn't quite match what you implement. First, you don't replace > > it unconditionally, but use a CAS (you can't just throw in an > > "atomically" here because the allocations are mixed in and they aren't). > > Second, you use the weak CAS but the loop includes alloc/dealloc; see > > below. > > Hmm, right, that is unnecessary. I have updated the comment in the > attached patch. > > /* Perform an acquire MO load on *PLACE. If *PLACE is not NULL, > return *PLACE. Otherwise, call ALLOCATE (CLOSURE), yielding > RESULT. If RESULT equals NULL, return NULL. Otherwise, atomically > replace the NULL value in *PLACE with the RESULT value. If it You don't replace it atomically though, you really do run a CAS that ensures that you replace iff *PLACE is still NULL. > turns out that *PLACE was updated concurrently, call DEALLOCATE > (CLOSURE, RESULT) to undo the effect of ALLOCATE, and return the > new value of *PLACE (after an acquire MO load). If DEALLOCATE is > NULL, call free (RESULT) instead. > > The net effect is that if libc_once_retry returns a non-NULL value, > the caller can assume that pointed-to data has been initialized > according to the ALLOCATE function. That's a useful addition, and might even be the first sentence :) > >> + do > >> > >> + { > >> > >> + result = allocate (closure); > >> > >> + if (result == NULL) > >> > >> + return NULL; > >> > >> + > >> > >> + /* Synchronizes with the acquire MO load in > >> > >> + __libc_once_retry. */ > >> > >> + void *expected = NULL; > >> > >> + if (atomic_compare_exchange_weak_release (place, &expected, result)) > >> > >> + return result; > > > > It seems you're looking for a strong CAS here, so why don't you make a > > loop around the weak CAS? Using an acq_rel CAS, this is simpler. > > It's the only CAS we have: weak with relaxed MO on failure. I need > strong with acquire MO on failure, and currently, the only way to get > that is with the loop and the additional load. Sorry. That's fine. The strong CAS is really simple to do with a weak CAS if one ignores back-off.