From 80b1aaab0946de7f0b6cc7421ac60bb8f90360ea Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Tue, 24 Sep 2019 20:47:24 -0400 Subject: [PATCH 1/9] sys_heap: some cleanups to make the code clearer First, some renames to make accessors more explicit: size() --> chunk_size() used() --> chunk_used() free_prev() --> prev_free_chunk() free_next() --> next_free_chunk() Then, the return type of chunk_size() is changed from chunkid_t to size_t, and chunk_used() from chunkid_t to bool. The left_size() accessor is used only once and can be easily substituted by left_chunk(), so it is removed. And in free_list_add() the variable b is renamed to bi so to be consistent with usage in sys_heap_alloc(). Signed-off-by: Nicolas Pitre --- lib/os/heap-validate.c | 36 ++++++++++++++++---------------- lib/os/heap.c | 47 +++++++++++++++++++++--------------------- lib/os/heap.h | 19 +++++++---------- 3 files changed, 49 insertions(+), 53 deletions(-) diff --git a/lib/os/heap-validate.c b/lib/os/heap-validate.c index 486fdf76a2aa9..9934d622a539a 100644 --- a/lib/os/heap-validate.c +++ b/lib/os/heap-validate.c @@ -26,17 +26,17 @@ static bool in_bounds(struct z_heap *h, chunkid_t c) { return (c >= h->chunk0) && (c <= max_chunkid(h)) - && (size(h, c) < h->len); + && (chunk_size(h, c) < h->len); } static bool valid_chunk(struct z_heap *h, chunkid_t c) { - return (size(h, c) > 0 - && (c + size(h, c) <= h->len) + return (chunk_size(h, c) > 0 + && (c + chunk_size(h, c) <= h->len) && in_bounds(h, c) - && ((c == h->chunk0) || in_bounds(h, c - left_size(h, c))) - && (used(h, c) || in_bounds(h, free_prev(h, c))) - && (used(h, c) || in_bounds(h, free_next(h, c)))); + && ((c == h->chunk0) || in_bounds(h, left_chunk(h, c))) + && (chunk_used(h, c) || in_bounds(h, prev_free_chunk(h, c))) + && (chunk_used(h, c) || in_bounds(h, next_free_chunk(h, c)))); } /* Validate multiple state dimensions for the bucket "next" pointer @@ -60,11 +60,11 @@ static inline void check_nexts(struct z_heap *h, int bidx) } if (b->list_size == 2) { - CHECK(free_next(h, b->next) == free_prev(h, b->next)); - CHECK(free_next(h, b->next) != b->next); + CHECK(next_free_chunk(h, b->next) == prev_free_chunk(h, b->next)); + CHECK(next_free_chunk(h, b->next) != b->next); } else if (b->list_size == 1) { - CHECK(free_next(h, b->next) == free_prev(h, b->next)); - CHECK(free_next(h, b->next) == b->next); + CHECK(next_free_chunk(h, b->next) == prev_free_chunk(h, b->next)); + CHECK(next_free_chunk(h, b->next) == b->next); } } @@ -84,7 +84,7 @@ bool sys_heap_validate(struct sys_heap *heap) check_nexts(h, b); for (c = c0; c != 0 && (n == 0 || c != c0); - n++, c = free_next(h, c)) { + n++, c = next_free_chunk(h, c)) { if (!valid_chunk(h, c)) { return false; } @@ -112,22 +112,22 @@ bool sys_heap_validate(struct sys_heap *heap) * blocks were found during enumeration). Mark all blocks * UNUSED */ - size_t prev_size = 0; + chunkid_t prev_chunk = 0; for (c = h->chunk0; c <= max_chunkid(h); c = right_chunk(h, c)) { if (!valid_chunk(h, c)) { return false; } - if (!used(h, c)) { + if (!chunk_used(h, c)) { return false; } if (c != h->chunk0) { - if (left_size(h, c) != prev_size) { + if (left_chunk(h, c) != prev_chunk) { return false; } } - prev_size = size(h, c); + prev_chunk = c; chunk_set_used(h, c, false); } @@ -147,8 +147,8 @@ bool sys_heap_validate(struct sys_heap *heap) continue; } - for (c = c0; n == 0 || c != c0; n++, c = free_next(h, c)) { - if (used(h, c)) { + for (c = c0; n == 0 || c != c0; n++, c = next_free_chunk(h, c)) { + if (chunk_used(h, c)) { return false; } chunk_set_used(h, c, true); @@ -159,7 +159,7 @@ bool sys_heap_validate(struct sys_heap *heap) * fields. One more linear pass to fix them up */ for (c = h->chunk0; c <= max_chunkid(h); c = right_chunk(h, c)) { - chunk_set_used(h, c, !used(h, c)); + chunk_set_used(h, c, !chunk_used(h, c)); } return true; } diff --git a/lib/os/heap.c b/lib/os/heap.c index f14f1720d61c2..ae998cac93e6f 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -21,7 +21,7 @@ static void free_list_remove(struct z_heap *h, int bidx, { struct z_heap_bucket *b = &h->buckets[bidx]; - CHECK(!used(h, c)); + CHECK(!chunk_used(h, c)); CHECK(b->next != 0); CHECK(b->list_size > 0); CHECK((((h->avail_buckets & (1 << bidx)) == 0) @@ -33,7 +33,8 @@ static void free_list_remove(struct z_heap *h, int bidx, h->avail_buckets &= ~(1 << bidx); b->next = 0; } else { - chunkid_t first = free_prev(h, c), second = free_next(h, c); + chunkid_t first = prev_free_chunk(h, c), + second = next_free_chunk(h, c); b->next = second; chunk_set(h, first, FREE_NEXT, second); @@ -43,21 +44,21 @@ static void free_list_remove(struct z_heap *h, int bidx, static void free_list_add(struct z_heap *h, chunkid_t c) { - int b = bucket_idx(h, size(h, c)); + int bi = bucket_idx(h, chunk_size(h, c)); - if (h->buckets[b].list_size++ == 0) { - CHECK(h->buckets[b].next == 0); - CHECK((h->avail_buckets & (1 << b)) == 0); + if (h->buckets[bi].list_size++ == 0) { + CHECK(h->buckets[bi].next == 0); + CHECK((h->avail_buckets & (1 << bi)) == 0); /* Empty list, first item */ - h->avail_buckets |= (1 << b); - h->buckets[b].next = c; + h->avail_buckets |= (1 << bi); + h->buckets[bi].next = c; chunk_set(h, c, FREE_PREV, c); chunk_set(h, c, FREE_NEXT, c); } else { /* Insert before (!) the "next" pointer */ - chunkid_t second = h->buckets[b].next; - chunkid_t first = free_prev(h, second); + chunkid_t second = h->buckets[bi].next; + chunkid_t first = prev_free_chunk(h, second); chunk_set(h, c, FREE_PREV, first); chunk_set(h, c, FREE_NEXT, second); @@ -65,12 +66,12 @@ static void free_list_add(struct z_heap *h, chunkid_t c) chunk_set(h, second, FREE_PREV, c); } - CHECK(h->avail_buckets & (1 << bucket_idx(h, size(h, c)))); + CHECK(h->avail_buckets & (1 << bucket_idx(h, chunk_size(h, c)))); } static ALWAYS_INLINE bool last_chunk(struct z_heap *h, chunkid_t c) { - return (c + size(h, c)) == h->len; + return (c + chunk_size(h, c)) == h->len; } /* Allocates (fit check has already been perfomred) from the next @@ -79,14 +80,14 @@ static ALWAYS_INLINE bool last_chunk(struct z_heap *h, chunkid_t c) static void *split_alloc(struct z_heap *h, int bidx, size_t sz) { CHECK(h->buckets[bidx].next != 0 - && sz <= size(h, h->buckets[bidx].next)); + && sz <= chunk_size(h, h->buckets[bidx].next)); chunkid_t c = h->buckets[bidx].next; free_list_remove(h, bidx, c); /* Split off remainder if it's usefully large */ - size_t rem = size(h, c) - sz; + size_t rem = chunk_size(h, c) - sz; CHECK(rem < h->len); @@ -119,11 +120,11 @@ void sys_heap_free(struct sys_heap *heap, void *mem) - (uint8_t *)h->buf) / CHUNK_UNIT; /* Merge with right chunk? We can just absorb it. */ - if (!last_chunk(h, c) && !used(h, right_chunk(h, c))) { + if (!last_chunk(h, c) && !chunk_used(h, right_chunk(h, c))) { chunkid_t rc = right_chunk(h, c); - size_t newsz = size(h, c) + size(h, rc); + size_t newsz = chunk_size(h, c) + chunk_size(h, rc); - free_list_remove(h, bucket_idx(h, size(h, rc)), rc); + free_list_remove(h, bucket_idx(h, chunk_size(h, rc)), rc); chunk_set(h, c, SIZE_AND_USED, newsz); if (!last_chunk(h, c)) { chunk_set(h, right_chunk(h, c), LEFT_SIZE, newsz); @@ -131,13 +132,13 @@ void sys_heap_free(struct sys_heap *heap, void *mem) } /* Merge with left chunk? It absorbs us. */ - if (c != h->chunk0 && !used(h, left_chunk(h, c))) { + if (c != h->chunk0 && !chunk_used(h, left_chunk(h, c))) { chunkid_t lc = left_chunk(h, c); chunkid_t rc = right_chunk(h, c); - size_t csz = size(h, c); - size_t merged_sz = csz + size(h, lc); + size_t csz = chunk_size(h, c); + size_t merged_sz = csz + chunk_size(h, lc); - free_list_remove(h, bucket_idx(h, size(h, lc)), lc); + free_list_remove(h, bucket_idx(h, chunk_size(h, lc)), lc); chunk_set(h, lc, SIZE_AND_USED, merged_sz); if (!last_chunk(h, lc)) { chunk_set(h, rc, LEFT_SIZE, merged_sz); @@ -179,10 +180,10 @@ void *sys_heap_alloc(struct sys_heap *heap, size_t bytes) for (int i = 0; i < loops; i++) { CHECK(b->next != 0); - if (size(h, b->next) >= sz) { + if (chunk_size(h, b->next) >= sz) { return split_alloc(h, bi, sz); } - b->next = free_next(h, b->next); + b->next = next_free_chunk(h, b->next); } /* Otherwise pick the smallest non-empty bucket guaranteed to diff --git a/lib/os/heap.h b/lib/os/heap.h index 6e2ca0989abf0..4f2bbac1aaf57 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -98,12 +98,12 @@ static inline void chunk_set(struct z_heap *h, chunkid_t c, } } -static inline chunkid_t used(struct z_heap *h, chunkid_t c) +static inline bool chunk_used(struct z_heap *h, chunkid_t c) { return (chunk_field(h, c, SIZE_AND_USED) & ~h->size_mask) != 0; } -static ALWAYS_INLINE chunkid_t size(struct z_heap *h, chunkid_t c) +static ALWAYS_INLINE size_t chunk_size(struct z_heap *h, chunkid_t c) { return chunk_field(h, c, SIZE_AND_USED) & h->size_mask; } @@ -112,32 +112,27 @@ static inline void chunk_set_used(struct z_heap *h, chunkid_t c, bool used) { chunk_set(h, c, SIZE_AND_USED, - size(h, c) | (used ? (h->size_mask + 1) : 0)); + chunk_size(h, c) | (used ? (h->size_mask + 1) : 0)); } -static inline chunkid_t left_size(struct z_heap *h, chunkid_t c) -{ - return chunk_field(h, c, LEFT_SIZE); -} - -static inline chunkid_t free_prev(struct z_heap *h, chunkid_t c) +static inline chunkid_t prev_free_chunk(struct z_heap *h, chunkid_t c) { return chunk_field(h, c, FREE_PREV); } -static inline chunkid_t free_next(struct z_heap *h, chunkid_t c) +static inline chunkid_t next_free_chunk(struct z_heap *h, chunkid_t c) { return chunk_field(h, c, FREE_NEXT); } static inline chunkid_t left_chunk(struct z_heap *h, chunkid_t c) { - return c - left_size(h, c); + return c - chunk_field(h, c, LEFT_SIZE); } static inline chunkid_t right_chunk(struct z_heap *h, chunkid_t c) { - return c + size(h, c); + return c + chunk_size(h, c); } static inline size_t chunk_header_bytes(struct z_heap *h) From a36105bb7d3ec5772ede7c612d829189cbd6ccd0 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Tue, 24 Sep 2019 23:20:53 -0400 Subject: [PATCH 2/9] sys_heap: provide more chunk_fields accessors Let's provide accessors for getting and setting every field to make the chunk header layout abstracted away from the main code. Those are: SIZE_AND_USED: chunk_used(), chunk_size(), set_chunk_used() and chunk_size(). LEFT_SIZE: left_chunk() and set_left_chunk_size(). FREE_PREV: prev_free_chunk() and set_prev_free_chunk(). FREE_NEXT: next_free_chunk() and set_next_free_chunk(). To be consistent, the former chunk_set_used() is now set_chunk_used(). Signed-off-by: Nicolas Pitre --- lib/os/heap-validate.c | 8 ++++---- lib/os/heap.c | 38 +++++++++++++++++++------------------- lib/os/heap.h | 27 +++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/lib/os/heap-validate.c b/lib/os/heap-validate.c index 9934d622a539a..9fde502808818 100644 --- a/lib/os/heap-validate.c +++ b/lib/os/heap-validate.c @@ -88,7 +88,7 @@ bool sys_heap_validate(struct sys_heap *heap) if (!valid_chunk(h, c)) { return false; } - chunk_set_used(h, c, true); + set_chunk_used(h, c, true); } bool empty = (h->avail_buckets & (1 << b)) == 0; @@ -129,7 +129,7 @@ bool sys_heap_validate(struct sys_heap *heap) } prev_chunk = c; - chunk_set_used(h, c, false); + set_chunk_used(h, c, false); } if (c != h->len) { return false; /* Should have exactly consumed the buffer */ @@ -151,7 +151,7 @@ bool sys_heap_validate(struct sys_heap *heap) if (chunk_used(h, c)) { return false; } - chunk_set_used(h, c, true); + set_chunk_used(h, c, true); } } @@ -159,7 +159,7 @@ bool sys_heap_validate(struct sys_heap *heap) * fields. One more linear pass to fix them up */ for (c = h->chunk0; c <= max_chunkid(h); c = right_chunk(h, c)) { - chunk_set_used(h, c, !chunk_used(h, c)); + set_chunk_used(h, c, !chunk_used(h, c)); } return true; } diff --git a/lib/os/heap.c b/lib/os/heap.c index ae998cac93e6f..e7c524d382813 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -37,8 +37,8 @@ static void free_list_remove(struct z_heap *h, int bidx, second = next_free_chunk(h, c); b->next = second; - chunk_set(h, first, FREE_NEXT, second); - chunk_set(h, second, FREE_PREV, first); + set_next_free_chunk(h, first, second); + set_prev_free_chunk(h, second, first); } } @@ -53,17 +53,17 @@ static void free_list_add(struct z_heap *h, chunkid_t c) /* Empty list, first item */ h->avail_buckets |= (1 << bi); h->buckets[bi].next = c; - chunk_set(h, c, FREE_PREV, c); - chunk_set(h, c, FREE_NEXT, c); + set_prev_free_chunk(h, c, c); + set_next_free_chunk(h, c, c); } else { /* Insert before (!) the "next" pointer */ chunkid_t second = h->buckets[bi].next; chunkid_t first = prev_free_chunk(h, second); - chunk_set(h, c, FREE_PREV, first); - chunk_set(h, c, FREE_NEXT, second); - chunk_set(h, first, FREE_NEXT, c); - chunk_set(h, second, FREE_PREV, c); + set_prev_free_chunk(h, c, first); + set_next_free_chunk(h, c, second); + set_next_free_chunk(h, first, c); + set_prev_free_chunk(h, second, c); } CHECK(h->avail_buckets & (1 << bucket_idx(h, chunk_size(h, c)))); @@ -95,16 +95,16 @@ static void *split_alloc(struct z_heap *h, int bidx, size_t sz) chunkid_t c2 = c + sz; chunkid_t c3 = right_chunk(h, c); - chunk_set(h, c, SIZE_AND_USED, sz); - chunk_set(h, c2, SIZE_AND_USED, rem); - chunk_set(h, c2, LEFT_SIZE, sz); + set_chunk_size(h, c, sz); + set_chunk_size(h, c2, rem); + set_left_chunk_size(h, c2, sz); if (!last_chunk(h, c2)) { - chunk_set(h, c3, LEFT_SIZE, rem); + set_left_chunk_size(h, c3, rem); } free_list_add(h, c2); } - chunk_set_used(h, c, true); + set_chunk_used(h, c, true); return chunk_mem(h, c); } @@ -125,9 +125,9 @@ void sys_heap_free(struct sys_heap *heap, void *mem) size_t newsz = chunk_size(h, c) + chunk_size(h, rc); free_list_remove(h, bucket_idx(h, chunk_size(h, rc)), rc); - chunk_set(h, c, SIZE_AND_USED, newsz); + set_chunk_size(h, c, newsz); if (!last_chunk(h, c)) { - chunk_set(h, right_chunk(h, c), LEFT_SIZE, newsz); + set_left_chunk_size(h, right_chunk(h, c), newsz); } } @@ -139,15 +139,15 @@ void sys_heap_free(struct sys_heap *heap, void *mem) size_t merged_sz = csz + chunk_size(h, lc); free_list_remove(h, bucket_idx(h, chunk_size(h, lc)), lc); - chunk_set(h, lc, SIZE_AND_USED, merged_sz); + set_chunk_size(h, lc, merged_sz); if (!last_chunk(h, lc)) { - chunk_set(h, rc, LEFT_SIZE, merged_sz); + set_left_chunk_size(h, rc, merged_sz); } c = lc; } - chunk_set_used(h, c, false); + set_chunk_used(h, c, false); free_list_add(h, c); } @@ -234,6 +234,6 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) heap->heap->buckets[i].next = 0; } - chunk_set(h, h->chunk0, SIZE_AND_USED, buf_sz - h->chunk0); + set_chunk_size(h, h->chunk0, buf_sz - h->chunk0); free_list_add(h, h->chunk0); } diff --git a/lib/os/heap.h b/lib/os/heap.h index 4f2bbac1aaf57..39542e737e81a 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -108,13 +108,18 @@ static ALWAYS_INLINE size_t chunk_size(struct z_heap *h, chunkid_t c) return chunk_field(h, c, SIZE_AND_USED) & h->size_mask; } -static inline void chunk_set_used(struct z_heap *h, chunkid_t c, - bool used) +static inline void set_chunk_used(struct z_heap *h, chunkid_t c, bool used) { chunk_set(h, c, SIZE_AND_USED, chunk_size(h, c) | (used ? (h->size_mask + 1) : 0)); } +static inline void set_chunk_size(struct z_heap *h, chunkid_t c, size_t size) +{ + chunk_set(h, c, SIZE_AND_USED, + size | (chunk_used(h, c) ? (h->size_mask + 1) : 0)); +} + static inline chunkid_t prev_free_chunk(struct z_heap *h, chunkid_t c) { return chunk_field(h, c, FREE_PREV); @@ -125,6 +130,18 @@ static inline chunkid_t next_free_chunk(struct z_heap *h, chunkid_t c) return chunk_field(h, c, FREE_NEXT); } +static inline void set_prev_free_chunk(struct z_heap *h, chunkid_t c, + chunkid_t prev) +{ + chunk_set(h, c, FREE_PREV, prev); +} + +static inline void set_next_free_chunk(struct z_heap *h, chunkid_t c, + chunkid_t next) +{ + chunk_set(h, c, FREE_NEXT, next); +} + static inline chunkid_t left_chunk(struct z_heap *h, chunkid_t c) { return c - chunk_field(h, c, LEFT_SIZE); @@ -135,6 +152,12 @@ static inline chunkid_t right_chunk(struct z_heap *h, chunkid_t c) return c + chunk_size(h, c); } +static inline void set_left_chunk_size(struct z_heap *h, chunkid_t c, + size_t size) +{ + chunk_set(h, c, LEFT_SIZE, size); +} + static inline size_t chunk_header_bytes(struct z_heap *h) { return big_heap(h) ? 8 : 4; From 1dd33f62e20cdf6688c2cebc462e5cd7f7f6996e Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Wed, 25 Sep 2019 12:30:16 -0400 Subject: [PATCH 3/9] sys_heap: optimize usage of size and used flags By storing the used flag in the LSB, it is no longer necessary to have a size_mask variable to locate that flag. This produces smaller and faster code. Replace the validation check in chunk_set() to base it on the storage type. Also clarify the semantics of set_chunk_size() which allows for clearing the used flag bit unconditionally which simplifies the code further. The idea of moving the used flag bit into the LEFT_SIZE field was raised. It turns out that this isn't as beneficial as it may seem because the used bit is set only once i.e. when the memory is handed off to a user and the size field becomes frozen at that point. Modifications on the leftward chunk may still occur and extra instructions to preserve that bit would be necessary if it were moved there. Signed-off-by: Nicolas Pitre --- lib/os/heap.c | 1 - lib/os/heap.h | 42 +++++++++++++++++++++++++++++------------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/lib/os/heap.c b/lib/os/heap.c index e7c524d382813..4f83f254d697e 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -221,7 +221,6 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) h->buf = (uint64_t *)addr; h->buckets = (void *)(addr + CHUNK_UNIT * hdr_chunks); h->len = buf_sz; - h->size_mask = (1 << (big_heap(h) ? 31 : 15)) - 1; h->avail_buckets = 0; size_t buckets_bytes = ((bucket_idx(h, buf_sz) + 1) diff --git a/lib/os/heap.h b/lib/os/heap.h index 39542e737e81a..efff2efb2262c 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -34,7 +34,7 @@ * * The fields are: * SIZE_AND_USED: the total size (including header) of the chunk in - * 8-byte units. The top bit stores a "used" flag. + * 8-byte units. The bottom bit stores a "used" flag. * LEFT_SIZE: The size of the left (next lower chunk in memory) * neighbor chunk. * FREE_PREV: Chunk ID of the previous node in a free list. @@ -55,7 +55,6 @@ struct z_heap { uint64_t *buf; struct z_heap_bucket *buckets; uint32_t len; - uint32_t size_mask; uint32_t chunk0; uint32_t avail_buckets; }; @@ -86,38 +85,55 @@ static inline void chunk_set(struct z_heap *h, chunkid_t c, enum chunk_fields f, chunkid_t val) { CHECK(c >= h->chunk0 && c < h->len); - CHECK((val & ~((h->size_mask << 1) + 1)) == 0); - CHECK((val & h->size_mask) < h->len); void *cmem = &h->buf[c]; if (big_heap(h)) { - ((uint32_t *)cmem)[f] = (uint32_t) val; + CHECK(val == (uint32_t)val); + ((uint32_t *)cmem)[f] = val; } else { - ((uint16_t *)cmem)[f] = (uint16_t) val; + CHECK(val == (uint16_t)val); + ((uint16_t *)cmem)[f] = val; } } static inline bool chunk_used(struct z_heap *h, chunkid_t c) { - return (chunk_field(h, c, SIZE_AND_USED) & ~h->size_mask) != 0; + return chunk_field(h, c, SIZE_AND_USED) & 1; } -static ALWAYS_INLINE size_t chunk_size(struct z_heap *h, chunkid_t c) +static inline size_t chunk_size(struct z_heap *h, chunkid_t c) { - return chunk_field(h, c, SIZE_AND_USED) & h->size_mask; + return chunk_field(h, c, SIZE_AND_USED) >> 1; } static inline void set_chunk_used(struct z_heap *h, chunkid_t c, bool used) { - chunk_set(h, c, SIZE_AND_USED, - chunk_size(h, c) | (used ? (h->size_mask + 1) : 0)); + void *cmem = &h->buf[c]; + + if (big_heap(h)) { + if (used) { + ((uint32_t *)cmem)[SIZE_AND_USED] |= 1; + } else { + ((uint32_t *)cmem)[SIZE_AND_USED] &= ~1; + } + } else { + if (used) { + ((uint16_t *)cmem)[SIZE_AND_USED] |= 1; + } else { + ((uint16_t *)cmem)[SIZE_AND_USED] &= ~1; + } + } } +/* + * Note: no need to preserve the used bit here as the chunk is never in use + * when its size is modified, and potential set_chunk_used() is always + * invoked after set_chunk_size(). + */ static inline void set_chunk_size(struct z_heap *h, chunkid_t c, size_t size) { - chunk_set(h, c, SIZE_AND_USED, - size | (chunk_used(h, c) ? (h->size_mask + 1) : 0)); + chunk_set(h, c, SIZE_AND_USED, size << 1); } static inline chunkid_t prev_free_chunk(struct z_heap *h, chunkid_t c) From e51896a2cfd22eb8da2c3e39378f110808b009b4 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Wed, 25 Sep 2019 16:06:26 -0400 Subject: [PATCH 4/9] sys_heap: optimize struct z_heap It is possible to remove a few fields from struct z_heap, removing some runtime indirections by doing so: - The buf pointer is actually the same as the struct z_heap pointer itself. So let's simply create chunk_buf() that perform a type conversion. That type is also chunk_unit_t now rather than u64_t so it can be defined based on CHUNK_UNIT. - Replace the struct z_heap_bucket pointer by a zero-sized array at the end of struct z_heap. - Make chunk #0 into an actual chunk with its own header. This allows for removing the chunk0 field and streamlining the code. This way h->chunk0 becomes right_chunk(h, 0). This sets the table for further simplifications to come. Signed-off-by: Nicolas Pitre --- lib/os/heap-validate.c | 15 ++++++-------- lib/os/heap.c | 46 ++++++++++++++++++++++-------------------- lib/os/heap.h | 34 ++++++++++++++++++++----------- 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/lib/os/heap-validate.c b/lib/os/heap-validate.c index 9fde502808818..31e9bed6ed774 100644 --- a/lib/os/heap-validate.c +++ b/lib/os/heap-validate.c @@ -24,7 +24,7 @@ static size_t max_chunkid(struct z_heap *h) static bool in_bounds(struct z_heap *h, chunkid_t c) { - return (c >= h->chunk0) + return (c >= right_chunk(h, 0)) && (c <= max_chunkid(h)) && (chunk_size(h, c) < h->len); } @@ -34,7 +34,7 @@ static bool valid_chunk(struct z_heap *h, chunkid_t c) return (chunk_size(h, c) > 0 && (c + chunk_size(h, c) <= h->len) && in_bounds(h, c) - && ((c == h->chunk0) || in_bounds(h, left_chunk(h, c))) + && (!left_chunk(h, c) || in_bounds(h, left_chunk(h, c))) && (chunk_used(h, c) || in_bounds(h, prev_free_chunk(h, c))) && (chunk_used(h, c) || in_bounds(h, next_free_chunk(h, c)))); } @@ -114,18 +114,15 @@ bool sys_heap_validate(struct sys_heap *heap) */ chunkid_t prev_chunk = 0; - for (c = h->chunk0; c <= max_chunkid(h); c = right_chunk(h, c)) { + for (c = right_chunk(h, 0); c <= max_chunkid(h); c = right_chunk(h, c)) { if (!valid_chunk(h, c)) { return false; } if (!chunk_used(h, c)) { return false; } - - if (c != h->chunk0) { - if (left_chunk(h, c) != prev_chunk) { - return false; - } + if (left_chunk(h, c) != prev_chunk) { + return false; } prev_chunk = c; @@ -158,7 +155,7 @@ bool sys_heap_validate(struct sys_heap *heap) /* Now we are valid, but have managed to invert all the in-use * fields. One more linear pass to fix them up */ - for (c = h->chunk0; c <= max_chunkid(h); c = right_chunk(h, c)) { + for (c = right_chunk(h, 0); c <= max_chunkid(h); c = right_chunk(h, c)) { set_chunk_used(h, c, !chunk_used(h, c)); } return true; diff --git a/lib/os/heap.c b/lib/os/heap.c index 4f83f254d697e..20f9e8bf0ce4a 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -9,7 +9,8 @@ static void *chunk_mem(struct z_heap *h, chunkid_t c) { - uint8_t *ret = ((uint8_t *)&h->buf[c]) + chunk_header_bytes(h); + chunk_unit_t *buf = chunk_buf(h); + uint8_t *ret = ((uint8_t *)&buf[c]) + chunk_header_bytes(h); CHECK(!(((size_t)ret) & (big_heap(h) ? 7 : 3))); @@ -117,7 +118,7 @@ void sys_heap_free(struct sys_heap *heap, void *mem) struct z_heap *h = heap->heap; chunkid_t c = ((uint8_t *)mem - chunk_header_bytes(h) - - (uint8_t *)h->buf) / CHUNK_UNIT; + - (uint8_t *)chunk_buf(h)) / CHUNK_UNIT; /* Merge with right chunk? We can just absorb it. */ if (!last_chunk(h, c) && !chunk_used(h, right_chunk(h, c))) { @@ -132,7 +133,7 @@ void sys_heap_free(struct sys_heap *heap, void *mem) } /* Merge with left chunk? It absorbs us. */ - if (c != h->chunk0 && !chunk_used(h, left_chunk(h, c))) { + if (!chunk_used(h, left_chunk(h, c))) { chunkid_t lc = left_chunk(h, c); chunkid_t rc = right_chunk(h, c); size_t csz = chunk_size(h, c); @@ -202,37 +203,38 @@ void *sys_heap_alloc(struct sys_heap *heap, size_t bytes) void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) { - /* Must fit in a 32 bit count of u64's */ -#if __SIZEOF_SIZE_T__ > 4 - CHECK(bytes < 0x800000000ULL); -#endif + /* Must fit in a 32 bit count of HUNK_UNIT */ + CHECK(bytes / CHUNK_UNIT <= 0xffffffffU); /* Round the start up, the end down */ - size_t addr = ((size_t)mem + CHUNK_UNIT - 1) & ~(CHUNK_UNIT - 1); - size_t end = ((size_t)mem + bytes) & ~(CHUNK_UNIT - 1); + uintptr_t addr = ROUND_UP(mem, CHUNK_UNIT); + uintptr_t end = ROUND_DOWN((uint8_t *)mem + bytes, CHUNK_UNIT); size_t buf_sz = (end - addr) / CHUNK_UNIT; - size_t hdr_chunks = chunksz(sizeof(struct z_heap)); CHECK(end > addr); + CHECK(buf_sz > chunksz(sizeof(struct z_heap))); struct z_heap *h = (struct z_heap *)addr; - - heap->heap = (struct z_heap *)addr; - h->buf = (uint64_t *)addr; - h->buckets = (void *)(addr + CHUNK_UNIT * hdr_chunks); + heap->heap = h; + h->chunk0_hdr_area = 0; h->len = buf_sz; h->avail_buckets = 0; - size_t buckets_bytes = ((bucket_idx(h, buf_sz) + 1) - * sizeof(struct z_heap_bucket)); + int nb_buckets = bucket_idx(h, buf_sz) + 1; + size_t chunk0_size = chunksz(sizeof(struct z_heap) + + nb_buckets * sizeof(struct z_heap_bucket)); - h->chunk0 = hdr_chunks + chunksz(buckets_bytes); + CHECK(chunk0_size < buf_sz); - for (int i = 0; i <= bucket_idx(heap->heap, heap->heap->len); i++) { - heap->heap->buckets[i].list_size = 0; - heap->heap->buckets[i].next = 0; + for (int i = 0; i < nb_buckets; i++) { + h->buckets[i].list_size = 0; + h->buckets[i].next = 0; } - set_chunk_size(h, h->chunk0, buf_sz - h->chunk0); - free_list_add(h, h->chunk0); + set_chunk_size(h, 0, chunk0_size); + set_chunk_used(h, 0, true); + + set_chunk_size(h, chunk0_size, buf_sz - chunk0_size); + set_left_chunk_size(h, chunk0_size, chunk0_size); + free_list_add(h, chunk0_size); } diff --git a/lib/os/heap.h b/lib/os/heap.h index efff2efb2262c..a8a23a7c9bc7a 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -49,30 +49,38 @@ typedef size_t chunkid_t; #define CHUNK_UNIT 8 -enum chunk_fields { SIZE_AND_USED, LEFT_SIZE, FREE_PREV, FREE_NEXT }; +typedef struct { char bytes[CHUNK_UNIT]; } chunk_unit_t; -struct z_heap { - uint64_t *buf; - struct z_heap_bucket *buckets; - uint32_t len; - uint32_t chunk0; - uint32_t avail_buckets; -}; +enum chunk_fields { SIZE_AND_USED, LEFT_SIZE, FREE_PREV, FREE_NEXT }; struct z_heap_bucket { chunkid_t next; size_t list_size; }; +struct z_heap { + uint64_t chunk0_hdr_area; /* matches the largest header */ + uint32_t len; + uint32_t avail_buckets; + struct z_heap_bucket buckets[0]; +}; + static inline bool big_heap(struct z_heap *h) { return sizeof(size_t) > 4 || h->len > 0x7fff; } +static inline chunk_unit_t *chunk_buf(struct z_heap *h) +{ + /* the struct z_heap matches with the first chunk */ + return (chunk_unit_t *)h; +} + static inline size_t chunk_field(struct z_heap *h, chunkid_t c, enum chunk_fields f) { - void *cmem = &h->buf[c]; + chunk_unit_t *buf = chunk_buf(h); + void *cmem = &buf[c]; if (big_heap(h)) { return ((uint32_t *)cmem)[f]; @@ -84,9 +92,10 @@ static inline size_t chunk_field(struct z_heap *h, chunkid_t c, static inline void chunk_set(struct z_heap *h, chunkid_t c, enum chunk_fields f, chunkid_t val) { - CHECK(c >= h->chunk0 && c < h->len); + CHECK(c < h->len); - void *cmem = &h->buf[c]; + chunk_unit_t *buf = chunk_buf(h); + void *cmem = &buf[c]; if (big_heap(h)) { CHECK(val == (uint32_t)val); @@ -109,7 +118,8 @@ static inline size_t chunk_size(struct z_heap *h, chunkid_t c) static inline void set_chunk_used(struct z_heap *h, chunkid_t c, bool used) { - void *cmem = &h->buf[c]; + chunk_unit_t *buf = chunk_buf(h); + void *cmem = &buf[c]; if (big_heap(h)) { if (used) { From adfff607fcf742e4ce8e000dabda5e22ac6db6b1 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Wed, 25 Sep 2019 17:15:18 -0400 Subject: [PATCH 5/9] sys_heap: introduce min_chunk_size() With this we can remove magic constants, especially those used with big_heap(). Signed-off-by: Nicolas Pitre --- lib/os/heap-validate.c | 2 +- lib/os/heap.c | 4 ++-- lib/os/heap.h | 12 +++++++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/os/heap-validate.c b/lib/os/heap-validate.c index 31e9bed6ed774..9c124856cc502 100644 --- a/lib/os/heap-validate.c +++ b/lib/os/heap-validate.c @@ -19,7 +19,7 @@ static size_t max_chunkid(struct z_heap *h) { - return h->len - bytes_to_chunksz(h, 1); + return h->len - min_chunk_size(h); } static bool in_bounds(struct z_heap *h, chunkid_t c) diff --git a/lib/os/heap.c b/lib/os/heap.c index 20f9e8bf0ce4a..8028b42beb8c8 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -92,7 +92,7 @@ static void *split_alloc(struct z_heap *h, int bidx, size_t sz) CHECK(rem < h->len); - if (rem >= (big_heap(h) ? 2 : 1)) { + if (rem >= min_chunk_size(h)) { chunkid_t c2 = c + sz; chunkid_t c3 = right_chunk(h, c); @@ -224,7 +224,7 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) size_t chunk0_size = chunksz(sizeof(struct z_heap) + nb_buckets * sizeof(struct z_heap_bucket)); - CHECK(chunk0_size < buf_sz); + CHECK(chunk0_size + min_chunk_size(h) < buf_sz); for (int i = 0; i < nb_buckets; i++) { h->buckets[i].list_size = 0; diff --git a/lib/os/heap.h b/lib/os/heap.h index a8a23a7c9bc7a..2133df266ea7e 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -199,10 +199,16 @@ static inline size_t bytes_to_chunksz(struct z_heap *h, size_t bytes) return chunksz(chunk_header_bytes(h) + bytes); } -static int bucket_idx(struct z_heap *h, size_t sz) +static inline int min_chunk_size(struct z_heap *h) { - /* A chunk of size 2 is the minimum size on big heaps */ - return 31 - __builtin_clz(sz) - (big_heap(h) ? 1 : 0); + return bytes_to_chunksz(h, 1); } +static inline int bucket_idx(struct z_heap *h, size_t sz) +{ + size_t usable_sz = sz - min_chunk_size(h) + 1; + return 31 - __builtin_clz(usable_sz); +} + + #endif /* ZEPHYR_INCLUDE_LIB_OS_HEAP_H_ */ From 752c6371ed093932500cb7faed2ffbe74b8c960a Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Thu, 26 Sep 2019 01:59:35 -0400 Subject: [PATCH 6/9] sys_heap: remove need for last_chunk() We already have chunk #0 containing our struct z_heap and marked as used. We can add a partial chunk at the very end that is also marked as used. By doing so there is no longer a need for checking heap boundaries at run time when merging/splitting chunks meaning fewer conditionals in the code's hot path. Signed-off-by: Nicolas Pitre --- include/mempool_heap.h | 10 +++++----- lib/os/heap-validate.c | 3 ++- lib/os/heap.c | 31 ++++++++++++++++--------------- lib/os/heap.h | 19 +++++++++++++++++-- 4 files changed, 40 insertions(+), 23 deletions(-) diff --git a/include/mempool_heap.h b/include/mempool_heap.h index df64322b5014e..54b2323505ef6 100644 --- a/include/mempool_heap.h +++ b/include/mempool_heap.h @@ -36,17 +36,17 @@ struct k_mem_pool { * objects defined, and include extra so there's enough metadata space * available for the maximum number of minimum-sized objects to be * stored: 8 bytes for each desired chunk header, and a 24 word block - * to reserve room for a "typical" set of bucket list heads (this size - * was picked more to conform with existing test expectations than any - * rigorous theory -- we have tests that rely on being able to - * allocate the blocks promised and ones that make assumptions about + * to reserve room for a "typical" set of bucket list heads and one heap + * footer(this size was picked more to conform with existing test + * expectations than any rigorous theory -- we have tests that rely on being + * able to allocate the blocks promised and ones that make assumptions about * when memory will run out). */ #define Z_MEM_POOL_DEFINE(name, minsz, maxsz, nmax, align) \ K_HEAP_DEFINE(poolheap_##name, \ ((maxsz) * (nmax)) \ + 8 * ((maxsz) * (nmax) / (minsz)) \ - + 24 * sizeof(void *)); \ + + 25 * sizeof(void *)); \ struct k_mem_pool name = { \ .heap = &poolheap_##name \ } diff --git a/lib/os/heap-validate.c b/lib/os/heap-validate.c index 9c124856cc502..cf552e41fae70 100644 --- a/lib/os/heap-validate.c +++ b/lib/os/heap-validate.c @@ -34,7 +34,8 @@ static bool valid_chunk(struct z_heap *h, chunkid_t c) return (chunk_size(h, c) > 0 && (c + chunk_size(h, c) <= h->len) && in_bounds(h, c) - && (!left_chunk(h, c) || in_bounds(h, left_chunk(h, c))) + && (right_chunk(h, left_chunk(h, c)) == c) + && (left_chunk(h, right_chunk(h, c)) == c) && (chunk_used(h, c) || in_bounds(h, prev_free_chunk(h, c))) && (chunk_used(h, c) || in_bounds(h, next_free_chunk(h, c)))); } diff --git a/lib/os/heap.c b/lib/os/heap.c index 8028b42beb8c8..806ab2471a1eb 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -70,11 +70,6 @@ static void free_list_add(struct z_heap *h, chunkid_t c) CHECK(h->avail_buckets & (1 << bucket_idx(h, chunk_size(h, c)))); } -static ALWAYS_INLINE bool last_chunk(struct z_heap *h, chunkid_t c) -{ - return (c + chunk_size(h, c)) == h->len; -} - /* Allocates (fit check has already been perfomred) from the next * chunk at the specified bucket level */ @@ -99,9 +94,7 @@ static void *split_alloc(struct z_heap *h, int bidx, size_t sz) set_chunk_size(h, c, sz); set_chunk_size(h, c2, rem); set_left_chunk_size(h, c2, sz); - if (!last_chunk(h, c2)) { - set_left_chunk_size(h, c3, rem); - } + set_left_chunk_size(h, c3, rem); free_list_add(h, c2); } @@ -121,15 +114,13 @@ void sys_heap_free(struct sys_heap *heap, void *mem) - (uint8_t *)chunk_buf(h)) / CHUNK_UNIT; /* Merge with right chunk? We can just absorb it. */ - if (!last_chunk(h, c) && !chunk_used(h, right_chunk(h, c))) { + if (!chunk_used(h, right_chunk(h, c))) { chunkid_t rc = right_chunk(h, c); size_t newsz = chunk_size(h, c) + chunk_size(h, rc); free_list_remove(h, bucket_idx(h, chunk_size(h, rc)), rc); set_chunk_size(h, c, newsz); - if (!last_chunk(h, c)) { - set_left_chunk_size(h, right_chunk(h, c), newsz); - } + set_left_chunk_size(h, right_chunk(h, c), newsz); } /* Merge with left chunk? It absorbs us. */ @@ -141,9 +132,7 @@ void sys_heap_free(struct sys_heap *heap, void *mem) free_list_remove(h, bucket_idx(h, chunk_size(h, lc)), lc); set_chunk_size(h, lc, merged_sz); - if (!last_chunk(h, lc)) { - set_left_chunk_size(h, rc, merged_sz); - } + set_left_chunk_size(h, rc, merged_sz); c = lc; } @@ -206,6 +195,10 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) /* Must fit in a 32 bit count of HUNK_UNIT */ CHECK(bytes / CHUNK_UNIT <= 0xffffffffU); + /* Reserve the final marker chunk's header */ + CHECK(bytes > heap_footer_bytes(bytes)); + bytes -= heap_footer_bytes(bytes); + /* Round the start up, the end down */ uintptr_t addr = ROUND_UP(mem, CHUNK_UNIT); uintptr_t end = ROUND_DOWN((uint8_t *)mem + bytes, CHUNK_UNIT); @@ -231,10 +224,18 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) h->buckets[i].next = 0; } + /* chunk containing our struct z_heap */ set_chunk_size(h, 0, chunk0_size); set_chunk_used(h, 0, true); + /* chunk containing the free heap */ set_chunk_size(h, chunk0_size, buf_sz - chunk0_size); set_left_chunk_size(h, chunk0_size, chunk0_size); + + /* the end marker chunk */ + set_chunk_size(h, buf_sz, 0); + set_left_chunk_size(h, buf_sz, buf_sz - chunk0_size); + set_chunk_used(h, buf_sz, true); + free_list_add(h, chunk0_size); } diff --git a/lib/os/heap.h b/lib/os/heap.h index 2133df266ea7e..2aee40ef6f392 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -65,9 +65,19 @@ struct z_heap { struct z_heap_bucket buckets[0]; }; +static inline bool big_heap_chunks(size_t chunks) +{ + return sizeof(void *) > 4 || chunks > 0x7fff; +} + +static inline bool big_heap_bytes(size_t bytes) +{ + return big_heap_chunks(bytes / CHUNK_UNIT); +} + static inline bool big_heap(struct z_heap *h) { - return sizeof(size_t) > 4 || h->len > 0x7fff; + return big_heap_chunks(h->len); } static inline chunk_unit_t *chunk_buf(struct z_heap *h) @@ -92,7 +102,7 @@ static inline size_t chunk_field(struct z_heap *h, chunkid_t c, static inline void chunk_set(struct z_heap *h, chunkid_t c, enum chunk_fields f, chunkid_t val) { - CHECK(c < h->len); + CHECK(c <= h->len); chunk_unit_t *buf = chunk_buf(h); void *cmem = &buf[c]; @@ -189,6 +199,11 @@ static inline size_t chunk_header_bytes(struct z_heap *h) return big_heap(h) ? 8 : 4; } +static inline size_t heap_footer_bytes(size_t size) +{ + return big_heap_bytes(size) ? 8 : 4; +} + static inline size_t chunksz(size_t bytes) { return (bytes + CHUNK_UNIT - 1) / CHUNK_UNIT; From a6fbb6d95643e072a01b8d931708f32c68ad0efe Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Thu, 26 Sep 2019 15:22:24 -0400 Subject: [PATCH 7/9] sys_heap: simplify some complex checks Avoid redundancy and bucket_idx() usage when possible. Signed-off-by: Nicolas Pitre --- lib/os/heap.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/os/heap.c b/lib/os/heap.c index 806ab2471a1eb..dbd35eae63d27 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -25,8 +25,7 @@ static void free_list_remove(struct z_heap *h, int bidx, CHECK(!chunk_used(h, c)); CHECK(b->next != 0); CHECK(b->list_size > 0); - CHECK((((h->avail_buckets & (1 << bidx)) == 0) - == (h->buckets[bidx].next == 0))); + CHECK(h->avail_buckets & (1 << bidx)); b->list_size--; @@ -67,7 +66,7 @@ static void free_list_add(struct z_heap *h, chunkid_t c) set_prev_free_chunk(h, second, c); } - CHECK(h->avail_buckets & (1 << bucket_idx(h, chunk_size(h, c)))); + CHECK(h->avail_buckets & (1 << bi)); } /* Allocates (fit check has already been perfomred) from the next From 16ade11f64cfb6ac3073004b5bd79546001db067 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Thu, 26 Sep 2019 15:55:50 -0400 Subject: [PATCH 8/9] sys_heap: perform cheap overflow detection on freed memory Make the LEFT_SIZE field first and SIZE_AND_USED field last (for an allocated chunk) so they sit right next to the allocated memory. The current chunk's SIZE_AND_USED field points to the next (right) chunk, and from there the LEFT_SIZE field should point back to the current chunk. Many trivial memory overflows should trip that test. One way to make this test more robust could involve xor'ing the values within respective accessor pairs. But at least the fact that the size value is shifted by one bit already prevent fooling the test with a same-byte corruption. Signed-off-by: Nicolas Pitre --- lib/os/heap.c | 14 ++++++++++++++ lib/os/heap.h | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/os/heap.c b/lib/os/heap.c index dbd35eae63d27..4ac9061c33ac5 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -112,6 +112,20 @@ void sys_heap_free(struct sys_heap *heap, void *mem) chunkid_t c = ((uint8_t *)mem - chunk_header_bytes(h) - (uint8_t *)chunk_buf(h)) / CHUNK_UNIT; + /* + * This should catch many double-free cases. + * This is cheap enough so let's do it all the time. + */ + __ASSERT(chunk_used(h, c), + "unexpected heap state (double-free?) for memory at %p", mem); + /* + * It is easy to catch many common memory overflow cases with + * a quick check on this and next chunk header fields that are + * immediately before and after the freed memory. + */ + __ASSERT(left_chunk(h, right_chunk(h, c)) == c, + "corrupted heap bounds (buffer overflow?) for memory at %p", mem); + /* Merge with right chunk? We can just absorb it. */ if (!chunk_used(h, right_chunk(h, c))) { chunkid_t rc = right_chunk(h, c); diff --git a/lib/os/heap.h b/lib/os/heap.h index 2aee40ef6f392..d45bf043ec54d 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -51,7 +51,7 @@ typedef size_t chunkid_t; typedef struct { char bytes[CHUNK_UNIT]; } chunk_unit_t; -enum chunk_fields { SIZE_AND_USED, LEFT_SIZE, FREE_PREV, FREE_NEXT }; +enum chunk_fields { LEFT_SIZE, SIZE_AND_USED, FREE_PREV, FREE_NEXT }; struct z_heap_bucket { chunkid_t next; From 9b3d50ca5c5b6b68be10378526d9fbc18ef32f34 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Wed, 23 Oct 2019 19:43:16 -0400 Subject: [PATCH 9/9] sys_heap: reduce the size of struct z_heap_bucket by half This struct is taking up most of the heap's constant footprint overhead. We can easily get rid of the list_size member as it is mostly used to determine if the list is empty, and that can be determined through other means. Signed-off-by: Nicolas Pitre --- include/mempool_heap.h | 6 +++--- lib/os/heap-validate.c | 15 +-------------- lib/os/heap.c | 32 +++++++++++++++----------------- lib/os/heap.h | 1 - 4 files changed, 19 insertions(+), 35 deletions(-) diff --git a/include/mempool_heap.h b/include/mempool_heap.h index 54b2323505ef6..d4e268a75436c 100644 --- a/include/mempool_heap.h +++ b/include/mempool_heap.h @@ -35,8 +35,8 @@ struct k_mem_pool { * that k_heap does not. We make space for the number of maximum * objects defined, and include extra so there's enough metadata space * available for the maximum number of minimum-sized objects to be - * stored: 8 bytes for each desired chunk header, and a 24 word block - * to reserve room for a "typical" set of bucket list heads and one heap + * stored: 8 bytes for each desired chunk header, and a 15 word block + * to reserve room for a "typical" set of bucket list heads and the heap * footer(this size was picked more to conform with existing test * expectations than any rigorous theory -- we have tests that rely on being * able to allocate the blocks promised and ones that make assumptions about @@ -46,7 +46,7 @@ struct k_mem_pool { K_HEAP_DEFINE(poolheap_##name, \ ((maxsz) * (nmax)) \ + 8 * ((maxsz) * (nmax) / (minsz)) \ - + 25 * sizeof(void *)); \ + + 15 * sizeof(void *)); \ struct k_mem_pool name = { \ .heap = &poolheap_##name \ } diff --git a/lib/os/heap-validate.c b/lib/os/heap-validate.c index cf552e41fae70..79071ff27746c 100644 --- a/lib/os/heap-validate.c +++ b/lib/os/heap-validate.c @@ -50,8 +50,7 @@ static inline void check_nexts(struct z_heap *h, int bidx) bool emptybit = (h->avail_buckets & (1 << bidx)) == 0; bool emptylist = b->next == 0; - bool emptycount = b->list_size == 0; - bool empties_match = emptybit == emptylist && emptybit == emptycount; + bool empties_match = emptybit == emptylist; (void)empties_match; CHECK(empties_match); @@ -59,14 +58,6 @@ static inline void check_nexts(struct z_heap *h, int bidx) if (b->next != 0) { CHECK(valid_chunk(h, b->next)); } - - if (b->list_size == 2) { - CHECK(next_free_chunk(h, b->next) == prev_free_chunk(h, b->next)); - CHECK(next_free_chunk(h, b->next) != b->next); - } else if (b->list_size == 1) { - CHECK(next_free_chunk(h, b->next) == prev_free_chunk(h, b->next)); - CHECK(next_free_chunk(h, b->next) == b->next); - } } bool sys_heap_validate(struct sys_heap *heap) @@ -102,10 +93,6 @@ bool sys_heap_validate(struct sys_heap *heap) if (empty && h->buckets[b].next != 0) { return false; } - - if (n != h->buckets[b].list_size) { - return false; - } } /* Walk through the chunks linearly, verifying sizes and end diff --git a/lib/os/heap.c b/lib/os/heap.c index 4ac9061c33ac5..9e4cfe5629893 100644 --- a/lib/os/heap.c +++ b/lib/os/heap.c @@ -24,12 +24,10 @@ static void free_list_remove(struct z_heap *h, int bidx, CHECK(!chunk_used(h, c)); CHECK(b->next != 0); - CHECK(b->list_size > 0); CHECK(h->avail_buckets & (1 << bidx)); - b->list_size--; - - if (b->list_size == 0) { + if (next_free_chunk(h, c) == c) { + /* this is the last chunk */ h->avail_buckets &= ~(1 << bidx); b->next = 0; } else { @@ -46,8 +44,7 @@ static void free_list_add(struct z_heap *h, chunkid_t c) { int bi = bucket_idx(h, chunk_size(h, c)); - if (h->buckets[bi].list_size++ == 0) { - CHECK(h->buckets[bi].next == 0); + if (h->buckets[bi].next == 0) { CHECK((h->avail_buckets & (1 << bi)) == 0); /* Empty list, first item */ @@ -56,6 +53,8 @@ static void free_list_add(struct z_heap *h, chunkid_t c) set_prev_free_chunk(h, c, c); set_next_free_chunk(h, c, c); } else { + CHECK(h->avail_buckets & (1 << bi)); + /* Insert before (!) the "next" pointer */ chunkid_t second = h->buckets[bi].next; chunkid_t first = prev_free_chunk(h, second); @@ -65,8 +64,6 @@ static void free_list_add(struct z_heap *h, chunkid_t c) set_next_free_chunk(h, first, c); set_prev_free_chunk(h, second, c); } - - CHECK(h->avail_buckets & (1 << bi)); } /* Allocates (fit check has already been perfomred) from the next @@ -179,14 +176,16 @@ void *sys_heap_alloc(struct sys_heap *heap, size_t bytes) * fragmentation waste of the order of the block allocated * only. */ - int loops = MIN(b->list_size, CONFIG_SYS_HEAP_ALLOC_LOOPS); - - for (int i = 0; i < loops; i++) { - CHECK(b->next != 0); - if (chunk_size(h, b->next) >= sz) { - return split_alloc(h, bi, sz); - } - b->next = next_free_chunk(h, b->next); + if (b->next) { + chunkid_t first = b->next; + int i = CONFIG_SYS_HEAP_ALLOC_LOOPS; + do { + if (chunk_size(h, b->next) >= sz) { + return split_alloc(h, bi, sz); + } + b->next = next_free_chunk(h, b->next); + CHECK(b->next != 0); + } while (--i && b->next != first); } /* Otherwise pick the smallest non-empty bucket guaranteed to @@ -233,7 +232,6 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes) CHECK(chunk0_size + min_chunk_size(h) < buf_sz); for (int i = 0; i < nb_buckets; i++) { - h->buckets[i].list_size = 0; h->buckets[i].next = 0; } diff --git a/lib/os/heap.h b/lib/os/heap.h index d45bf043ec54d..3fa22e7aa2445 100644 --- a/lib/os/heap.h +++ b/lib/os/heap.h @@ -55,7 +55,6 @@ enum chunk_fields { LEFT_SIZE, SIZE_AND_USED, FREE_PREV, FREE_NEXT }; struct z_heap_bucket { chunkid_t next; - size_t list_size; }; struct z_heap {