From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Return-Path: <gorcunov@gmail.com>
Date: Mon, 6 May 2019 14:03:26 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: Re: [PATCH 2/3] slab_arena: Provide slab_arena_madvise_create to
 madvice slabs
Message-ID: <20190506110326.GQ2488@uranus.lan>
References: <20190501155006.14546-1-gorcunov@gmail.com>
 <20190501155006.14546-3-gorcunov@gmail.com>
 <20190506103836.gxvrre4upkga7tej@esperanza>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190506103836.gxvrre4upkga7tej@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 06, 2019 at 01:38:36PM +0300, Vladimir Davydov wrote:
> >  
> >  	if (arena->prealloc) {
> >  		arena->arena = mmap_checked(arena->prealloc,
> > @@ -205,6 +208,12 @@ slab_map(struct slab_arena *arena)
> >  		__sync_sub_and_fetch(&arena->used, arena->slab_size);
> >  		quota_release(arena->quota, arena->slab_size);
> >  	}
> > +
> > +#ifdef TARANTOOL_SMALL_HAS_MADVISE
> > +	if (ptr && pm_atomic_load(&arena->advice) != MADV_NORMAL)
> > +		madvise(ptr, arena->slab_size, arena->advice);
> > +#endif
> > +
> 
> Why use atomic? The arena cannot be used until it's fully initialized.

Because otherwise we have a race: with current api madvise may
be set up in any moment, say one thread setting up madvise and
second thread calls for slab_map. Not a big deal probably but
to pair with atomic write into @advice variable.
...
> > +int
> > +slab_arena_madvise_create(struct slab_arena *arena, int advice)
> 
> I don't see much point in changing madvise settings on the fly.

All this to not break backward compatibility. If we're allowed to
change it, this won't be a problem to adjust the code.

> Let's instead allow to pass DONTDUMP flag at creation time.
> This means we have to combine MAP_ and MADV_ flags. I guess it
> was a bad idea to pass MAP_ flags directly to small_create() in
> the first place. What about introducing small-specific flags like
> 
>   SLAB_ARENA_SHARED
>   SLAB_ARENA_DONTDUMP
> 
> Come to think of it, I assume we don't even need to introduce
> SLAB_ARENA_SHARED - we always pass MAP_PRIVATE to small_create().
> Let's drop it, perhaps?
> 
> It will break backward compatibility, but I don't think it really
> matters, as tarantool/small is only used by tarantool - it isn't
> a big deal if we change its API. We only need to bump the version
> number, I guess.
> 
> Alexander, please amend me if I'm wrong.

OK, I'll rework in a separate branch, so we will jump back if needed.

	Cyrill