From d32969701be070c0161c2643ee3c3df16066bbb8 Mon Sep 17 00:00:00 2001 From: Greg Hackmann Date: Wed, 13 Feb 2013 14:41:48 -0800 Subject: [PATCH] bionic: make property area expandable The property area is initially one 4K region, automatically expanding as needed up to 64 regions. To avoid duplicating code, __system_property_area_init() now allocates and initializes the first region (previously it was allocated in init's init_property_area() and initialized in bionic). For testing purposes, __system_property_set_filename() may be used to override the file used to map in regions. Change-Id: Ibe00ef52464bfa590953c4699a6d98383b0142b1 Signed-off-by: Greg Hackmann --- libc/bionic/system_properties.c | 187 ++++++++++++++++++++++++++-------- libc/include/sys/_system_properties.h | 19 ++-- tests/property_benchmark.cpp | 56 ++++++++-- tests/system_properties_test.cpp | 76 ++++++++++---- 4 files changed, 264 insertions(+), 74 deletions(-) diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c index 5197ef3ff..800c8b84f 100644 --- a/libc/bionic/system_properties.c +++ b/libc/bionic/system_properties.c @@ -55,7 +55,6 @@ struct prop_area { unsigned volatile serial; unsigned magic; unsigned version; - unsigned reserved[4]; unsigned toc[1]; }; @@ -70,10 +69,9 @@ struct prop_info { typedef struct prop_info prop_info; static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME; +static char property_filename[PATH_MAX] = PROP_FILENAME; -static unsigned dummy_props = 0; - -prop_area *__system_property_area__ = (void*) &dummy_props; +prop_area *__system_property_regions__[PA_REGION_COUNT] = { NULL, }; static int get_fd_from_env(void) { @@ -86,27 +84,79 @@ static int get_fd_from_env(void) return atoi(env); } -void __system_property_area_init(void *data) +static int map_prop_region_rw(size_t region) { - prop_area *pa = data; + prop_area *pa; + int fd; + size_t offset = region * PA_SIZE; + + if (__system_property_regions__[region]) { + return 0; + } + + /* dev is a tmpfs that we can use to carve a shared workspace + * out of, so let's do that... + */ + fd = open(property_filename, O_RDWR | O_CREAT | O_NOFOLLOW, 0644); + if (fd < 0) { + if (errno == EACCES) { + /* for consistency with the case where the process has already + * mapped the page in and segfaults when trying to write to it + */ + abort(); + } + return -1; + } + + if (ftruncate(fd, offset + PA_SIZE) < 0) + goto out; + + pa = mmap(NULL, PA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset); + if(pa == MAP_FAILED) + goto out; + memset(pa, 0, PA_SIZE); pa->magic = PROP_AREA_MAGIC; pa->version = PROP_AREA_VERSION; /* plug into the lib property services */ - __system_property_area__ = pa; + __system_property_regions__[region] = pa; + + close(fd); + return 0; + +out: + close(fd); + return -1; +} + +int __system_property_set_filename(const char *filename) +{ + size_t len = strlen(filename); + if (len >= sizeof(property_filename)) + return -1; + + strcpy(property_filename, filename); + return 0; +} + +int __system_property_area_init() +{ + return map_prop_region_rw(0); } -int __system_properties_init(void) +static int map_prop_region(size_t region) { bool fromFile = true; + bool swapped; + size_t offset = region * PA_SIZE; int result = -1; - if(__system_property_area__ != ((void*) &dummy_props)) { + if(__system_property_regions__[region]) { return 0; } - int fd = open(PROP_FILENAME, O_RDONLY | O_NOFOLLOW); + int fd = open(property_filename, O_RDONLY | O_NOFOLLOW); if ((fd < 0) && (errno == ENOENT)) { /* @@ -133,23 +183,33 @@ int __system_properties_init(void) if ((fd_stat.st_uid != 0) || (fd_stat.st_gid != 0) - || ((fd_stat.st_mode & (S_IWGRP | S_IWOTH)) != 0)) { + || ((fd_stat.st_mode & (S_IWGRP | S_IWOTH)) != 0) + || (fd_stat.st_size < offset + PA_SIZE) ) { goto cleanup; } - prop_area *pa = mmap(NULL, fd_stat.st_size, PROT_READ, MAP_SHARED, fd, 0); + prop_area *pa = mmap(NULL, PA_SIZE, PROT_READ, MAP_SHARED, fd, offset); if (pa == MAP_FAILED) { goto cleanup; } if((pa->magic != PROP_AREA_MAGIC) || (pa->version != PROP_AREA_VERSION)) { - munmap(pa, fd_stat.st_size); + munmap(pa, PA_SIZE); goto cleanup; } - __system_property_area__ = pa; result = 0; + swapped = __sync_bool_compare_and_swap(&__system_property_regions__[region], + NULL, pa); + if (!swapped) { + /** + * In the event of a race either mapping is equally good, so + * the thread that lost can just throw its mapping away and proceed as + * normal. + */ + munmap(pa, PA_SIZE); + } cleanup: if (fromFile) { @@ -159,17 +219,31 @@ cleanup: return result; } +int __system_properties_init() +{ + return map_prop_region(0); +} + int __system_property_foreach( void (*propfn)(const prop_info *pi, void *cookie), void *cookie) { - prop_area *pa = __system_property_area__; - unsigned i; + size_t region; + + for (region = 0; region < PA_REGION_COUNT; region++) { + prop_area *pa; + unsigned i; + + int err = map_prop_region(region); + if (err < 0) + break; + pa = __system_property_regions__[region]; - for (i = 0; i < pa->count; i++) { - unsigned entry = pa->toc[i]; - prop_info *pi = TOC_TO_INFO(pa, entry); - propfn(pi, cookie); + for (i = 0; i < pa->count; i++) { + unsigned entry = pa->toc[i]; + prop_info *pi = TOC_TO_INFO(pa, entry); + propfn(pi, cookie); + } } return 0; @@ -177,9 +251,15 @@ int __system_property_foreach( const prop_info *__system_property_find_nth(unsigned n) { - prop_area *pa = __system_property_area__; + size_t region = n / PA_COUNT_MAX; + prop_area *pa; + + int err = map_prop_region(region); + if (err < 0) + return NULL; + pa = __system_property_regions__[region]; - if(n >= pa->count) { + if((n % PA_COUNT_MAX) >= pa->count) { return 0; } else { return TOC_TO_INFO(pa, pa->toc[n]); @@ -188,25 +268,36 @@ const prop_info *__system_property_find_nth(unsigned n) const prop_info *__system_property_find(const char *name) { - prop_area *pa = __system_property_area__; - unsigned count = pa->count; - unsigned *toc = pa->toc; unsigned len = strlen(name); - prop_info *pi; + size_t region; if (len >= PROP_NAME_MAX) return 0; if (len < 1) return 0; - while(count--) { - unsigned entry = *toc++; - if(TOC_NAME_LEN(entry) != len) continue; + for (region = 0; region < PA_REGION_COUNT; region++) { + prop_area *pa; + unsigned count; + unsigned *toc; + prop_info *pi; + + int err = map_prop_region(region); + if (err < 0) + return 0; + pa = __system_property_regions__[region]; + count = pa->count; + toc = pa->toc; + + while(count--) { + unsigned entry = *toc++; + if(TOC_NAME_LEN(entry) != len) continue; - pi = TOC_TO_INFO(pa, entry); - if(memcmp(name, pi->name, len)) continue; + pi = TOC_TO_INFO(pa, entry); + if(memcmp(name, pi->name, len)) continue; - return pi; + return pi; + } } return 0; @@ -333,7 +424,7 @@ int __system_property_wait(const prop_info *pi) { unsigned n; if(pi == 0) { - prop_area *pa = __system_property_area__; + prop_area *pa = __system_property_regions__[0]; n = pa->serial; do { __futex_wait(&pa->serial, n, 0); @@ -349,7 +440,7 @@ int __system_property_wait(const prop_info *pi) int __system_property_update(prop_info *pi, const char *value, unsigned int len) { - prop_area *pa = __system_property_area__; + prop_area *pa = __system_property_regions__[0]; if (len >= PROP_VALUE_MAX) return -1; @@ -368,12 +459,11 @@ int __system_property_update(prop_info *pi, const char *value, unsigned int len) int __system_property_add(const char *name, unsigned int namelen, const char *value, unsigned int valuelen) { - prop_area *pa = __system_property_area__; - prop_info *pa_info_array = (void*) (((char*) pa) + PA_INFO_START); + prop_area *pa; + prop_info *pa_info_array; prop_info *pi; + size_t region; - if (pa->count == PA_COUNT_MAX) - return -1; if (namelen >= PROP_NAME_MAX) return -1; if (valuelen >= PROP_VALUE_MAX) @@ -381,13 +471,28 @@ int __system_property_add(const char *name, unsigned int namelen, if (namelen < 1) return -1; + for (region = 0; region < PA_REGION_COUNT; region++) + { + int err = map_prop_region_rw(region); + if (err < 0) + return -1; + + pa = __system_property_regions__[region]; + + if (pa->count < PA_COUNT_MAX) + break; + } + + if (region == PA_REGION_COUNT) + return -1; + + pa_info_array = (void*) (((char*) pa) + PA_INFO_START); pi = pa_info_array + pa->count; pi->serial = (valuelen << 24); memcpy(pi->name, name, namelen + 1); memcpy(pi->value, value, valuelen + 1); - pa->toc[pa->count] = - (namelen << 24) | (((unsigned) pi) - ((unsigned) pa)); + pa->toc[pa->count] = (namelen << 24) | (((unsigned) pi) - ((unsigned) pa)); pa->count++; pa->serial++; @@ -403,7 +508,7 @@ unsigned int __system_property_serial(const prop_info *pi) unsigned int __system_property_wait_any(unsigned int serial) { - prop_area *pa = __system_property_area__; + prop_area *pa = __system_property_regions__[0]; do { __futex_wait(&pa->serial, serial, 0); diff --git a/libc/include/sys/_system_properties.h b/libc/include/sys/_system_properties.h index c5bc2235b..4971a4c12 100644 --- a/libc/include/sys/_system_properties.h +++ b/libc/include/sys/_system_properties.h @@ -42,12 +42,13 @@ typedef struct prop_msg prop_msg; #define PROP_SERVICE_NAME "property_service" #define PROP_FILENAME "/dev/__properties__" -/* (8 header words + 247 toc words) = 1020 bytes */ -/* 1024 bytes header and toc + 247 prop_infos @ 128 bytes = 32640 bytes */ +/* (4 header words + 28 toc words) = 128 bytes */ +/* 128 bytes header and toc + 28 prop_infos @ 128 bytes = 3712 bytes */ -#define PA_COUNT_MAX 247 -#define PA_INFO_START 1024 -#define PA_SIZE 32768 +#define PA_COUNT_MAX 28 +#define PA_REGION_COUNT 128 +#define PA_INFO_START 128 +#define PA_SIZE 4096 #define TOC_NAME_LEN(toc) ((toc) >> 24) #define TOC_TO_INFO(area, toc) ((prop_info*) (((char*) area) + ((toc) & 0xFFFFFF))) @@ -97,11 +98,17 @@ struct prop_msg #define PROP_PATH_FACTORY "/factory/factory.prop" /* +** Map the property area from the specified filename. This +** method is for testing only. +*/ +int __system_property_set_filename(const char *filename); + +/* ** Initialize the area to be used to store properties. Can ** only be done by a single process that has write access to ** the property area. */ -void __system_property_area_init(void *data); +int __system_property_area_init(); /* Add a new system property. Can only be done by a single ** process that has write access to the property area, and diff --git a/tests/property_benchmark.cpp b/tests/property_benchmark.cpp index 2c8e2a115..7266bd08a 100644 --- a/tests/property_benchmark.cpp +++ b/tests/property_benchmark.cpp @@ -15,23 +15,40 @@ */ #include "benchmark.h" +#include #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ #include #include +#include -extern void *__system_property_area__; +extern void *__system_property_regions__[PA_REGION_COUNT]; #define TEST_NUM_PROPS \ - Arg(1)->Arg(4)->Arg(16)->Arg(64)->Arg(128)->Arg(247) + Arg(1)->Arg(4)->Arg(16)->Arg(64)->Arg(128)->Arg(256)->Arg(512)->Arg(1024) struct LocalPropertyTestState { - LocalPropertyTestState(int nprops) : nprops(nprops) { + LocalPropertyTestState(int nprops) : nprops(nprops), valid(false) { static const char prop_name_chars[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ.-_"; - old_pa = __system_property_area__; - pa = malloc(PA_SIZE); - __system_property_area_init(pa); + + char dir_template[] = "/data/nativetest/prop-XXXXXX"; + char *dirname = mkdtemp(dir_template); + if (!dirname) { + perror("making temp file for test state failed (is /data/nativetest writable?)"); + return; + } + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + old_pa[i] = __system_property_regions__[i]; + __system_property_regions__[i] = NULL; + } + + pa_dirname = dirname; + pa_filename = pa_dirname + "/__properties__"; + + __system_property_set_filename(pa_filename.c_str()); + __system_property_area_init(); names = new char* [nprops]; name_lens = new int[nprops]; @@ -54,10 +71,22 @@ struct LocalPropertyTestState { } __system_property_add(names[i], name_lens[i], values[i], value_lens[i]); } + + valid = true; } ~LocalPropertyTestState() { - __system_property_area__ = old_pa; + if (!valid) + return; + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + __system_property_regions__[i] = old_pa[i]; + } + + __system_property_set_filename(PROP_FILENAME); + unlink(pa_filename.c_str()); + rmdir(pa_dirname.c_str()); + for (int i = 0; i < nprops; i++) { delete names[i]; delete values[i]; @@ -66,7 +95,6 @@ struct LocalPropertyTestState { delete[] name_lens; delete[] values; delete[] value_lens; - free(pa); } public: const int nprops; @@ -74,10 +102,12 @@ public: int *name_lens; char **values; int *value_lens; + bool valid; private: - void *pa; - void *old_pa; + std::string pa_dirname; + std::string pa_filename; + void *old_pa[PA_REGION_COUNT]; }; static void BM_property_get(int iters, int nprops) @@ -87,6 +117,9 @@ static void BM_property_get(int iters, int nprops) LocalPropertyTestState pa(nprops); char value[PROP_VALUE_MAX]; + if (!pa.valid) + return; + srandom(iters * nprops); StartBenchmarkTiming(); @@ -104,6 +137,9 @@ static void BM_property_find(int iters, int nprops) LocalPropertyTestState pa(nprops); + if (!pa.valid) + return; + srandom(iters * nprops); StartBenchmarkTiming(); diff --git a/tests/system_properties_test.cpp b/tests/system_properties_test.cpp index 70ff1d662..50bdfdfe4 100644 --- a/tests/system_properties_test.cpp +++ b/tests/system_properties_test.cpp @@ -16,32 +16,61 @@ #include #include +#include +#include #if __BIONIC__ #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ #include -extern void *__system_property_area__; +extern void *__system_property_regions__[PA_REGION_COUNT]; struct LocalPropertyTestState { - LocalPropertyTestState() { - old_pa = __system_property_area__; - pa = malloc(PA_SIZE); - __system_property_area_init(pa); + LocalPropertyTestState() : valid(false) { + char dir_template[] = "/data/nativetest/prop-XXXXXX"; + char *dirname = mkdtemp(dir_template); + if (!dirname) { + perror("making temp file for test state failed (is /data/nativetest writable?)"); + return; + } + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + old_pa[i] = __system_property_regions__[i]; + __system_property_regions__[i] = NULL; + } + + pa_dirname = dirname; + pa_filename = pa_dirname + "/__properties__"; + + __system_property_set_filename(pa_filename.c_str()); + __system_property_area_init(); + valid = true; } ~LocalPropertyTestState() { - __system_property_area__ = old_pa; - free(pa); + if (!valid) + return; + + for (size_t i = 0; i < PA_REGION_COUNT; i++) { + __system_property_regions__[i] = old_pa[i]; + } + + __system_property_set_filename(PROP_FILENAME); + unlink(pa_filename.c_str()); + rmdir(pa_dirname.c_str()); } +public: + bool valid; private: - void *pa; - void *old_pa; + std::string pa_dirname; + std::string pa_filename; + void *old_pa[PA_REGION_COUNT]; }; TEST(properties, add) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char propvalue[PROP_VALUE_MAX]; @@ -61,6 +90,7 @@ TEST(properties, add) { TEST(properties, update) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char propvalue[PROP_VALUE_MAX]; prop_info *pi; @@ -91,27 +121,34 @@ TEST(properties, update) { ASSERT_STREQ(propvalue, "value6"); } -// 247 = max # of properties supported by current implementation -// (this should never go down) -TEST(properties, fill_247) { +TEST(properties, fill) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char prop_name[PROP_NAME_MAX]; char prop_value[PROP_VALUE_MAX]; char prop_value_ret[PROP_VALUE_MAX]; + int count = 0; int ret; - for (int i = 0; i < 247; i++) { - ret = snprintf(prop_name, PROP_NAME_MAX - 1, "property_%d", i); + while (true) { + ret = snprintf(prop_name, PROP_NAME_MAX - 1, "property_%d", count); memset(prop_name + ret, 'a', PROP_NAME_MAX - 1 - ret); - ret = snprintf(prop_value, PROP_VALUE_MAX - 1, "value_%d", i); + ret = snprintf(prop_value, PROP_VALUE_MAX - 1, "value_%d", count); memset(prop_value + ret, 'b', PROP_VALUE_MAX - 1 - ret); prop_name[PROP_NAME_MAX - 1] = 0; prop_value[PROP_VALUE_MAX - 1] = 0; - ASSERT_EQ(0, __system_property_add(prop_name, PROP_NAME_MAX - 1, prop_value, PROP_VALUE_MAX - 1)); + ret = __system_property_add(prop_name, PROP_NAME_MAX - 1, prop_value, PROP_VALUE_MAX - 1); + if (ret < 0) + break; + + count++; } - for (int i = 0; i < 247; i++) { + // For historical reasons at least 247 properties must be supported + ASSERT_GE(count, 247); + + for (int i = 0; i < count; i++) { ret = snprintf(prop_name, PROP_NAME_MAX - 1, "property_%d", i); memset(prop_name + ret, 'a', PROP_NAME_MAX - 1 - ret); ret = snprintf(prop_value, PROP_VALUE_MAX - 1, "value_%d", i); @@ -134,6 +171,7 @@ static void foreach_test_callback(const prop_info *pi, void* cookie) { TEST(properties, foreach) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); size_t count = 0; ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6)); @@ -146,6 +184,7 @@ TEST(properties, foreach) { TEST(properties, find_nth) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6)); ASSERT_EQ(0, __system_property_add("other_property", 14, "value2", 6)); @@ -165,6 +204,7 @@ TEST(properties, find_nth) { TEST(properties, errors) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); char prop_value[PROP_NAME_MAX]; ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6)); @@ -181,6 +221,7 @@ TEST(properties, errors) { TEST(properties, serial) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); const prop_info *pi; unsigned int serial; @@ -206,6 +247,7 @@ static void *PropertyWaitHelperFn(void *arg) TEST(properties, wait) { LocalPropertyTestState pa; + ASSERT_TRUE(pa.valid); unsigned int serial; prop_info *pi; pthread_t t; -- 2.11.0