From owner-svn-src-user@FreeBSD.ORG Sat Nov 13 12:24:49 2010 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CF9121065672; Sat, 13 Nov 2010 12:24:49 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id 08A2D8FC13; Sat, 13 Nov 2010 12:24:49 +0000 (UTC) Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132]) by mx1.stack.nl (Postfix) with ESMTP id 3326935A864; Sat, 13 Nov 2010 13:24:48 +0100 (CET) Received: by turtle.stack.nl (Postfix, from userid 1677) id 1CE5F17349; Sat, 13 Nov 2010 13:24:48 +0100 (CET) Date: Sat, 13 Nov 2010 13:24:48 +0100 From: Jilles Tjoelker To: David Xu Message-ID: <20101113122447.GA79975@stack.nl> References: <201011100127.oAA1Rmrh069656@svn.freebsd.org> <20101112131941.GA57806@stack.nl> <4CDD411D.4060304@freebsd.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="BOKacYhQ+x31HxR3" Content-Disposition: inline In-Reply-To: <4CDD411D.4060304@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r215071 - in user/davidxu/libthr: include lib/libc lib/libc/gen lib/libc/stdio lib/libthr lib/libthr/thread X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Nov 2010 12:24:49 -0000 --BOKacYhQ+x31HxR3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Nov 12, 2010 at 09:29:01PM +0800, David Xu wrote: > Jilles Tjoelker wrote: > > Apart from supporting process-shared, this also helps avoid the "array > > of synchronization objects" anti-pattern (false sharing). It is not so > > bad for the old struct pthread_mutex which is 64 bytes on i386, but in > > other cases one cache line may contain parts of multiple unrelated > > synchronization objects. > Don't know what did you mean, did you think that sizeof struct > pthread_mutex > smaller than a cache line is a bad thing ? It is a bad thing if the rest of the cache line is not used for a related purpose. Code like: lock1 = malloc(sizeof(struct pthread_mutex)); lock2 = malloc(sizeof(struct pthread_mutex)); is likely to place the two locks in the same cache line, which may defeat much of the advantage of having two locks instead of one. With the new pthread_mutex_t, applications can do struct foo { int protectedfield1; int protectedfield2; pthread_mutex_t lock; }; and the cache line fetch for the lock will bring in the protected fields as well. Applications are responsible for making the struct large enough to avoid false sharing. > > In this regard, it would be better for stdio to allocate > > struct { FILE file; pthread_mutex_t lock; } > > rather than separate FILEs and locks. > This breaks ABI. :-) Yes, but you can keep the pthread_mutex_t *_fl_mutex and keep accessing the mutex using that. My concern is only about the way things are laid out in memory, your approach is otherwise good. I'm including a lightly tested patch with what I'm thinking of. Of course, stdio is not a high-performance I/O facility, but it is used quite a bit and doing it right here may serve as a good example. > > Like the sem_t change, this changes the ABI in a way symver can only > > partially deal with (think plugins with pthread_mutex_t in struct in > > public header file). However, I think the change should be made > > regardless. > Yes, we found that some GNU libraries have to bump version. -- Jilles Tjoelker --BOKacYhQ+x31HxR3 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="stdio-mutex-layout.patch" Index: user/davidxu/libthr/lib/libc/stdio/fwalk.c =================================================================== --- user/davidxu/libthr/lib/libc/stdio/fwalk.c (revision 215181) +++ user/davidxu/libthr/lib/libc/stdio/fwalk.c (working copy) @@ -46,7 +46,7 @@ _fwalk(function) int (*function)(FILE *); { - FILE *fp; + struct fullfile *ff; int n, ret; struct glue *g; @@ -60,8 +60,9 @@ * introduce a potential deadlock in [at least] refill.c. */ for (g = &__sglue; g != NULL; g = g->next) - for (fp = g->iobs, n = g->niobs; --n >= 0; fp++) - if ((fp->_flags != 0) && ((fp->_flags & __SIGN) == 0)) - ret |= (*function)(fp); + for (ff = g->iobs, n = g->niobs; --n >= 0; ff++) + if ((ff->file._flags != 0) && + ((ff->file._flags & __SIGN) == 0)) + ret |= (*function)(&ff->file); return (ret); } Index: user/davidxu/libthr/lib/libc/stdio/glue.h =================================================================== --- user/davidxu/libthr/lib/libc/stdio/glue.h (revision 215181) +++ user/davidxu/libthr/lib/libc/stdio/glue.h (working copy) @@ -33,6 +33,11 @@ * $FreeBSD$ */ +struct fullfile { + FILE file; + pthread_mutex_t lock; +}; + /* * The first few FILEs are statically allocated; others are dynamically * allocated and linked in via this glue structure. @@ -40,6 +45,6 @@ struct glue { struct glue *next; int niobs; - FILE *iobs; + struct fullfile *iobs; }; extern struct glue __sglue; Index: user/davidxu/libthr/lib/libc/stdio/findfp.c =================================================================== --- user/davidxu/libthr/lib/libc/stdio/findfp.c (revision 215181) +++ user/davidxu/libthr/lib/libc/stdio/findfp.c (working copy) @@ -55,8 +55,7 @@ #define NDYNAMIC 10 /* add ten more whenever necessary */ - -#define std(flags, file, lock) { \ +#define std(flags, file, lock) { { \ ._flags = (flags), \ ._file = (file), \ ._cookie = __sF + (file), \ @@ -65,25 +64,20 @@ ._seek = __sseek, \ ._write = __swrite, \ ._fl_mutex = &lock, \ -} +}, PTHREAD_MUTEX_INITIALIZER } /* the usual - (stdin + stdout + stderr) */ -static FILE usual[FOPEN_MAX - 3]; +static struct fullfile usual[FOPEN_MAX - 3]; static struct glue uglue = { NULL, FOPEN_MAX - 3, usual }; -static pthread_mutex_t sfLOCK[3] = { - PTHREAD_MUTEX_INITIALIZER, - PTHREAD_MUTEX_INITIALIZER, - PTHREAD_MUTEX_INITIALIZER -}; -static FILE __sF[3] = { - std(__SRD, STDIN_FILENO, sfLOCK[0]), - std(__SWR, STDOUT_FILENO, sfLOCK[1]), - std(__SWR|__SNBF, STDERR_FILENO, sfLOCK[2]) +static struct fullfile __sF[3] = { + std(__SRD, STDIN_FILENO, __sF[0].lock), + std(__SWR, STDOUT_FILENO, __sF[1].lock), + std(__SWR|__SNBF, STDERR_FILENO, __sF[2].lock) }; -FILE *__stdinp = &__sF[0]; -FILE *__stdoutp = &__sF[1]; -FILE *__stderrp = &__sF[2]; +FILE *__stdinp = &__sF[0].file; +FILE *__stdoutp = &__sF[1].file; +FILE *__stderrp = &__sF[2].file; struct glue __sglue = { &uglue, 3, __sF }; static struct glue *lastglue = &uglue; @@ -105,18 +99,23 @@ int n; { struct glue *g; - static FILE empty = { ._fl_mutex = NULL }; - FILE *p; + static struct fullfile empty = { .file = { ._fl_mutex = NULL } }; + struct fullfile *p; - g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); + g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + + n * sizeof(struct fullfile)); if (g == NULL) return (NULL); - p = (FILE *)ALIGN(g + 1); + p = (struct fullfile *)ALIGN(g + 1); g->next = NULL; g->niobs = n; g->iobs = p; - while (--n >= 0) - *p++ = empty; + while (--n >= 0) { + *p = empty; + p->file._fl_mutex = &p->lock; + _pthread_mutex_init(&p->lock, NULL); + p++; + } return (g); } @@ -127,6 +126,7 @@ __sfp() { FILE *fp; + struct fullfile *ff; int n; struct glue *g; @@ -137,9 +137,11 @@ */ THREAD_LOCK(); for (g = &__sglue; g != NULL; g = g->next) { - for (fp = g->iobs, n = g->niobs; --n >= 0; fp++) - if (fp->_flags == 0) + for (ff = g->iobs, n = g->niobs; --n >= 0; ff++) + if (ff->file._flags == 0) { + fp = &ff->file; goto found; + } } THREAD_UNLOCK(); /* don't hold lock while malloc()ing. */ if ((g = moreglue(NDYNAMIC)) == NULL) @@ -147,7 +149,7 @@ THREAD_LOCK(); /* reacquire the lock */ SET_GLUE_PTR(lastglue->next, g); /* atomically append glue to list */ lastglue = g; /* not atomic; only accessed when locked */ - fp = g->iobs; + fp = &g->iobs->file; found: fp->_flags = 1; /* reserve this slot; caller sets real flags */ THREAD_UNLOCK(); @@ -163,10 +165,6 @@ fp->_ub._size = 0; fp->_lb._base = NULL; /* no line buffer */ fp->_lb._size = 0; - if (fp->_fl_mutex == NULL) { /* once set always set (reused) */ - fp->_fl_mutex = malloc(sizeof(struct pthread_mutex)); - _pthread_mutex_init(fp->_fl_mutex, NULL); - } fp->_orientation = 0; memset(&fp->_mbstate, 0, sizeof(mbstate_t)); return (fp); Index: user/davidxu/libthr/lib/libc/gen/opendir.c =================================================================== --- user/davidxu/libthr/lib/libc/gen/opendir.c (revision 215181) +++ user/davidxu/libthr/lib/libc/gen/opendir.c (working copy) @@ -112,6 +112,11 @@ int saved_errno; int unionstack; struct stat statb; + struct { + DIR dir; + pthread_mutex_t lock; + struct _telldir td; + } *mem; dirp = NULL; /* _fstat() the open handler because the file may have changed. */ @@ -122,10 +127,11 @@ goto fail; } if (_fcntl(fd, F_SETFD, FD_CLOEXEC) == -1 || - (dirp = malloc(sizeof(DIR) + sizeof(struct _telldir))) == NULL) + (mem = malloc(sizeof(*mem))) == NULL) goto fail; - dirp->dd_td = (struct _telldir *)((char *)dirp + sizeof(DIR)); + dirp = &mem->dir; + dirp->dd_td = &mem->td; LIST_INIT(&dirp->dd_td->td_locq); dirp->dd_td->td_loccnt = 0; @@ -298,7 +304,7 @@ dirp->dd_loc = 0; dirp->dd_fd = fd; dirp->dd_flags = flags; - dirp->dd_lock = malloc(sizeof(struct pthread_mutex)); + dirp->dd_lock = &mem->lock; _pthread_mutex_init(dirp->dd_lock, NULL); /* Index: user/davidxu/libthr/lib/libc/gen/closedir.c =================================================================== --- user/davidxu/libthr/lib/libc/gen/closedir.c (revision 215181) +++ user/davidxu/libthr/lib/libc/gen/closedir.c (working copy) @@ -64,7 +64,6 @@ if (__isthreaded) { _pthread_mutex_unlock(dirp->dd_lock); _pthread_mutex_destroy(dirp->dd_lock); - free(dirp->dd_lock); } free((void *)dirp); return(_close(fd)); --BOKacYhQ+x31HxR3--