From patchwork Tue May 12 10:12:20 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeremy Kerr X-Patchwork-Id: 471220 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 71E4A14016A for ; Tue, 12 May 2015 20:12:39 +1000 (AEST) Received: from ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 551E11A0127 for ; Tue, 12 May 2015 20:12:39 +1000 (AEST) X-Original-To: skiboot@lists.ozlabs.org Delivered-To: skiboot@lists.ozlabs.org Received: from ozlabs.org (ozlabs.org [103.22.144.67]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 633051A0127 for ; Tue, 12 May 2015 20:12:27 +1000 (AEST) Received: by ozlabs.org (Postfix, from userid 1023) id 43F77140D25; Tue, 12 May 2015 20:12:27 +1000 (AEST) MIME-Version: 1.0 Message-Id: <1431425540.369385.198769938566.1.gpush@pablo> To: skiboot@lists.ozlabs.org From: Jeremy Kerr Date: Tue, 12 May 2015 18:12:20 +0800 Subject: [Skiboot] [PATCH 1/5 v3] core: Move free-list locking to a separate per-region lock X-BeenThere: skiboot@lists.ozlabs.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Mailing list for skiboot development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Skiboot" Currently, we have a single lock for the entire mem_region system; this protects both the global region list, and the allocations from each region. This means we can't allocate memory while traversing the global region list, as any malloc/realloc/free will try to acquire the mem_region lock again. This change separates the locking into different functions. We keep the mem_region_lock to protect the regions list, and introduce a per-region lock to protect allocations from the regions' free_lists. Then we remove the open-coded invocations of mem_alloc, where we'd avoided malloc() due to the above locking issue. Signed-off-by: Jeremy Kerr --- core/malloc.c | 12 ++++----- core/mem_region.c | 39 +++++++++++++++++--------------- core/test/run-malloc-speed.c | 2 - core/test/run-malloc.c | 28 +++++++++++----------- core/test/run-mem_region.c | 2 - core/test/run-mem_region_init.c | 2 - include/mem_region.h | 3 ++ 7 files changed, 47 insertions(+), 41 deletions(-) diff --git a/core/malloc.c b/core/malloc.c index 9e715f6..669cc59 100644 --- a/core/malloc.c +++ b/core/malloc.c @@ -25,9 +25,9 @@ void *__memalign(size_t blocksize, size_t bytes, const char *location) { void *p; - lock(&mem_region_lock); + lock(&skiboot_heap.free_list_lock); p = mem_alloc(&skiboot_heap, bytes, blocksize, location); - unlock(&mem_region_lock); + unlock(&skiboot_heap.free_list_lock); return p; } @@ -39,9 +39,9 @@ void *__malloc(size_t bytes, const char *location) void __free(void *p, const char *location) { - lock(&mem_region_lock); + lock(&skiboot_heap.free_list_lock); mem_free(&skiboot_heap, p, location); - unlock(&mem_region_lock); + unlock(&skiboot_heap.free_list_lock); } void *__realloc(void *ptr, size_t size, const char *location) @@ -56,7 +56,7 @@ void *__realloc(void *ptr, size_t size, const char *location) if (!ptr) return __malloc(size, location); - lock(&mem_region_lock); + lock(&skiboot_heap.free_list_lock); if (mem_resize(&skiboot_heap, ptr, size, location)) { newptr = ptr; } else { @@ -70,7 +70,7 @@ void *__realloc(void *ptr, size_t size, const char *location) mem_free(&skiboot_heap, ptr, location); } } - unlock(&mem_region_lock); + unlock(&skiboot_heap.free_list_lock); return newptr; } diff --git a/core/mem_region.c b/core/mem_region.c index d7c677d..8adb036 100644 --- a/core/mem_region.c +++ b/core/mem_region.c @@ -30,6 +30,19 @@ #define POISON_MEM_REGION_WITH 0x99 #define POISON_MEM_REGION_LIMIT 1*1024*1024*1024 +/* Locking: The mem_region_lock protects the regions list from concurrent + * updates. Additions to, or removals from, the region list must be done + * with this lock held. This is typically done when we're establishing + * the memory & reserved regions. + * + * Each region has a lock (region->free_list_lock) to protect the free list + * from concurrent modification. This lock is used when we're allocating + * memory out of a specific region. + * + * If both locks are needed (eg, __local_alloc, where we need to find a region, + * then allocate from it), the mem_region_lock must be acquired before (and + * released after) the per-region lock. + */ struct lock mem_region_lock = LOCK_UNLOCKED; static struct list_head regions = LIST_HEAD_INIT(regions); @@ -542,9 +555,7 @@ static struct mem_region *new_region(const char *name, { struct mem_region *region; - /* Avoid lock recursion, call mem_alloc directly. */ - region = mem_alloc(&skiboot_heap, - sizeof(*region), __alignof__(*region), __location__); + region = malloc(sizeof(*region)); if (!region) return NULL; @@ -554,6 +565,7 @@ static struct mem_region *new_region(const char *name, region->mem_node = mem_node; region->type = type; region->free_list.n.next = NULL; + init_lock(®ion->free_list_lock); return region; } @@ -629,8 +641,7 @@ static bool add_region(struct mem_region *region) assert(r->start == region->start); assert(r->len == region->len); list_del_from(®ions, &r->list); - /* We already hold mem_region lock */ - mem_free(&skiboot_heap, r, __location__); + free(r); } /* Finally, add in our own region. */ @@ -695,7 +706,9 @@ restart: } /* Second pass, match anything */ + lock(®ion->free_list_lock); p = mem_alloc(region, size, align, location); + unlock(®ion->free_list_lock); if (p) break; } @@ -809,10 +822,7 @@ void mem_region_init(void) char *name; len = strlen(names->prop + n) + 1; - - name = mem_alloc(&skiboot_heap, len, - __alignof__(*name), __location__); - memcpy(name, names->prop + n, len); + name = strdup(names->prop + n); region = new_region(name, dt_get_number(range, 2), @@ -930,15 +940,8 @@ void mem_region_add_dt_reserved(void) ranges_len += 2 * sizeof(uint64_t); } - /* Allocate property data with mem_alloc; malloc() acquires - * mem_region_lock */ - names = mem_alloc(&skiboot_heap, names_len, - __alignof__(*names), __location__); - ranges = mem_alloc(&skiboot_heap, ranges_len, - __alignof__(*ranges), __location__); - - name = names; - range = ranges; + name = names = malloc(names_len); + range = ranges = malloc(ranges_len); printf("Reserved regions:\n"); /* Second pass: populate property data */ diff --git a/core/test/run-malloc-speed.c b/core/test/run-malloc-speed.c index 18c0de9..713a74b 100644 --- a/core/test/run-malloc-speed.c +++ b/core/test/run-malloc-speed.c @@ -90,7 +90,7 @@ int main(void) + skiboot_heap.len); } assert(mem_check(&skiboot_heap)); - assert(mem_region_lock.lock_val == 0); + assert(skiboot_heap.free_list_lock.lock_val == 0); free(region_start(&skiboot_heap)); real_free(p); return 0; diff --git a/core/test/run-malloc.c b/core/test/run-malloc.c index bbd2a48..723cb10 100644 --- a/core/test/run-malloc.c +++ b/core/test/run-malloc.c @@ -92,45 +92,45 @@ int main(void) assert(p); assert(p > (char *)test_heap); assert(p + (1ULL << i) <= (char *)test_heap + TEST_HEAP_SIZE); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); free(p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); assert(heap_empty()); } /* Realloc as malloc. */ - mem_region_lock.lock_val = 0; + skiboot_heap.free_list_lock.lock_val = 0; p = realloc(NULL, 100); assert(p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); /* Realloc as free. */ p = realloc(p, 0); assert(!p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); assert(heap_empty()); /* Realloc longer. */ p = realloc(NULL, 100); assert(p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); p2 = realloc(p, 200); assert(p2 == p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); free(p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); assert(heap_empty()); /* Realloc shorter. */ - mem_region_lock.lock_val = 0; + skiboot_heap.free_list_lock.lock_val = 0; p = realloc(NULL, 100); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); assert(p); p2 = realloc(p, 1); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); assert(p2 == p); free(p); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); assert(heap_empty()); /* zalloc failure */ @@ -152,7 +152,7 @@ int main(void) assert(p2[63] == 'c'); free(p2); assert(heap_empty()); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); /* Realloc with failure to allocate new size */ p2 = malloc(TEST_HEAP_SIZE - sizeof(struct alloc_hdr)*2); @@ -176,7 +176,7 @@ int main(void) free(p); free(p4); assert(heap_empty()); - assert(!mem_region_lock.lock_val); + assert(!skiboot_heap.free_list_lock.lock_val); real_free(test_heap); return 0; diff --git a/core/test/run-mem_region.c b/core/test/run-mem_region.c index db1ca02..e27459b 100644 --- a/core/test/run-mem_region.c +++ b/core/test/run-mem_region.c @@ -244,7 +244,7 @@ int main(void) list_del(&r->list); mem_free(&skiboot_heap, r, __location__); } - assert(mem_region_lock.lock_val == 0); + assert(skiboot_heap.free_list_lock.lock_val == 0); __free(test_heap, ""); return 0; } diff --git a/core/test/run-mem_region_init.c b/core/test/run-mem_region_init.c index bb606ef..7624057 100644 --- a/core/test/run-mem_region_init.c +++ b/core/test/run-mem_region_init.c @@ -177,7 +177,7 @@ int main(void) } assert(mem_check(&skiboot_heap)); } - assert(mem_region_lock.lock_val == 0); + assert(skiboot_heap.free_list_lock.lock_val == 0); real_free(heap); return 0; } diff --git a/include/mem_region.h b/include/mem_region.h index d6dd296..e5cf0f6 100644 --- a/include/mem_region.h +++ b/include/mem_region.h @@ -19,6 +19,8 @@ #include #include +#include + enum mem_region_type { /* ranges allocatable by mem_alloc: this will be most of memory */ REGION_SKIBOOT_HEAP, @@ -41,6 +43,7 @@ struct mem_region { struct dt_node *mem_node; enum mem_region_type type; struct list_head free_list; + struct lock free_list_lock; }; extern struct lock mem_region_lock;