Date: Thu, 19 Nov 2009 16:55:31 -0500 From: John Baldwin <jhb@freebsd.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: svn-src-head@freebsd.org, Dmitry Pryanishnikov <lynx.ripe@gmail.com>, svn-src-all@freebsd.org, src-committers@freebsd.org, Edwin Groothuis <edwin@freebsd.org> Subject: Re: svn commit: r194783 - head/lib/libc/stdtime Message-ID: <200911191655.32008.jhb@freebsd.org> In-Reply-To: <200911180841.55183.jhb@freebsd.org> References: <4B01E548.7040708@gmail.com> <20091117182501.GA70742@stack.nl> <200911180841.55183.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Boundary-00=_T7bBLr0AvhuQLBB Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline On Wednesday 18 November 2009 8:41:54 am John Baldwin wrote: > On Tuesday 17 November 2009 1:25:01 pm Jilles Tjoelker wrote: > > On Tue, Nov 17, 2009 at 01:50:32AM +0200, Dmitry Pryanishnikov wrote: > > > > Author: edwin > > > > Date: Tue Jun 23 22:28:44 2009 > > > > New Revision: 194783 > > > > URL: http://svn.freebsd.org/changeset/base/194783 > > > > > > Log: > > > > Remove duplicate if-statement on gmt_is_set in gmtsub(). > > > > > > MFC after: 1 week > > > > > > Modified: > > > > head/lib/libc/stdtime/localtime.c > > > > > This change looks like a (small?) pessimization to me: before it, > > > _MUTEX_LOCK/_MUTEX_UNLOCK pair would be skipped for the case gmt_is_set > > > == TRUE (all invocations except the first one), now it won't. I'm not > > > sure whether this is critical here though... > > > > It is certainly less efficient, but the old code was (most likely) > > wrong. It used an idiom known as "double checked locking", which is > > incorrect in most memory models. The problem is that the store to > > gmt_is_set may become visible without stores to other memory (gmtptr and > > what it points to) becoming visible. > > That is easily fixed with a memory barrier. Just use atomic_store_rel() to > set gmt_is_set at the end of the setup phase. Alan Cox suggested that it might be best to provide an abstraction for this sort of thing rather than having N copies of using various memory barriers and atomic ops, etc. This patch adds a new internal method to libc _once() that is just like pthread_once(). It is called _once() because it has some extra trickery to use pthread_once() for a multithreaded process and to use an internal stub version for single-threaded processes. I also converted the gmt_is_set stuff to use this instead. This should restore the lockless code for the common case. Edwin, can you test this against the bug you were seeing? -- John Baldwin --Boundary-00=_T7bBLr0AvhuQLBB Content-Type: text/x-diff; charset="iso-8859-1"; name="stdtime.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="stdtime.patch" Index: gen/Makefile.inc =================================================================== --- gen/Makefile.inc (revision 199529) +++ gen/Makefile.inc (working copy) @@ -5,7 +5,8 @@ .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,67 @@ +/*- + * Copyright (c) 2009 Advanced Computing Technologies LLC + * Written by: John H. Baldwin <jhb@FreeBSD.org> + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#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 multi-threaded 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); --Boundary-00=_T7bBLr0AvhuQLBB--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911191655.32008.jhb>