Re: [PATCH v2] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062]

[Carlos O'Donell <carlos@redhat.com>]

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.