Date: Sat, 13 Nov 2010 13:24:48 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: David Xu <davidxu@freebsd.org> 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 Message-ID: <20101113122447.GA79975@stack.nl> In-Reply-To: <4CDD411D.4060304@freebsd.org> References: <201011100127.oAA1Rmrh069656@svn.freebsd.org> <20101112131941.GA57806@stack.nl> <4CDD411D.4060304@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101113122447.GA79975>