Merge pull request #437 from lplewa/numa_list

bratpiorka · web-flow · commit 7df0dffae367 · 2024-04-18T12:34:56.000+02:00
change os memory provider numa config
diff --git a/include/umf/providers/provider_os_memory.h b/include/umf/providers/provider_os_memory.h
@@ -63,12 +63,12 @@ typedef struct umf_os_memory_provider_params_t {
     unsigned protection;
 
     // NUMA config
-    /// Points to a bit mask of nodes containing up to maxnode bits, depending on
-    /// selected numa_mode newly allocated memory will be bound to those nodes
-    unsigned long *nodemask;
-    /// Max number of bits in nodemask
-    unsigned long maxnode;
-    /// Describes how nodemask is interpreted
+    /// ordered list of numa nodes
+    unsigned *numa_list;
+    /// length of numa_list
+    unsigned numa_list_len;
+
+    /// Describes how node list is interpreted
     umf_numa_mode_t numa_mode;
 } umf_os_memory_provider_params_t;
 
@@ -91,8 +91,8 @@ static inline umf_os_memory_provider_params_t
 umfOsMemoryProviderParamsDefault(void) {
     umf_os_memory_provider_params_t params = {
         UMF_PROTECTION_READ | UMF_PROTECTION_WRITE, /* protection */
-        NULL,                                       /* nodemask */
-        0,                                          /* maxnode */
+        NULL,                                       /* numa_list */
+        0,                                          /* numa_list_len */
         UMF_NUMA_MODE_DEFAULT,                      /* numa_mode */
     };
 
diff --git a/src/memory_targets/memory_target_numa.c b/src/memory_targets/memory_target_numa.c
@@ -21,7 +21,7 @@
 #include "topology.h"
 
 struct numa_memory_target_t {
-    size_t physical_id;
+    unsigned physical_id;
 };
 
 static umf_result_t numa_initialize(void *params, void **memTarget) {
@@ -45,77 +45,15 @@ static umf_result_t numa_initialize(void *params, void **memTarget) {
 
 static void numa_finalize(void *memTarget) { umf_ba_global_free(memTarget); }
 
-// sets maxnode and allocates and initializes mask based on provided memory targets
-static umf_result_t
-numa_targets_create_nodemask(struct numa_memory_target_t **targets,
-                             size_t numTargets, unsigned long **mask,
-                             unsigned *maxnode, size_t *mask_size) {
-    assert(targets);
-    assert(mask);
-    assert(maxnode);
-
-    hwloc_bitmap_t bitmap = hwloc_bitmap_alloc();
-    if (!bitmap) {
-        return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
-    }
-
-    for (size_t i = 0; i < numTargets; i++) {
-        if (hwloc_bitmap_set(bitmap, targets[i]->physical_id)) {
-            hwloc_bitmap_free(bitmap);
-            return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
-        }
-    }
-
-    int lastBit = hwloc_bitmap_last(bitmap);
-    if (lastBit == -1) {
-        // no node is set
-        hwloc_bitmap_free(bitmap);
-        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
-    }
-
-    *maxnode = lastBit + 1;
-
-    // Do not use hwloc_bitmap_nr_ulongs due to:
-    // https://github.com/open-mpi/hwloc/issues/429
-    unsigned bits_per_long = sizeof(unsigned long) * 8;
-    int nrUlongs = (lastBit + bits_per_long) / bits_per_long;
-
-    *mask_size = sizeof(unsigned long) * nrUlongs;
-
-    unsigned long *nodemask = umf_ba_global_alloc(*mask_size);
-    if (!nodemask) {
-        hwloc_bitmap_free(bitmap);
-        return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
-    }
-
-    int ret = hwloc_bitmap_to_ulongs(bitmap, nrUlongs, nodemask);
-    hwloc_bitmap_free(bitmap);
-
-    if (ret) {
-        umf_ba_global_free(nodemask);
-        return UMF_RESULT_ERROR_UNKNOWN;
-    }
-
-    *mask = nodemask;
-
-    return UMF_RESULT_SUCCESS;
-}
-
-static enum umf_result_t numa_memory_provider_create_from_memspace(
+static umf_result_t numa_memory_provider_create_from_memspace(
     umf_memspace_handle_t memspace, void **memTargets, size_t numTargets,
     umf_memspace_policy_handle_t policy,
     umf_memory_provider_handle_t *provider) {
-    (void)memspace;
-    (void)numTargets;
     // TODO: apply policy
     (void)policy;
-
     struct numa_memory_target_t **numaTargets =
         (struct numa_memory_target_t **)memTargets;
 
-    unsigned long *nodemask;
-    unsigned maxnode;
-    size_t nodemask_size;
     size_t numNodesProvider;
 
     if (memspace == umfMemspaceHighestCapacityGet()) {
@@ -126,21 +64,31 @@ static enum umf_result_t numa_memory_provider_create_from_memspace(
         numNodesProvider = numTargets;
     }
 
-    umf_result_t ret = numa_targets_create_nodemask(
-        numaTargets, numNodesProvider, &nodemask, &maxnode, &nodemask_size);
-    if (ret != UMF_RESULT_SUCCESS) {
-        return ret;
+    if (numNodesProvider == 0) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
     umf_os_memory_provider_params_t params = umfOsMemoryProviderParamsDefault();
-    params.nodemask = nodemask;
-    params.maxnode = maxnode;
+    params.numa_list =
+        umf_ba_global_alloc(sizeof(*params.numa_list) * numNodesProvider);
+
+    if (!params.numa_list) {
+        return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
+    }
+
+    for (size_t i = 0; i < numNodesProvider; i++) {
+        params.numa_list[i] = numaTargets[i]->physical_id;
+    }
+
+    params.numa_list_len = numNodesProvider;
     params.numa_mode = UMF_NUMA_MODE_BIND;
 
     umf_memory_provider_handle_t numaProvider = NULL;
-    ret = umfMemoryProviderCreate(umfOsMemoryProviderOps(), &params,
-                                  &numaProvider);
-    umf_ba_global_free(nodemask);
+    int ret = umfMemoryProviderCreate(umfOsMemoryProviderOps(), &params,
+                                      &numaProvider);
+
+    umf_ba_global_free(params.numa_list);
+
     if (ret) {
         return ret;
     }
diff --git a/src/memspaces/memspace_host_all.c b/src/memspaces/memspace_host_all.c
@@ -40,7 +40,7 @@ static umf_result_t umfMemspaceHostAllCreate(umf_memspace_handle_t *hMemspace) {
         goto err;
     }
 
-    size_t *nodeIds = umf_ba_global_alloc(nNodes * sizeof(size_t));
+    unsigned *nodeIds = umf_ba_global_alloc(nNodes * sizeof(*nodeIds));
     if (!nodeIds) {
         umf_ret = UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
         goto err;
diff --git a/src/memspaces/memspace_numa.c b/src/memspaces/memspace_numa.c
@@ -15,7 +15,7 @@
 #include "memspace_numa.h"
 
 enum umf_result_t
-umfMemspaceCreateFromNumaArray(size_t *nodeIds, size_t numIds,
+umfMemspaceCreateFromNumaArray(unsigned *nodeIds, size_t numIds,
                                umf_memspace_handle_t *hMemspace) {
     if (!nodeIds || numIds == 0 || !hMemspace) {
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
diff --git a/src/memspaces/memspace_numa.h b/src/memspaces/memspace_numa.h
@@ -1,6 +1,6 @@
 /*
  *
- * Copyright (C) 2023 Intel Corporation
+ * Copyright (C) 2023-2024 Intel Corporation
  *
  * Under the Apache License v2.0 with LLVM Exceptions. See LICENSE.TXT.
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
@@ -24,7 +24,7 @@ extern "C" {
 /// \param hMemspace [out] handle to the newly created memspace
 /// \return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
 ///
-umf_result_t umfMemspaceCreateFromNumaArray(size_t *nodeIds, size_t numIds,
+umf_result_t umfMemspaceCreateFromNumaArray(unsigned *nodeIds, size_t numIds,
                                             umf_memspace_handle_t *hMemspace);
 
 #ifdef __cplusplus
diff --git a/src/provider/provider_os_memory.c b/src/provider/provider_os_memory.c
@@ -81,29 +81,28 @@ static void os_store_last_native_error(int32_t native_error, int errno_value) {
     TLS_last_native_error.errno_value = errno_value;
 }
 
-static umf_result_t nodemask_to_hwloc_nodeset(const unsigned long *nodemask,
-                                              unsigned long maxnode,
+static umf_result_t nodemask_to_hwloc_nodeset(const unsigned *nodelist,
+                                              unsigned long listsize,
                                               hwloc_bitmap_t *out_nodeset) {
     if (out_nodeset == NULL) {
         return UMF_RESULT_ERROR_INVALID_ARGUMENT;
     }
 
-    if (maxnode > UINT_MAX) {
-        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
-    }
-
     *out_nodeset = hwloc_bitmap_alloc();
     if (!*out_nodeset) {
         return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
     }
 
-    if (maxnode == 0 || nodemask == NULL) {
+    if (listsize == 0) {
         return UMF_RESULT_SUCCESS;
     }
 
-    unsigned bits_per_mask = sizeof(unsigned long) * 8;
-    hwloc_bitmap_from_ulongs(
-        *out_nodeset, (maxnode + bits_per_mask - 1) / bits_per_mask, nodemask);
+    for (unsigned long i = 0; i < listsize; i++) {
+        if (hwloc_bitmap_set(*out_nodeset, nodelist[i])) {
+            hwloc_bitmap_free(*out_nodeset);
+            return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
+        }
+    }
 
     return UMF_RESULT_SUCCESS;
 }
@@ -193,7 +192,7 @@ static umf_result_t translate_params(umf_os_memory_provider_params_t *in_params,
     }
 
     // NUMA config
-    int emptyNodeset = (!in_params->maxnode || !in_params->nodemask);
+    int emptyNodeset = in_params->numa_list_len == 0;
     result = translate_numa_mode(in_params->numa_mode, emptyNodeset,
                                  &provider->numa_policy);
     if (result != UMF_RESULT_SUCCESS) {
@@ -203,8 +202,8 @@ static umf_result_t translate_params(umf_os_memory_provider_params_t *in_params,
 
     provider->numa_flags = getHwlocMembindFlags(in_params->numa_mode);
 
-    return nodemask_to_hwloc_nodeset(in_params->nodemask, in_params->maxnode,
-                                     &provider->nodeset);
+    return nodemask_to_hwloc_nodeset(
+        in_params->numa_list, in_params->numa_list_len, &provider->nodeset);
 }
 
 static umf_result_t os_initialize(void *params, void **provider) {
diff --git a/test/memspaces/memspace_helpers.hpp b/test/memspaces/memspace_helpers.hpp
@@ -36,7 +36,7 @@ struct numaNodesTest : ::umf_test::test {
         }
     }
 
-    std::vector<size_t> nodeIds;
+    std::vector<unsigned> nodeIds;
     unsigned long maxNodeId = 0;
 };
 
diff --git a/test/provider_os_memory.cpp b/test/provider_os_memory.cpp
@@ -128,16 +128,16 @@ static void test_alloc_failure(umf_memory_provider_handle_t provider,
 // negative tests for umfMemoryProviderCreate()
 
 static umf_result_t create_os_provider_with_mode(umf_numa_mode_t mode,
-                                                 unsigned long *nodemask,
-                                                 unsigned long maxnode) {
+                                                 unsigned *node_list,
+                                                 unsigned node_list_size) {
     umf_result_t umf_result;
     umf_memory_provider_handle_t os_memory_provider = nullptr;
     umf_os_memory_provider_params_t os_memory_provider_params =
         umfOsMemoryProviderParamsDefault();
 
     os_memory_provider_params.numa_mode = mode;
-    os_memory_provider_params.nodemask = nodemask;
-    os_memory_provider_params.maxnode = maxnode;
+    os_memory_provider_params.numa_list = node_list;
+    os_memory_provider_params.numa_list_len = node_list_size;
 
     umf_result = umfMemoryProviderCreate(umfOsMemoryProviderOps(),
                                          &os_memory_provider_params,
@@ -152,18 +152,18 @@ static umf_result_t create_os_provider_with_mode(umf_numa_mode_t mode,
     return umf_result;
 }
 
-static unsigned long valid_nodemask = 0x1;
-static unsigned long valid_maxnode = 2;
+static unsigned valid_list = 0x1;
+static unsigned long valid_list_len = 1;
 
 TEST_F(test, create_WRONG_NUMA_MODE_DEFAULT) {
-    auto ret = create_os_provider_with_mode(UMF_NUMA_MODE_DEFAULT,
-                                            &valid_nodemask, valid_maxnode);
+    auto ret = create_os_provider_with_mode(UMF_NUMA_MODE_DEFAULT, &valid_list,
+                                            valid_list_len);
     ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
 }
 
 TEST_F(test, create_WRONG_NUMA_MODE_LOCAL) {
-    auto ret = create_os_provider_with_mode(UMF_NUMA_MODE_LOCAL,
-                                            &valid_nodemask, valid_maxnode);
+    auto ret = create_os_provider_with_mode(UMF_NUMA_MODE_LOCAL, &valid_list,
+                                            valid_list_len);
     ASSERT_EQ(ret, UMF_RESULT_ERROR_INVALID_ARGUMENT);
 }
 
diff --git a/test/provider_os_memory_config.cpp b/test/provider_os_memory_config.cpp
@@ -155,8 +155,18 @@ TEST_P(providerConfigTestNumaMode, numa_modes) {
     if (params.numa_mode != UMF_NUMA_MODE_DEFAULT &&
         params.numa_mode != UMF_NUMA_MODE_LOCAL) {
         allowed_nodes = numa_get_mems_allowed();
-        params.nodemask = allowed_nodes->maskp;
-        params.maxnode = allowed_nodes->size;
+        // convert bitmask to array of nodes
+        params.numa_list_len = numa_bitmask_weight(allowed_nodes);
+        params.numa_list = (unsigned *)malloc(params.numa_list_len *
+                                              sizeof(*params.numa_list));
+        ASSERT_NE(params.numa_list, nullptr);
+        unsigned count = 0;
+        for (unsigned i = 0; i < params.numa_list_len; i++) {
+            if (numa_bitmask_isbitset(allowed_nodes, i)) {
+                params.numa_list[count++] = i;
+            }
+        }
+        ASSERT_EQ(count, params.numa_list_len);
     }
 
     create_provider(&params);
@@ -182,9 +192,10 @@ TEST_P(providerConfigTestNumaMode, numa_modes) {
         // MPOL_PREFERRED_* is equivalent to MPOL_LOCAL if no node is set
         if (actual_mode == MPOL_PREFERRED ||
             actual_mode == MPOL_PREFERRED_MANY) {
-            ASSERT_EQ(params.maxnode, 0);
+            ASSERT_EQ(params.numa_list_len, 0);
         } else {
             ASSERT_EQ(actual_mode, MPOL_LOCAL);
         }
     }
+    free(params.numa_list);
 }
diff --git a/test/provider_os_memory_multiple_numa_nodes.cpp b/test/provider_os_memory_multiple_numa_nodes.cpp
@@ -83,12 +83,15 @@ INSTANTIATE_TEST_SUITE_P(
 // available on the system. The available nodes are returned in vector from the
 // get_available_numa_nodes_numbers() function and passed to test as parameters.
 TEST_P(testNumaNodes, checkNumaNodesAllocations) {
-    int numa_node_number = GetParam();
+    int param = GetParam();
+    ASSERT_GE(param, 0);
+    unsigned numa_node_number = param;
     umf_os_memory_provider_params_t os_memory_provider_params =
         UMF_OS_MEMORY_PROVIDER_PARAMS_TEST;
-    os_memory_provider_params.maxnode = numa_node_number + 1;
+
+    os_memory_provider_params.numa_list = &numa_node_number;
     numa_bitmask_setbit(nodemask, numa_node_number);
-    os_memory_provider_params.nodemask = nodemask->maskp;
+    os_memory_provider_params.numa_list_len = 1;
     os_memory_provider_params.numa_mode = UMF_NUMA_MODE_BIND;
     initOsProvider(os_memory_provider_params);
 

Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ static umf_result_t umfMemspaceHostAllCreate(umf_memspace_handle_t *hMemspace) {`
`40`	`40`	`goto err;`
`41`	`41`	`}`
`42`	`42`
`43`		`- size_t nodeIds = umf_ba_global_alloc(nNodes sizeof(size_t));`
	`43`	`+ unsigned nodeIds = umf_ba_global_alloc(nNodes sizeof(*nodeIds));`
`44`	`44`	`if (!nodeIds) {`
`45`	`45`	`umf_ret = UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;`
`46`	`46`	`goto err;`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ struct numaNodesTest : ::umf_test::test {`
`36`	`36`	`}`
`37`	`37`	`}`
`38`	`38`
`39`		`- std::vector<size_t> nodeIds;`
	`39`	`+ std::vector<unsigned> nodeIds;`
`40`	`40`	`unsigned long maxNodeId = 0;`
`41`	`41`	`};`
`42`	`42`