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

[Andrey Mazo]

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.

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.