Merge pull request #887 from ldorau/Deprecate_the_UMF_MEM_MAP_SYNC_flag

Remove the `UMF_MEM_MAP_SYNC` flag

Author: ldorau

README.md

@@ -207,10 +207,10 @@ 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` or `UMF_MEM_MAP_SYNC` memory `visibility` mode
+IPC API requires the `UMF_MEM_MAP_SHARED` memory `visibility` mode
 (`UMF_RESULT_ERROR_INVALID_ARGUMENT` is returned otherwise).
 
-The memory visibility mode parameter must be set to `UMF_MEM_MAP_SYNC` in case of FSDAX.
+The memory visibility mode parameter must be set to `UMF_MEM_MAP_SHARED` in case of FSDAX.
 
 ##### Requirements
 

examples/dram_and_fsdax/dram_and_fsdax.c

@@ -50,8 +50,8 @@ static umf_memory_pool_handle_t create_fsdax_pool(const char *path) {
 
     umf_file_memory_provider_params_t params_fsdax =
         umfFileMemoryProviderParamsDefault(path);
-    // FSDAX requires mapping the UMF_MEM_MAP_SYNC flag
-    params_fsdax.visibility = UMF_MEM_MAP_SYNC;
+    // FSDAX requires mapping the UMF_MEM_MAP_SHARED flag
+    params_fsdax.visibility = UMF_MEM_MAP_SHARED;
 
     umf_result = umfMemoryProviderCreate(umfFileMemoryProviderOps(),
                                          &params_fsdax, &provider_fsdax);

include/umf/memory_provider.h

@@ -21,7 +21,6 @@ extern "C" {
 typedef enum umf_memory_visibility_t {
     UMF_MEM_MAP_PRIVATE = 1, ///< private memory mapping
     UMF_MEM_MAP_SHARED,      ///< shared memory mapping (Linux only)
-    UMF_MEM_MAP_SYNC, ///< direct mapping of persistent memory (supported only for files supporting DAX, Linux only)
 } umf_memory_visibility_t;
 
 /// @brief Protection of the memory allocations

src/provider/provider_devdax_memory.c

@@ -139,21 +139,31 @@ static umf_result_t devdax_initialize(void *params, void **provider) {
         goto err_free_devdax_provider;
     }
 
-    unsigned map_sync_flag = 0;
-    utils_translate_mem_visibility_flag(UMF_MEM_MAP_SYNC, &map_sync_flag);
+    bool is_dax = false;
 
-    // mmap /dev/dax with the MAP_SYNC xor MAP_SHARED flag (if MAP_SYNC fails)
-    devdax_provider->base = utils_mmap_file(NULL, devdax_provider->size,
-                                            devdax_provider->protection,
-                                            map_sync_flag, fd, 0 /* offset */);
+    // mmap /dev/dax with the MAP_SYNC
+    devdax_provider->base = utils_mmap_file(
+        NULL, devdax_provider->size, devdax_provider->protection, 0 /* flags */,
+        fd, 0 /* offset */, &is_dax);
     utils_close_fd(fd);
     if (devdax_provider->base == NULL) {
-        LOG_PDEBUG("devdax memory mapping failed (path=%s, size=%zu)",
+        LOG_PDEBUG("mapping the devdax failed (path=%s, size=%zu)",
                    in_params->path, devdax_provider->size);
         ret = UMF_RESULT_ERROR_UNKNOWN;
         goto err_free_devdax_provider;
     }
 
+    if (!is_dax) {
+        LOG_ERR("mapping the devdax with MAP_SYNC failed: %s", in_params->path);
+        ret = UMF_RESULT_ERROR_UNKNOWN;
+
+        if (devdax_provider->base) {
+            utils_munmap(devdax_provider->base, devdax_provider->size);
+        }
+
+        goto err_free_devdax_provider;
+    }
+
     LOG_DEBUG("devdax memory mapped (path=%s, size=%zu, addr=%p)",
               in_params->path, devdax_provider->size, devdax_provider->base);
 
@@ -433,6 +443,8 @@ static umf_result_t devdax_open_ipc_handle(void *provider,
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
+    *ptr = NULL;
+
     devdax_ipc_data_t *devdax_ipc_data = (devdax_ipc_data_t *)providerIpcData;
 
     int fd = utils_devdax_open(devdax_ipc_data->path);
@@ -441,9 +453,6 @@ static umf_result_t devdax_open_ipc_handle(void *provider,
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
-    unsigned map_sync_flag = 0;
-    utils_translate_mem_visibility_flag(UMF_MEM_MAP_SYNC, &map_sync_flag);
-
     // It is just a workaround for case when
     // devdax_alloc() was called with the size argument
     // that is not a multiplier of DEVDAX_PAGE_SIZE_2MB.
@@ -452,34 +461,42 @@ static umf_result_t devdax_open_ipc_handle(void *provider,
     utils_align_ptr_down_size_up((void **)&offset_aligned, &length_aligned,
                                  DEVDAX_PAGE_SIZE_2MB);
 
-    // mmap /dev/dax with the MAP_SYNC xor MAP_SHARED flag (if MAP_SYNC fails)
+    bool is_dax = false;
+
+    // mmap /dev/dax with the MAP_SYNC
     char *addr =
         utils_mmap_file(NULL, length_aligned, devdax_ipc_data->protection,
-                        map_sync_flag, fd, offset_aligned);
+                        0 /* flags */, fd, offset_aligned, &is_dax);
+    (void)utils_close_fd(fd);
     if (addr == NULL) {
-        devdax_store_last_native_error(UMF_DEVDAX_RESULT_ERROR_ALLOC_FAILED,
-                                       errno);
-
         LOG_PERR("devdax mapping failed (path: %s, size: %zu, protection: %i, "
                  "fd: %i, offset: %zu)",
                  devdax_ipc_data->path, length_aligned,
                  devdax_ipc_data->protection, fd, offset_aligned);
 
-        *ptr = NULL;
-        (void)utils_close_fd(fd);
-
+        devdax_store_last_native_error(UMF_DEVDAX_RESULT_ERROR_ALLOC_FAILED,
+                                       errno);
         return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
     }
 
+    if (!is_dax) {
+        LOG_ERR("mapping the devdax with MAP_SYNC failed: %s",
+                devdax_ipc_data->path);
+
+        if (addr) {
+            utils_munmap(addr, length_aligned);
+        }
+
+        return UMF_RESULT_ERROR_UNKNOWN;
+    }
+
     LOG_DEBUG("devdax mapped (path: %s, size: %zu, protection: %i, fd: %i, "
               "offset: %zu) to address %p",
               devdax_ipc_data->path, length_aligned,
               devdax_ipc_data->protection, fd, offset_aligned, addr);
 
     *ptr = addr;
 
-    (void)utils_close_fd(fd);
-
     return UMF_RESULT_SUCCESS;
 }
 

src/provider/provider_file_memory.c

@@ -51,7 +51,7 @@ typedef struct file_memory_provider_t {
     unsigned visibility; // memory visibility mode
     size_t page_size;    // minimum page size
 
-    // IPC is enabled only for UMF_MEM_MAP_SHARED or UMF_MEM_MAP_SYNC visibility
+    // IPC is enabled only for the UMF_MEM_MAP_SHARED visibility
     bool IPC_enabled;
 
     critnib *mmaps; // a critnib map storing mmap mappings (addr, size)
@@ -114,9 +114,8 @@ file_translate_params(umf_file_memory_provider_params_t *in_params,
         return result;
     }
 
-    // IPC is enabled only for UMF_MEM_MAP_SHARED or UMF_MEM_MAP_SYNC visibility
-    provider->IPC_enabled = (in_params->visibility == UMF_MEM_MAP_SHARED ||
-                             in_params->visibility == UMF_MEM_MAP_SYNC);
+    // IPC is enabled only for the UMF_MEM_MAP_SHARED visibility
+    provider->IPC_enabled = (in_params->visibility == UMF_MEM_MAP_SHARED);
 
     return UMF_RESULT_SUCCESS;
 }
@@ -293,8 +292,8 @@ static umf_result_t file_mmap_aligned(file_memory_provider_t *file_provider,
     ASSERT_IS_ALIGNED(extended_size, page_size);
     ASSERT_IS_ALIGNED(aligned_offset_fd, page_size);
 
-    void *ptr =
-        utils_mmap_file(NULL, extended_size, prot, flag, fd, aligned_offset_fd);
+    void *ptr = utils_mmap_file(NULL, extended_size, prot, flag, fd,
+                                aligned_offset_fd, NULL);
     if (ptr == NULL) {
         LOG_PERR("memory mapping failed");
         return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
@@ -594,8 +593,7 @@ static umf_result_t file_get_ipc_handle_size(void *provider, size_t *size) {
 
     file_memory_provider_t *file_provider = (file_memory_provider_t *)provider;
     if (!file_provider->IPC_enabled) {
-        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED nor "
-                "UMF_MEM_MAP_SYNC")
+        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED")
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
@@ -612,8 +610,7 @@ static umf_result_t file_get_ipc_handle(void *provider, const void *ptr,
 
     file_memory_provider_t *file_provider = (file_memory_provider_t *)provider;
     if (!file_provider->IPC_enabled) {
-        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED nor "
-                "UMF_MEM_MAP_SYNC")
+        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED")
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
@@ -643,8 +640,7 @@ static umf_result_t file_put_ipc_handle(void *provider, void *providerIpcData) {
 
     file_memory_provider_t *file_provider = (file_memory_provider_t *)provider;
     if (!file_provider->IPC_enabled) {
-        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED nor "
-                "UMF_MEM_MAP_SYNC")
+        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED")
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
@@ -665,8 +661,7 @@ static umf_result_t file_open_ipc_handle(void *provider, void *providerIpcData,
 
     file_memory_provider_t *file_provider = (file_memory_provider_t *)provider;
     if (!file_provider->IPC_enabled) {
-        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED nor "
-                "UMF_MEM_MAP_SYNC")
+        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED")
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
@@ -683,7 +678,7 @@ static umf_result_t file_open_ipc_handle(void *provider, void *providerIpcData,
 
     char *addr = utils_mmap_file(
         NULL, file_ipc_data->size, file_ipc_data->protection,
-        file_ipc_data->visibility, fd, file_ipc_data->offset_fd);
+        file_ipc_data->visibility, fd, file_ipc_data->offset_fd, NULL);
     (void)utils_close_fd(fd);
     if (addr == NULL) {
         file_store_last_native_error(UMF_FILE_RESULT_ERROR_ALLOC_FAILED, errno);
@@ -712,8 +707,7 @@ static umf_result_t file_close_ipc_handle(void *provider, void *ptr,
 
     file_memory_provider_t *file_provider = (file_memory_provider_t *)provider;
     if (!file_provider->IPC_enabled) {
-        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED nor "
-                "UMF_MEM_MAP_SYNC")
+        LOG_ERR("memory visibility mode is not UMF_MEM_MAP_SHARED")
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 

src/utils/utils_common.h

@@ -11,6 +11,7 @@
 #define UMF_COMMON_H 1
 
 #include <assert.h>
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ -136,7 +137,7 @@ void *utils_mmap(void *hint_addr, size_t length, int prot, int flag, int fd,
                  size_t fd_offset);
 
 void *utils_mmap_file(void *hint_addr, size_t length, int prot, int flags,
-                      int fd, size_t fd_offset);
+                      int fd, size_t fd_offset, bool *map_sync);
 
 int utils_munmap(void *addr, size_t length);
 

src/utils/utils_linux_common.c

@@ -9,6 +9,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <stdbool.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
@@ -31,78 +32,78 @@ utils_translate_mem_visibility_flag(umf_memory_visibility_t in_flag,
     case UMF_MEM_MAP_SHARED:
         *out_flag = MAP_SHARED;
         return UMF_RESULT_SUCCESS;
-    case UMF_MEM_MAP_SYNC:
-        *out_flag = MAP_SYNC;
-        return UMF_RESULT_SUCCESS;
     }
     return UMF_RESULT_ERROR_INVALID_ARGUMENT;
 }
 
 /*
  * Map given file into memory.
- * If (flags & MAP_PRIVATE) it uses just mmap. Otherwise, if (flags & MAP_SYNC)
- * it tries to mmap with (flags | MAP_SHARED_VALIDATE | MAP_SYNC)
- * which allows flushing from the user-space. If MAP_SYNC fails and the user
- * did not specify it by himself it tries to mmap with (flags | MAP_SHARED).
+ * If (flags & MAP_PRIVATE) it uses just mmap. Otherwise, it tries to mmap
+ * with (flags | MAP_SHARED_VALIDATE | MAP_SYNC) which allows flushing
+ * from the user-space. If MAP_SYNC fails and if the user did not specify
+ * this flag by himself, it falls back to the mmap with (flags | MAP_SHARED).
  */
 void *utils_mmap_file(void *hint_addr, size_t length, int prot, int flags,
-                      int fd, size_t fd_offset) {
+                      int fd, size_t fd_offset, bool *map_sync) {
     void *addr;
 
+    if (map_sync) {
+        *map_sync = false;
+    }
+
     /*
      * MAP_PRIVATE and MAP_SHARED are mutually exclusive,
      * therefore mmap with MAP_PRIVATE is executed separately.
      */
     if (flags & MAP_PRIVATE) {
         addr = utils_mmap(hint_addr, length, prot, flags, fd, fd_offset);
-        if (addr == MAP_FAILED) {
+        if (addr == NULL) {
             LOG_PERR("mapping file with the MAP_PRIVATE flag failed (fd=%i, "
-                     "offset=%zu, length=%zu)",
-                     fd, fd_offset, length);
+                     "offset=%zu, length=%zu, flags=%i)",
+                     fd, fd_offset, length, flags);
             return NULL;
         }
 
         LOG_DEBUG("file mapped with the MAP_PRIVATE flag (fd=%i, offset=%zu, "
-                  "length=%zu)",
-                  fd, fd_offset, length);
+                  "length=%zu, flags=%i)",
+                  fd, fd_offset, length, flags);
 
         return addr;
     }
 
     errno = 0;
 
-    if (flags & MAP_SYNC) {
-        /* try to mmap with MAP_SYNC flag */
-        const int sync_flags = MAP_SHARED_VALIDATE | MAP_SYNC;
-        addr = utils_mmap(hint_addr, length, prot, flags | sync_flags, fd,
-                          fd_offset);
-        if (addr) {
-            LOG_DEBUG("file mapped with the MAP_SYNC flag (fd=%i, offset=%zu, "
-                      "length=%zu)",
-                      fd, fd_offset, length);
-            return addr;
+    /* try to mmap with MAP_SYNC flag */
+    const int sync_flags = flags | MAP_SHARED_VALIDATE | MAP_SYNC;
+    addr = utils_mmap(hint_addr, length, prot, sync_flags, fd, fd_offset);
+    if (addr) {
+        LOG_DEBUG("file mapped with the MAP_SYNC flag (fd=%i, offset=%zu, "
+                  "length=%zu, flags=%i)",
+                  fd, fd_offset, length, sync_flags);
+        if (map_sync) {
+            *map_sync = true;
         }
-
-        LOG_PERR("mapping file with the MAP_SYNC flag failed (fd=%i, "
-                 "offset=%zu, length=%zu)",
-                 fd, fd_offset, length);
+        return addr;
     }
 
-    if ((!(flags & MAP_SYNC)) || errno == EINVAL || errno == ENOTSUP ||
-        errno == EOPNOTSUPP) {
-        /* try to mmap with MAP_SHARED flag (without MAP_SYNC) */
-        const int shared_flags = (flags & (~MAP_SYNC)) | MAP_SHARED;
+    LOG_PERR("mapping file with the MAP_SYNC flag failed (fd=%i, offset=%zu, "
+             "length=%zu, flags=%i)",
+             fd, fd_offset, length, sync_flags);
+
+    /* try to mmap with MAP_SHARED flag (without MAP_SYNC) */
+    if (errno == EINVAL || errno == ENOTSUP || errno == EOPNOTSUPP) {
+        const int shared_flags = flags | MAP_SHARED;
         addr = utils_mmap(hint_addr, length, prot, shared_flags, fd, fd_offset);
         if (addr) {
             LOG_DEBUG("file mapped with the MAP_SHARED flag (fd=%i, "
-                      "offset=%zu, length=%zu)",
-                      fd, fd_offset, length);
+                      "offset=%zu, length=%zu, flags=%i)",
+                      fd, fd_offset, length, shared_flags);
             return addr;
         }
 
         LOG_PERR("mapping file with the MAP_SHARED flag failed (fd=%i, "
-                 "offset=%zu, length=%zu)",
-                 fd, fd_offset, length);
+                 "offset=%zu, length=%zu, flags=%i)",
+                 fd, fd_offset, length, shared_flags);
     }
 
     return NULL;