From 255c9104e52fbdf2c21189fb269ba6241ebea788 Mon Sep 17 00:00:00 2001 From: Michael Lotz Date: Mon, 3 Aug 2009 13:59:45 +0000 Subject: [PATCH] * Rework the heap locking strategy. Use a read-write lock for the area lock to allow for more parallelism. Also introduce seperate locks for the bins and for page allocation. This greatly reduces lock contention and reduces the duration the locks are held due to them overall protecting less code. Now only allocations of the same size hitting the same allocator or allocating larger chunks of memory should block. Previously, basically any allocation and also free would be mutually exclusive, making it scale pretty badely. * Added memalign_nogrow(). As it uses heap_memalign() anyway, there's no real reason not to allow for an alignment. * Some cleanup. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@32074 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- headers/private/kernel/heap.h | 5 +- src/system/kernel/heap.cpp | 189 +++++++++++++++++++--------------- 2 files changed, 111 insertions(+), 83 deletions(-) diff --git a/headers/private/kernel/heap.h b/headers/private/kernel/heap.h index 6125789fdc..0341e8db49 100644 --- a/headers/private/kernel/heap.h +++ b/headers/private/kernel/heap.h @@ -41,8 +41,9 @@ typedef struct heap_allocator_s heap_allocator; extern "C" { #endif -// malloc_nogrow disallows waiting for a grow to happen - only to be used by -// vm functions that may deadlock on a triggered area creation +// malloc- and memalign_nogrow disallow waiting for a grow to happen - only to +// be used by vm functions that may deadlock on a triggered area creation. +void *memalign_nogrow(size_t alignment, size_t size); void *malloc_nogrow(size_t size); void *memalign(size_t alignment, size_t size); diff --git a/src/system/kernel/heap.cpp b/src/system/kernel/heap.cpp index 915d85ef92..bd0057c657 100644 --- a/src/system/kernel/heap.cpp +++ b/src/system/kernel/heap.cpp @@ -88,13 +88,15 @@ typedef struct heap_page_s { } heap_page; typedef struct heap_bin_s { + mutex lock; uint32 element_size; uint16 max_free_count; heap_page * page_list; // sorted so that the desired page is always first } heap_bin; struct heap_allocator_s { - mutex lock; + rw_lock area_lock; + mutex page_lock; const char *name; uint32 bin_count; @@ -628,7 +630,7 @@ caller_info_compare_count(const void* _a, const void* _b) static bool -analyze_allocation_callers(heap_allocator* heap) +analyze_allocation_callers(heap_allocator *heap) { // go through all the pages in all the areas heap_area *area = heap->all_areas; @@ -661,7 +663,7 @@ analyze_allocation_callers(heap_allocator* heap) info = (heap_leak_check_info *)(base + elementSize - sizeof(heap_leak_check_info)); - caller_info* callerInfo = get_caller_info(info->caller); + caller_info *callerInfo = get_caller_info(info->caller); if (callerInfo == NULL) { kprintf("out of space for caller infos\n"); return 0; @@ -685,7 +687,7 @@ analyze_allocation_callers(heap_allocator* heap) info = (heap_leak_check_info *)(base + pageCount * heap->page_size - sizeof(heap_leak_check_info)); - caller_info* callerInfo = get_caller_info(info->caller); + caller_info *callerInfo = get_caller_info(info->caller); if (callerInfo == NULL) { kprintf("out of space for caller infos\n"); return false; @@ -710,7 +712,7 @@ static int dump_allocations_per_caller(int argc, char **argv) { bool sortBySize = true; - heap_allocator* heap = NULL; + heap_allocator *heap = NULL; for (int32 i = 1; i < argc; i++) { if (strcmp(argv[i], "-c") == 0) { @@ -756,8 +758,8 @@ dump_allocations_per_caller(int argc, char **argv) caller_info& info = sCallerInfoTable[i]; kprintf("%10ld %10ld %#08lx", info.count, info.size, info.caller); - const char* symbol; - const char* imageName; + const char *symbol; + const char *imageName; bool exactMatch; addr_t baseAddress; @@ -780,7 +782,10 @@ dump_allocations_per_caller(int argc, char **argv) static void heap_validate_heap(heap_allocator *heap) { - mutex_lock(&heap->lock); + ReadLocker areaReadLocker(heap->area_lock); + for (uint32 i = 0; i < heap->bin_count; i++) + mutex_lock(&heap->bins[i].lock); + MutexLocker pageLocker(heap->page_lock); uint32 totalPageCount = 0; uint32 totalFreePageCount = 0; @@ -862,7 +867,6 @@ heap_validate_heap(heap_allocator *heap) // validate the bins for (uint32 i = 0; i < heap->bin_count; i++) { heap_bin *bin = &heap->bins[i]; - heap_page *lastPage = NULL; heap_page *page = bin->page_list; lastFreeCount = 0; @@ -938,7 +942,10 @@ heap_validate_heap(heap_allocator *heap) } } - mutex_unlock(&heap->lock); + pageLocker.Unlock(); + for (uint32 i = 0; i < heap->bin_count; i++) + mutex_unlock(&heap->bins[i].lock); + areaReadLocker.Unlock(); } #endif // PARANOID_HEAP_VALIDATION @@ -949,8 +956,6 @@ heap_validate_heap(heap_allocator *heap) static void heap_add_area(heap_allocator *heap, area_id areaID, addr_t base, size_t size) { - mutex_lock(&heap->lock); - heap_area *area = (heap_area *)base; area->area = areaID; @@ -986,8 +991,10 @@ heap_add_area(heap_allocator *heap, area_id areaID, addr_t base, size_t size) area->free_pages = &area->page_table[0]; area->free_page_count = pageCount; area->page_table[0].prev = NULL; - area->next = NULL; + + WriteLocker areaWriteLocker(heap->area_lock); + MutexLocker pageLocker(heap->page_lock); if (heap->areas == NULL) { // it's the only (empty) area in that heap area->prev = NULL; @@ -1018,13 +1025,14 @@ heap_add_area(heap_allocator *heap, area_id areaID, addr_t base, size_t size) heap->total_pages += area->page_count; heap->total_free_pages += area->free_page_count; - if (areaID >= B_OK) { + if (areaID >= 0) { // this later on deletable area is yet empty - the empty count will be // decremented as soon as this area is used for the first time heap->empty_areas++; } - mutex_unlock(&heap->lock); + pageLocker.Unlock(); + areaWriteLocker.Unlock(); dprintf("heap_add_area: area %ld added to %s heap %p - usable range 0x%08lx " "- 0x%08lx\n", area->area, heap->name, heap, area->base, @@ -1033,11 +1041,8 @@ heap_add_area(heap_allocator *heap, area_id areaID, addr_t base, size_t size) static status_t -heap_remove_area(heap_allocator *heap, heap_area *area, bool locked) +heap_remove_area(heap_allocator *heap, heap_area *area) { - if (!locked) - mutex_lock(&heap->lock); - if (area->free_page_count != area->page_count) { panic("tried removing heap area that has still pages in use"); return B_ERROR; @@ -1075,12 +1080,10 @@ heap_remove_area(heap_allocator *heap, heap_area *area, bool locked) heap->total_pages -= area->page_count; heap->total_free_pages -= area->free_page_count; - if (!locked) - mutex_unlock(&heap->lock); - dprintf("heap_remove_area: area %ld with range 0x%08lx - 0x%08lx removed " "from %s heap %p\n", area->area, area->base, area->base + area->size, heap->name, heap); + return B_OK; } @@ -1114,6 +1117,7 @@ heap_create_allocator(const char *name, addr_t base, size_t size, continue; heap_bin *bin = &heap->bins[heap->bin_count]; + mutex_init(&bin->lock, "heap bin lock"); bin->element_size = binSize; bin->max_free_count = heap->page_size / binSize; bin->page_list = NULL; @@ -1126,14 +1130,15 @@ heap_create_allocator(const char *name, addr_t base, size_t size, base += heap->bin_count * sizeof(heap_bin); size -= heap->bin_count * sizeof(heap_bin); - mutex_init(&heap->lock, "heap_mutex"); + rw_lock_init(&heap->area_lock, "heap area rw lock"); + mutex_init(&heap->page_lock, "heap page lock"); heap_add_area(heap, -1, base, size); return heap; } -static inline area_id +static inline void heap_free_pages_added(heap_allocator *heap, heap_area *area, uint32 pageCount) { area->free_page_count += pageCount; @@ -1170,29 +1175,15 @@ heap_free_pages_added(heap_allocator *heap, heap_area *area, uint32 pageCount) } } - // can and should we free this area? - if (area->free_page_count == area->page_count && area->area >= B_OK) { - if (heap->empty_areas > 0) { - // we already have at least another empty area, just free this one - if (heap_remove_area(heap, area, true) == B_OK) { - // we cannot delete the area here, because it would result - // in calls to free, which would necessarily deadlock as we - // are locked at this point - return area->area; - } - } - + if (area->free_page_count == area->page_count && area->area >= 0) heap->empty_areas++; - } - - return -1; } static inline void heap_free_pages_removed(heap_allocator *heap, heap_area *area, uint32 pageCount) { - if (area->free_page_count == area->page_count && area->area >= B_OK) { + if (area->free_page_count == area->page_count && area->area >= 0) { // this area was completely empty heap->empty_areas--; } @@ -1239,6 +1230,7 @@ heap_unlink_page(heap_page *page, heap_page **list) static heap_page * heap_allocate_contiguous_pages(heap_allocator *heap, uint32 pageCount) { + MutexLocker pageLocker(heap->page_lock); heap_area *area = heap->areas; while (area) { bool found = false; @@ -1286,10 +1278,14 @@ heap_allocate_contiguous_pages(heap_allocator *heap, uint32 pageCount) static void * heap_raw_alloc(heap_allocator *heap, size_t size, uint32 binIndex) { + TRACE(("raw_alloc: heap %p; size %lu; bin %lu\n", heap, size, binIndex)); if (binIndex < heap->bin_count) { heap_bin *bin = &heap->bins[binIndex]; + MutexLocker binLocker(bin->lock); + heap_page *page = bin->page_list; if (page == NULL) { + MutexLocker pageLocker(heap->page_lock); heap_area *area = heap->areas; if (area == NULL) { TRACE(("heap %p: no free pages to allocate %lu bytes\n", heap, @@ -1305,7 +1301,13 @@ heap_raw_alloc(heap_allocator *heap, size_t size, uint32 binIndex) page->next->prev = NULL; heap_free_pages_removed(heap, area, 1); + + if (page->in_use) + panic("got an in use page from the free pages list\n"); page->in_use = 1; + + pageLocker.Unlock(); + page->bin_index = binIndex; page->free_count = bin->max_free_count; page->empty_index = 0; @@ -1336,6 +1338,8 @@ heap_raw_alloc(heap_allocator *heap, size_t size, uint32 binIndex) page->next = page->prev = NULL; } + binLocker.Unlock(); + #if KERNEL_HEAP_LEAK_CHECK heap_leak_check_info *info = (heap_leak_check_info *)((addr_t)address + bin->element_size - sizeof(heap_leak_check_info)); @@ -1396,8 +1400,6 @@ heap_memalign(heap_allocator *heap, size_t alignment, size_t size) panic("memalign() with an alignment which is not a power of 2\n"); #endif - mutex_lock(&heap->lock); - #if KERNEL_HEAP_LEAK_CHECK size += sizeof(heap_leak_check_info); #endif @@ -1420,15 +1422,14 @@ heap_memalign(heap_allocator *heap, size_t alignment, size_t size) void *address = heap_raw_alloc(heap, size, binIndex); - TRACE(("memalign(): asked to allocate %lu bytes, returning pointer %p\n", - size, address)); - #if KERNEL_HEAP_LEAK_CHECK size -= sizeof(heap_leak_check_info); #endif + TRACE(("memalign(): asked to allocate %lu bytes, returning pointer %p\n", + size, address)); + T(Allocate((addr_t)address, size)); - mutex_unlock(&heap->lock); if (address == NULL) return address; @@ -1453,8 +1454,7 @@ heap_free(heap_allocator *heap, void *address) if (address == NULL) return B_OK; - mutex_lock(&heap->lock); - + ReadLocker areaReadLocker(heap->area_lock); heap_area *area = heap->all_areas; while (area) { // since the all_areas list is ordered by base with the biggest @@ -1476,7 +1476,6 @@ heap_free(heap_allocator *heap, void *address) if (area == NULL) { // this address does not belong to us - mutex_unlock(&heap->lock); return B_ENTRY_NOT_FOUND; } @@ -1490,19 +1489,18 @@ heap_free(heap_allocator *heap, void *address) if (page->bin_index > heap->bin_count) { panic("free(): page %p: invalid bin_index %d\n", page, page->bin_index); - mutex_unlock(&heap->lock); return B_ERROR; } - area_id areaToDelete = -1; if (page->bin_index < heap->bin_count) { // small allocation heap_bin *bin = &heap->bins[page->bin_index]; + MutexLocker binLocker(bin->lock); + if (((addr_t)address - area->base - page->index * heap->page_size) % bin->element_size != 0) { panic("free(): passed invalid pointer %p supposed to be in bin for " "element size %ld\n", address, bin->element_size); - mutex_unlock(&heap->lock); return B_ERROR; } @@ -1515,7 +1513,6 @@ heap_free(heap_allocator *heap, void *address) if (temp == address) { panic("free(): address %p already exists in page free " "list\n", address); - mutex_unlock(&heap->lock); return B_ERROR; } } @@ -1525,7 +1522,6 @@ heap_free(heap_allocator *heap, void *address) if (bin->element_size % 4 != 0) { panic("free(): didn't expect a bin element size that is not a " "multiple of 4\n"); - mutex_unlock(&heap->lock); return B_ERROR; } @@ -1542,10 +1538,11 @@ heap_free(heap_allocator *heap, void *address) if (page->free_count == bin->max_free_count) { // we are now empty, remove the page from the bin list + MutexLocker pageLocker(heap->page_lock); heap_unlink_page(page, &bin->page_list); page->in_use = 0; heap_link_page(page, &area->free_pages); - areaToDelete = heap_free_pages_added(heap, area, 1); + heap_free_pages_added(heap, area, 1); } else if (page->free_count == 1) { // we need to add ourselfs to the page list of the bin heap_link_page(page, &bin->page_list); @@ -1572,6 +1569,8 @@ heap_free(heap_allocator *heap, void *address) uint32 allocationID = page->allocation_id; uint32 maxPages = area->page_count - page->index; uint32 pageCount = 0; + + MutexLocker pageLocker(heap->page_lock); for (uint32 i = 0; i < maxPages; i++) { // loop until we find the end of this allocation if (!page[i].in_use || page[i].bin_index != heap->bin_count @@ -1587,16 +1586,27 @@ heap_free(heap_allocator *heap, void *address) pageCount++; } - areaToDelete = heap_free_pages_added(heap, area, pageCount); + heap_free_pages_added(heap, area, pageCount); } T(Free((addr_t)address)); - mutex_unlock(&heap->lock); + areaReadLocker.Unlock(); - if (areaToDelete >= B_OK) { - // adding free pages caused an area to become empty and freeable that - // we can now delete as we don't hold the heap lock anymore - delete_area(areaToDelete); + if (heap->empty_areas > 1) { + WriteLocker areaWriteLocker(heap->area_lock); + MutexLocker pageLocker(heap->page_lock); + + area = heap->areas; + while (area != NULL && heap->empty_areas > 1) { + if (area->area >= 0 + && area->free_page_count == area->page_count + && heap_remove_area(heap, area) == B_OK) { + delete_area(area->area); + heap->empty_areas--; + } + + area = area->next; + } } return B_OK; @@ -1616,19 +1626,29 @@ static status_t heap_realloc(heap_allocator *heap, void *address, void **newAddress, size_t newSize) { - mutex_lock(&heap->lock); + ReadLocker heapReadLocker(heap->area_lock); heap_area *area = heap->all_areas; while (area) { - if ((addr_t)address >= area->base - && (addr_t)address < area->base + area->size) + // since the all_areas list is ordered by base with the biggest + // base at the top, we need only find the first area with a base + // smaller than our address to become our only candidate for + // reallocating + if (area->base <= (addr_t)address) { + if ((addr_t)address >= area->base + area->size) { + // the only candidate area doesn't contain the address, + // set it to NULL so we return below (none of the other areas + // can contain the address as the list is ordered) + area = NULL; + } + break; + } area = area->all_next; } if (area == NULL) { // this address does not belong to us - mutex_unlock(&heap->lock); return B_ENTRY_NOT_FOUND; } @@ -1639,7 +1659,6 @@ heap_realloc(heap_allocator *heap, void *address, void **newAddress, if (page->bin_index > heap->bin_count) { panic("realloc(): page %p: invalid bin_index %d\n", page, page->bin_index); - mutex_unlock(&heap->lock); return B_ERROR; } @@ -1657,6 +1676,8 @@ heap_realloc(heap_allocator *heap, void *address, void **newAddress, uint32 allocationID = page->allocation_id; uint32 maxPages = area->page_count - page->index; maxSize = heap->page_size; + + MutexLocker pageLocker(heap->page_lock); for (uint32 i = 1; i < maxPages; i++) { if (!page[i].in_use || page[i].bin_index != heap->bin_count || page[i].allocation_id != allocationID) @@ -1667,7 +1688,7 @@ heap_realloc(heap_allocator *heap, void *address, void **newAddress, } } - mutex_unlock(&heap->lock); + heapReadLocker.Unlock(); #if KERNEL_HEAP_LEAK_CHECK newSize += sizeof(heap_leak_check_info); @@ -2001,11 +2022,11 @@ memalign(size_t alignment, size_t size) void * -malloc_nogrow(size_t size) +memalign_nogrow(size_t alignment, size_t size) { // use dedicated memory in the grow thread by default if (thread_get_current_thread_id() == sHeapGrowThread) { - void *result = heap_memalign(sGrowHeap, 0, size); + void *result = heap_memalign(sGrowHeap, alignment, size); if (!sAddGrowHeap && heap_should_grow(sGrowHeap)) { // hopefully the heap grower will manage to create a new heap // before running out of private memory... @@ -2020,7 +2041,7 @@ malloc_nogrow(size_t size) // try public memory, there might be something available heap_allocator *heap = sHeaps[heap_class_for(size)]; - void *result = heap_memalign(heap, 0, size); + void *result = heap_memalign(heap, alignment, size); if (result != NULL) return result; @@ -2034,6 +2055,13 @@ malloc_nogrow(size_t size) } +void * +malloc_nogrow(size_t size) +{ + return memalign_nogrow(0, size); +} + + void * malloc(size_t size) { @@ -2169,36 +2197,35 @@ calloc(size_t numElements, size_t size) void -deferred_free(void* block) +deferred_free(void *block) { if (block == NULL) return; - DeferredFreeListEntry* entry = new(block) DeferredFreeListEntry; + DeferredFreeListEntry *entry = new(block) DeferredFreeListEntry; InterruptsSpinLocker _(sDeferredFreeListLock); sDeferredFreeList.Add(entry); } -void* +void * malloc_referenced(size_t size) { - int32* referencedData = (int32*)malloc(size + 4); + int32 *referencedData = (int32 *)malloc(size + 4); if (referencedData == NULL) return NULL; *referencedData = 1; - return referencedData + 1; } -void* -malloc_referenced_acquire(void* data) +void * +malloc_referenced_acquire(void *data) { if (data != NULL) { - int32* referencedData = (int32*)data - 1; + int32 *referencedData = (int32 *)data - 1; atomic_add(referencedData, 1); } @@ -2207,12 +2234,12 @@ malloc_referenced_acquire(void* data) void -malloc_referenced_release(void* data) +malloc_referenced_release(void *data) { if (data == NULL) return; - int32* referencedData = (int32*)data - 1; + int32 *referencedData = (int32 *)data - 1; if (atomic_add(referencedData, -1) < 1) free(referencedData); } @@ -2224,7 +2251,7 @@ DeferredDeletable::~DeferredDeletable() void -deferred_delete(DeferredDeletable* deletable) +deferred_delete(DeferredDeletable *deletable) { if (deletable == NULL) return;