Merge pull request #773 from ldorau/Move_free_to_optional_ext_provider_ops

Move free() to optional (ext) provider ops

Author: bratpiorka

examples/custom_file_provider/custom_file_provider.c

@@ -251,11 +251,11 @@ static umf_memory_provider_ops_t file_ops = {
     .initialize = file_init,
     .finalize = file_deinit,
     .alloc = file_alloc,
-    .free = file_free,
     .get_name = file_get_name,
     .get_last_native_error = file_get_last_native_error,
     .get_recommended_page_size = file_get_recommended_page_size,
     .get_min_page_size = file_get_min_page_size,
+    .ext.free = file_free,
 };
 
 // Main function

include/umf/memory_provider_ops.h

@@ -22,6 +22,15 @@ extern "C" {
 /// can keep them NULL.
 ///
 typedef struct umf_memory_provider_ext_ops_t {
+    ///
+    /// @brief Frees the memory space pointed by \p ptr from the memory \p provider
+    /// @param provider pointer to the memory provider
+    /// @param ptr pointer to the allocated memory to free
+    /// @param size size of the allocation
+    /// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure
+    ///
+    umf_result_t (*free)(void *provider, void *ptr, size_t size);
+
     ///
     /// @brief Discard physical pages within the virtual memory mapping associated at the given addr
     ///        and \p size. This call is asynchronous and may delay purging the pages indefinitely.
@@ -172,15 +181,6 @@ typedef struct umf_memory_provider_ops_t {
     umf_result_t (*alloc)(void *provider, size_t size, size_t alignment,
                           void **ptr);
 
-    ///
-    /// @brief Frees the memory space pointed by \p ptr from the memory \p provider
-    /// @param provider pointer to the memory provider
-    /// @param ptr pointer to the allocated memory to free
-    /// @param size size of the allocation
-    /// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure
-    ///
-    umf_result_t (*free)(void *provider, void *ptr, size_t size);
-
     ///
     /// @brief Retrieve string representation of the underlying provider specific
     ///        result reported by the last API that returned

src/cpp_helpers.hpp

@@ -84,7 +84,7 @@ template <typename T> umf_memory_provider_ops_t providerOpsBase() {
     ops.version = UMF_VERSION_CURRENT;
     ops.finalize = [](void *obj) { delete reinterpret_cast<T *>(obj); };
     UMF_ASSIGN_OP(ops, T, alloc, UMF_RESULT_ERROR_UNKNOWN);
-    UMF_ASSIGN_OP(ops, T, free, UMF_RESULT_ERROR_UNKNOWN);
+    UMF_ASSIGN_OP(ops.ext, T, free, UMF_RESULT_ERROR_UNKNOWN);
     UMF_ASSIGN_OP_NORETURN(ops, T, get_last_native_error);
     UMF_ASSIGN_OP(ops, T, get_recommended_page_size, UMF_RESULT_ERROR_UNKNOWN);
     UMF_ASSIGN_OP(ops, T, get_min_page_size, UMF_RESULT_ERROR_UNKNOWN);

src/memory_pool.c

@@ -43,8 +43,7 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
     if (!(flags & UMF_POOL_CREATE_FLAG_DISABLE_TRACKING)) {
         // Wrap provider with memory tracking provider.
         // Check if the provider supports the free() operation.
-        bool upstreamDoesNotFree = (umfMemoryProviderFree(provider, NULL, 0) ==
-                                    UMF_RESULT_ERROR_NOT_SUPPORTED);
+        bool upstreamDoesNotFree = umfIsFreeOpDefault(provider);
         ret = umfTrackingMemoryProviderCreate(provider, pool, &pool->provider,
                                               upstreamDoesNotFree);
         if (ret != UMF_RESULT_SUCCESS) {

src/memory_provider.c

@@ -25,6 +25,13 @@ typedef struct umf_memory_provider_t {
     void *provider_priv;
 } umf_memory_provider_t;
 
+static umf_result_t umfDefaultFree(void *provider, void *ptr, size_t size) {
+    (void)provider;
+    (void)ptr;
+    (void)size;
+    return UMF_RESULT_ERROR_NOT_SUPPORTED;
+}
+
 static umf_result_t umfDefaultPurgeLazy(void *provider, void *ptr,
                                         size_t size) {
     (void)provider;
@@ -99,6 +106,9 @@ static umf_result_t umfDefaultCloseIPCHandle(void *provider, void *ptr,
 }
 
 void assignOpsExtDefaults(umf_memory_provider_ops_t *ops) {
+    if (!ops->ext.free) {
+        ops->ext.free = umfDefaultFree;
+    }
     if (!ops->ext.purge_lazy) {
         ops->ext.purge_lazy = umfDefaultPurgeLazy;
     }
@@ -133,7 +143,7 @@ void assignOpsIpcDefaults(umf_memory_provider_ops_t *ops) {
 
 static bool validateOpsMandatory(const umf_memory_provider_ops_t *ops) {
     // Mandatory ops should be non-NULL
-    return ops->alloc && ops->free && ops->get_recommended_page_size &&
+    return ops->alloc && ops->get_recommended_page_size &&
            ops->get_min_page_size && ops->initialize && ops->finalize &&
            ops->get_last_native_error && ops->get_name;
 }
@@ -159,6 +169,10 @@ static bool validateOps(const umf_memory_provider_ops_t *ops) {
            validateOpsIpc(&(ops->ipc));
 }
 
+bool umfIsFreeOpDefault(umf_memory_provider_handle_t hProvider) {
+    return (hProvider->ops.ext.free == umfDefaultFree);
+}
+
 umf_result_t umfMemoryProviderCreate(const umf_memory_provider_ops_t *ops,
                                      void *params,
                                      umf_memory_provider_handle_t *hProvider) {
@@ -219,7 +233,8 @@ umf_result_t umfMemoryProviderAlloc(umf_memory_provider_handle_t hProvider,
 umf_result_t umfMemoryProviderFree(umf_memory_provider_handle_t hProvider,
                                    void *ptr, size_t size) {
     UMF_CHECK((hProvider != NULL), UMF_RESULT_ERROR_INVALID_ARGUMENT);
-    umf_result_t res = hProvider->ops.free(hProvider->provider_priv, ptr, size);
+    umf_result_t res =
+        hProvider->ops.ext.free(hProvider->provider_priv, ptr, size);
     checkErrorAndSetLastProvider(res, hProvider);
     return res;
 }

src/memory_provider_internal.h

@@ -10,6 +10,8 @@
 #ifndef UMF_MEMORY_PROVIDER_INTERNAL_H
 #define UMF_MEMORY_PROVIDER_INTERNAL_H 1
 
+#include <stdbool.h>
+
 #include <umf/memory_provider.h>
 
 #ifdef __cplusplus
@@ -18,6 +20,7 @@ extern "C" {
 
 void *umfMemoryProviderGetPriv(umf_memory_provider_handle_t hProvider);
 umf_memory_provider_handle_t *umfGetLastFailedMemoryProviderPtr(void);
+bool umfIsFreeOpDefault(umf_memory_provider_handle_t hProvider);
 
 #ifdef __cplusplus
 }

src/provider/provider_cuda.c

@@ -348,11 +348,11 @@ static struct umf_memory_provider_ops_t UMF_CUDA_MEMORY_PROVIDER_OPS = {
     .initialize = cu_memory_provider_initialize,
     .finalize = cu_memory_provider_finalize,
     .alloc = cu_memory_provider_alloc,
-    .free = cu_memory_provider_free,
     .get_last_native_error = cu_memory_provider_get_last_native_error,
     .get_recommended_page_size = cu_memory_provider_get_recommended_page_size,
     .get_min_page_size = cu_memory_provider_get_min_page_size,
     .get_name = cu_memory_provider_get_name,
+    .ext.free = cu_memory_provider_free,
     // TODO
     /*
     .ext.purge_lazy = cu_memory_provider_purge_lazy,

src/provider/provider_devdax_memory.c

@@ -247,15 +247,6 @@ static umf_result_t devdax_alloc(void *provider, size_t size, size_t alignment,
     return UMF_RESULT_SUCCESS;
 }
 
-// free() is not supported
-static umf_result_t devdax_free(void *provider, void *ptr, size_t size) {
-    (void)provider; // unused
-    (void)ptr;      // unused
-    (void)size;     // unused
-
-    return UMF_RESULT_ERROR_NOT_SUPPORTED;
-}
-
 static void devdax_get_last_native_error(void *provider, const char **ppMessage,
                                          int32_t *pError) {
     (void)provider; // unused
@@ -520,7 +511,6 @@ static umf_memory_provider_ops_t UMF_DEVDAX_MEMORY_PROVIDER_OPS = {
     .initialize = devdax_initialize,
     .finalize = devdax_finalize,
     .alloc = devdax_alloc,
-    .free = devdax_free,
     .get_last_native_error = devdax_get_last_native_error,
     .get_recommended_page_size = devdax_get_recommended_page_size,
     .get_min_page_size = devdax_get_min_page_size,

src/provider/provider_file_memory.c

@@ -392,15 +392,6 @@ static umf_result_t file_alloc(void *provider, size_t size, size_t alignment,
     return UMF_RESULT_SUCCESS;
 }
 
-// free() is not supported
-static umf_result_t file_free(void *provider, void *ptr, size_t size) {
-    (void)provider; // unused
-    (void)ptr;      // unused
-    (void)size;     // unused
-
-    return UMF_RESULT_ERROR_NOT_SUPPORTED;
-}
-
 static void file_get_last_native_error(void *provider, const char **ppMessage,
                                        int32_t *pError) {
     (void)provider; // unused
@@ -688,7 +679,6 @@ static umf_memory_provider_ops_t UMF_FILE_MEMORY_PROVIDER_OPS = {
     .initialize = file_initialize,
     .finalize = file_finalize,
     .alloc = file_alloc,
-    .free = file_free,
     .get_last_native_error = file_get_last_native_error,
     .get_recommended_page_size = file_get_recommended_page_size,
     .get_min_page_size = file_get_min_page_size,

src/provider/provider_level_zero.c

@@ -545,11 +545,11 @@ static struct umf_memory_provider_ops_t UMF_LEVEL_ZERO_MEMORY_PROVIDER_OPS = {
     .initialize = ze_memory_provider_initialize,
     .finalize = ze_memory_provider_finalize,
     .alloc = ze_memory_provider_alloc,
-    .free = ze_memory_provider_free,
     .get_last_native_error = ze_memory_provider_get_last_native_error,
     .get_recommended_page_size = ze_memory_provider_get_recommended_page_size,
     .get_min_page_size = ze_memory_provider_get_min_page_size,
     .get_name = ze_memory_provider_get_name,
+    .ext.free = ze_memory_provider_free,
     .ext.purge_lazy = ze_memory_provider_purge_lazy,
     .ext.purge_force = ze_memory_provider_purge_force,
     .ext.allocation_merge = ze_memory_provider_allocation_merge,

src/provider/provider_os_memory.c

@@ -1311,11 +1311,11 @@ static umf_memory_provider_ops_t UMF_OS_MEMORY_PROVIDER_OPS = {
     .initialize = os_initialize,
     .finalize = os_finalize,
     .alloc = os_alloc,
-    .free = os_free,
     .get_last_native_error = os_get_last_native_error,
     .get_recommended_page_size = os_get_recommended_page_size,
     .get_min_page_size = os_get_min_page_size,
     .get_name = os_get_name,
+    .ext.free = os_free,
     .ext.purge_lazy = os_purge_lazy,
     .ext.purge_force = os_purge_force,
     .ext.allocation_merge = os_allocation_merge,

src/provider/provider_tracking.c

@@ -665,11 +665,11 @@ umf_memory_provider_ops_t UMF_TRACKING_MEMORY_PROVIDER_OPS = {
     .initialize = trackingInitialize,
     .finalize = trackingFinalize,
     .alloc = trackingAlloc,
-    .free = trackingFree,
     .get_last_native_error = trackingGetLastError,
     .get_min_page_size = trackingGetMinPageSize,
     .get_recommended_page_size = trackingGetRecommendedPageSize,
     .get_name = trackingName,
+    .ext.free = trackingFree,
     .ext.purge_force = trackingPurgeForce,
     .ext.purge_lazy = trackingPurgeLazy,
     .ext.allocation_split = trackingAllocationSplit,

test/common/provider_null.c

@@ -134,11 +134,11 @@ umf_memory_provider_ops_t UMF_NULL_PROVIDER_OPS = {
     .initialize = nullInitialize,
     .finalize = nullFinalize,
     .alloc = nullAlloc,
-    .free = nullFree,
     .get_last_native_error = nullGetLastError,
     .get_recommended_page_size = nullGetRecommendedPageSize,
     .get_min_page_size = nullGetPageSize,
     .get_name = nullName,
+    .ext.free = nullFree,
     .ext.purge_lazy = nullPurgeLazy,
     .ext.purge_force = nullPurgeForce,
     .ext.allocation_merge = nullAllocationMerge,

test/common/provider_trace.c

@@ -195,11 +195,11 @@ umf_memory_provider_ops_t UMF_TRACE_PROVIDER_OPS = {
     .initialize = traceInitialize,
     .finalize = traceFinalize,
     .alloc = traceAlloc,
-    .free = traceFree,
     .get_last_native_error = traceGetLastError,
     .get_recommended_page_size = traceGetRecommendedPageSize,
     .get_min_page_size = traceGetPageSize,
     .get_name = traceName,
+    .ext.free = traceFree,
     .ext.purge_lazy = tracePurgeLazy,
     .ext.purge_force = tracePurgeForce,
     .ext.allocation_merge = traceAllocationMerge,

test/memoryProviderAPI.cpp

@@ -86,6 +86,19 @@ TEST_F(test, memoryProviderTrace) {
     ASSERT_EQ(calls.size(), ++call_count);
 }
 
+TEST_F(test, memoryProviderOpsNullFreeField) {
+    umf_memory_provider_ops_t provider_ops = UMF_NULL_PROVIDER_OPS;
+    provider_ops.ext.free = nullptr;
+    umf_memory_provider_handle_t hProvider;
+    auto ret = umfMemoryProviderCreate(&provider_ops, nullptr, &hProvider);
+    ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
+
+    ret = umfMemoryProviderFree(hProvider, nullptr, 0);
+    ASSERT_EQ(ret, UMF_RESULT_ERROR_NOT_SUPPORTED);
+
+    umfMemoryProviderDestroy(hProvider);
+}
+
 TEST_F(test, memoryProviderOpsNullPurgeLazyField) {
     umf_memory_provider_ops_t provider_ops = UMF_NULL_PROVIDER_OPS;
     provider_ops.ext.purge_lazy = nullptr;
@@ -155,14 +168,6 @@ TEST_F(test, memoryProviderOpsNullAllocField) {
     ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
 }
 
-TEST_F(test, memoryProviderOpsNullFreeField) {
-    umf_memory_provider_ops_t provider_ops = UMF_NULL_PROVIDER_OPS;
-    provider_ops.free = nullptr;
-    umf_memory_provider_handle_t hProvider;
-    auto ret = umfMemoryProviderCreate(&provider_ops, nullptr, &hProvider);
-    ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
-}
-
 TEST_F(test, memoryProviderOpsNullGetLastNativeErrorField) {
     umf_memory_provider_ops_t provider_ops = UMF_NULL_PROVIDER_OPS;
     provider_ops.get_last_native_error = nullptr;

test/pools/disjoint_pool.cpp

@@ -82,10 +82,7 @@ TEST_F(test, sharedLimits) {
         }
         umf_result_t free(void *ptr, [[maybe_unused]] size_t size) noexcept {
             ::free(ptr);
-            // umfMemoryProviderFree(provider, NULL, 0) is called inside umfPoolCreateInternal()
-            if (ptr != NULL && size != 0) {
-                numFrees++;
-            }
+            numFrees++;
             return UMF_RESULT_SUCCESS;
         }
     };