From patchwork Wed Jun 22 01:29:19 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 13297 Received: (qmail 101326 invoked by alias); 22 Jun 2016 01:29:29 -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 101286 invoked by uid 89); 22 Jun 2016 01:29:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=lastly, 8006, H*r:sk:12.2016, persons X-HELO: mail-io0-f174.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-transfer-encoding; bh=7ubdWreb/Us17fO9mne/aFUmMHJ4CnSuCp/7r9CB+R0=; b=nEfG6ALx93SQdUQEGoHYE54VVe6FwkobC45iw7F6X72agk7ZT51Vnpq+C0Wr/3ELyH wGAJlFuogGdHyJuM7dYiCp/J3u1UIr0srYhA8S4zwJQClkP+vmmBooevsCUnly/WqJp4 kDq5nQjmsdP6jsVpNanIB8gCHhKX3G4blnh5ndqZbkiEwjmobozRrWrTQ97KUV4cxcoZ 5e646EdStwQhfQBRqP1U3XB0f0VyJCIAyoTN5dwDmszgxdCnCpoZZr5ldQ3zqYNpz069 dPjv15bK+VAnPRnQoFh6pGQaIU05CDJA+YFXuSL7oAkNn3OXGeA5ht7SY03PD0kn1LYg ufPg== X-Gm-Message-State: ALyK8tIG112Q6XqlB1HnU6GO2Ey+/Uf3myryTvD2j9F63pJfGp9/hT4/hTzQy9szjp2/FgLW X-Received: by 10.107.2.77 with SMTP id 74mr37090128ioc.9.1466558961913; Tue, 21 Jun 2016 18:29:21 -0700 (PDT) Subject: Re: [PATCH] malloc: Avoid premature fallback to mmap [BZ #20284] To: Florian Weimer , libc-alpha@sourceware.org References: <20160621152118.20F8F402276E4@oldenburg.str.redhat.com> From: Carlos O'Donell Message-ID: Date: Tue, 21 Jun 2016 21:29:19 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160621152118.20F8F402276E4@oldenburg.str.redhat.com> On 06/21/2016 11:21 AM, Florian Weimer wrote: > 2016-06-21 Florian Weimer > > [BZ #20284] > * malloc/arena.c (reused_arena): Do not return NULL if we start > out with a non-corrupted arena. > > diff --git a/malloc/arena.c b/malloc/arena.c > index ed5a4d5..229783f 100644 > --- a/malloc/arena.c > +++ b/malloc/arena.c > @@ -771,14 +771,12 @@ reused_arena (mstate avoid_arena) > { > result = result->next; > if (result == begin) > - break; > + /* We looped around the arena list. We could not find any > + arena that was either not corrupted or not the one we > + wanted to avoid. */ > + return NULL; > } > > - /* We could not find any arena that was either not corrupted or not the one > - we wanted to avoid. */ > - if (result == begin || result == avoid_arena) > - return NULL; > - > /* No arena available without contention. Wait for the next in line. */ > LIBC_PROBE (memory_arena_reuse_wait, 3, &result->mutex, result, avoid_arena); > (void) mutex_lock (&result->mutex); > Thank you very much for working on this bug and getting it checked in. I reviewed the cases again, and I'm just shocked at how utterly _bad_ this code is here. This isn't any one particular persons problem, we've just let this get ugly over the years without some serious logical simplification. I wanted to just note that... 743 reused_arena (mstate avoid_arena) 744 { 745 mstate result; 746 /* FIXME: Access to next_to_use suffers from data races. */ 747 static mstate next_to_use; 748 if (next_to_use == NULL) 749 next_to_use = &main_arena; 750 751 /* Iterate over all arenas (including those linked from 752 free_list). */ 753 result = next_to_use; 754 do 755 { 756 if (!arena_is_corrupt (result) && !mutex_trylock (&result->mutex)) This does not check for 'avoid_arena', which means a failing main_arena allocation will simply be tried again. I believe that's fine because it will fall back to mmap eventually if it's sbrk that's failing. Given that we're on the failing path I don't care that this is non-optimal, and keeping the logic simple is good. But we should probably add a comment (see below). 757 goto out; 758 759 /* FIXME: This is a data race, see _int_new_arena. */ 760 result = result->next; 761 } 762 while (result != next_to_use); I was thinking about adding comments like this to clarify: diff --git a/malloc/arena.c b/malloc/arena.c index 229783f..0750bd6 100644 --- a/malloc/arena.c +++ b/malloc/arena.c @@ -737,7 +737,8 @@ get_free_list (void) /* Lock and return an arena that can be reused for memory allocation. Avoid AVOID_ARENA as we have already failed to allocate memory in - it and it is currently locked. */ + it and it is currently locked. Note that AVOID_ARENA is either + NULL or the main_arena given the logic in arena_get_retry. */ static mstate reused_arena (mstate avoid_arena) { @@ -752,6 +753,11 @@ reused_arena (mstate avoid_arena) result = next_to_use; do { + /* This does not take into account avoid_arena. We don't care about + that case and it would complicate the logic. AVOID_ARENA is either + NULL or &main_arena, in the case of the latter (the only case + where this matters) we might return back &main_arena after just + having failed there, retry, and fall back to sysmalloc. */ if (!arena_is_corrupt (result) && !mutex_trylock (&result->mutex)) goto out; @@ -800,6 +806,11 @@ out: return result; } +/* Get an arena from the free list, or a new one if one isn't free, + or lastly try to reuse an existing arena. Note that AVOID_ARENA + is either NULL or the main_arena because of the logic in + arena_get_retry which is the only place where arena_get2 is called + with a non-NULL avoid_arena. */ static mstate internal_function arena_get2 (size_t size, mstate avoid_arena)