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

[S.Çağlar Onur]

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.

>
> On Thu, 19 Dec 2013 16:29:52 +0400, Stéphane Graber <stgraber at ubuntu.com>
> wrote:
>>
>> On Thu, Dec 19, 2013 at 11:25:33AM +0100, Stéphane Graber wrote:
>>>
>>> On Wed, Dec 18, 2013 at 11:22:27PM -0600, Serge Hallyn wrote:
>>> > Quoting S.Çağlar Onur (caglar at 10ur.org):
>
> [snip]
>
>>> > > I ended up placing values in the thread local storage pool, instead
>>> > > of doing "unlock the lock in the child" dance
>>> > >
>>> > > Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
>>> >
>>> > only question is, does this work in bionic?  So long as it does,
>>> >
>>> > Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
>>>
>>> I'll do a test build in a minute, if that works, I'll push to master.
>>
>>
>> Test build passed fine using the Android NDK.
>>
>> I don't have hardware around me to test the result, but I think the
>> concerned was more about building/linking than run time.
>>
>> Commit pushed to master.
>
> [snip]
>
>>> > > diff --git a/src/lxc/utils.c b/src/lxc/utils.c
>>> > > index b605307..785f3e6 100644
>>> > > --- a/src/lxc/utils.c
>>> > > +++ b/src/lxc/utils.c
>>> > > @@ -253,8 +253,8 @@ const char *lxc_global_config_value(const char
>>> > > *option_name)
>>> > >             { "cgroup.use",      NULL            },
>>> > >             { NULL, NULL },
>>> > >     };
>>> > > -   /* Protected by a mutex to eliminate conflicting load and store
>>> > > operations */
>>> > > -   static const char *values[sizeof(options) / sizeof(options[0])] =
>>> > > { 0 };
>>> > > +   /* placed in the thread local storage pool */
>>> > > +   static __thread const char *values[sizeof(options) /
>>> > > sizeof(options[0])] = { 0 };
>>> > >     const char *(*ptr)[2];
>>> > >     const char *value;
>>> > >     size_t i;
>
> [snip]
>
> P.S.
> It seems, that linuxcontainers.org points to [3] while the actual
> mailing-list info and archive reside at [4] (note https vs http).
> Is it a transient error?
>
> [1] https://github.com/lxc/lxc/pull/106#issuecomment-30920691
> [2]
> https://android.googlesource.com/platform/bionic/+/0493a6f7be42e22d68e1d6ddb8eb2edaf818756f
> [3] https://lists.linuxcontainers.org/listinfo/lxc-devel
> [4] http://lists.linuxcontainers.org/listinfo/lxc-devel
>
> --
> Andrey Mazo.
>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel



-- 
S.Çağlar Onur <caglar at 10ur.org>