From patchwork Wed Oct 28 20:36:16 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 537561 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id C4ACF140D7C for ; Thu, 29 Oct 2015 07:36:28 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b=F8AiMz38; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type; q=dns; s=default; b=XhqH bEEcDJ1HV0ESFEYQWZHVgKCvC+C/qLFoEeusQZ4vrqhFW1GwRN3HKDlqSpxCm5bs hZiXFns/vIoYj99JL9OCDSQAtriTmccUo9Si/KzpraJDvfdoK+UYxkNUqvbb5aUv mhMmVgFTAQsXDCoT4aXDsSlrLxa/KqYTqgdXfMs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type; s=default; bh=810E0YVBUh MvcgcC+4e+YY+7xHc=; b=F8AiMz38QTuw2J78SEJT8xO0ac6E3CmCNDCYaRSy84 pXMqYkPkpc6j7/e8bjV2TjgCMkEKXNLiCi65k8qbGVOV4Eoo6XA++WcLPB25EtGP IThWPEG8nTrhJy/MLvbTe0F87yYVOwY5JjOSUhh7QzrXmhBN8DPnKGfxVQx2LFjG c= Received: (qmail 71073 invoked by alias); 28 Oct 2015 20:36:22 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 71064 invoked by uid 89); 28 Oct 2015 20:36:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Subject: [PATCH v4 COMMITTED] malloc: Prevent arena free_list from turning cyclic [BZ #19048] To: "Carlos O'Donell" References: <5617E843.3060504@redhat.com> <5618075F.105@reserved-bit.com> <561CEB8F.8010703@redhat.com> <56293B70.70108@redhat.com> <56306E59.5080602@redhat.com> Cc: GNU C Library , Siddhesh Poyarekar From: Florian Weimer Message-ID: <563131C0.1030603@redhat.com> Date: Wed, 28 Oct 2015 21:36:16 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56306E59.5080602@redhat.com> On 10/28/2015 07:42 AM, Carlos O'Donell wrote: >> diff --git a/NEWS b/NEWS >> index 00e3b03..d4a7863 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -19,9 +19,9 @@ Version 2.23 >> 18857, 18863, 18870, 18872, 18873, 18875, 18887, 18918, 18921, 18928, >> 18951, 18952, 18953, 18956, 18961, 18966, 18967, 18969, 18970, 18977, >> 18980, 18981, 18982, 18985, 19003, 19007, 19012, 19016, 19018, 19032, >> - 19046, 19049, 19050, 19059, 19071, 19074, 19076, 19077, 19078, 19079, >> - 19085, 19086, 19088, 19094, 19095, 19124, 19125, 19129, 19134, 19137, >> - 19156. >> + 19046, 19048, 19049, 19050, 19059, 19071, 19074, 19076, 19077, 19078, >> + 19079, 19085, 19086, 19088, 19094, 19095, 19124, 19125, 19129, 19134, >> + 19137, 19156. > > Please keep NEWS out of the diff. It makes it easier to use cli pwclient > to fetch and apply patch without needing a merge driver. We'll soon do away > with this requirement by automating the generation of the list from bugzilla. Okay, I will try to remember that. > Should we add a NEWS item to highlight the behavioural change in malloc? > Why check `result != NULL`? > > if (replaced_arena != NULL) > detach_arena (replaced_arena); > > ? Oops. There is already a check in detach_arena, so I removed the condition. > I wouldn't expect thread_arena to ever be NULL, so it seems > like we should be able to just write: > > deatch_arena (replaced_arena); > > and be done. I assume replaced_arena could be NULL if the first arena is attached to the thread. I'm attaching what I've committed. I have backported this to Fedora 22, and the desktop environment still works. Florian 2015-10-28 Florian Weimer [BZ# 19048] * malloc/malloc.c (struct malloc_state): Update comment. Add attached_threads member. (main_arena): Initialize attached_threads. * malloc/arena.c (list_lock): Update comment. (ptmalloc_lock_all, ptmalloc_unlock_all): Likewise. (ptmalloc_unlock_all2): Reinitialize arena reference counts. (deattach_arena): New function. (_int_new_arena): Initialize arena reference count and deattach replaced arena. (get_free_list, reused_arena): Update reference count and deattach replaced arena. (arena_thread_freeres): Update arena reference count and only put unreferenced arenas on the free list. * A defect in the malloc implementation, present since glibc 2.15 (2012) or glibc 2.10 via --enable-experimental-malloc (2009), could result in the unnecessary serialization of memory allocation requests across threads. The defect is now corrected. Users should see a substantial increase in the concurent throughput of allocation requests for applications which trigger this bug. Affected applications typically create create and destroy threads frequently. (Bug 19048 was reported and analyzed by Ericsson.) diff --git a/malloc/arena.c b/malloc/arena.c index caf718d..0f00afa 100644 --- a/malloc/arena.c +++ b/malloc/arena.c @@ -68,7 +68,10 @@ extern int sanity_check_heap_info_alignment[(sizeof (heap_info) static __thread mstate thread_arena attribute_tls_model_ie; -/* Arena free list. */ +/* Arena free list. list_lock protects the free_list variable below, + and the next_free and attached_threads members of the mstate + objects. No other (malloc) locks must be taken while list_lock is + active, otherwise deadlocks may occur. */ static mutex_t list_lock = MUTEX_INITIALIZER; static size_t narenas = 1; @@ -225,7 +228,10 @@ ptmalloc_lock_all (void) save_free_hook = __free_hook; __malloc_hook = malloc_atfork; __free_hook = free_atfork; - /* Only the current thread may perform malloc/free calls now. */ + /* Only the current thread may perform malloc/free calls now. + save_arena will be reattached to the current thread, in + ptmalloc_lock_all, so save_arena->attached_threads is not + updated. */ save_arena = thread_arena; thread_arena = ATFORK_ARENA_PTR; out: @@ -243,6 +249,9 @@ ptmalloc_unlock_all (void) if (--atfork_recursive_cntr != 0) return; + /* Replace ATFORK_ARENA_PTR with save_arena. + save_arena->attached_threads was not changed in ptmalloc_lock_all + and is still correct. */ thread_arena = save_arena; __malloc_hook = save_malloc_hook; __free_hook = save_free_hook; @@ -274,12 +283,19 @@ ptmalloc_unlock_all2 (void) thread_arena = save_arena; __malloc_hook = save_malloc_hook; __free_hook = save_free_hook; + + /* Push all arenas to the free list, except save_arena, which is + attached to the current thread. */ + if (save_arena != NULL) + ((mstate) save_arena)->attached_threads = 1; free_list = NULL; for (ar_ptr = &main_arena;; ) { mutex_init (&ar_ptr->mutex); if (ar_ptr != save_arena) { + /* This arena is no longer attached to any thread. */ + ar_ptr->attached_threads = 0; ar_ptr->next_free = free_list; free_list = ar_ptr; } @@ -718,6 +734,22 @@ heap_trim (heap_info *heap, size_t pad) /* Create a new arena with initial size "size". */ +/* If REPLACED_ARENA is not NULL, detach it from this thread. Must be + called while list_lock is held. */ +static void +detach_arena (mstate replaced_arena) +{ + if (replaced_arena != NULL) + { + assert (replaced_arena->attached_threads > 0); + /* The current implementation only detaches from main_arena in + case of allocation failure. This means that it is likely not + beneficial to put the arena on free_list even if the + reference count reaches zero. */ + --replaced_arena->attached_threads; + } +} + static mstate _int_new_arena (size_t size) { @@ -739,6 +771,7 @@ _int_new_arena (size_t size) } a = h->ar_ptr = (mstate) (h + 1); malloc_init_state (a); + a->attached_threads = 1; /*a->next = NULL;*/ a->system_mem = a->max_system_mem = h->size; arena_mem += h->size; @@ -752,12 +785,15 @@ _int_new_arena (size_t size) set_head (top (a), (((char *) h + h->size) - ptr) | PREV_INUSE); LIBC_PROBE (memory_arena_new, 2, a, size); + mstate replaced_arena = thread_arena; thread_arena = a; mutex_init (&a->mutex); (void) mutex_lock (&a->mutex); (void) mutex_lock (&list_lock); + detach_arena (replaced_arena); + /* Add the new arena to the global list. */ a->next = main_arena.next; atomic_write_barrier (); @@ -772,13 +808,23 @@ _int_new_arena (size_t size) static mstate get_free_list (void) { + mstate replaced_arena = thread_arena; mstate result = free_list; if (result != NULL) { (void) mutex_lock (&list_lock); result = free_list; if (result != NULL) - free_list = result->next_free; + { + free_list = result->next_free; + + /* Arenas on the free list are not attached to any thread. */ + assert (result->attached_threads == 0); + /* But the arena will now be attached to this thread. */ + result->attached_threads = 1; + + detach_arena (replaced_arena); + } (void) mutex_unlock (&list_lock); if (result != NULL) @@ -837,6 +883,14 @@ reused_arena (mstate avoid_arena) (void) mutex_lock (&result->mutex); out: + { + mstate replaced_arena = thread_arena; + (void) mutex_lock (&list_lock); + detach_arena (replaced_arena); + ++result->attached_threads; + (void) mutex_unlock (&list_lock); + } + LIBC_PROBE (memory_arena_reuse, 2, result, avoid_arena); thread_arena = result; next_to_use = result->next; @@ -931,8 +985,14 @@ arena_thread_freeres (void) if (a != NULL) { (void) mutex_lock (&list_lock); - a->next_free = free_list; - free_list = a; + /* If this was the last attached thread for this arena, put the + arena on the free list. */ + assert (a->attached_threads > 0); + if (--a->attached_threads == 0) + { + a->next_free = free_list; + free_list = a; + } (void) mutex_unlock (&list_lock); } } diff --git a/malloc/malloc.c b/malloc/malloc.c index 9371b5a..35c8863 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -1709,9 +1709,15 @@ struct malloc_state /* Linked list */ struct malloc_state *next; - /* Linked list for free arenas. */ + /* Linked list for free arenas. Access to this field is serialized + by list_lock in arena.c. */ struct malloc_state *next_free; + /* Number of threads attached to this arena. 0 if the arena is on + the free list. Access to this field is serialized by list_lock + in arena.c. */ + INTERNAL_SIZE_T attached_threads; + /* Memory allocated from the system in this arena. */ INTERNAL_SIZE_T system_mem; INTERNAL_SIZE_T max_system_mem; @@ -1755,7 +1761,8 @@ struct malloc_par static struct malloc_state main_arena = { .mutex = MUTEX_INITIALIZER, - .next = &main_arena + .next = &main_arena, + .attached_threads = 1 }; /* There is only one instance of the malloc parameters. */ -- 2.4.3