Date: Thu, 19 Nov 2009 15:39:47 -0500 From: John Baldwin <jhb@freebsd.org> To: Daniel Eischen <deischen@freebsd.org> Cc: threads@freebsd.org Subject: Re: Using pthread_once() in libc Message-ID: <200911191539.47588.jhb@freebsd.org> In-Reply-To: <Pine.GSO.4.64.0911191458540.9230@sea.ntplx.net> References: <200911191030.14151.jhb@freebsd.org> <200911191254.50902.jhb@freebsd.org> <Pine.GSO.4.64.0911191458540.9230@sea.ntplx.net>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
On Thursday 19 November 2009 3:00:19 pm Daniel Eischen wrote:
> On Thu, 19 Nov 2009, John Baldwin wrote:
>
> > On Thursday 19 November 2009 12:09:33 pm Daniel Eischen wrote:
> >> On Thu, 19 Nov 2009, John Baldwin wrote:
> >>
> >>> On Thursday 19 November 2009 11:48:54 am Daniel Eischen wrote:
> >>>> On Thu, 19 Nov 2009, John Baldwin wrote:
> >>>>
> >>>>> I would like to provide a pthread_once()-like facility in libc that library
> >>>>> bits can use to initialize data safely rather than trying to home-roll their
> >>>>> own variants (see the recent commit to stdtime in libc). Ideally what I
> >>>>> would like to do is have libc use the "real" pthread_once() when libthr is
> >>>>> linked in and fall back to a simple stub without libthr linked in. I know we
> >>>>> already do something like this for _spinlock() and friends. My question is
> >>>>> what is the most correct way to do this? Should libc grow a new _once()
> >>>>> symbol ala _spinlock() that is a weak symbol to a stub version and
> >>>>> pthread_once() in thr_once.c would override that, or should there be a
> >>>>> _pthread_once() in libc that is a stub in place of the current stub_zero? I
> >>>>> noticed a comment in thr_spinlock.c saying the spinlock stuff is kept for
> >>>>> backwards compat. Does this mean that for the future we would like to expose
> >>>>> pthread symbols directly in libc? Meaning would we rather have libc export a
> >>>>> pthread_once() and that ideally libc would be using pthread_mutex_lock/unlock
> >>>>> instead of _spinlock/unlock?
> >>>>
> >>>> pthread_once() is already a stub in libc that gets overloaded with the
> >>>> real thing when libthr is linked. See libc/gen/_pthread_stubs.c.
> >>>> Isn't that what you want or does it not serve your purpose?
> >>>
> >>> Hmm, the libc stub will never run the init routine. I would like to do
> >>> something like this:
> >>
> >> Well, I suppose you could do that. But what happens if libthr gets
> >> dlopen()'d and your once function needs to initialize a mutex or
> >> something that can only be properly done by a real threads library?
> >> Can we envision a scenario where that would be a problem?
> >
> > Hmmm, so I guess __is_threaded is how the dlopen() case is handled now for
> > mutex lock/unlock as that avoids resolving the symbol until pthread_create()
> > has been invoked? I guess then we could take an approach that works
> > something like this:
> >
> > /* libc-internal function */
> > int
> > _once(pthread_once_t *once_control, void (*init_routine)(void))
> > {
> >
> > if (__is_threaded)
> > return (_pthread_once(once_control, init_routine));
> >
> > return (_stub_once(once_control, init_routine));
> > }
> >
> > It is probably still a good idea to have the stub_once() patch I think so
> > that pthread_once() DTRT in a single-threaded program.
>
> I guess that works.
Ok, after a few rounds with the compiler the attached patch actually finished
a buildworld. Do you have any feedback on it?
--
John Baldwin
[-- Attachment #2 --]
Index: gen/Makefile.inc
===================================================================
--- gen/Makefile.inc (revision 199529)
+++ gen/Makefile.inc (working copy)
@@ -5,7 +5,7 @@
.PATH: ${.CURDIR}/${MACHINE_ARCH}/gen ${.CURDIR}/gen
SRCS+= __getosreldate.c __xuname.c \
- _pthread_stubs.c _rand48.c _spinlock_stub.c _thread_init.c \
+ _once_stub.c _pthread_stubs.c _rand48.c _spinlock_stub.c _thread_init.c \
alarm.c arc4random.c assert.c basename.c check_utility_compat.c \
clock.c closedir.c confstr.c \
crypt.c ctermid.c daemon.c devname.c dirname.c disklabel.c \
Index: gen/_once_stub.c
===================================================================
--- gen/_once_stub.c (revision 0)
+++ gen/_once_stub.c (revision 0)
@@ -0,0 +1,44 @@
+/*-
+ * XXX: Copyright
+ */
+
+#include <sys/cdefs.h>
+__FBSDID("$FreeBSD$");
+
+#include "namespace.h"
+#include <pthread.h>
+#include "un-namespace.h"
+#include "libc_private.h"
+
+/*
+ * This implements pthread_once() for the single-threaded case. It is
+ * non-static so that it can be used by _pthread_stubs.c.
+ */
+int
+_libc_once(pthread_once_t *once_control, void (*init_routine)(void))
+{
+
+ if (once_control->state == PTHREAD_NEEDS_INIT)
+ return (0);
+ init_routine();
+ once_control->state = PTHREAD_DONE_INIT;
+ return (0);
+}
+
+/*
+ * This is the internal interface provided to libc. It will use
+ * pthread_once() from the threading library in a multi-threaded
+ * process and _libc_once() for a single-threaded library. Because
+ * _libc_once() uses the same ABI for the values in the pthread_once_t
+ * structure as the threading library, it is safe for a process to
+ * switch from _libc_once() to pthread_once() when threading is
+ * enabled.
+ */
+int
+_once(pthread_once_t *once_control, void (*init_routine)(void))
+{
+
+ if (__isthreaded)
+ return (_pthread_once(once_control, init_routine));
+ return (_libc_once(once_control, init_routine));
+}
Property changes on: gen/_once_stub.c
___________________________________________________________________
Added: svn:mime-type
+ text/plain
Added: svn:keywords
+ FreeBSD=%H
Added: svn:eol-style
+ native
Index: gen/_pthread_stubs.c
===================================================================
--- gen/_pthread_stubs.c (revision 199529)
+++ gen/_pthread_stubs.c (working copy)
@@ -105,7 +105,7 @@
{PJT_DUAL_ENTRY(stub_zero)}, /* PJT_MUTEX_LOCK */
{PJT_DUAL_ENTRY(stub_zero)}, /* PJT_MUTEX_TRYLOCK */
{PJT_DUAL_ENTRY(stub_zero)}, /* PJT_MUTEX_UNLOCK */
- {PJT_DUAL_ENTRY(stub_zero)}, /* PJT_ONCE */
+ {PJT_DUAL_ENTRY(_libc_once)}, /* PJT_ONCE */
{PJT_DUAL_ENTRY(stub_zero)}, /* PJT_RWLOCK_DESTROY */
{PJT_DUAL_ENTRY(stub_zero)}, /* PJT_RWLOCK_INIT */
{PJT_DUAL_ENTRY(stub_zero)}, /* PJT_RWLOCK_RDLOCK */
Index: stdtime/localtime.c
===================================================================
--- stdtime/localtime.c (revision 199529)
+++ stdtime/localtime.c (working copy)
@@ -235,9 +235,8 @@
static char lcl_TZname[TZ_STRLEN_MAX + 1];
static int lcl_is_set;
-static int gmt_is_set;
+static pthread_once_t gmt_once = PTHREAD_ONCE_INIT;
static pthread_rwlock_t lcl_rwlock = PTHREAD_RWLOCK_INITIALIZER;
-static pthread_mutex_t gmt_mutex = PTHREAD_MUTEX_INITIALIZER;
char * tzname[2] = {
wildabbr,
@@ -1464,6 +1463,17 @@
return tmp;
}
+static void
+gmt_init(void)
+{
+
+#ifdef ALL_STATE
+ gmtptr = (struct state *) malloc(sizeof *gmtptr);
+ if (gmtptr != NULL)
+#endif /* defined ALL_STATE */
+ gmtload(gmtptr);
+}
+
/*
** gmtsub is to gmtime as localsub is to localtime.
*/
@@ -1476,16 +1486,7 @@
{
register struct tm * result;
- _MUTEX_LOCK(&gmt_mutex);
- if (!gmt_is_set) {
-#ifdef ALL_STATE
- gmtptr = (struct state *) malloc(sizeof *gmtptr);
- if (gmtptr != NULL)
-#endif /* defined ALL_STATE */
- gmtload(gmtptr);
- gmt_is_set = TRUE;
- }
- _MUTEX_UNLOCK(&gmt_mutex);
+ _once(&gmt_once, gmt_init);
result = timesub(timep, offset, gmtptr, tmp);
#ifdef TM_ZONE
/*
Index: include/libc_private.h
===================================================================
--- include/libc_private.h (revision 199529)
+++ include/libc_private.h (working copy)
@@ -34,6 +34,7 @@
#ifndef _LIBC_PRIVATE_H_
#define _LIBC_PRIVATE_H_
+#include <sys/_pthreadtypes.h>
/*
* This global flag is non-zero when a process has created one
@@ -147,6 +148,13 @@
void _init_tls(void);
/*
+ * Provides pthread_once()-like functionality for both single-threaded and
+ * multithreaded applications.
+ */
+int _once(pthread_once_t *, void (*)(void));
+int _libc_once(pthread_once_t *, void (*)(void));
+
+/*
* Set the TLS thread pointer
*/
void _set_tp(void *tp);
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911191539.47588.jhb>