Remove the disable_provider_free parameter of jemalloc pool

ldorau · ldorau · commit 326eabd87ae5 · 2024-11-26T09:02:34.000+01:00
Remove the disable_provider_free parameter of jemalloc pool. Replace it with the Coarse provider in the example. Fixes: #904 Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
diff --git a/README.md b/README.md
@@ -186,12 +186,6 @@ Additionally, required for tests:
 A memory provider that provides memory from a device DAX (a character device file /dev/daxX.Y).
 It can be used when large memory mappings are needed.
 
-The DevDax memory provider does not support the free operation
-(`umfMemoryProviderFree()` always returns `UMF_RESULT_ERROR_NOT_SUPPORTED`),
-so it should be used with a pool manager that will take over
-the managing of the provided memory - for example the jemalloc pool
-with the `disable_provider_free` parameter set to true.
-
 ##### Requirements
 
 1) Linux OS
@@ -201,12 +195,6 @@ with the `disable_provider_free` parameter set to true.
 
 A memory provider that provides memory by mapping a regular, extendable file.
 
-The file memory provider does not support the free operation
-(`umfMemoryProviderFree()` always returns `UMF_RESULT_ERROR_NOT_SUPPORTED`),
-so it should be used with a pool manager that will take over
-the managing of the provided memory - for example the jemalloc pool
-with the `disable_provider_free` parameter set to true.
-
 IPC API requires the `UMF_MEM_MAP_SHARED` memory `visibility` mode
 (`UMF_RESULT_ERROR_INVALID_ARGUMENT` is returned otherwise).
 
diff --git a/examples/dram_and_fsdax/dram_and_fsdax.c b/examples/dram_and_fsdax/dram_and_fsdax.c
@@ -61,41 +61,14 @@ static umf_memory_pool_handle_t create_fsdax_pool(const char *path) {
     }
 
     // Create an FSDAX memory pool
-    //
-    // The file memory provider does not support the free operation
-    // (`umfMemoryProviderFree()` always returns `UMF_RESULT_ERROR_NOT_SUPPORTED`),
-    // so it should be used with a pool manager that will take over
-    // the managing of the provided memory - for example the jemalloc pool
-    // with the `disable_provider_free` parameter set to true.
-    umf_jemalloc_pool_params_handle_t pool_params;
-    umf_result = umfJemallocPoolParamsCreate(&pool_params);
-    if (umf_result != UMF_RESULT_SUCCESS) {
-        fprintf(stderr, "Failed to create jemalloc params!\n");
-        umfMemoryProviderDestroy(provider_fsdax);
-        return NULL;
-    }
-    umf_result = umfJemallocPoolParamsSetKeepAllMemory(pool_params, true);
-    if (umf_result != UMF_RESULT_SUCCESS) {
-        fprintf(stderr, "Failed to set KeepAllMemory!\n");
-        umfMemoryProviderDestroy(provider_fsdax);
-        return NULL;
-    }
-
-    // Create an FSDAX memory pool
-    umf_result =
-        umfPoolCreate(umfJemallocPoolOps(), provider_fsdax, pool_params,
-                      UMF_POOL_CREATE_FLAG_OWN_PROVIDER, &pool_fsdax);
+    umf_result = umfPoolCreate(umfJemallocPoolOps(), provider_fsdax, NULL,
+                               UMF_POOL_CREATE_FLAG_OWN_PROVIDER, &pool_fsdax);
     if (umf_result != UMF_RESULT_SUCCESS) {
         fprintf(stderr, "Failed to create an FSDAX memory pool!\n");
         umfMemoryProviderDestroy(provider_fsdax);
         return NULL;
     }
 
-    umf_result = umfJemallocPoolParamsDestroy(pool_params);
-    if (umf_result != UMF_RESULT_SUCCESS) {
-        fprintf(stderr, "Failed to destroy jemalloc params!\n");
-    }
-
     return pool_fsdax;
 }
 
