Re: Thread local data setup and destruct
On Mon, Jan 04, 2021 at 06:03:46PM +0100, Mark Kettenis wrote:
> > Date: Sun, 3 Jan 2021 13:47:45 +0100
> > From: Otto Moerbeek
> >
> > On Thu, Dec 31, 2020 at 05:54:06PM +0100, Alexander Bluhm wrote:
> >
> > > On Tue, Dec 29, 2020 at 04:07:19PM +0100, Otto Moerbeek wrote:
> > > > This workds better, checking the flags does not work if the thread is
> > > > already on the road to desctruction.
> > >
> > > This diff survived a full regress run on amd64.
> > >
> > > bluhm
> >
> > anybody wants to ok?
>
> Don't forget the debug leftovers (char buf[100] and int len).
>
> Also, is the #include in thread/rthread_cb.c really needed?
nope, that was part of the debug code.
>
> Otherwise, this is ok kettenis@
>
>
> > > > Index: asr/asr.c
> > > > ===
> > > > RCS file: /cvs/src/lib/libc/asr/asr.c,v
> > > > retrieving revision 1.64
> > > > diff -u -p -r1.64 asr.c
> > > > --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> > > > +++ asr/asr.c 29 Dec 2020 15:05:45 -
> > > > @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> > > > _asr_ctx_unref(ac);
> > > > return;
> > > > } else {
> > > > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > > > + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> > > > if (*priv == NULL)
> > > > return;
> > > > asr = *priv;
> > > > @@ -128,6 +128,23 @@ _asr_resolver_done(void *arg)
> > > > free(asr);
> > > > }
> > > >
> > > > +static void
> > > > +_asr_resolver_done_tp(void *arg)
> > > > +{
> > > > + char buf[100];
> > > > + int len;
> > > > + struct asr **priv = arg;
> > > > + struct asr *asr;
> > > > +
> > > > + if (*priv == NULL)
> > > > + return;
> > > > + asr = *priv;
> > > > +
> > > > + _asr_ctx_unref(asr->a_ctx);
> > > > + free(asr);
> > > > + free(priv);
> > > > +}
> > > > +
> > > > void *
> > > > asr_resolver_from_string(const char *str)
> > > > {
> > > > @@ -349,7 +366,8 @@ _asr_use_resolver(void *arg)
> > > > }
> > > > else {
> > > > DPRINT("using thread-local resolver\n");
> > > > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > > > + priv = _THREAD_PRIVATE_DT(_asr, _asr,
> > > > _asr_resolver_done_tp,
> > > > + &_asr);
> > > > if (*priv == NULL) {
> > > > DPRINT("setting up thread-local resolver\n");
> > > > *priv = _asr_resolver();
> > > > Index: include/thread_private.h
> > > > ===
> > > > RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> > > > retrieving revision 1.35
> > > > diff -u -p -r1.35 thread_private.h
> > > > --- include/thread_private.h13 Feb 2019 13:22:14 - 1.35
> > > > +++ include/thread_private.h29 Dec 2020 15:05:45 -
> > > > @@ -98,7 +98,8 @@ struct thread_callbacks {
> > > > void(*tc_mutex_destroy)(void **);
> > > > void(*tc_tag_lock)(void **);
> > > > void(*tc_tag_unlock)(void **);
> > > > - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> > > > + void*(*tc_tag_storage)(void **, void *, size_t, void
> > > > (*)(void *),
> > > > + void *);
> > > > __pid_t (*tc_fork)(void);
> > > > __pid_t (*tc_vfork)(void);
> > > > void(*tc_thread_release)(struct pthread *);
> > > > @@ -142,6 +143,7 @@ __END_HIDDEN_DECLS
> > > > #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> > > > #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> > > > #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> > > > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> > > > &(storage)
> > > > #define _MUTEX_LOCK(mutex) do {} while (0)
> > > > #define _MUTEX_UNLOCK(mutex) do {} while (0)
> > > > #define _MUTEX_DESTROY(mutex) do {} while (0)
> > > > @@ -168,7 +170,12 @@ __END_HIDDEN_DECLS
> > > > #define _THREAD_PRIVATE(keyname, storage, error)
> > > > \
> > > > (_thread_cb.tc_tag_storage == NULL ? &(storage) :
> > > > \
> > > > _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),
> > > > \
> > > > - &(storage), sizeof(storage), error))
> > > > + &(storage), sizeof(storage), NULL, (error)))
> > > > +
> > > > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> > > > \
> > > > + (_thread_cb.tc_tag_storage == NULL ? &(storage) :
> > > > \
> > > > + _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),
> > > > \
> > > > + &(storage),
Re: Thread local data setup and destruct
> Date: Sun, 3 Jan 2021 13:47:45 +0100
> From: Otto Moerbeek
>
> On Thu, Dec 31, 2020 at 05:54:06PM +0100, Alexander Bluhm wrote:
>
> > On Tue, Dec 29, 2020 at 04:07:19PM +0100, Otto Moerbeek wrote:
> > > This workds better, checking the flags does not work if the thread is
> > > already on the road to desctruction.
> >
> > This diff survived a full regress run on amd64.
> >
> > bluhm
>
> anybody wants to ok?
Don't forget the debug leftovers (char buf[100] and int len).
Also, is the #include in thread/rthread_cb.c really needed?
Otherwise, this is ok kettenis@
> > > Index: asr/asr.c
> > > ===
> > > RCS file: /cvs/src/lib/libc/asr/asr.c,v
> > > retrieving revision 1.64
> > > diff -u -p -r1.64 asr.c
> > > --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> > > +++ asr/asr.c 29 Dec 2020 15:05:45 -
> > > @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> > > _asr_ctx_unref(ac);
> > > return;
> > > } else {
> > > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > > + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> > > if (*priv == NULL)
> > > return;
> > > asr = *priv;
> > > @@ -128,6 +128,23 @@ _asr_resolver_done(void *arg)
> > > free(asr);
> > > }
> > >
> > > +static void
> > > +_asr_resolver_done_tp(void *arg)
> > > +{
> > > + char buf[100];
> > > + int len;
> > > + struct asr **priv = arg;
> > > + struct asr *asr;
> > > +
> > > + if (*priv == NULL)
> > > + return;
> > > + asr = *priv;
> > > +
> > > + _asr_ctx_unref(asr->a_ctx);
> > > + free(asr);
> > > + free(priv);
> > > +}
> > > +
> > > void *
> > > asr_resolver_from_string(const char *str)
> > > {
> > > @@ -349,7 +366,8 @@ _asr_use_resolver(void *arg)
> > > }
> > > else {
> > > DPRINT("using thread-local resolver\n");
> > > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > > + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> > > + &_asr);
> > > if (*priv == NULL) {
> > > DPRINT("setting up thread-local resolver\n");
> > > *priv = _asr_resolver();
> > > Index: include/thread_private.h
> > > ===
> > > RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> > > retrieving revision 1.35
> > > diff -u -p -r1.35 thread_private.h
> > > --- include/thread_private.h 13 Feb 2019 13:22:14 - 1.35
> > > +++ include/thread_private.h 29 Dec 2020 15:05:45 -
> > > @@ -98,7 +98,8 @@ struct thread_callbacks {
> > > void(*tc_mutex_destroy)(void **);
> > > void(*tc_tag_lock)(void **);
> > > void(*tc_tag_unlock)(void **);
> > > - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> > > + void*(*tc_tag_storage)(void **, void *, size_t, void (*)(void *),
> > > +void *);
> > > __pid_t (*tc_fork)(void);
> > > __pid_t (*tc_vfork)(void);
> > > void(*tc_thread_release)(struct pthread *);
> > > @@ -142,6 +143,7 @@ __END_HIDDEN_DECLS
> > > #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> > > #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> > > #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> > > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error) &(storage)
> > > #define _MUTEX_LOCK(mutex) do {} while (0)
> > > #define _MUTEX_UNLOCK(mutex) do {} while (0)
> > > #define _MUTEX_DESTROY(mutex)do {} while (0)
> > > @@ -168,7 +170,12 @@ __END_HIDDEN_DECLS
> > > #define _THREAD_PRIVATE(keyname, storage, error) \
> > > (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> > > _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> > > - &(storage), sizeof(storage), error))
> > > + &(storage), sizeof(storage), NULL, (error)))
> > > +
> > > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> > > \
> > > + (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> > > + _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> > > + &(storage), sizeof(storage), (dt), (error)))
> > >
> > > /*
> > > * Macros used in libc to access mutexes.
> > > Index: thread/rthread_cb.h
> > > ===
> > > RCS file: /cvs/src/lib/libc/thread/rthread_cb.h,v
> > > retrieving revision 1.2
> > > diff -u -p -r1.2 rthread_cb.h
> > > --- thread/rthread_cb.h 5 Sep 2017 02:40:54 - 1.2
> > > +++ thread/rthread_cb.h 29 Dec 2020 15:05:45 -
> > > @@ -35,5 +35,5 @@ void_thread_mutex_unlock(void **);
> > > void _thread_mutex_destroy(void **);
> > > void _thread_tag_lock(void **);
> > > void _thread_tag_unlock(void **);
> > > -void
Re: Thread local data setup and destruct
On Thu, Dec 31, 2020 at 05:54:06PM +0100, Alexander Bluhm wrote:
> On Tue, Dec 29, 2020 at 04:07:19PM +0100, Otto Moerbeek wrote:
> > This workds better, checking the flags does not work if the thread is
> > already on the road to desctruction.
>
> This diff survived a full regress run on amd64.
>
> bluhm
anybody wants to ok?
-Otto
>
> > Index: asr/asr.c
> > ===
> > RCS file: /cvs/src/lib/libc/asr/asr.c,v
> > retrieving revision 1.64
> > diff -u -p -r1.64 asr.c
> > --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> > +++ asr/asr.c 29 Dec 2020 15:05:45 -
> > @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> > _asr_ctx_unref(ac);
> > return;
> > } else {
> > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> > if (*priv == NULL)
> > return;
> > asr = *priv;
> > @@ -128,6 +128,23 @@ _asr_resolver_done(void *arg)
> > free(asr);
> > }
> >
> > +static void
> > +_asr_resolver_done_tp(void *arg)
> > +{
> > + char buf[100];
> > + int len;
> > + struct asr **priv = arg;
> > + struct asr *asr;
> > +
> > + if (*priv == NULL)
> > + return;
> > + asr = *priv;
> > +
> > + _asr_ctx_unref(asr->a_ctx);
> > + free(asr);
> > + free(priv);
> > +}
> > +
> > void *
> > asr_resolver_from_string(const char *str)
> > {
> > @@ -349,7 +366,8 @@ _asr_use_resolver(void *arg)
> > }
> > else {
> > DPRINT("using thread-local resolver\n");
> > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> > + &_asr);
> > if (*priv == NULL) {
> > DPRINT("setting up thread-local resolver\n");
> > *priv = _asr_resolver();
> > Index: include/thread_private.h
> > ===
> > RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> > retrieving revision 1.35
> > diff -u -p -r1.35 thread_private.h
> > --- include/thread_private.h13 Feb 2019 13:22:14 - 1.35
> > +++ include/thread_private.h29 Dec 2020 15:05:45 -
> > @@ -98,7 +98,8 @@ struct thread_callbacks {
> > void(*tc_mutex_destroy)(void **);
> > void(*tc_tag_lock)(void **);
> > void(*tc_tag_unlock)(void **);
> > - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> > + void*(*tc_tag_storage)(void **, void *, size_t, void (*)(void *),
> > + void *);
> > __pid_t (*tc_fork)(void);
> > __pid_t (*tc_vfork)(void);
> > void(*tc_thread_release)(struct pthread *);
> > @@ -142,6 +143,7 @@ __END_HIDDEN_DECLS
> > #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> > #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> > #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)&(storage)
> > #define _MUTEX_LOCK(mutex) do {} while (0)
> > #define _MUTEX_UNLOCK(mutex) do {} while (0)
> > #define _MUTEX_DESTROY(mutex) do {} while (0)
> > @@ -168,7 +170,12 @@ __END_HIDDEN_DECLS
> > #define _THREAD_PRIVATE(keyname, storage, error) \
> > (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> > _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> > - &(storage), sizeof(storage), error))
> > + &(storage), sizeof(storage), NULL, (error)))
> > +
> > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> > \
> > + (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> > + _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> > + &(storage), sizeof(storage), (dt), (error)))
> >
> > /*
> > * Macros used in libc to access mutexes.
> > Index: thread/rthread_cb.h
> > ===
> > RCS file: /cvs/src/lib/libc/thread/rthread_cb.h,v
> > retrieving revision 1.2
> > diff -u -p -r1.2 rthread_cb.h
> > --- thread/rthread_cb.h 5 Sep 2017 02:40:54 - 1.2
> > +++ thread/rthread_cb.h 29 Dec 2020 15:05:45 -
> > @@ -35,5 +35,5 @@ void _thread_mutex_unlock(void **);
> > void _thread_mutex_destroy(void **);
> > void _thread_tag_lock(void **);
> > void _thread_tag_unlock(void **);
> > -void *_thread_tag_storage(void **, void *, size_t, void *);
> > +void *_thread_tag_storage(void **, void *, size_t, void (*)(void*),
> > void *);
> > __END_HIDDEN_DECLS
> > Index: thread/rthread_libc.c
> > ===
> > RCS file: /cvs/src/lib/libc/thread/rthread_libc.c,v
Re: Thread local data setup and destruct
On Tue, Dec 29, 2020 at 04:07:19PM +0100, Otto Moerbeek wrote:
> This workds better, checking the flags does not work if the thread is
> already on the road to desctruction.
This diff survived a full regress run on amd64.
bluhm
> Index: asr/asr.c
> ===
> RCS file: /cvs/src/lib/libc/asr/asr.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 asr.c
> --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> +++ asr/asr.c 29 Dec 2020 15:05:45 -
> @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> _asr_ctx_unref(ac);
> return;
> } else {
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> if (*priv == NULL)
> return;
> asr = *priv;
> @@ -128,6 +128,23 @@ _asr_resolver_done(void *arg)
> free(asr);
> }
>
> +static void
> +_asr_resolver_done_tp(void *arg)
> +{
> + char buf[100];
> + int len;
> + struct asr **priv = arg;
> + struct asr *asr;
> +
> + if (*priv == NULL)
> + return;
> + asr = *priv;
> +
> + _asr_ctx_unref(asr->a_ctx);
> + free(asr);
> + free(priv);
> +}
> +
> void *
> asr_resolver_from_string(const char *str)
> {
> @@ -349,7 +366,8 @@ _asr_use_resolver(void *arg)
> }
> else {
> DPRINT("using thread-local resolver\n");
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> + &_asr);
> if (*priv == NULL) {
> DPRINT("setting up thread-local resolver\n");
> *priv = _asr_resolver();
> Index: include/thread_private.h
> ===
> RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> retrieving revision 1.35
> diff -u -p -r1.35 thread_private.h
> --- include/thread_private.h 13 Feb 2019 13:22:14 - 1.35
> +++ include/thread_private.h 29 Dec 2020 15:05:45 -
> @@ -98,7 +98,8 @@ struct thread_callbacks {
> void(*tc_mutex_destroy)(void **);
> void(*tc_tag_lock)(void **);
> void(*tc_tag_unlock)(void **);
> - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> + void*(*tc_tag_storage)(void **, void *, size_t, void (*)(void *),
> +void *);
> __pid_t (*tc_fork)(void);
> __pid_t (*tc_vfork)(void);
> void(*tc_thread_release)(struct pthread *);
> @@ -142,6 +143,7 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error) &(storage)
> #define _MUTEX_LOCK(mutex) do {} while (0)
> #define _MUTEX_UNLOCK(mutex) do {} while (0)
> #define _MUTEX_DESTROY(mutex)do {} while (0)
> @@ -168,7 +170,12 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE(keyname, storage, error) \
> (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> - &(storage), sizeof(storage), error))
> + &(storage), sizeof(storage), NULL, (error)))
> +
> +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> \
> + (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> + _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> + &(storage), sizeof(storage), (dt), (error)))
>
> /*
> * Macros used in libc to access mutexes.
> Index: thread/rthread_cb.h
> ===
> RCS file: /cvs/src/lib/libc/thread/rthread_cb.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 rthread_cb.h
> --- thread/rthread_cb.h 5 Sep 2017 02:40:54 - 1.2
> +++ thread/rthread_cb.h 29 Dec 2020 15:05:45 -
> @@ -35,5 +35,5 @@ void_thread_mutex_unlock(void **);
> void _thread_mutex_destroy(void **);
> void _thread_tag_lock(void **);
> void _thread_tag_unlock(void **);
> -void *_thread_tag_storage(void **, void *, size_t, void *);
> +void *_thread_tag_storage(void **, void *, size_t, void (*)(void*), void *);
> __END_HIDDEN_DECLS
> Index: thread/rthread_libc.c
> ===
> RCS file: /cvs/src/lib/libc/thread/rthread_libc.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 rthread_libc.c
> --- thread/rthread_libc.c 10 Jan 2019 18:45:33 - 1.3
> +++ thread/rthread_libc.c 29 Dec 2020 15:05:45 -
> @@ -5,6 +5,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
Re: Thread local data setup and destruct
> Date: Tue, 29 Dec 2020 16:07:19 +0100
> From: Otto Moerbeek
>
> On Tue, Dec 29, 2020 at 01:42:57PM +0100, Otto Moerbeek wrote:
>
> > On Tue, Dec 29, 2020 at 12:46:54PM +0100, Mark Kettenis wrote:
> >
> > > > Date: Tue, 29 Dec 2020 12:21:25 +0100
> > > > From: Otto Moerbeek
> > > >
> > > > Hi,
> > > >
> > > > this is a continuation from the threads on bugs@
> > > >
> > > > This version makes it explicit to *only* setup "TLS" (which actually
> > > > is just a pointer to static data) with the data provided if we're
> > > > running single threaded (ie.. no -pthread or -pthread but no pthread
> > > > function that triggers multi-threaded init called yet). In all other
> > > > cases the real allocated TLS is zero'ed. This avoids the setup issue
> > > > asr is having. This diff also makes sure asr uses a specialized
> > > > destructor for it's TLS to kill a mem leak that occurs on thread
> > > > destruction.
> > > >
> > > > All in all I'm still a bit fond of the constructor approach to
> > > > librthread init: it makes the distinction between having a single
> > > > thread and being the main thread in a multi-threaded setup moot.
> > > >
> > > > Anyway, we can consider that later, please test this.
> > >
> > > This isn't right. The memcpy() call you're removing is there such
> > > that the contents of the per-thread storage for the initial (main)
> > > thread remains the same.
> > >
> > > With your change it reverts back to zero when the first thread is
> > > created. In the particular case at hand that means the initial thread
> > > would get a fresh resolver context at that point.
> >
> > Yes, but if I do not that crashes happen on destruct. Note that your
> > initial suggestion (in asr.c)
> >
> > if (priv != &_asr && *priv == _asr)
> > *priv = NULL;
> >
> > has the same property: it clears the contents if they happen to be the
> > same as _asr.
> >
> > The crash happens if the pre-mt (static) and the TLS of the main
> > thread share the same data. I did not find a way to make that work, so
> > I decided to use the big hammer. I might be overlooking something
> > obvious though...
>
>
> This workds better, checking the flags does not work if the thread is
> already on the road to desctruction.
That looks better. Minus the obvious debug leftovers ;). But those
shouldn't hurt any testing.
> Index: asr/asr.c
> ===
> RCS file: /cvs/src/lib/libc/asr/asr.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 asr.c
> --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> +++ asr/asr.c 29 Dec 2020 15:05:45 -
> @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> _asr_ctx_unref(ac);
> return;
> } else {
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> if (*priv == NULL)
> return;
> asr = *priv;
> @@ -128,6 +128,23 @@ _asr_resolver_done(void *arg)
> free(asr);
> }
>
> +static void
> +_asr_resolver_done_tp(void *arg)
> +{
> + char buf[100];
> + int len;
> + struct asr **priv = arg;
> + struct asr *asr;
> +
> + if (*priv == NULL)
> + return;
> + asr = *priv;
> +
> + _asr_ctx_unref(asr->a_ctx);
> + free(asr);
> + free(priv);
> +}
> +
> void *
> asr_resolver_from_string(const char *str)
> {
> @@ -349,7 +366,8 @@ _asr_use_resolver(void *arg)
> }
> else {
> DPRINT("using thread-local resolver\n");
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> + &_asr);
> if (*priv == NULL) {
> DPRINT("setting up thread-local resolver\n");
> *priv = _asr_resolver();
> Index: include/thread_private.h
> ===
> RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> retrieving revision 1.35
> diff -u -p -r1.35 thread_private.h
> --- include/thread_private.h 13 Feb 2019 13:22:14 - 1.35
> +++ include/thread_private.h 29 Dec 2020 15:05:45 -
> @@ -98,7 +98,8 @@ struct thread_callbacks {
> void(*tc_mutex_destroy)(void **);
> void(*tc_tag_lock)(void **);
> void(*tc_tag_unlock)(void **);
> - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> + void*(*tc_tag_storage)(void **, void *, size_t, void (*)(void *),
> +void *);
> __pid_t (*tc_fork)(void);
> __pid_t (*tc_vfork)(void);
> void(*tc_thread_release)(struct pthread *);
> @@ -142,6 +143,7 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> #define _THREAD_PRIVATE(keyname, storage, error)
Re: Thread local data setup and destruct
On Tue, Dec 29, 2020 at 01:42:57PM +0100, Otto Moerbeek wrote:
> On Tue, Dec 29, 2020 at 12:46:54PM +0100, Mark Kettenis wrote:
>
> > > Date: Tue, 29 Dec 2020 12:21:25 +0100
> > > From: Otto Moerbeek
> > >
> > > Hi,
> > >
> > > this is a continuation from the threads on bugs@
> > >
> > > This version makes it explicit to *only* setup "TLS" (which actually
> > > is just a pointer to static data) with the data provided if we're
> > > running single threaded (ie.. no -pthread or -pthread but no pthread
> > > function that triggers multi-threaded init called yet). In all other
> > > cases the real allocated TLS is zero'ed. This avoids the setup issue
> > > asr is having. This diff also makes sure asr uses a specialized
> > > destructor for it's TLS to kill a mem leak that occurs on thread
> > > destruction.
> > >
> > > All in all I'm still a bit fond of the constructor approach to
> > > librthread init: it makes the distinction between having a single
> > > thread and being the main thread in a multi-threaded setup moot.
> > >
> > > Anyway, we can consider that later, please test this.
> >
> > This isn't right. The memcpy() call you're removing is there such
> > that the contents of the per-thread storage for the initial (main)
> > thread remains the same.
> >
> > With your change it reverts back to zero when the first thread is
> > created. In the particular case at hand that means the initial thread
> > would get a fresh resolver context at that point.
>
> Yes, but if I do not that crashes happen on destruct. Note that your
> initial suggestion (in asr.c)
>
> if (priv != &_asr && *priv == _asr)
> *priv = NULL;
>
> has the same property: it clears the contents if they happen to be the
> same as _asr.
>
> The crash happens if the pre-mt (static) and the TLS of the main
> thread share the same data. I did not find a way to make that work, so
> I decided to use the big hammer. I might be overlooking something
> obvious though...
This workds better, checking the flags does not work if the thread is
already on the road to desctruction.
-Otto
Index: asr/asr.c
===
RCS file: /cvs/src/lib/libc/asr/asr.c,v
retrieving revision 1.64
diff -u -p -r1.64 asr.c
--- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
+++ asr/asr.c 29 Dec 2020 15:05:45 -
@@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
_asr_ctx_unref(ac);
return;
} else {
- priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
+ priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
if (*priv == NULL)
return;
asr = *priv;
@@ -128,6 +128,23 @@ _asr_resolver_done(void *arg)
free(asr);
}
+static void
+_asr_resolver_done_tp(void *arg)
+{
+ char buf[100];
+ int len;
+ struct asr **priv = arg;
+ struct asr *asr;
+
+ if (*priv == NULL)
+ return;
+ asr = *priv;
+
+ _asr_ctx_unref(asr->a_ctx);
+ free(asr);
+ free(priv);
+}
+
void *
asr_resolver_from_string(const char *str)
{
@@ -349,7 +366,8 @@ _asr_use_resolver(void *arg)
}
else {
DPRINT("using thread-local resolver\n");
- priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
+ priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
+ &_asr);
if (*priv == NULL) {
DPRINT("setting up thread-local resolver\n");
*priv = _asr_resolver();
Index: include/thread_private.h
===
RCS file: /cvs/src/lib/libc/include/thread_private.h,v
retrieving revision 1.35
diff -u -p -r1.35 thread_private.h
--- include/thread_private.h13 Feb 2019 13:22:14 - 1.35
+++ include/thread_private.h29 Dec 2020 15:05:45 -
@@ -98,7 +98,8 @@ struct thread_callbacks {
void(*tc_mutex_destroy)(void **);
void(*tc_tag_lock)(void **);
void(*tc_tag_unlock)(void **);
- void*(*tc_tag_storage)(void **, void *, size_t, void *);
+ void*(*tc_tag_storage)(void **, void *, size_t, void (*)(void *),
+ void *);
__pid_t (*tc_fork)(void);
__pid_t (*tc_vfork)(void);
void(*tc_thread_release)(struct pthread *);
@@ -142,6 +143,7 @@ __END_HIDDEN_DECLS
#define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
#define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
#define _THREAD_PRIVATE(keyname, storage, error) &(storage)
+#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)&(storage)
#define _MUTEX_LOCK(mutex) do {} while (0)
#define _MUTEX_UNLOCK(mutex) do {} while (0)
#define _MUTEX_DESTROY(mutex) do {} while (0)
@@
Re: Thread local data setup and destruct
On Tue, Dec 29, 2020 at 12:46:54PM +0100, Mark Kettenis wrote:
> > Date: Tue, 29 Dec 2020 12:21:25 +0100
> > From: Otto Moerbeek
> >
> > Hi,
> >
> > this is a continuation from the threads on bugs@
> >
> > This version makes it explicit to *only* setup "TLS" (which actually
> > is just a pointer to static data) with the data provided if we're
> > running single threaded (ie.. no -pthread or -pthread but no pthread
> > function that triggers multi-threaded init called yet). In all other
> > cases the real allocated TLS is zero'ed. This avoids the setup issue
> > asr is having. This diff also makes sure asr uses a specialized
> > destructor for it's TLS to kill a mem leak that occurs on thread
> > destruction.
> >
> > All in all I'm still a bit fond of the constructor approach to
> > librthread init: it makes the distinction between having a single
> > thread and being the main thread in a multi-threaded setup moot.
> >
> > Anyway, we can consider that later, please test this.
>
> This isn't right. The memcpy() call you're removing is there such
> that the contents of the per-thread storage for the initial (main)
> thread remains the same.
>
> With your change it reverts back to zero when the first thread is
> created. In the particular case at hand that means the initial thread
> would get a fresh resolver context at that point.
Yes, but if I do not that crashes happen on destruct. Note that your
initial suggestion (in asr.c)
if (priv != &_asr && *priv == _asr)
*priv = NULL;
has the same property: it clears the contents if they happen to be the
same as _asr.
The crash happens if the pre-mt (static) and the TLS of the main
thread share the same data. I did not find a way to make that work, so
I decided to use the big hammer. I might be overlooking something
obvious though...
-Otto
>
> > Index: asr/asr.c
> > ===
> > RCS file: /cvs/src/lib/libc/asr/asr.c,v
> > retrieving revision 1.64
> > diff -u -p -r1.64 asr.c
> > --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> > +++ asr/asr.c 29 Dec 2020 11:10:08 -
> > @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> > _asr_ctx_unref(ac);
> > return;
> > } else {
> > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> > if (*priv == NULL)
> > return;
> > asr = *priv;
> > @@ -128,6 +128,21 @@ _asr_resolver_done(void *arg)
> > free(asr);
> > }
> >
> > +static void
> > +_asr_resolver_done_tp(void *arg)
> > +{
> > + struct asr **priv = arg;
> > + struct asr *asr;
> > +
> > + if (*priv == NULL)
> > + return;
> > + asr = *priv;
> > +
> > + _asr_ctx_unref(asr->a_ctx);
> > + free(asr);
> > + free(priv);
> > +}
> > +
> > void *
> > asr_resolver_from_string(const char *str)
> > {
> > @@ -349,7 +364,8 @@ _asr_use_resolver(void *arg)
> > }
> > else {
> > DPRINT("using thread-local resolver\n");
> > - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> > + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> > + &_asr);
> > if (*priv == NULL) {
> > DPRINT("setting up thread-local resolver\n");
> > *priv = _asr_resolver();
> > Index: include/thread_private.h
> > ===
> > RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> > retrieving revision 1.35
> > diff -u -p -r1.35 thread_private.h
> > --- include/thread_private.h13 Feb 2019 13:22:14 - 1.35
> > +++ include/thread_private.h29 Dec 2020 11:10:08 -
> > @@ -51,7 +51,7 @@ PROTO_NORMAL(_malloc_init);
> > * tc_tag_storage:
> > * Returns a pointer to per-thread instance of data associated
> > * with the given tag. If the given tag is NULL a tag is
> > - * allocated and initialized automatically.
> > + * allocated and cleared automatically.
> > *
> > * tc_fork, tc_vfork:
> > * If not NULL, they are called instead of the syscall stub, so that
> > @@ -98,7 +98,7 @@ struct thread_callbacks {
> > void(*tc_mutex_destroy)(void **);
> > void(*tc_tag_lock)(void **);
> > void(*tc_tag_unlock)(void **);
> > - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> > + void*(*tc_tag_storage)(void **, size_t, void (*)(void *), void *);
> > __pid_t (*tc_fork)(void);
> > __pid_t (*tc_vfork)(void);
> > void(*tc_thread_release)(struct pthread *);
> > @@ -142,6 +142,7 @@ __END_HIDDEN_DECLS
> > #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> > #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> > #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> > +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
Re: Thread local data setup and destruct
> Date: Tue, 29 Dec 2020 12:21:25 +0100
> From: Otto Moerbeek
>
> Hi,
>
> this is a continuation from the threads on bugs@
>
> This version makes it explicit to *only* setup "TLS" (which actually
> is just a pointer to static data) with the data provided if we're
> running single threaded (ie.. no -pthread or -pthread but no pthread
> function that triggers multi-threaded init called yet). In all other
> cases the real allocated TLS is zero'ed. This avoids the setup issue
> asr is having. This diff also makes sure asr uses a specialized
> destructor for it's TLS to kill a mem leak that occurs on thread
> destruction.
>
> All in all I'm still a bit fond of the constructor approach to
> librthread init: it makes the distinction between having a single
> thread and being the main thread in a multi-threaded setup moot.
>
> Anyway, we can consider that later, please test this.
This isn't right. The memcpy() call you're removing is there such
that the contents of the per-thread storage for the initial (main)
thread remains the same.
With your change it reverts back to zero when the first thread is
created. In the particular case at hand that means the initial thread
would get a fresh resolver context at that point.
> Index: asr/asr.c
> ===
> RCS file: /cvs/src/lib/libc/asr/asr.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 asr.c
> --- asr/asr.c 6 Jul 2020 13:33:05 - 1.64
> +++ asr/asr.c 29 Dec 2020 11:10:08 -
> @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> _asr_ctx_unref(ac);
> return;
> } else {
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> if (*priv == NULL)
> return;
> asr = *priv;
> @@ -128,6 +128,21 @@ _asr_resolver_done(void *arg)
> free(asr);
> }
>
> +static void
> +_asr_resolver_done_tp(void *arg)
> +{
> + struct asr **priv = arg;
> + struct asr *asr;
> +
> + if (*priv == NULL)
> + return;
> + asr = *priv;
> +
> + _asr_ctx_unref(asr->a_ctx);
> + free(asr);
> + free(priv);
> +}
> +
> void *
> asr_resolver_from_string(const char *str)
> {
> @@ -349,7 +364,8 @@ _asr_use_resolver(void *arg)
> }
> else {
> DPRINT("using thread-local resolver\n");
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> + &_asr);
> if (*priv == NULL) {
> DPRINT("setting up thread-local resolver\n");
> *priv = _asr_resolver();
> Index: include/thread_private.h
> ===
> RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> retrieving revision 1.35
> diff -u -p -r1.35 thread_private.h
> --- include/thread_private.h 13 Feb 2019 13:22:14 - 1.35
> +++ include/thread_private.h 29 Dec 2020 11:10:08 -
> @@ -51,7 +51,7 @@ PROTO_NORMAL(_malloc_init);
> * tc_tag_storage:
> * Returns a pointer to per-thread instance of data associated
> * with the given tag. If the given tag is NULL a tag is
> - * allocated and initialized automatically.
> + * allocated and cleared automatically.
> *
> * tc_fork, tc_vfork:
> * If not NULL, they are called instead of the syscall stub, so that
> @@ -98,7 +98,7 @@ struct thread_callbacks {
> void(*tc_mutex_destroy)(void **);
> void(*tc_tag_lock)(void **);
> void(*tc_tag_unlock)(void **);
> - void*(*tc_tag_storage)(void **, void *, size_t, void *);
> + void*(*tc_tag_storage)(void **, size_t, void (*)(void *), void *);
> __pid_t (*tc_fork)(void);
> __pid_t (*tc_vfork)(void);
> void(*tc_thread_release)(struct pthread *);
> @@ -142,6 +142,7 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error) &(storage)
> #define _MUTEX_LOCK(mutex) do {} while (0)
> #define _MUTEX_UNLOCK(mutex) do {} while (0)
> #define _MUTEX_DESTROY(mutex)do {} while (0)
> @@ -168,7 +169,12 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE(keyname, storage, error) \
> (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)),\
> - &(storage), sizeof(storage), error))
> + sizeof(storage), NULL, (error)))
> +
> +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> \
> + (_thread_cb.tc_tag_storage == NULL ?
