bionic: revert to a single (larger) property area

d329697 is too complicated.  Change the multiple property pages back to
a single 128K property area that's mapped in entirely at initialization
(the memory will not get allocated until the pages are touched).

d329697 has other changes useful for testing (moving property area
initialization inside bionic and adding __system_property_set_filename)
so undo the change manually rather than with git revert.

Change-Id: Icd137669a4f8bc248e9dd2c1e8cc54e9193c9a6d
Signed-off-by: Greg Hackmann <ghackmann@google.com>

diff --git a/libc/bionic/system_properties.c b/libc/bionic/system_properties.c
index 126fea5..d4054d2 100644 (file)
--- a/libc/bionic/system_properties.c
+++ b/libc/bionic/system_properties.c
@@ -109,7 +109,7 @@ typedef struct prop_bt prop_bt;
 static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME;
 static char property_filename[PATH_MAX] = PROP_FILENAME;
 
-prop_area *__system_property_regions__[PA_REGION_COUNT] = { NULL, };
+prop_area *__system_property_area__ = NULL;
 
 const size_t PA_DATA_SIZE = PA_SIZE - sizeof(prop_area);
 
@@ -124,15 +124,10 @@ static int get_fd_from_env(void)
     return atoi(env);
 }
 
-static int map_prop_region_rw(size_t region)
+static int map_prop_area_rw()
 {
     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...
@@ -148,24 +143,21 @@ static int map_prop_region_rw(size_t region)
         return -1;
     }
 
-    if (ftruncate(fd, offset + PA_SIZE) < 0)
+    if (ftruncate(fd, PA_SIZE) < 0)
         goto out;
 
