Merge pull request #101 from struct/6_11_22_perf_v2

doc fixes, perf fixes through removing redundant code

Author: struct

.github/workflows/testsuite.yml

@@ -49,6 +49,12 @@ jobs:
       - run: sudo apt install clang-12
       - run: make tests
       - run: make cpp_tests
+  testsuite-clang12-perf:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - run: sudo apt install clang-12
+      - run: make malloc_cmp_test
   testsuite-gcc:
     runs-on: ubuntu-latest
     steps:

PERFORMANCE.md

@@ -14,6 +14,8 @@ All data fetches from a zone bitmap are 64 bits at a time which takes advantage
 
 All bitmaps pages allocated with `mmap` are passed to the `madvise` syscall with the advice arguments `MADV_WILLNEED`. All user pages allocated with `mmap` are passed to the `madvise` syscall with the advice arguments `MADV_WILLNEED`. Global caches, the root, and zone bitmaps (but not pages that hold user data) are created with `MAP_POPULATE` which instructs the kernel to pre-populate the page tables which reduces page faults and results in better performance. You can disable this with the `PRE_POPULATE_PAGES` Makefile flag. Note that by default at zone creation time user pages will have canaries written at random aligned offsets. This will cause page faults and populate those PTE's when the pages are first written to whether those pages are ever used at runtime or not.
 
+The `MAX_ZONES` value in `conf.h` limits the total number of zones that can be allocated at runtime. If your program is being killed with OOM errors you can safely increase this value, however its max value is 65535. However it will result in a larger allocation for the `root->zones` array which holds meta data for each zone whether that zone is currently mapped and in use or not. To calculate the total number of bytes available for allocations you can do (`MAX_ZONES * ZONE_USER_SIZE`). Note that `ZONE_USER_SIZE` is not configurable in `conf.h`.
+
 Default zones for common sizes are created in the library constructor. This helps speed up allocations for long running programs. New zones are created on demand when needed but this will incur a small performance penalty in the allocation path.
 
 By default user chunks are not sanitized upon free. While this helps mitigate uninitialized memory vulnerabilities it is a very slow operation. You can enable this feature by changing the `SANITIZE_CHUNKS` flag in the Makefile.

README.md

