From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0748545C304 for ; Tue, 15 Dec 2020 02:41:53 +0300 (MSK) References: <20201211153703.7450-1-i.kosarev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <0d06d8f9-680a-efd1-d2a5-fc84b017a44e@tarantool.org> Date: Tue, 15 Dec 2020 00:41:51 +0100 MIME-Version: 1.0 In-Reply-To: <20201211153703.7450-1-i.kosarev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] quota: add is_enabled field List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev , alyapunov@tarantool.org Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! See 5 comments below. On 11.12.2020 16:37, Ilya Kosarev wrote: > By default the quota is enabled. If it is set to false, quota_use will > allow to overuse the total available memory limit. > In case of disabled quota smalloc() allocates (and frees) large slabs > using malloc so that the quota will be able to shrink back after those > slabs are freed. Test introduced. > > Part of tarantool/tarantool#3807 > --- > Branch: https://github.com/tarantool/small/tree/i.kosarev/gh-3807-safe-alloc-on-truncation > Issue: https://github.com/tarantool/tarantool/issues/3807 > > small/quota.h | 21 ++++++++++++++++++++- > small/slab_cache.c | 6 ++++++ > small/slab_cache.h | 3 +++ > small/small.c | 10 ++++++---- > test/small_alloc.c | 32 ++++++++++++++++++++++++++++++++ > 5 files changed, 67 insertions(+), 5 deletions(-) > > diff --git a/small/quota.h b/small/quota.h > index 3d3b4f0..c71ee5d 100644 > --- a/small/quota.h > +++ b/small/quota.h > @@ -57,6 +57,12 @@ struct quota { > * QUOTA_UNIT_SIZE. > */ > uint64_t value; > + /** > + * By default the quota is enabled. If it is set to > + * false, quota_use will allow to overuse the total > + * available memory limit. > + */ > + bool is_enabled; 1. Quota is accessed from many threads. This is why the 'value' is updated using atomic cmpxchg operations. Here it seems you update the quota is_enabled from tx thread, but the flag is used by all threads. Perhaps we could make the flag atomic as well. But I don't know how often is the quota accessed, and if we can add more atomic operations on each use(). I suggest you to ask Alexander. Since he is with us now, he should be able to tell if we are going to the right direction. > }; > > /** > diff --git a/small/slab_cache.c b/small/slab_cache.c > index 2ba56c5..b3b7004 100644 > --- a/small/slab_cache.c > +++ b/small/slab_cache.c > @@ -462,3 +462,9 @@ slab_cache_check(struct slab_cache *cache) > return; > abort(); > } > + > +bool > +slab_cache_quota_enabled(struct slab_cache *cache) 2. For checks we always include 'is' as a part of the name. Also 'cache' can be declared 'const' here. > +{ > + return cache->arena->quota->is_enabled; > +} > diff --git a/small/small.c b/small/small.c > index 48085fb..154a0e9 100644 > --- a/small/small.c > +++ b/small/small.c > @@ -228,7 +228,7 @@ smalloc(struct small_alloc *alloc, size_t size) > struct mempool *pool; > int idx = (size - 1) >> STEP_SIZE_LB; > idx = (idx > (int) alloc->step_pool0_step_count) ? idx - alloc->step_pool0_step_count : 0; > - if (idx < STEP_POOL_MAX) { > + if (idx < STEP_POOL_MAX && slab_cache_quota_enabled(alloc->cache)) { 3. These checks are super expensive - you dereference 3! pointers for each such check. Why do you even needed? Why isn't it enough to patch the quota? > /* Allocate in a stepped pool. */ > pool = &alloc->step_pools[idx]; > assert(size <= pool->objsize && > diff --git a/test/small_alloc.c b/test/small_alloc.c > index 421442a..1563cc9 100644 > --- a/test/small_alloc.c > +++ b/test/small_alloc.c > @@ -128,6 +128,37 @@ small_alloc_large(void) > footer(); > } > > +static void > +small_quota_release(void) > +{ > + header(); > + > + size_t total, used; > + size_t alloc_size = 1000; > + quota_get_total_and_used("a, &total, &used); > + size_t amount = (total - used) / alloc_size; > + > + int **a = calloc(amount, sizeof(int *)); > + int i = 0; > + while (quota_use("a, 1) >= 0) { > + quota_disable("a); > + a[i++] = smalloc(&alloc, alloc_size); > + quota_enable("a); > + } > + > + quota_get_total_and_used("a, &total, &used); > + fail_unless((int)total - (int)used < 0); 4. Why so complex? Why can't you simply write 'total < used'? The same below. > + > + quota_disable("a); > + smfree(&alloc, a[0], alloc_size); 5. So you did 'amount' allocations, but freed only the first one. Why? > + quota_enable("a); > + free(a); > + > + quota_get_total_and_used("a, &total, &used); > + fail_unless((int)total - (int)used > 0); > + footer(); > +}