[lxc-devel] [PATCH] remove static_lock()/static_unlock() and start to use thread local storage (v2)

[Andrey Mazo]

Since lxc_global_config_value() tries to cache return value (exactly in "values" array) to avoid rereading config file multiple times for the same option, we can't just allocate "values" array on stack.
Also, it would require all callers to free() returned memory.
ifdef approach should work though.

But we can also go another way: instead of parsing config file LXC_GLOBAL_CONF on demand (and reading through it multiple times),
we can parse it once upon startup (before becoming multithreaded) thus eliminating the need for __thread or locking and still keeping "values" array static.
Or am I missing something?

On Fri, 20 Dec 2013 23:37:09 +0400, S.Çağlar Onur <caglar at 10ur.org> wrote:

> Hey Andrey,
>
> On Fri, Dec 20, 2013 at 10:39 AM, Andrey Mazo <mazo at telum.ru> wrote:
>> Hi all,
>>
>> Sorry for coming too late to the discussion.
>> In fact, I've already acked [1] the changeset.
>> But after sleeping more on it, I'm concerned about __thread and bionic.
>> Quoting android-ndk-r9c/docs/text/system/libc/OVERVIEW.text:
>>   At the moment, thread-local storage defined through the __thread compiler
>>   keyword is not supported by the Bionic C library and dynamic linker.
>>
>> The same OVERVIEW.TXT was present until recently in bionic sources [2].
>> Moreover, grepping (today's git HEAD) bionic sources for __thread, PT_TLS
>> (thread-local storage segment), .tbss and .tdata section names shows
>> nothing.
>> So, while utils.c compiles fine (due to GNU gcc/binutils support for
>> __thread), lxc-* will likely segfault on the first access to the "values"
>> array (because of missing memory segment due to bionic dynamic loader's lack
>> of __thread support).
>> I suppose, we could allocate a TLS slot for "values" pointer via
>> pthread_key_create() and set it to a per-thread malloc()'ed memory chunk via
>> pthread_setspecific(). (this seems to be quite verbose though)
>>
>> Saying this, it would be very nice to run-time test related changes.
>
> Hmm that's not good news. I guess there are couple of other solutions
> (or workarounds depending on how you look), we can ifdef the
> declaration given the fact no concurrency is expected on Android
> platform or alternatively could remove the static keyword from values
> array to make it stack allocated as it's not a huge data structure.

-- 
Andrey Mazo.