Re: [Tarantool-patches] [PATCH] quota: add is_enabled field

[Vladislav Shpilevoy <v.shpilevoy@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(&quota, &total, &used);
> +	size_t amount = (total - used) / alloc_size;
> +
> +	int **a = calloc(amount, sizeof(int *));
> +	int i = 0;
> +	while (quota_use(&quota, 1) >= 0) {
> +		quota_disable(&quota);
> +		a[i++] = smalloc(&alloc, alloc_size);
> +		quota_enable(&quota);
> +	}
> +
> +	quota_get_total_and_used(&quota, &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(&quota);
> +	smfree(&alloc, a[0], alloc_size);

5. So you did 'amount' allocations, but freed only the
first one. Why?

> +	quota_enable(&quota);
> +	free(a);
> +
> +	quota_get_total_and_used(&quota, &total, &used);
> +	fail_unless((int)total - (int)used > 0);
> +	footer();
> +}