Re: [PATCH 2/3] slab_arena: Provide slab_arena_madvise_create to madvice slabs

[Cyrill Gorcunov <gorcunov@gmail.com>]

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