From 5044dfc319156ef6883acdddf95d8833f8738b66 Mon Sep 17 00:00:00 2001 From: ranshid Date: Wed, 13 Nov 2024 19:48:39 +0000 Subject: [PATCH 1/8] compile time option to force activedefrag Introduce compile time option to force activedefrag to run even when jemalloc is not used as the allocator. This is in order to be able to run tests with defrag enabled while using memory instrumantation tools. Signed-off-by: ranshid --- src/Makefile | 6 ++++++ src/defrag.c | 17 +++++++++++++++++ src/zmalloc.c | 16 ++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/src/Makefile b/src/Makefile index 020b70d6d5..0cce5b14fe 100644 --- a/src/Makefile +++ b/src/Makefile @@ -143,6 +143,12 @@ FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(SERVER_CFLAGS) ifeq ($(SERVER_TEST),yes) FINAL_CFLAGS +=-DSERVER_TEST=1 endif + +# Special case of forcing defrag to run even though we have no Jemlloc support +ifeq ($(FORCE_DEFRAG), yes) + FINAL_CFLAGS +=-DHAVE_DEFRAG -DFORCE_DEFRAG +endif + FINAL_LDFLAGS=$(LDFLAGS) $(OPT) $(SERVER_LDFLAGS) $(DEBUG) FINAL_LIBS=-lm DEBUG=-g -ggdb diff --git a/src/defrag.c b/src/defrag.c index 4d34009f8b..abb7cb8507 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -755,6 +755,15 @@ void defragScanCallback(void *privdata, const dictEntry *de) { * or not, a false detection can cause the defragmenter to waste a lot of CPU * without the possibility of getting any results. */ float getAllocatorFragmentation(size_t *out_frag_bytes) { + /* In case we are forcing defrag to run without Jemalloc support we cannot get any + * good statistics from the allocator regarding extarnal fragmentation. + * This is why we are forcing the report to reflect fragmented system conditions based on the existing configurations. */ +#if !defined(USE_JEMALLOC) && defined(FORCE_DEFRAG) + + *out_frag_bytes = server.active_defrag_ignore_bytes+1; + return server.active_defrag_threshold_upper; +#else + size_t resident, active, allocated, frag_smallbins_bytes; zmalloc_get_allocator_info(&allocated, &active, &resident, NULL, NULL, &frag_smallbins_bytes); @@ -769,6 +778,7 @@ float getAllocatorFragmentation(size_t *out_frag_bytes) { serverLog(LL_DEBUG, "allocated=%zu, active=%zu, resident=%zu, frag=%.2f%% (%.2f%% rss), frag_bytes=%zu (%zu rss)", allocated, active, resident, frag_pct, rss_pct, frag_smallbins_bytes, rss_bytes); return frag_pct; +#endif } /* Defrag scan callback for the pubsub dictionary. */ @@ -1134,6 +1144,13 @@ void activeDefragCycle(void) { } } +#if !(defined(USE_JEMALLOC) && defined(JEMALLOC_FRAG_HINT)) +int je_get_defrag_hint(void *ptr) { + UNUSED(ptr); + return 1; +} +#endif + #else /* HAVE_DEFRAG */ void activeDefragCycle(void) { diff --git a/src/zmalloc.c b/src/zmalloc.c index e18fa8bac2..296e8f5279 100644 --- a/src/zmalloc.c +++ b/src/zmalloc.c @@ -211,6 +211,7 @@ void *zmalloc_usable(size_t size, size_t *usable) { * and go straight to the allocator arena bins. * Currently implemented only for jemalloc. Used for online defragmentation. */ #ifdef HAVE_DEFRAG +#if defined(USE_JEMALLOC) void *zmalloc_no_tcache(size_t size) { if (size >= SIZE_MAX / 2) zmalloc_oom_handler(size); void *ptr = mallocx(size + PREFIX_SIZE, MALLOCX_TCACHE_NONE); @@ -224,6 +225,21 @@ void zfree_no_tcache(void *ptr) { update_zmalloc_stat_free(zmalloc_size(ptr)); dallocx(ptr, MALLOCX_TCACHE_NONE); } +#else +void *zmalloc_no_tcache(size_t size) { + if (size >= SIZE_MAX / 2) zmalloc_oom_handler(size); + void *ptr = malloc(size + PREFIX_SIZE); + if (!ptr) zmalloc_oom_handler(size); + update_zmalloc_stat_alloc(zmalloc_size(ptr)); + return ptr; +} + +void zfree_no_tcache(void *ptr) { + if (ptr == NULL) return; + update_zmalloc_stat_free(zmalloc_size(ptr)); + free(ptr); +} +#endif #endif /* Try allocating memory and zero it, and return NULL if failed. From c62beb2478f3124eda911c9bccd38ff4dbd98340 Mon Sep 17 00:00:00 2001 From: ranshid Date: Wed, 13 Nov 2024 19:48:39 +0000 Subject: [PATCH 2/8] compile time option to force activedefrag Introduce compile time option to force activedefrag to run even when jemalloc is not used as the allocator. This is in order to be able to run tests with defrag enabled while using memory instrumantation tools. Signed-off-by: ranshid --- src/Makefile | 6 ++++++ src/defrag.c | 17 +++++++++++++++++ src/zmalloc.c | 16 ++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/src/Makefile b/src/Makefile index ae2de1c626..1aae983c50 100644 --- a/src/Makefile +++ b/src/Makefile @@ -134,6 +134,12 @@ FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(SERVER_CFLAGS) ifeq ($(SERVER_TEST),yes) FINAL_CFLAGS +=-DSERVER_TEST=1 endif + +# Special case of forcing defrag to run even though we have no Jemlloc support +ifeq ($(FORCE_DEFRAG), yes) + FINAL_CFLAGS +=-DHAVE_DEFRAG -DFORCE_DEFRAG +endif + FINAL_LDFLAGS=$(LDFLAGS) $(OPT) $(SERVER_LDFLAGS) $(DEBUG) FINAL_LIBS=-lm DEBUG=-g -ggdb diff --git a/src/defrag.c b/src/defrag.c index 4d34009f8b..abb7cb8507 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -755,6 +755,15 @@ void defragScanCallback(void *privdata, const dictEntry *de) { * or not, a false detection can cause the defragmenter to waste a lot of CPU * without the possibility of getting any results. */ float getAllocatorFragmentation(size_t *out_frag_bytes) { + /* In case we are forcing defrag to run without Jemalloc support we cannot get any + * good statistics from the allocator regarding extarnal fragmentation. + * This is why we are forcing the report to reflect fragmented system conditions based on the existing configurations. */ +#if !defined(USE_JEMALLOC) && defined(FORCE_DEFRAG) + + *out_frag_bytes = server.active_defrag_ignore_bytes+1; + return server.active_defrag_threshold_upper; +#else + size_t resident, active, allocated, frag_smallbins_bytes; zmalloc_get_allocator_info(&allocated, &active, &resident, NULL, NULL, &frag_smallbins_bytes); @@ -769,6 +778,7 @@ float getAllocatorFragmentation(size_t *out_frag_bytes) { serverLog(LL_DEBUG, "allocated=%zu, active=%zu, resident=%zu, frag=%.2f%% (%.2f%% rss), frag_bytes=%zu (%zu rss)", allocated, active, resident, frag_pct, rss_pct, frag_smallbins_bytes, rss_bytes); return frag_pct; +#endif } /* Defrag scan callback for the pubsub dictionary. */ @@ -1134,6 +1144,13 @@ void activeDefragCycle(void) { } } +#if !(defined(USE_JEMALLOC) && defined(JEMALLOC_FRAG_HINT)) +int je_get_defrag_hint(void *ptr) { + UNUSED(ptr); + return 1; +} +#endif + #else /* HAVE_DEFRAG */ void activeDefragCycle(void) { diff --git a/src/zmalloc.c b/src/zmalloc.c index e18fa8bac2..296e8f5279 100644 --- a/src/zmalloc.c +++ b/src/zmalloc.c @@ -211,6 +211,7 @@ void *zmalloc_usable(size_t size, size_t *usable) { * and go straight to the allocator arena bins. * Currently implemented only for jemalloc. Used for online defragmentation. */ #ifdef HAVE_DEFRAG +#if defined(USE_JEMALLOC) void *zmalloc_no_tcache(size_t size) { if (size >= SIZE_MAX / 2) zmalloc_oom_handler(size); void *ptr = mallocx(size + PREFIX_SIZE, MALLOCX_TCACHE_NONE); @@ -224,6 +225,21 @@ void zfree_no_tcache(void *ptr) { update_zmalloc_stat_free(zmalloc_size(ptr)); dallocx(ptr, MALLOCX_TCACHE_NONE); } +#else +void *zmalloc_no_tcache(size_t size) { + if (size >= SIZE_MAX / 2) zmalloc_oom_handler(size); + void *ptr = malloc(size + PREFIX_SIZE); + if (!ptr) zmalloc_oom_handler(size); + update_zmalloc_stat_alloc(zmalloc_size(ptr)); + return ptr; +} + +void zfree_no_tcache(void *ptr) { + if (ptr == NULL) return; + update_zmalloc_stat_free(zmalloc_size(ptr)); + free(ptr); +} +#endif #endif /* Try allocating memory and zero it, and return NULL if failed. From 31a04dca52d02b7f1298b95a61ece9d0941b2f00 Mon Sep 17 00:00:00 2001 From: ranshid Date: Thu, 14 Nov 2024 06:46:33 +0000 Subject: [PATCH 3/8] Add Cmake support Signed-off-by: ranshid --- cmake/Modules/ValkeySetup.cmake | 6 ++++++ deps/CMakeLists.txt | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/cmake/Modules/ValkeySetup.cmake b/cmake/Modules/ValkeySetup.cmake index e935c3b308..4e341033a8 100644 --- a/cmake/Modules/ValkeySetup.cmake +++ b/cmake/Modules/ValkeySetup.cmake @@ -168,6 +168,11 @@ endif () message(STATUS "Using ${MALLOC_LIB}") +# DFERAG Override +if (FORCE_DEFRAG) + add_valkey_server_compiler_options("-DHAVE_DEFRAG -DFORCE_DEFRAG") +endif () + # TLS support if (BUILD_TLS) valkey_parse_build_option(${BUILD_TLS} USE_TLS) @@ -379,3 +384,4 @@ unset(BUILD_MALLOC CACHE) unset(USE_JEMALLOC CACHE) unset(BUILD_TLS_MODULE CACHE) unset(BUILD_TLS_BUILTIN CACHE) +unset(FORCE_DEFRAG CACHE) diff --git a/deps/CMakeLists.txt b/deps/CMakeLists.txt index c904b94031..3f5b04dc22 100644 --- a/deps/CMakeLists.txt +++ b/deps/CMakeLists.txt @@ -1,4 +1,6 @@ -add_subdirectory(jemalloc) +if (USE_JEMALLOC) + add_subdirectory(jemalloc) +endif () add_subdirectory(lua) # Set hiredis options. We need to disable the defaults set in the OPTION(..) we do this by setting them in the CACHE From a42e565b73fa2c18f4302cec8f99780b8b0b68dd Mon Sep 17 00:00:00 2001 From: ranshid Date: Thu, 14 Nov 2024 10:17:03 +0000 Subject: [PATCH 4/8] Make defrag always active when forced Signed-off-by: ranshid --- src/defrag.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/defrag.c b/src/defrag.c index abb7cb8507..c32d033dec 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -966,7 +966,7 @@ void activeDefragCycle(void) { mstime_t latency; int all_stages_finished = 0; int quit = 0; - +#if !defined(FORCE_DEFRAG) if (!server.active_defrag_enabled) { if (server.active_defrag_running) { /* if active defrag was disabled mid-run, start from fresh next time. */ @@ -985,7 +985,10 @@ void activeDefragCycle(void) { } return; } - +#else + /* Avoid compiler warning */ + if (0) goto update_metrics; +#endif if (hasActiveChildProcess()) return; /* Defragging memory while there's a fork will just do damage. */ /* Once a second, check if the fragmentation justfies starting a scan From 1f26c5d79e32939f62591baca2cec547be314dfd Mon Sep 17 00:00:00 2001 From: ranshid Date: Thu, 14 Nov 2024 10:18:01 +0000 Subject: [PATCH 5/8] Simplify the FORCE_DEFRAG Cmake logic Signed-off-by: ranshid --- CMakeLists.txt | 3 ++- cmake/Modules/ValkeySetup.cmake | 6 ------ src/CMakeLists.txt | 6 ++++++ 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ad0bab8896..d08e943e59 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.20) +cmake_minimum_required(VERSION 3.17) # Must be done first if (APPLE) @@ -41,3 +41,4 @@ unset(BUILD_UNIT_TESTS CACHE) unset(BUILD_TEST_MODULES CACHE) unset(BUILD_EXAMPLE_MODULES CACHE) unset(USE_TLS CACHE) +unset(FORCE_DEFRAG CACHE) diff --git a/cmake/Modules/ValkeySetup.cmake b/cmake/Modules/ValkeySetup.cmake index 4e341033a8..e935c3b308 100644 --- a/cmake/Modules/ValkeySetup.cmake +++ b/cmake/Modules/ValkeySetup.cmake @@ -168,11 +168,6 @@ endif () message(STATUS "Using ${MALLOC_LIB}") -# DFERAG Override -if (FORCE_DEFRAG) - add_valkey_server_compiler_options("-DHAVE_DEFRAG -DFORCE_DEFRAG") -endif () - # TLS support if (BUILD_TLS) valkey_parse_build_option(${BUILD_TLS} USE_TLS) @@ -384,4 +379,3 @@ unset(BUILD_MALLOC CACHE) unset(USE_JEMALLOC CACHE) unset(BUILD_TLS_MODULE CACHE) unset(BUILD_TLS_BUILTIN CACHE) -unset(FORCE_DEFRAG CACHE) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b7e328163b..c167aeb9a7 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -22,6 +22,12 @@ if (VALKEY_RELEASE_BUILD) set_property(TARGET valkey-server PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE) endif () +if (FORCE_DEFRAG) + message(STATUS "Forcing Active Defrag run on valkey-server") + target_compile_definitions(valkey-server PRIVATE FORCE_DEFRAG) + target_compile_definitions(valkey-server PRIVATE HAVE_DEFRAG) +endif () + # Target: valkey-cli list(APPEND CLI_LIBS "linenoise") valkey_build_and_install_bin(valkey-cli "${VALKEY_CLI_SRCS}" "${VALKEY_SERVER_LDFLAGS}" "${CLI_LIBS}" "redis-cli") From f36ee0893b3dbd87a9f426e8b324fdbeaa7f5062 Mon Sep 17 00:00:00 2001 From: ranshid Date: Thu, 14 Nov 2024 10:43:16 +0000 Subject: [PATCH 6/8] order and fix some uneeded changes Signed-off-by: ranshid --- CMakeLists.txt | 2 +- src/defrag.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d08e943e59..00016f66c8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.17) +cmake_minimum_required(VERSION 3.20) # Must be done first if (APPLE) diff --git a/src/defrag.c b/src/defrag.c index c32d033dec..1b9c7ab448 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -758,7 +758,7 @@ float getAllocatorFragmentation(size_t *out_frag_bytes) { /* In case we are forcing defrag to run without Jemalloc support we cannot get any * good statistics from the allocator regarding extarnal fragmentation. * This is why we are forcing the report to reflect fragmented system conditions based on the existing configurations. */ -#if !defined(USE_JEMALLOC) && defined(FORCE_DEFRAG) +#if defined(FORCE_DEFRAG) || !defined(USE_JEMALLOC) *out_frag_bytes = server.active_defrag_ignore_bytes+1; return server.active_defrag_threshold_upper; @@ -1147,7 +1147,7 @@ void activeDefragCycle(void) { } } -#if !(defined(USE_JEMALLOC) && defined(JEMALLOC_FRAG_HINT)) +#if defined(FORCE_DEFRAG) || !defined(JEMALLOC_FRAG_HINT) int je_get_defrag_hint(void *ptr) { UNUSED(ptr); return 1; From 27f42902728a23e35f11e010bf5ca6e99259d681 Mon Sep 17 00:00:00 2001 From: ranshid Date: Thu, 14 Nov 2024 11:40:51 +0000 Subject: [PATCH 7/8] make changes to SERVER_FLAGS Signed-off-by: ranshid --- src/Makefile | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Makefile b/src/Makefile index 1aae983c50..cd19a36252 100644 --- a/src/Makefile +++ b/src/Makefile @@ -130,16 +130,15 @@ ifdef REDIS_LDFLAGS SERVER_LDFLAGS := $(REDIS_LDFLAGS) endif -FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(SERVER_CFLAGS) -ifeq ($(SERVER_TEST),yes) - FINAL_CFLAGS +=-DSERVER_TEST=1 -endif - # Special case of forcing defrag to run even though we have no Jemlloc support ifeq ($(FORCE_DEFRAG), yes) - FINAL_CFLAGS +=-DHAVE_DEFRAG -DFORCE_DEFRAG + SERVER_CFLAGS +=-DHAVE_DEFRAG -DFORCE_DEFRAG endif +FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(SERVER_CFLAGS) +ifeq ($(SERVER_TEST),yes) + FINAL_CFLAGS +=-DSERVER_TEST=1 +endif FINAL_LDFLAGS=$(LDFLAGS) $(OPT) $(SERVER_LDFLAGS) $(DEBUG) FINAL_LIBS=-lm DEBUG=-g -ggdb From 4643d97a0241995256cfcad34eab310175522846 Mon Sep 17 00:00:00 2001 From: Ran Shidlansik Date: Thu, 14 Nov 2024 14:01:39 +0200 Subject: [PATCH 8/8] fix spell and format issues Signed-off-by: Ran Shidlansik --- src/defrag.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/defrag.c b/src/defrag.c index 1b9c7ab448..3c39f046ef 100644 --- a/src/defrag.c +++ b/src/defrag.c @@ -755,13 +755,13 @@ void defragScanCallback(void *privdata, const dictEntry *de) { * or not, a false detection can cause the defragmenter to waste a lot of CPU * without the possibility of getting any results. */ float getAllocatorFragmentation(size_t *out_frag_bytes) { - /* In case we are forcing defrag to run without Jemalloc support we cannot get any - * good statistics from the allocator regarding extarnal fragmentation. + /* In case we are forcing defrag to run without Jemalloc support we cannot get any + * good statistics from the allocator regarding external fragmentation. * This is why we are forcing the report to reflect fragmented system conditions based on the existing configurations. */ #if defined(FORCE_DEFRAG) || !defined(USE_JEMALLOC) - - *out_frag_bytes = server.active_defrag_ignore_bytes+1; - return server.active_defrag_threshold_upper; + + *out_frag_bytes = server.active_defrag_ignore_bytes + 1; + return server.active_defrag_threshold_upper; #else size_t resident, active, allocated, frag_smallbins_bytes;