From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Ilya Kosarev <i.kosarev@tarantool.org>, alyapunov@tarantool.org Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] quota: add is_enabled field Date: Tue, 15 Dec 2020 00:41:51 +0100 [thread overview] Message-ID: <0d06d8f9-680a-efd1-d2a5-fc84b017a44e@tarantool.org> (raw) In-Reply-To: <20201211153703.7450-1-i.kosarev@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(); > +}
next prev parent reply other threads:[~2020-12-14 23:41 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-11 15:37 Ilya Kosarev 2020-12-14 23:41 ` Vladislav Shpilevoy [this message] -- strict thread matches above, loose matches on Subject: below -- 2020-02-14 19:31 Ilya Kosarev 2020-02-14 22:48 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0d06d8f9-680a-efd1-d2a5-fc84b017a44e@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=alyapunov@tarantool.org \ --cc=i.kosarev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] quota: add is_enabled field' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox