This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Implement libc_once_retry for atomic initialization with allocation
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.