From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Return-Path: <gorcunov@gmail.com>
Date: Mon, 13 May 2019 13:33:55 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: Re: [PATCH] box/memtx: Allow to skip tuple memory from coredump
Message-ID: <20190513103355.GC2544@uranus.lan>
References: <20190507172611.8085-1-gorcunov@gmail.com>
 <20190513102121.q5twlgfemojneiqm@esperanza>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190513102121.q5twlgfemojneiqm@esperanza>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: tml <tarantool-patches@freelists.org>, Alexander Turenko <alexander.turenko@tarantool.org>
List-ID: <tarantool-patches.dev.tarantool.org>

On Mon, May 13, 2019 at 01:21:21PM +0300, Vladimir Davydov wrote:
> [Cc += Alexander]
> 
> I don't quite like the name: memtx_memory_dontdump. We might want to
> extend it to work on vinyl memory as well one day - I don't think we'd
> want to introduce another cfg parameter for that. What about strip_core?

I don't mind.

> 
> Alexander, what do you think about the new configuration parameter name?
> Do you have a better name in mind?
> 
> Cyrill, please see a few comments inline.
> 
> 
> Please write a doc bot request in the end of the commit message
> (after Fixes ...). Take a look at this for a reference:
> 
> https://github.com/tarantool/tarantool/commit/22db9c264c9119d19a3130e6ad8e7fded0e16c6d

Thanks, will do.

> >  7 files changed, 19 insertions(+), 9 deletions(-)
> 
> Please run all tests. I think box/cfg will break after this.

Will take a look.

> >  static void
> >  engine_init()
> >  {
> > +	uint32_t slab_flags = SLAB_ARENA_PRIVATE;
> > +	if (cfg_geti("memtx_memory_dontdump"))
> > +		slab_flags |= SLAB_ARENA_DONTDUMP;
> > +
> 
> Interpreting dontdump option here and converting it to slab_flags kinda
> breaks encapsulation. I'd pass cfg parameter to memtx_engine_new and let
> it interpret it.

ok

> > @@ -54,7 +54,7 @@ vy_mem_env_create(struct vy_mem_env *env, size_t memory)
> >  	/* Vinyl memory is limited by vy_quota. */
> >  	quota_init(&env->quota, QUOTA_MAX);
> >  	tuple_arena_create(&env->arena, &env->quota, memory,
> > -			   SLAB_SIZE, "vinyl");
> > +			   SLAB_SIZE, SLAB_ARENA_PRIVATE, "vinyl");
> 
> I think it's safe to skip dumping this arena, too.

ok


	Cyrill