From 9831ad3ce6bd5b22da16a275ed67e7236eae3d1f Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Mon, 29 Aug 2011 21:43:46 +0200 Subject: [PATCH] libc: speed-up flockfile()/funlockfile() For Honeycomb, we added proper file thread-safety for all FILE* operations. However, we did implement that by using an out-of-band hash table to map FILE* pointers to phtread_mutex_t mutexes, because we couldn't change the size of 'struct _sFILE' without breaking the ABI. It turns out that our BSD-derived code already has some support code to extend FILE* objects, so use it instead. See libc/stdio/fileext.h This patch gets rid of the hash table, and put the mutex directly into the sFILE extension. Change-Id: If1c3fe0a0a89da49c568e9a7560b7827737ff4d0 --- libc/include/pthread.h | 10 ++- libc/stdio/fileext.h | 17 +++++ libc/stdio/findfp.c | 7 +- libc/stdio/flockfile.c | 163 ++++------------------------------------- 4 files changed, 44 insertions(+), 153 deletions(-) diff --git a/libc/include/pthread.h b/libc/include/pthread.h index 9d05769ef..2015ac08a 100644 --- a/libc/include/pthread.h +++ b/libc/include/pthread.h @@ -42,9 +42,13 @@ typedef struct int volatile value; } pthread_mutex_t; -#define PTHREAD_MUTEX_INITIALIZER {0} -#define PTHREAD_RECURSIVE_MUTEX_INITIALIZER {0x4000} -#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER {0x8000} +#define __PTHREAD_MUTEX_INIT_VALUE 0 +#define __PTHREAD_RECURSIVE_MUTEX_INIT_VALUE 0x4000 +#define __PTHREAD_ERRORCHECK_MUTEX_INIT_VALUE 0x8000 + +#define PTHREAD_MUTEX_INITIALIZER {__PTHREAD_MUTEX_INIT_VALUE} +#define PTHREAD_RECURSIVE_MUTEX_INITIALIZER {__PTHREAD_RECURSIVE_MUTEX_INIT_VALUE} +#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER {__PTHREAD_ERRORCHECK_MUTEX_INIT_VALUE} enum { PTHREAD_MUTEX_NORMAL = 0, diff --git a/libc/stdio/fileext.h b/libc/stdio/fileext.h index 2d0704302..b36a44866 100644 --- a/libc/stdio/fileext.h +++ b/libc/stdio/fileext.h @@ -29,24 +29,41 @@ * $Citrus$ */ +#include +#include "wcio.h" + /* * file extension */ struct __sfileext { struct __sbuf _ub; /* ungetc buffer */ struct wchar_io_data _wcio; /* wide char io status */ + pthread_mutex_t _lock; /* file lock */ }; +#define _FILEEXT_INITIALIZER {{NULL,0},{0},PTHREAD_RECURSIVE_MUTEX_INITIALIZER} + #define _EXT(fp) ((struct __sfileext *)((fp)->_ext._base)) #define _UB(fp) _EXT(fp)->_ub +#define _FLOCK(fp) _EXT(fp)->_lock #define _FILEEXT_INIT(fp) \ do { \ _UB(fp)._base = NULL; \ _UB(fp)._size = 0; \ WCIO_INIT(fp); \ + _FLOCK_INIT(fp); \ } while (0) +/* Helper macros to avoid a function call when you know that fp is not NULL. + * Notice that we keep _FLOCK_INIT() fast by slightly breaking our pthread + * encapsulation. + */ +#define _FLOCK_INIT(fp) _FLOCK(fp).value = __PTHREAD_RECURSIVE_MUTEX_INIT_VALUE +#define _FLOCK_LOCK(fp) pthread_mutex_lock(&_FLOCK(fp)) +#define _FLOCK_TRYLOCK(fp) pthread_mutex_trylock(&_FLOCK(fp)) +#define _FLOCK_UNLOCK(fp) pthread_mutex_unlock(&_FLOCK(fp)) + #define _FILEEXT_SETUP(f, fext) \ do { \ (f)->_ext._base = (unsigned char *)(fext); \ diff --git a/libc/stdio/findfp.c b/libc/stdio/findfp.c index a659c871c..76ed5ee67 100644 --- a/libc/stdio/findfp.c +++ b/libc/stdio/findfp.c @@ -58,7 +58,12 @@ static struct glue uglue = { 0, FOPEN_MAX - 3, usual }; static struct glue *lastglue = &uglue; _THREAD_PRIVATE_MUTEX(__sfp_mutex); -static struct __sfileext __sFext[3]; +static struct __sfileext __sFext[3] = { + _FILEEXT_INITIALIZER, + _FILEEXT_INITIALIZER, + _FILEEXT_INITIALIZER, +}; + FILE __sF[3] = { std(__SRD, STDIN_FILENO), /* stdin */ std(__SWR, STDOUT_FILENO), /* stdout */ diff --git a/libc/stdio/flockfile.c b/libc/stdio/flockfile.c index e8c74c561..368fb1582 100644 --- a/libc/stdio/flockfile.c +++ b/libc/stdio/flockfile.c @@ -31,122 +31,23 @@ * we can't use the OpenBSD implementation which uses kernel-specific * APIs not available on Linux. * - * Ideally, this would be trivially implemented by adding a - * pthread_mutex_t field to struct __sFILE as defined in - * . - * - * However, since we don't want to bring pthread into the mix - * as well as change the size of a public API/ABI structure, - * we're going to store the data out-of-band. - * - * we use a hash-table to map FILE* pointers to recursive mutexes - * fclose() will call __fremovelock() defined below to remove - * a pointer from the table. + * Instead, we use a pthread_mutex_t within the FILE* internal state. + * See fileext.h for details. * * the behaviour, if fclose() is called while the corresponding * file is locked is totally undefined. */ #include -#include #include +#include +#include "fileext.h" -/* a node in the hash table */ -typedef struct FileLock { - struct FileLock* next; - FILE* file; - pthread_mutex_t mutex; -} FileLock; - -/* use a static hash table. We assume that we're not going to - * lock a really large number of FILE* objects on an embedded - * system. - */ -#define FILE_LOCK_BUCKETS 32 - -typedef struct { - pthread_mutex_t lock; - FileLock* buckets[ FILE_LOCK_BUCKETS ]; -} LockTable; - -static LockTable* _lockTable; -static pthread_once_t _lockTable_once = PTHREAD_ONCE_INIT; - -static void -lock_table_init( void ) -{ - _lockTable = malloc(sizeof(*_lockTable)); - if (_lockTable != NULL) { - pthread_mutex_init(&_lockTable->lock, NULL); - memset(_lockTable->buckets, 0, sizeof(_lockTable->buckets)); - } -} - -static LockTable* -lock_table_lock( void ) -{ - pthread_once( &_lockTable_once, lock_table_init ); - pthread_mutex_lock( &_lockTable->lock ); - return _lockTable; -} - -static void -lock_table_unlock( LockTable* t ) -{ - pthread_mutex_unlock( &t->lock ); -} - -static FileLock** -lock_table_lookup( LockTable* t, FILE* f ) -{ - uint32_t hash = (uint32_t)(void*)f; - FileLock** pnode; - - hash = (hash >> 2) ^ (hash << 17); - pnode = &t->buckets[hash % FILE_LOCK_BUCKETS]; - for (;;) { - FileLock* node = *pnode; - if (node == NULL || node->file == f) - break; - pnode = &node->next; - } - return pnode; -} void flockfile(FILE * fp) { - LockTable* t = lock_table_lock(); - - if (t != NULL) { - FileLock** lookup = lock_table_lookup(t, fp); - FileLock* lock = *lookup; - - if (lock == NULL) { - pthread_mutexattr_t attr; - - /* create a new node in the hash table */ - lock = malloc(sizeof(*lock)); - if (lock == NULL) { - lock_table_unlock(t); - return; - } - lock->next = NULL; - lock->file = fp; - - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - pthread_mutex_init( &lock->mutex, &attr ); - - *lookup = lock; - } - lock_table_unlock(t); - - /* we assume that another thread didn't destroy 'lock' - * by calling fclose() on the FILE*. This can happen if - * the client is *really* buggy, but we don't care about - * such code here. - */ - pthread_mutex_lock(&lock->mutex); + if (fp != NULL) { + _FLOCK_LOCK(fp); } } @@ -154,21 +55,13 @@ flockfile(FILE * fp) int ftrylockfile(FILE *fp) { - int ret = -1; - LockTable* t = lock_table_lock(); + /* The specification for ftrylockfile() says it returns 0 on success, + * or non-zero on error. So return an errno code directly on error. + */ + int ret = EINVAL; - if (t != NULL) { - FileLock** lookup = lock_table_lookup(t, fp); - FileLock* lock = *lookup; - - lock_table_unlock(t); - - /* see above comment about why we assume that 'lock' can - * be accessed from here - */ - if (lock != NULL && !pthread_mutex_trylock(&lock->mutex)) { - ret = 0; /* signal success */ - } + if (fp != NULL) { + ret = _FLOCK_TRYLOCK(fp); } return ret; } @@ -176,35 +69,7 @@ ftrylockfile(FILE *fp) void funlockfile(FILE * fp) { - LockTable* t = lock_table_lock(); - - if (t != NULL) { - FileLock** lookup = lock_table_lookup(t, fp); - FileLock* lock = *lookup; - - if (lock != NULL) - pthread_mutex_unlock(&lock->mutex); - - lock_table_unlock(t); - } -} - - -/* called from fclose() to remove the file lock */ -__LIBC_HIDDEN__ void -__fremovelock(FILE* fp) -{ - LockTable* t = lock_table_lock(); - - if (t != NULL) { - FileLock** lookup = lock_table_lookup(t, fp); - FileLock* lock = *lookup; - - if (lock != NULL) { - *lookup = lock->next; - lock->file = NULL; - } - lock_table_unlock(t); - free(lock); + if (fp != NULL) { + _FLOCK_UNLOCK(fp); } }