From patchwork Tue May 7 14:30:49 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Wilco Dijkstra X-Patchwork-Id: 32588 Received: (qmail 125529 invoked by alias); 7 May 2019 14:30:55 -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 125500 invoked by uid 89); 7 May 2019 14:30:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=2950 X-HELO: EUR02-VE1-obe.outbound.protection.outlook.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=LrjNmKIT9rnr+H6Slzub9luZPS5hMQ8kk7cSQDKXXZI=; b=cL7CNNjjcE//x8yL2RIW1TVmCfWHsYUKan0MJVsU5FSFxN/3qL9CFfqzzMjq/C2+AQtU3rZ4goaKLVUXed7GTnDQi+1rcv2+TrH4eR7NVkbcLAOS7qY9DMmXvZnIvn+mPCSCB1vHy3RziHuRElM7xOxn4QeOUjuJ9bbz7SRhhgs= From: Wilco Dijkstra To: Carlos O'Donell , "libc-alpha@sourceware.org" CC: nd , DJ Delorie Subject: Re: [PATCH] Fix tcache count maximum Date: Tue, 7 May 2019 14:30:49 +0000 Message-ID: References: , <81ca9d6c-46ed-7a93-8dd8-a479a24a239b@redhat.com> In-Reply-To: <81ca9d6c-46ed-7a93-8dd8-a479a24a239b@redhat.com> authentication-results: spf=none (sender IP is ) smtp.mailfrom=Wilco.Dijkstra@arm.com; x-ms-oob-tlc-oobclassifiers: OLM:10000; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 MIME-Version: 1.0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED Hi Carlos, > Please create a bug for this. > > This is a publicly visible issue with tcache and tunables. Sure, BZ 24531. > This patch conflates two issues. > > (a) Adding checking to tunable. > > (b) Lifting limit. > > Please split into two bugs. One to fix tunables, another to raise the > tcache chunk caching limit. > If you are going to do (b) and change the sign of counts then you need > to go through and fix other code that expects to possibly have a > negative value. If there is any code that expects it to be negative that's a serious bug... Char is neither signed nor unsigned, the valid range for char is 0..127. 2939   ++(tcache->counts[tc_idx]);         ^^^^^^^^^^^^^^^^^^^^^^^^^^^         assert (tcache->counts[tc_idx] != 0);         See below for discussion. In all cases we already check tcache->counts[tc_idx] < mp_.tcache_count, so there can be no overflow iff mp_.tcache_count is the maximum value of tcache->counts[] entries. So no checks needed. 2947   tcache_entry *e = tcache->entries[tc_idx]; 2948   assert (tc_idx < TCACHE_MAX_BINS); 2949   assert (tcache->counts[tc_idx] > 0);         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^         Always true now if counts is only positive.         Remove? Yes this assert is redundant since we already checked there is a valid entry (or just added several new entries). So this assert can never trigger, it only fails if tcache_put has an overflow bug. 2950   tcache->entries[tc_idx] = e->next; 2951   --(tcache->counts[tc_idx]);         ^^^^^^^^^^^^^^^^^^^^^^^^^^^         May wrap on error, should we check that and assert?         We expect the caller to check for != NULL entry,         indicating at least one entry. It's possible the list         is corrupt and 'e' is pointing to garbage, so an         assert might be good here?         assert (tcache->counts[tc_idx] != MAX_TCACHE_COUNT); No this can't underflow after we fix the overflow bug. > The manual/memory.texi needs updating because you made the > count twice the size, and the rough estimates for size of > tcache should be updated. The manual should also list the > actual limit being imposed. Which size do you mean? I can't find anything in manual/memory.texi refering to tcache. I did update the tunables which incorrectly states there is no limit on glibc.malloc.tcache_count. See updated patch below - this should be simple and safe to backport. Cheers, Wilco [PATCH v2] Fix tcache count maximum (BZ #24531) The tcache counts[] array is a char, which has a very small range and thus may overflow. When setting tcache_count tunable, there is no overflow check. However the tunable must not be larger than the maximum value of the tcache counts[] array, otherwise it can overflow when filling the tcache. Eg. export GLIBC_TUNABLES=glibc.malloc.tcache_count=4096 leads to crashes in benchtests: Running /build/glibc/benchtests/bench-strcoll bench-strcoll: malloc.c:2949: tcache_get: Assertion `tcache->counts[tc_idx] > 0' failed. Aborted ChangeLog: 2019-05-07 Wilco Dijkstra [BZ #24531] * malloc/malloc.c (MAX_TCACHE_COUNT): New define. (tcache_put): Remove redundant assert. (do_set_tcache_count): Only update if count is small enough. * manual/tunables.texi (glibc.malloc.tcache_count): Document max value. Reviewed-by: Carlos O'Donell diff --git a/malloc/malloc.c b/malloc/malloc.c index 0e3d4dd5163f5fa8fb07b71fb7e318e7b10f5cfd..e03a14aabe5d4a1ca28eb5c0865e03606a70e1d6 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -2905,6 +2905,8 @@ typedef struct tcache_perthread_struct tcache_entry *entries[TCACHE_MAX_BINS]; } tcache_perthread_struct; +#define MAX_TCACHE_COUNT 127 /* Maximum value of counts[] entries. */ + static __thread bool tcache_shutting_down = false; static __thread tcache_perthread_struct *tcache = NULL; @@ -2932,7 +2934,6 @@ tcache_get (size_t tc_idx) { tcache_entry *e = tcache->entries[tc_idx]; assert (tc_idx < TCACHE_MAX_BINS); - assert (tcache->counts[tc_idx] > 0); tcache->entries[tc_idx] = e->next; --(tcache->counts[tc_idx]); e->key = NULL; @@ -5098,8 +5099,11 @@ do_set_tcache_max (size_t value) static __always_inline int do_set_tcache_count (size_t value) { - LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count); - mp_.tcache_count = value; + if (value <= MAX_TCACHE_COUNT) + { + LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count); + mp_.tcache_count = value; + } return 1; } diff --git a/manual/tunables.texi b/manual/tunables.texi index 749cabff1b003f20e36f793a268f5f77944aafbb..ae638823a21b9cc7aca3684c8e3067cb8cd287e0 100644 --- a/manual/tunables.texi +++ b/manual/tunables.texi @@ -189,8 +189,8 @@ per-thread cache. The default (and maximum) value is 1032 bytes on @deftp Tunable glibc.malloc.tcache_count The maximum number of chunks of each size to cache. The default is 7. -There is no upper limit, other than available system memory. If set -to zero, the per-thread cache is effectively disabled. +The upper limit is 127. If set to zero, the per-thread cache is effectively +disabled. The approximate maximum overhead of the per-thread cache is thus equal to the number of bins times the chunk count in each bin times the size