-    pa = mmap(NULL, PA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset);
+    pa = mmap(NULL, PA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
     if(pa == MAP_FAILED)
         goto out;
 
     memset(pa, 0, PA_SIZE);
     pa->magic = PROP_AREA_MAGIC;
     pa->version = PROP_AREA_VERSION;
-
-    if (!region) {
-        /* reserve root node */
-        pa->bytes_used += sizeof(prop_bt);
-    }
+    /* reserve root node */
+    pa->bytes_used = sizeof(prop_bt);
 
     /* plug into the lib property services */
-    __system_property_regions__[region] = pa;
+    __system_property_area__ = pa;
 
     close(fd);
     return 0;
@@ -187,20 +179,14 @@ int __system_property_set_filename(const char *filename)
 
 int __system_property_area_init()
 {
-    return map_prop_region_rw(0);
+    return map_prop_area_rw();
 }
 
-static int map_prop_region(size_t region)
+static int map_prop_area()
 {
     bool fromFile = true;
-    bool swapped;
-    size_t offset = region * PA_SIZE;
     int result = -1;
 
-    if(__system_property_regions__[region]) {
-        return 0;
-    }
-
     int fd = open(property_filename, O_RDONLY | O_NOFOLLOW);
 
     if ((fd < 0) && (errno == ENOENT)) {
@@ -229,11 +215,11 @@ static int map_prop_region(size_t region)
     if ((fd_stat.st_uid != 0)
             || (fd_stat.st_gid != 0)
             || ((fd_stat.st_mode & (S_IWGRP | S_IWOTH)) != 0)
-            || (fd_stat.st_size < offset + PA_SIZE) ) {
+            || (fd_stat.st_size < PA_SIZE) ) {
         goto cleanup;
     }
 
-    prop_area *pa = mmap(NULL, PA_SIZE, PROT_READ, MAP_SHARED, fd, offset);
+    prop_area *pa = mmap(NULL, PA_SIZE, PROT_READ, MAP_SHARED, fd, 0);
 
     if (pa == MAP_FAILED) {
         goto cleanup;
@@ -245,16 +231,8 @@ static int map_prop_region(size_t region)
     }
 
     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);
-    }
+
+    __system_property_area__ = pa;
 
 cleanup:
     if (fromFile) {
@@ -266,33 +244,20 @@ cleanup:
 
 int __system_properties_init()
 {
-    return map_prop_region(0);
+    return map_prop_area();
 }
 
 static void *new_prop_obj(size_t size, prop_off_t *off)
 {
-    prop_area *pa;
-    size_t i, idx;
+    prop_area *pa = __system_property_area__;
     size = ALIGN(size, sizeof(uint32_t));
 
-    for (i = 0; i < PA_REGION_COUNT; i++) {
-        int err = map_prop_region_rw(i);
-        if (err < 0) {
-            return NULL;
-        }
-
-        pa = __system_property_regions__[i];
-        if (pa->bytes_used + size <= PA_DATA_SIZE)
-            break;
-    }
-
-    if (i == PA_REGION_COUNT)
+    if (pa->bytes_used + size > PA_DATA_SIZE)
         return NULL;
 
-    idx = pa->bytes_used;
-    *off = idx + i * PA_DATA_SIZE;
-    pa->bytes_used += size;
-    return pa->data + idx;
+    *off = pa->bytes_used;
+    __system_property_area__->bytes_used += size;
+    return __system_property_area__->data + *off;
 }
 
 static prop_bt *new_prop_bt(const char *name, uint8_t namelen, prop_off_t *off)
@@ -330,16 +295,10 @@ static prop_info *new_prop_info(const char *name, uint8_t namelen,
 
 static void *to_prop_obj(prop_off_t off)
 {
-    size_t region = off / PA_DATA_SIZE;
-    size_t idx = off % PA_DATA_SIZE;
-
-    if (region > PA_REGION_COUNT)
-        return NULL;
-
-    if (map_prop_region(region) < 0)
+    if (off > PA_DATA_SIZE)
         return NULL;
 
-    return __system_property_regions__[region]->data + idx;
+    return __system_property_area__->data + off;
 }
 
 static prop_bt *root_node()
@@ -570,7 +529,7 @@ int __system_property_wait(const prop_info *pi)
 {
     unsigned n;
     if(pi == 0) {
-        prop_area *pa = __system_property_regions__[0];
+        prop_area *pa = __system_property_area__;
         n = pa->serial;
         do {
             __futex_wait(&pa->serial, n, 0);
@@ -586,7 +545,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_regions__[0];
+    prop_area *pa = __system_property_area__;
 
     if (len >= PROP_VALUE_MAX)
         return -1;
@@ -607,7 +566,7 @@ 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_regions__[0];
+    prop_area *pa = __system_property_area__;
     const prop_info *pi;
 
     if (namelen >= PROP_NAME_MAX)
@@ -633,7 +592,7 @@ unsigned int __system_property_serial(const prop_info *pi)
 
 unsigned int __system_property_wait_any(unsigned int serial)
 {
-    prop_area *pa = __system_property_regions__[0];
+    prop_area *pa = __system_property_area__;
 
     do {
         __futex_wait(&pa->serial, serial, 0);

diff --git a/libc/include/sys/_system_properties.h b/libc/include/sys/_system_properties.h
index 9c48e1b..92e35e1 100644 (file)
--- a/libc/include/sys/_system_properties.h
+++ b/libc/include/sys/_system_properties.h
@@ -42,11 +42,7 @@ typedef struct prop_msg prop_msg;
 #define PROP_SERVICE_NAME "property_service"
 #define PROP_FILENAME "/dev/__properties__"
 
-/* (4 header words + 28 toc words) = 128 bytes */
-/* 128 bytes header and toc + 28 prop_infos @ 128 bytes = 3712 bytes */
-
-#define PA_REGION_COUNT 128
-#define PA_SIZE         4096
+#define PA_SIZE         (128 * 1024)
 
 #define SERIAL_VALUE_LEN(serial) ((serial) >> 24)
 #define SERIAL_DIRTY(serial) ((serial) & 1)

diff --git a/tests/property_benchmark.cpp b/tests/property_benchmark.cpp
index 7266bd0..d10be91 100644 (file)
--- a/tests/property_benchmark.cpp
+++ b/tests/property_benchmark.cpp
@@ -23,7 +23,7 @@
 #include <vector>
 #include <string>
 
-extern void *__system_property_regions__[PA_REGION_COUNT];
+extern void *__system_property_area__;
 
 #define TEST_NUM_PROPS \
     Arg(1)->Arg(4)->Arg(16)->Arg(64)->Arg(128)->Arg(256)->Arg(512)->Arg(1024)
@@ -39,10 +39,8 @@ struct LocalPropertyTestState {
             return;
         }
 
-        for (size_t i = 0; i < PA_REGION_COUNT; i++) {
-            old_pa[i] = __system_property_regions__[i];
-            __system_property_regions__[i] = NULL;
-        }
+        old_pa = __system_property_area__;
+        __system_property_area__ = NULL;
 
         pa_dirname = dirname;
         pa_filename = pa_dirname + "/__properties__";
@@ -79,9 +77,7 @@ struct LocalPropertyTestState {
         if (!valid)
             return;
 
-        for (size_t i = 0; i < PA_REGION_COUNT; i++) {
-            __system_property_regions__[i] = old_pa[i];
-        }
+        __system_property_area__ = old_pa;
 
         __system_property_set_filename(PROP_FILENAME);
         unlink(pa_filename.c_str());
@@ -107,7 +103,7 @@ public:
 private:
     std::string pa_dirname;
     std::string pa_filename;
-    void *old_pa[PA_REGION_COUNT];
+    void *old_pa;
 };
 
 static void BM_property_get(int iters, int nprops)

diff --git a/tests/system_properties_test.cpp b/tests/system_properties_test.cpp
index bafd2c3..b9256c6 100644 (file)
--- a/tests/system_properties_test.cpp
+++ b/tests/system_properties_test.cpp
@@ -24,7 +24,7 @@
 #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_
 #include <sys/_system_properties.h>
 
-extern void *__system_property_regions__[PA_REGION_COUNT];
+extern void *__system_property_area__;
 
 struct LocalPropertyTestState {
     LocalPropertyTestState() : valid(false) {
@@ -35,10 +35,8 @@ struct LocalPropertyTestState {
             return;
         }
 
-        for (size_t i = 0; i < PA_REGION_COUNT; i++) {
-            old_pa[i] = __system_property_regions__[i];
-            __system_property_regions__[i] = NULL;
-        }
+        old_pa = __system_property_area__;
+        __system_property_area__ = NULL;
 
         pa_dirname = dirname;
         pa_filename = pa_dirname + "/__properties__";
@@ -52,9 +50,7 @@ struct LocalPropertyTestState {
         if (!valid)
             return;
 
-        for (size_t i = 0; i < PA_REGION_COUNT; i++) {
-            __system_property_regions__[i] = old_pa[i];
-        }
+        __system_property_area__ = old_pa;
 
         __system_property_set_filename(PROP_FILENAME);
         unlink(pa_filename.c_str());
@@ -65,7 +61,7 @@ public:
 private:
     std::string pa_dirname;
     std::string pa_filename;
-    void *old_pa[PA_REGION_COUNT];
+    void *old_pa;
 };
 
 TEST(properties, add) {