diff --git a/include/umf/pools/pool_jemalloc.h b/include/umf/pools/pool_jemalloc.h
@@ -14,35 +14,8 @@
 extern "C" {
 #endif
 
-#include <stdbool.h>
 #include <umf/memory_pool_ops.h>
 
-struct umf_jemalloc_pool_params_t;
-
-/// @brief handle to the parameters of the jemalloc pool.
-typedef struct umf_jemalloc_pool_params_t *umf_jemalloc_pool_params_handle_t;
-
-/// @brief  Create a struct to store parameters of jemalloc pool.
-/// @param  hParams [out] handle to the newly created parameters struct.
-/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
-umf_result_t
-umfJemallocPoolParamsCreate(umf_jemalloc_pool_params_handle_t *hParams);
-
-/// @brief  Destroy parameters struct.
-/// @param  hParams handle to the parameters of the jemalloc pool.
-/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
-umf_result_t
-umfJemallocPoolParamsDestroy(umf_jemalloc_pool_params_handle_t hParams);
-
-/// @brief  Set if \p umfMemoryProviderFree() should never be called.
-/// @param  hParams handle to the parameters of the jemalloc pool.
-/// @param  keepAllMemory \p true if the jemalloc pool should not call
-/// \p umfMemoryProviderFree, \p false otherwise.
-/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
-umf_result_t
-umfJemallocPoolParamsSetKeepAllMemory(umf_jemalloc_pool_params_handle_t hParams,
-                                      bool keepAllMemory);
-
 umf_memory_pool_ops_t *umfJemallocPoolOps(void);
 
 #ifdef __cplusplus
diff --git a/src/pool/pool_jemalloc.c b/src/pool/pool_jemalloc.c
@@ -37,16 +37,8 @@
 typedef struct jemalloc_memory_pool_t {
     umf_memory_provider_handle_t provider;
     unsigned int arena_index; // index of jemalloc arena
-    // set to true if umfMemoryProviderFree() should never be called
-    bool disable_provider_free;
 } jemalloc_memory_pool_t;
 
-// Configuration of Jemalloc Pool
-typedef struct umf_jemalloc_pool_params_t {
-    /// Set to true if umfMemoryProviderFree() should never be called.
-    bool disable_provider_free;
-} umf_jemalloc_pool_params_t;
-
 static __TLS umf_result_t TLS_last_allocation_error;
 
 static jemalloc_memory_pool_t *pool_by_arena_index[MALLCTL_ARENAS_ALL];
@@ -59,52 +51,6 @@ static jemalloc_memory_pool_t *get_pool_by_arena_index(unsigned arena_ind) {
     return pool_by_arena_index[arena_ind];
 }
 
-umf_result_t
-umfJemallocPoolParamsCreate(umf_jemalloc_pool_params_handle_t *hParams) {
-    if (!hParams) {
-        LOG_ERR("jemalloc pool params handle is NULL");
-        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
-    }
-
-    umf_jemalloc_pool_params_t *params_data =
-        umf_ba_global_alloc(sizeof(*params_data));
-    if (!params_data) {
-        LOG_ERR("cannot allocate memory for jemalloc poolparams");
-        return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
-    }
-
-    params_data->disable_provider_free = false;
-
-    *hParams = (umf_jemalloc_pool_params_handle_t)params_data;
-
-    return UMF_RESULT_SUCCESS;
-}
-
-umf_result_t
-umfJemallocPoolParamsDestroy(umf_jemalloc_pool_params_handle_t hParams) {
-    if (!hParams) {
-        LOG_ERR("jemalloc pool params handle is NULL");
-        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
-    }
-
-    umf_ba_global_free(hParams);
-
-    return UMF_RESULT_SUCCESS;
-}
-
-umf_result_t
-umfJemallocPoolParamsSetKeepAllMemory(umf_jemalloc_pool_params_handle_t hParams,
-                                      bool keepAllMemory) {
-    if (!hParams) {
-        LOG_ERR("jemalloc pool params handle is NULL");
-        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
-    }
-
-    hParams->disable_provider_free = keepAllMemory;
-
-    return UMF_RESULT_SUCCESS;
-}
-
 // arena_extent_alloc - an extent allocation function conforms to the extent_alloc_t type and upon
 // success returns a pointer to size bytes of mapped memory on behalf of arena arena_ind such that
 // the extent's base address is a multiple of alignment, as well as setting *zero to indicate
@@ -134,9 +80,7 @@ static void *arena_extent_alloc(extent_hooks_t *extent_hooks, void *new_addr,
     }
 
     if (new_addr != NULL && ptr != new_addr) {
-        if (!pool->disable_provider_free) {
-            umfMemoryProviderFree(pool->provider, ptr, size);
-        }
+        umfMemoryProviderFree(pool->provider, ptr, size);
         return NULL;
     }
 
@@ -170,10 +114,6 @@ static void arena_extent_destroy(extent_hooks_t *extent_hooks, void *addr,
 
     jemalloc_memory_pool_t *pool = get_pool_by_arena_index(arena_ind);
 
-    if (pool->disable_provider_free) {
-        return;
-    }
-
     umf_result_t ret;
     ret = umfMemoryProviderFree(pool->provider, addr, size);
     if (ret != UMF_RESULT_SUCCESS) {
@@ -196,10 +136,6 @@ static bool arena_extent_dalloc(extent_hooks_t *extent_hooks, void *addr,
 
     jemalloc_memory_pool_t *pool = get_pool_by_arena_index(arena_ind);
 
-    if (pool->disable_provider_free) {
-        return true; // opt-out from deallocation
-    }
-
     umf_result_t ret;
     ret = umfMemoryProviderFree(pool->provider, addr, size);
     if (ret != UMF_RESULT_SUCCESS) {
@@ -452,9 +388,7 @@ static umf_result_t op_initialize(umf_memory_provider_handle_t provider,
                                   void *params, void **out_pool) {
     assert(provider);
     assert(out_pool);
-
-    umf_jemalloc_pool_params_handle_t je_params =
-        (umf_jemalloc_pool_params_handle_t)params;
+    (void)params; // unused
 
     extent_hooks_t *pHooks = &arena_extent_hooks;
     size_t unsigned_size = sizeof(unsigned);
@@ -468,12 +402,6 @@ static umf_result_t op_initialize(umf_memory_provider_handle_t provider,
 
     pool->provider = provider;
 
-    if (je_params) {
-        pool->disable_provider_free = je_params->disable_provider_free;
-    } else {
-        pool->disable_provider_free = false;
-    }
-
     unsigned arena_index;
     err = je_mallctl("arenas.create", (void *)&arena_index, &unsigned_size,
                      NULL, 0);
diff --git a/test/pools/jemalloc_pool.cpp b/test/pools/jemalloc_pool.cpp
@@ -71,31 +71,20 @@ struct umfJemallocPoolParamsTest
     static constexpr umf_memory_provider_ops_t VALIDATOR_PROVIDER_OPS =
         umf::providerMakeCOps<provider_validator, validation_params_t>();
 
-    umfJemallocPoolParamsTest() : expected_params{false}, params(nullptr) {}
-    void SetUp() override {
-        test::SetUp();
-        expected_params.keep_all_memory = this->GetParam();
-        umf_result_t ret = umfJemallocPoolParamsCreate(&params);
-        ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
-        ret = umfJemallocPoolParamsSetKeepAllMemory(
-            params, expected_params.keep_all_memory);
-        ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
-    }
+    umfJemallocPoolParamsTest() {}
+    void SetUp() override { test::SetUp(); }
 
-    void TearDown() override {
-        umfJemallocPoolParamsDestroy(params);
-        test::TearDown();
-    }
+    void TearDown() override { test::TearDown(); }
 
     umf::pool_unique_handle_t makePool() {
         umf_memory_provider_handle_t hProvider = nullptr;
         umf_memory_pool_handle_t hPool = nullptr;
 
-        auto ret = umfMemoryProviderCreate(&VALIDATOR_PROVIDER_OPS,
-                                           &expected_params, &hProvider);
+        auto ret = umfMemoryProviderCreate(&VALIDATOR_PROVIDER_OPS, nullptr,
+                                           &hProvider);
         EXPECT_EQ(ret, UMF_RESULT_SUCCESS);
 
-        ret = umfPoolCreate(umfJemallocPoolOps(), hProvider, params,
+        ret = umfPoolCreate(umfJemallocPoolOps(), hProvider, nullptr,
                             UMF_POOL_CREATE_FLAG_OWN_PROVIDER, &hPool);
         EXPECT_EQ(ret, UMF_RESULT_SUCCESS);
 
@@ -120,40 +109,11 @@ struct umfJemallocPoolParamsTest
             auto ret = umfPoolFree(pool.get(), ptrs[i]);
             ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
         }
-
-        // Now pool can call free during pool destruction
-        expected_params.keep_all_memory = false;
     }
-
-    validation_params_t expected_params;
-    umf_jemalloc_pool_params_handle_t params;
 };
 
 TEST_P(umfJemallocPoolParamsTest, allocFree) { allocFreeFlow(); }
 
-TEST_P(umfJemallocPoolParamsTest, updateParams) {
-    expected_params.keep_all_memory = !expected_params.keep_all_memory;
-    umf_result_t ret = umfJemallocPoolParamsSetKeepAllMemory(
-        params, expected_params.keep_all_memory);
-    ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
-
-    allocFreeFlow();
-}
-
-TEST_P(umfJemallocPoolParamsTest, invalidParams) {
-    umf_result_t ret = umfJemallocPoolParamsCreate(nullptr);
-    ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
-
-    ret = umfJemallocPoolParamsSetKeepAllMemory(nullptr, true);
-    ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
-
-    ret = umfJemallocPoolParamsSetKeepAllMemory(nullptr, false);
-    ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
-
-    ret = umfJemallocPoolParamsDestroy(nullptr);
-    ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
-}
-
 GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(umfJemallocPoolParamsTest);
 
 /* TODO: enable this test after the issue #903 is fixed.