@@ -153,7 +153,7 @@ If all else fails please file an issue on the [github project](https://github.co
 
 `void iso_free_permanently(void *p)` - Same as `iso_free` but marks the chunk in such a way that it will not be reallocated
 
-`void iso_free_from_zone(void *p, iso_alloc_zone_handle *zone)` - Free's a chunk from a private zone. Can take a tagged or untagged pointer if `MEMORY_TAGGING` is enabled.
+`void iso_free_from_zone(void *p, iso_alloc_zone_handle *zone)` - Free's a chunk from a private zone. Can take a tagged or untagged pointer if `MEMORY_TAGGING` is enabled. These chunks are not quarantined.
 
 `void iso_free_from_zone_permanently(void *p, iso_alloc_zone_handle *zone)` - Permanently free's a chunk from a zone
 

include/conf.h

@@ -21,7 +21,8 @@
 #define CANARY_COUNT_DIV 7
 
 /* If you're compiling for Android and want custom names
- * for internal mappings you can modify those here */
+ * for internal mappings that are viewable from procfs
+ * (i.e. /proc/pid/maps) you can modify those names here */
 #if NAMED_MAPPINGS
 #define SAMPLED_ALLOC_NAME "isoalloc sampled allocation"
 #define BIG_ZONE_UD_NAME "isoalloc big zone user data"
@@ -72,7 +73,8 @@
  * it does correspond to the size of the _root.zones
  * array. Currently the iso_alloc_zone_t structure is
  * roughly 2112 bytes so this results in 17301504 bytes
- * (~17 MB) for zone meta data */
+ * (~17 MB) for zone meta data. See PERFORMANCE.md for
+ * more information on this value. Max is 65335 */
 #define MAX_ZONES 8192
 
 /* We allocate zones at startup for common sizes.
@@ -94,7 +96,7 @@
  * certain decisions based on this value such as the
  * number of canary values in a zone. It is safe to
  * modify to a larger value but you will likely be
- * wasting memory by doing so */
+ * wasting memory by doing so. */
 #define MAX_DEFAULT_ZONE_SZ ZONE_8192
 
 /* If you have specific allocation pattern requirements

include/iso_alloc_internal.h

@@ -521,7 +521,7 @@ INTERNAL_HIDDEN INLINE void _flush_chunk_quarantine(void);
 INTERNAL_HIDDEN INLINE void clear_chunk_quarantine(void);
 INTERNAL_HIDDEN INLINE void clear_zone_cache(void);
 INTERNAL_HIDDEN iso_alloc_zone_t *is_zone_usable(iso_alloc_zone_t *zone, size_t size);
-INTERNAL_HIDDEN iso_alloc_zone_t *iso_find_zone_fit(size_t size);
+INTERNAL_HIDDEN iso_alloc_zone_t *find_suitable_zone(size_t size);
 INTERNAL_HIDDEN iso_alloc_zone_t *iso_new_zone(size_t size, bool internal);
 INTERNAL_HIDDEN iso_alloc_zone_t *_iso_new_zone(size_t size, bool internal, int32_t index);
 INTERNAL_HIDDEN iso_alloc_zone_t *iso_find_zone_bitmap_range(const void *p);
@@ -532,7 +532,6 @@ INTERNAL_HIDDEN bit_slot_t iso_scan_zone_free_slot(iso_alloc_zone_t *zone);
 INTERNAL_HIDDEN bit_slot_t get_next_free_bit_slot(iso_alloc_zone_t *zone);
 INTERNAL_HIDDEN iso_alloc_root *iso_alloc_new_root(void);
 INTERNAL_HIDDEN bool is_pow2(uint64_t sz);
-INTERNAL_HIDDEN bool iso_does_zone_fit(iso_alloc_zone_t *zone, size_t size);
 INTERNAL_HIDDEN bool _is_zone_retired(iso_alloc_zone_t *zone);
 INTERNAL_HIDDEN bool _refresh_zone_mem_tags(iso_alloc_zone_t *zone);
 INTERNAL_HIDDEN iso_alloc_zone_t *_iso_free_internal_unlocked(void *p, bool permanent, iso_alloc_zone_t *zone);

src/iso_alloc.c

@@ -380,6 +380,7 @@ INTERNAL_HIDDEN void iso_alloc_initialize_global_root(void) {
 
     _root->zones = (void *) (p + g_page_size);
     name_mapping(p, _root->zones_size, "isoalloc zone metadata");
+    MLOCK(_root->zones, _root->zones_size);
 
 #if !THREAD_SUPPORT
     size_t c = ROUND_UP_PAGE(CHUNK_QUARANTINE_SZ * sizeof(uintptr_t));
@@ -653,7 +654,7 @@ INTERNAL_HIDDEN iso_alloc_zone_t *iso_new_zone(size_t size, bool internal) {
 /* Requires the root is locked */
 INTERNAL_HIDDEN iso_alloc_zone_t *_iso_new_zone(size_t size, bool internal, int32_t index) {
     if(UNLIKELY(_root->zones_used >= MAX_ZONES) || UNLIKELY(index >= MAX_ZONES)) {
-        LOG_AND_ABORT("Cannot allocate additional zones. I have already allocated %d", _root->zones_used);
+        LOG_AND_ABORT("Cannot allocate additional zones. I have already allocated %d zones", _root->zones_used);
     }
 
     /* In order for our bitmap to be a power of 2
@@ -901,6 +902,12 @@ INTERNAL_HIDDEN bit_slot_t iso_scan_zone_free_slot_slow(iso_alloc_zone_t *zone)
 }
 
 INTERNAL_HIDDEN iso_alloc_zone_t *is_zone_usable(iso_alloc_zone_t *zone, size_t size) {
+#if CPU_PIN
+    if(zone->cpu_core != sched_getcpu()) {
+        return false;
+    }
+#endif
+
     /* If the zone is full it is not usable */
     if(zone->is_full == true) {
         return NULL;
@@ -913,7 +920,7 @@ INTERNAL_HIDDEN iso_alloc_zone_t *is_zone_usable(iso_alloc_zone_t *zone, size_t
      * sizes beyond ZONE_1024 bytes. In other words we can
      * live with some wasted space in zones that manage
      * chunks smaller than ZONE_1024 */
-    if(zone->internal == true && size > ZONE_1024 && zone->chunk_size >= (size << WASTED_SZ_MULTIPLIER_SHIFT)) {
+    if(size > ZONE_1024 && zone->chunk_size >= (size << WASTED_SZ_MULTIPLIER_SHIFT)) {
         return NULL;
     }
 
@@ -962,38 +969,8 @@ INTERNAL_HIDDEN iso_alloc_zone_t *is_zone_usable(iso_alloc_zone_t *zone, size_t
     }
 }
 
-/* Implements the check for iso_find_zone_fit */
-INTERNAL_HIDDEN bool iso_does_zone_fit(iso_alloc_zone_t *zone, size_t size) {
-#if CPU_PIN
-    if(zone->cpu_core != sched_getcpu()) {
-        return false;
-    }
-#endif
-
-    /* Don't return a zone that handles a size far larger
-     * than we need. This could lead to high memory usage
-     * depending on allocation patterns but helps enforce
-     * spatial separation based on sized */
-    if(zone->chunk_size >= ZONE_1024 && size <= ZONE_128) {
-        return false;
-    }
-
-    if(zone->chunk_size < size || zone->internal == false || zone->is_full == true) {
-        return false;
-    }
-
-    /* We found a zone, lets try to find a free slot in it */
-    zone = is_zone_usable(zone, size);
-
-    if(zone == NULL) {
-        return false;
-    } else {
-        return true;
-    }
-}
-
 /* Finds a zone that can fit this allocation request */
-INTERNAL_HIDDEN iso_alloc_zone_t *iso_find_zone_fit(size_t size) {
+INTERNAL_HIDDEN iso_alloc_zone_t *find_suitable_zone(size_t size) {
     iso_alloc_zone_t *zone = NULL;
     int32_t i = 0;
 
@@ -1016,7 +993,7 @@ INTERNAL_HIDDEN iso_alloc_zone_t *iso_find_zone_fit(size_t size) {
                 LOG_AND_ABORT("Lookup table should never contain private zones");
             }
 
-            if(iso_does_zone_fit(zone, size) == true) {
+            if(is_zone_usable(zone, size) != NULL) {
                 return zone;
             }
 
@@ -1052,7 +1029,16 @@ INTERNAL_HIDDEN iso_alloc_zone_t *iso_find_zone_fit(size_t size) {
     for(; i < _root->zones_used; i++) {
         zone = &_root->zones[i];
 
-        if(iso_does_zone_fit(zone, size) == true) {
+        if(zone->chunk_size < size || zone->internal == false) {
+            continue;
+        }
+
+        /* Don't waste memory, enforce spatial separation by size */
+        if(zone->chunk_size >= ZONE_1024 && size <= ZONE_128) {
+            continue;
+        }
+
+        if(is_zone_usable(zone, size) != NULL) {
             return zone;
         }
     }
@@ -1366,8 +1352,15 @@ INTERNAL_HIDDEN ASSUME_ALIGNED void *_iso_alloc(iso_alloc_zone_t *zone, size_t s
              * and this will speed up that operation */
             for(size_t i = 0; i < zone_cache_count; i++) {
                 if(zone_cache[i].chunk_size >= size) {
-                    if(iso_does_zone_fit(zone_cache[i].zone, size) == true) {
-                        zone = zone_cache[i].zone;
+                    iso_alloc_zone_t *_zone = zone_cache[i].zone;
+
+                    /* Don't waste memory, enforce spatial separation by size */
+                    if(_zone->chunk_size >= ZONE_1024 && size <= ZONE_128) {
+                        continue;
+                    }
+
+                    if(is_zone_usable(_zone, size) != NULL) {
+                        zone = _zone;
                         break;
                     }
                 }
@@ -1380,13 +1373,13 @@ INTERNAL_HIDDEN ASSUME_ALIGNED void *_iso_alloc(iso_alloc_zone_t *zone, size_t s
          * looking for a suitable one, this includes the
          * zones we cached above */
         if(zone == NULL) {
-            zone = iso_find_zone_fit(size);
+            zone = find_suitable_zone(size);
         }
 
-        /* We only need to check if the zone is usable
-         * if it's a private zone. If we chose this zone
-         * then its guaranteed to already be usable */
         if(LIKELY(zone != NULL)) {
+            /* We only need to check if the zone is usable
+             * if it's a private zone. If we chose this zone
+             * then its guaranteed to already be usable */
             if(zone->internal == false) {
                 zone = is_zone_usable(zone, size);
 
@@ -1919,7 +1912,7 @@ INTERNAL_HIDDEN void _iso_free_size(void *p, size_t size) {
     iso_alloc_zone_t *zone = iso_find_zone_range(p);
 
     if(UNLIKELY(zone == NULL)) {
-        LOG_AND_ABORT("Could not find zone for %p", p);
+        LOG_AND_ABORT("Could not find zone for 0x%p", p);
     }
 
     /* We can't check for an exact size match because
@@ -2001,16 +1994,14 @@ INTERNAL_HIDDEN iso_alloc_zone_t *_iso_free_internal_unlocked(void *p, bool perm
         if(zone->tagged == true) {
             if(_refresh_zone_mem_tags(zone) == false) {
                 /* We only need to refresh this single tag */
-                if(zone->tagged == true) {
-                    void *user_pages_start = UNMASK_USER_PTR(zone);
-                    uint8_t *_mtp = (user_pages_start - _root->system_page_size - ROUND_UP_PAGE(zone->chunk_count * MEM_TAG_SIZE));
-                    uint64_t chunk_offset = (uint64_t) (p - user_pages_start);
-                    _mtp += (chunk_offset >> zone->chunk_size_pow2);
-
-                    /* Generate and write a new tag for this chunk */
-                    uint8_t mem_tag = (uint8_t) rand_uint64();
-                    *_mtp = mem_tag;
-                }
+                void *user_pages_start = UNMASK_USER_PTR(zone);
+                uint8_t *_mtp = (user_pages_start - _root->system_page_size - ROUND_UP_PAGE(zone->chunk_count * MEM_TAG_SIZE));
+                uint64_t chunk_offset = (uint64_t) (p - user_pages_start);
+                _mtp += (chunk_offset >> zone->chunk_size_pow2);
+
+                /* Generate and write a new tag for this chunk */
+                uint8_t mem_tag = (uint8_t) rand_uint64();
+                *_mtp = mem_tag;
             }
         }
 #endif