Tarantool development patches archive
 help / color / mirror / Atom feed
* [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints
@ 2019-05-13 20:01 Cyrill Gorcunov
  2019-05-15 12:37 ` Vladimir Davydov
  0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov @ 2019-05-13 20:01 UTC (permalink / raw)
  To: tml; +Cc: Vladimir Davydov, Alexander Turenko, Cyrill Gorcunov

The purpose of this extension is to be able to eliminate
some of slab arenas from dumping, especially when huge
amount of memory involves.

To keep backward compatibility we map flags passed into
new SLAB_ARENA_x flags. To be able to distinguish old
and new interfaces we reserve high bit of the integer
value and remap flags internally to syscall compatible
form.

Same time I have to modify build script to define
_GNU_SOURCE symbol otherwise madvise bits won't be
found.

Part-of https://github.com/tarantool/tarantool/issues/3509
---
v2:
  - Encode madvise hint inside slab_arena_create flags, for backward compatibility
  - Enhance test to address dynamic slabs
  - Enhance build procedure to test all falgs and functions we're interested in
v3:
  - Keep SLAB_ARENA_ flags inside struct slab_arena
  - Use HAVE keyword in cmake
  - Move madvise into madvise_checked helper
  - Extend comment for SLAB_ARENA_ flags

 .gitignore           |   1 +
 CMakeLists.txt       |  18 ++++++
 small/config.h.cmake |  31 +++++++++++
 small/slab_arena.c   |  68 +++++++++++++++++++----
 small/slab_arena.h   |  23 +++++++-
 test/slab_arena.c    | 128 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 258 insertions(+), 11 deletions(-)
 create mode 100644 small/config.h.cmake

diff --git a/.gitignore b/.gitignore
index e2339c3..c4e4ade 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,3 +17,4 @@ build/
 CTestTestfile.cmake
 Testing
 CTestTestfile.cmake
+small/config.h
diff --git a/CMakeLists.txt b/CMakeLists.txt
index ad27423..f7582a3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,6 +1,9 @@
 project(small C CXX)
 cmake_minimum_required(VERSION 2.8 FATAL_ERROR)
 
+include(CheckFunctionExists)
+include(CheckSymbolExists)
+
 if(NOT CMAKE_BUILD_TYPE)
     set(CMAKE_BUILD_TYPE Debug)
 endif()
@@ -15,10 +18,25 @@ endif()
 # Enable GNU glibc extentions.
 add_definitions("-D_GNU_SOURCE")
 
+set(CMAKE_REQUIRED_DEFINITIONS "-D_GNU_SOURCE")
+
+check_symbol_exists(MAP_ANON sys/mman.h TARANTOOL_SMALL_HAVE_MAP_ANON)
+check_symbol_exists(MAP_ANONYMOUS sys/mman.h TARANTOOL_SMALL_HAVE_MAP_ANONYMOUS)
+
+check_function_exists(madvise TARANTOOL_SMALL_HAVE_MADVISE)
+check_symbol_exists(MADV_DONTDUMP sys/mman.h TARANTOOL_SMALL_HAVE_MADV_DONTDUMP)
+
+configure_file(
+    "small/config.h.cmake"
+    "small/config.h"
+    )
+message (STATUS "")
+
 # Valgrind
 include_directories(third_party)
 
 set(lib_headers
+    small/config.h
     small/ibuf.h
     small/lf_lifo.h
     small/lifo.h
diff --git a/small/config.h.cmake b/small/config.h.cmake
new file mode 100644
index 0000000..989ffd1
--- /dev/null
+++ b/small/config.h.cmake
@@ -0,0 +1,31 @@
+#ifndef TARANTOOL_SMALL_CONFIG_H_INCLUDED
+#define TARANTOOL_SMALL_CONFIG_H_INCLUDED
+
+/*
+ * Check for deprecated MAP_ANON.
+ */
+#cmakedefine TARANTOOL_SMALL_HAVE_MAP_ANON 1
+#cmakedefine TARANTOOL_SMALL_HAVE_MAP_ANONYMOUS 1
+
+#if !defined(TARANTOOL_SMALL_HAVE_MAP_ANONYMOUS) && defined(TARANTOOL_SMALL_HAVE_MAP_ANON)
+/*
+ * MAP_ANON is deprecated, MAP_ANONYMOUS should be used instead.
+ * Unfortunately, it's not universally present (e.g. not present
+ * on FreeBSD.
+ */
+# define MAP_ANONYMOUS MAP_ANON
+#endif
+
+/*
+ * Defined if this platform has madvise(..)
+ * and flags we're interested in.
+ */
+#cmakedefine TARANTOOL_SMALL_HAVE_MADVISE 1
+#cmakedefine TARANTOOL_SMALL_HAVE_MADV_DONTDUMP 1
+
+#if defined(TARANTOOL_SMALL_HAVE_MADVISE)	&& \
+    defined(TARANTOOL_SMALL_HAVE_MADV_DONTDUMP)
+# define TARANTOOL_SMALL_USE_MADVISE 1
+#endif
+
+#endif /* TARANTOOL_SMALL_CONFIG_H_INCLUDED */
diff --git a/small/slab_arena.c b/small/slab_arena.c
index 26e2620..9f4baee 100644
--- a/small/slab_arena.c
+++ b/small/slab_arena.c
@@ -41,17 +41,35 @@
 #include <valgrind/valgrind.h>
 #include <valgrind/memcheck.h>
 
-#if !defined(MAP_ANONYMOUS)
-#define MAP_ANONYMOUS MAP_ANON
+static void
+madvise_checked(void *ptr, size_t size, int flags)
+{
+#ifdef TARANTOOL_SMALL_USE_MADVISE
+	if (ptr && (flags & SLAB_ARENA_DONTDUMP)) {
+		if (madvise(ptr, size, MADV_DONTDUMP)) {
+			char *ignore_it;
+			char buf[64];
+
+			ignore_it = strerror_r(errno, buf, sizeof(buf));
+			(void)ignore_it;
+
+			fprintf(stderr, "Error in madvise(%p, %zu, 0x%x): %s\n",
+				ptr, size, MADV_DONTDUMP, buf);
+		}
+	}
+#else
+	(void)ptr;
+	(void)size;
+	(void)flags;
 #endif
+}
 
 static void
 munmap_checked(void *addr, size_t size)
 {
 	if (munmap(addr, size)) {
 		char buf[64];
-		intptr_t ignore_it = (intptr_t)strerror_r(errno, buf,
-							  sizeof(buf));
+		char *ignore_it = strerror_r(errno, buf, sizeof(buf));
 		(void)ignore_it;
 		fprintf(stderr, "Error in munmap(%p, %zu): %s\n",
 			addr, size, buf);
@@ -66,13 +84,18 @@ mmap_checked(size_t size, size_t align, int flags)
 	assert((align & (align - 1)) == 0);
 	/* The size must be a multiple of alignment */
 	assert((size & (align - 1)) == 0);
+
+	if (flags & SLAB_ARENA_PRIVATE)
+		flags = MAP_PRIVATE | MAP_ANONYMOUS;
+	else
+		flags = MAP_SHARED | MAP_ANONYMOUS;
+
 	/*
 	 * All mappings except the first are likely to
 	 * be aligned already.  Be optimistic by trying
 	 * to map exactly the requested amount.
 	 */
-	void *map = mmap(NULL, size, PROT_READ | PROT_WRITE,
-			 flags | MAP_ANONYMOUS, -1, 0);
+	void *map = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -1, 0);
 	if (map == MAP_FAILED)
 		return NULL;
 	if (((intptr_t) map & (align - 1)) == 0)
@@ -85,8 +108,7 @@ mmap_checked(size_t size, size_t align, int flags)
 	 * fragmentation depending on the kernels allocation
 	 * strategy.
 	 */
-	map = mmap(NULL, size + align, PROT_READ | PROT_WRITE,
-		   flags | MAP_ANONYMOUS, -1, 0);
+	map = mmap(NULL, size + align, PROT_READ | PROT_WRITE, flags, -1, 0);
 	if (map == MAP_FAILED)
 		return NULL;
 
@@ -123,11 +145,31 @@ pow2round(size_t size)
 #define MAX(a, b) ((a) > (b) ? (a) : (b))
 #define MIN(a, b) ((a) < (b) ? (a) : (b))
 
+static void
+slab_arena_init_flags(struct slab_arena *arena, int flags)
+{
+	/*
+	 * Old interface for backward compatibility, MAP_
+	 * flags are passed directly without SLAB_ARENA_FLAG_MARK,
+	 * map them to internal ones.
+	 */
+	if (!(flags & SLAB_ARENA_FLAG_MARK)) {
+		assert(flags & (MAP_PRIVATE | MAP_SHARED));
+		if (flags == MAP_PRIVATE)
+			arena->flags = SLAB_ARENA_PRIVATE;
+		else
+			arena->flags = SLAB_ARENA_SHARED;
+		return;
+	}
+
+	assert(flags & (SLAB_ARENA_PRIVATE | SLAB_ARENA_SHARED));
+	arena->flags = flags;
+}
+
 int
 slab_arena_create(struct slab_arena *arena, struct quota *quota,
 		  size_t prealloc, uint32_t slab_size, int flags)
 {
-	assert(flags & (MAP_PRIVATE | MAP_SHARED));
 	lf_lifo_init(&arena->cache);
 	VALGRIND_MAKE_MEM_DEFINED(&arena->cache, sizeof(struct lf_lifo));
 
@@ -148,7 +190,7 @@ slab_arena_create(struct slab_arena *arena, struct quota *quota,
 
 	arena->used = 0;
 
-	arena->flags = flags;
+	slab_arena_init_flags(arena, flags);
 
 	if (arena->prealloc) {
 		arena->arena = mmap_checked(arena->prealloc,
@@ -157,6 +199,9 @@ slab_arena_create(struct slab_arena *arena, struct quota *quota,
 	} else {
 		arena->arena = NULL;
 	}
+
+	madvise_checked(arena->arena, arena->slab_size, arena->flags);
+
 	return arena->prealloc && !arena->arena ? -1 : 0;
 }
 
@@ -205,6 +250,9 @@ slab_map(struct slab_arena *arena)
 		__sync_sub_and_fetch(&arena->used, arena->slab_size);
 		quota_release(arena->quota, arena->slab_size);
 	}
+
+	madvise_checked(ptr, arena->slab_size, arena->flags);
+
 	VALGRIND_MAKE_MEM_UNDEFINED(ptr, arena->slab_size);
 	return ptr;
 }
diff --git a/small/slab_arena.h b/small/slab_arena.h
index 364252f..2545b00 100644
--- a/small/slab_arena.h
+++ b/small/slab_arena.h
@@ -33,6 +33,7 @@
 #include "lf_lifo.h"
 #include <sys/mman.h>
 #include <limits.h>
+#include "config.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -45,6 +46,26 @@ enum {
 	SMALL_UNLIMITED = SIZE_MAX/2 + 1
 };
 
+/**
+ * Backward compatible flags to be used with slab_arena_create().
+ * Initially we have been passing MAP_SHARED or MAP_PRIVATE flags
+ * only, thus to continue supporting them we need to sign new API
+ * with some predefined value. For this sake we reserve high bit
+ * as a mark which allows us to distinguish system independent
+ * SLAB_ARENA_ flags from anything else.
+ */
+#define SLAB_ARENA_FLAG_MARK	(0x80000000)
+#define SLAB_ARENA_FLAG(x)	((x) | SLAB_ARENA_FLAG_MARK)
+
+enum {
+	/* mmap() flags */
+	SLAB_ARENA_PRIVATE	= SLAB_ARENA_FLAG(1 << 0),
+	SLAB_ARENA_SHARED	= SLAB_ARENA_FLAG(1 << 1),
+
+	/* madvise() flags */
+	SLAB_ARENA_DONTDUMP	= SLAB_ARENA_FLAG(1 << 2)
+};
+
 /**
  * slab_arena -- a source of large aligned blocks of memory.
  * MT-safe.
@@ -94,7 +115,7 @@ struct slab_arena {
 	 */
 	uint32_t slab_size;
 	/**
-	 * mmap() flags: MAP_SHARED or MAP_PRIVATE
+	 * SLAB_ARENA_ flags for mmap() and madvise() calls.
 	 */
 	int flags;
 };
diff --git a/test/slab_arena.c b/test/slab_arena.c
index d0a78d1..a9ea838 100644
--- a/test/slab_arena.c
+++ b/test/slab_arena.c
@@ -2,7 +2,9 @@
 #include <small/quota.h>
 #include <stdio.h>
 #include <limits.h>
+#include <unistd.h>
 #include <stdlib.h>
+#include <string.h>
 #include <time.h>
 #include "unit.h"
 
@@ -15,6 +17,130 @@ slab_arena_print(struct slab_arena *arena)
 	       arena->used, arena->slab_size);
 }
 
+#define is_hex_digit(c)				\
+	(((c) >= '0' && (c) <= '9')	||	\
+	 ((c) >= 'a' && (c) <= 'f')	||	\
+	 ((c) >= 'A' && (c) <= 'F'))
+
+static bool
+is_vma_range_fmt(char *line, unsigned long *start, unsigned long *end)
+{
+	char *p = line;
+	while (*line && is_hex_digit(*line))
+		line++;
+
+	if (*line++ != '-')
+		return false;
+
+	while (*line && is_hex_digit(*line))
+		line++;
+
+	if (*line++ != ' ')
+		return false;
+
+	sscanf(p, "%lx-%lx", start, end);
+	return true;
+}
+
+static bool
+vma_has_flag(unsigned long vm_start, const char *flag)
+{
+	unsigned long start = 0, end = 0;
+	char buf[1024], *tok = NULL;
+	bool found = false;
+	FILE *f = NULL;
+
+	f = fopen("/proc/self/smaps", "r");
+	if (!f) {
+		printf("ERROR: Can't open smaps for %lx\n", vm_start);
+		return false;
+	}
+
+	while (fgets(buf, sizeof(buf), f)) {
+		if (is_vma_range_fmt(buf, &start, &end))
+			continue;
+		if (strncmp(buf, "VmFlags: ", 9) || start != vm_start)
+			continue;
+		tok = buf;
+		break;
+	}
+
+	if (tok) {
+		for (tok = strtok(tok, " \n"); tok;
+		     tok = strtok(NULL, " \n")) {
+			if (strcmp(tok, flag))
+				continue;
+			found = true;
+			break;
+		}
+	}
+
+	fclose(f);
+	return found;
+}
+
+static void
+slab_test_madvise(void)
+{
+	struct slab_arena arena;
+	struct quota quota;
+	void *ptr;
+
+	/*
+	 * The system doesn't support flags fetching.
+	 */
+	if (access("/proc/self/smaps", F_OK))
+		return;
+
+	/*
+	 * If system supports madvise call, test that
+	 * preallocated area has been madvised.
+	 */
+	quota_init(&quota, 2000000);
+	slab_arena_create(&arena, &quota, 3000000, 1, SLAB_ARENA_DONTDUMP);
+
+	/*
+	 * Will fetch from preallocated area.
+	 */
+	ptr = slab_map(&arena);
+	if (!ptr) {
+		printf("ERROR: can't obtain preallocated slab\n");
+		goto out;
+	}
+
+	if (!vma_has_flag((unsigned long)ptr, "dd"))
+		goto no_dd;
+
+	slab_unmap(&arena, ptr);
+	slab_arena_destroy(&arena);
+
+	/*
+	 * A new slab for dynamic allocation.
+	 */
+	quota_init(&quota, 2000000);
+	slab_arena_create(&arena, &quota, 0, 0x10000, SLAB_ARENA_DONTDUMP);
+
+	/*
+	 * Will fetch newly allocated area.
+	 */
+	ptr = slab_map(&arena);
+	if (!ptr) {
+		printf("ERROR: can't obtain dynamic slab\n");
+		goto out;
+	}
+
+	if (!vma_has_flag((unsigned long)ptr, "dd"))
+		goto no_dd;
+
+out:
+	slab_unmap(&arena, ptr);
+	slab_arena_destroy(&arena);
+	return;
+no_dd:
+	printf("ERROR: Expected dd flag on VMA address %p\n", ptr);
+	goto out;
+}
+
 int main()
 {
 	struct quota quota;
@@ -42,4 +168,6 @@ int main()
 	slab_arena_create(&arena, &quota, 3000000, 1, MAP_PRIVATE);
 	slab_arena_print(&arena);
 	slab_arena_destroy(&arena);
+
+	slab_test_madvise();
 }
-- 
2.20.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints
  2019-05-13 20:01 [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints Cyrill Gorcunov
@ 2019-05-15 12:37 ` Vladimir Davydov
  2019-05-15 12:53   ` Cyrill Gorcunov
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2019-05-15 12:37 UTC (permalink / raw)
  To: Cyrill Gorcunov; +Cc: tml, Alexander Turenko

On Mon, May 13, 2019 at 11:01:26PM +0300, Cyrill Gorcunov wrote:
> The purpose of this extension is to be able to eliminate
> some of slab arenas from dumping, especially when huge
> amount of memory involves.
> 
> To keep backward compatibility we map flags passed into
> new SLAB_ARENA_x flags. To be able to distinguish old
> and new interfaces we reserve high bit of the integer
> value and remap flags internally to syscall compatible
> form.
> 
> Same time I have to modify build script to define
> _GNU_SOURCE symbol otherwise madvise bits won't be
> found.
> 
> Part-of https://github.com/tarantool/tarantool/issues/3509
> ---
> v2:
>   - Encode madvise hint inside slab_arena_create flags, for backward compatibility
>   - Enhance test to address dynamic slabs
>   - Enhance build procedure to test all falgs and functions we're interested in
> v3:
>   - Keep SLAB_ARENA_ flags inside struct slab_arena
>   - Use HAVE keyword in cmake
>   - Move madvise into madvise_checked helper
>   - Extend comment for SLAB_ARENA_ flags
> 
>  .gitignore           |   1 +
>  CMakeLists.txt       |  18 ++++++
>  small/config.h.cmake |  31 +++++++++++
>  small/slab_arena.c   |  68 +++++++++++++++++++----
>  small/slab_arena.h   |  23 +++++++-
>  test/slab_arena.c    | 128 +++++++++++++++++++++++++++++++++++++++++++

I'd also patch all places (tests, etc) where old flags (MAP_*) are used
to switch them to new flags. Sooner or later we should drop this compat
layer.

> diff --git a/small/slab_arena.c b/small/slab_arena.c
> index 26e2620..9f4baee 100644
> --- a/small/slab_arena.c
> +++ b/small/slab_arena.c
> @@ -41,17 +41,35 @@
>  #include <valgrind/valgrind.h>
>  #include <valgrind/memcheck.h>
>  
> -#if !defined(MAP_ANONYMOUS)
> -#define MAP_ANONYMOUS MAP_ANON
> +static void
> +madvise_checked(void *ptr, size_t size, int flags)
> +{
> +#ifdef TARANTOOL_SMALL_USE_MADVISE

It would be nice to have a warning in case MADV_DONTDUMP is unavailable.
It should be printed only once. May be, here, or, even better, in
Tarantool itself (via say_warn()).

> +	if (ptr && (flags & SLAB_ARENA_DONTDUMP)) {
> +		if (madvise(ptr, size, MADV_DONTDUMP)) {
> +			char *ignore_it;
> +			char buf[64];
> +
> +			ignore_it = strerror_r(errno, buf, sizeof(buf));
> +			(void)ignore_it;
> +
> +			fprintf(stderr, "Error in madvise(%p, %zu, 0x%x): %s\n",
> +				ptr, size, MADV_DONTDUMP, buf);
> +		}
> +	}
> +#else
> +	(void)ptr;
> +	(void)size;
> +	(void)flags;
>  #endif
> +}
>  
>  static void
>  munmap_checked(void *addr, size_t size)
>  {
>  	if (munmap(addr, size)) {
>  		char buf[64];
> -		intptr_t ignore_it = (intptr_t)strerror_r(errno, buf,
> -							  sizeof(buf));
> +		char *ignore_it = strerror_r(errno, buf, sizeof(buf));

This wouldn't compile on OS X. Please leave it as is. Same's fair for
madvise_checked.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints
  2019-05-15 12:37 ` Vladimir Davydov
@ 2019-05-15 12:53   ` Cyrill Gorcunov
  2019-05-15 13:08     ` Vladimir Davydov
  0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov @ 2019-05-15 12:53 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: tml, Alexander Turenko

On Wed, May 15, 2019 at 03:37:56PM +0300, Vladimir Davydov wrote:
> 
> I'd also patch all places (tests, etc) where old flags (MAP_*) are used
> to switch them to new flags. Sooner or later we should drop this compat
> layer.

Maybe we should do this in a separate patch? I would like the tests to sit
for some time with MAP_ flags to make sure everything works. And I'll prepare
the patch covering MAP_ next week, sounds OK?

> > -#if !defined(MAP_ANONYMOUS)
> > -#define MAP_ANONYMOUS MAP_ANON
> > +static void
> > +madvise_checked(void *ptr, size_t size, int flags)
> > +{
> > +#ifdef TARANTOOL_SMALL_USE_MADVISE
> 
> It would be nice to have a warning in case MADV_DONTDUMP is unavailable.
> It should be printed only once. May be, here, or, even better, in
> Tarantool itself (via say_warn()).

You know I thought if we could use small/conf.h inside tarantool.
For this sake I prefixed symbols with TARANROOL_SMALL_ to not interfere
with any other symbols.

Another option -- provide something like

static const uint64_t small_features = (bitset of features);

bool small_have_feature(uint64_t mask)
{
	return small_features & mask;
}

?

> >  
> >  static void
> >  munmap_checked(void *addr, size_t size)
> >  {
> >  	if (munmap(addr, size)) {
> >  		char buf[64];
> > -		intptr_t ignore_it = (intptr_t)strerror_r(errno, buf,
> > -							  sizeof(buf));
> > +		char *ignore_it = strerror_r(errno, buf, sizeof(buf));
> 
> This wouldn't compile on OS X. Please leave it as is. Same's fair for
> madvise_checked.

The problem is that if I leave it as is it doesn't compile on linux,
due to _GNU_SOURCE definition, which is required for MADV_ flags :/

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints
  2019-05-15 12:53   ` Cyrill Gorcunov
@ 2019-05-15 13:08     ` Vladimir Davydov
  2019-05-15 13:22       ` Cyrill Gorcunov
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2019-05-15 13:08 UTC (permalink / raw)
  To: Cyrill Gorcunov; +Cc: tml, Alexander Turenko

On Wed, May 15, 2019 at 03:53:04PM +0300, Cyrill Gorcunov wrote:
> On Wed, May 15, 2019 at 03:37:56PM +0300, Vladimir Davydov wrote:
> > 
> > I'd also patch all places (tests, etc) where old flags (MAP_*) are used
> > to switch them to new flags. Sooner or later we should drop this compat
> > layer.
> 
> Maybe we should do this in a separate patch? I would like the tests to sit
> for some time with MAP_ flags to make sure everything works. And I'll prepare
> the patch covering MAP_ next week, sounds OK?

Okay.

> 
> > > -#if !defined(MAP_ANONYMOUS)
> > > -#define MAP_ANONYMOUS MAP_ANON
> > > +static void
> > > +madvise_checked(void *ptr, size_t size, int flags)
> > > +{
> > > +#ifdef TARANTOOL_SMALL_USE_MADVISE
> > 
> > It would be nice to have a warning in case MADV_DONTDUMP is unavailable.
> > It should be printed only once. May be, here, or, even better, in
> > Tarantool itself (via say_warn()).
> 
> You know I thought if we could use small/conf.h inside tarantool.
> For this sake I prefixed symbols with TARANROOL_SMALL_ to not interfere
> with any other symbols.
> 
> Another option -- provide something like
> 
> static const uint64_t small_features = (bitset of features);

I would define HAVE_MADV_DONTNEED in tarantool's config.h.

> 
> bool small_have_feature(uint64_t mask)
> {
> 	return small_features & mask;
> }
> 
> ?
> 
> > >  
> > >  static void
> > >  munmap_checked(void *addr, size_t size)
> > >  {
> > >  	if (munmap(addr, size)) {
> > >  		char buf[64];
> > > -		intptr_t ignore_it = (intptr_t)strerror_r(errno, buf,
> > > -							  sizeof(buf));
> > > +		char *ignore_it = strerror_r(errno, buf, sizeof(buf));
> > 
> > This wouldn't compile on OS X. Please leave it as is. Same's fair for
> > madvise_checked.
> 
> The problem is that if I leave it as is it doesn't compile on linux,
> due to _GNU_SOURCE definition, which is required for MADV_ flags :/

It does compile on my box, which runs Linux. Note the cast: it should be
fine to explicitly cast (char *) to intptr_t.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints
  2019-05-15 13:08     ` Vladimir Davydov
@ 2019-05-15 13:22       ` Cyrill Gorcunov
  0 siblings, 0 replies; 5+ messages in thread
From: Cyrill Gorcunov @ 2019-05-15 13:22 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: tml, Alexander Turenko

On Wed, May 15, 2019 at 04:08:12PM +0300, Vladimir Davydov wrote:
> > 
> > You know I thought if we could use small/conf.h inside tarantool.
> > For this sake I prefixed symbols with TARANROOL_SMALL_ to not interfere
> > with any other symbols.
> > 
> > Another option -- provide something like
> > 
> > static const uint64_t small_features = (bitset of features);
> 
> I would define HAVE_MADV_DONTNEED in tarantool's config.h.

This won't work: you may have tarantool compiled on machine
where don't need not supported, but small compiled on a system
where madv is present (note that I'm talking about system libs,
the kernel may have own features enabled).

Also I think the main idea of "small" as a library is to separate
code as much as possible: tarantool should not do any own compile
testing but rather request the library if particular feature is
implemented (either via API or via config.h shipped with library).

Lets talk f2f.

> 
> It does compile on my box, which runs Linux. Note the cast: it should be
> fine to explicitly cast (char *) to intptr_t.

Weird, I'll retry.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-05-15 13:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 20:01 [PATCH v3] slab_arena: Enhance slab_arena_create to support madvise hints Cyrill Gorcunov
2019-05-15 12:37 ` Vladimir Davydov
2019-05-15 12:53   ` Cyrill Gorcunov
2019-05-15 13:08     ` Vladimir Davydov
2019-05-15 13:22       ` Cyrill Gorcunov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox