make disjoint pool a C structure #898

bratpiorka · 2024-11-14T13:30:35Z

make Disjoint Pool a C structure. With this PR:

Disjoint Pool code is written in C - NOTE: there are no changes in any algorithm - any optimizations should be done in separate PRs
it is no longer a lib - it is always added to UMF lib sources
UMF_BUILD_LIBUMF_POOL_DISJOINT option is removed
few white-box test cases where we look at internal structures are added - more could be added in separate PRs

src/pool/pool_disjoint.c

bratpiorka · 2024-12-11T14:59:52Z

@lplewa please review :)

ldorau

To be continued ...

ldorau · 2024-12-16T07:47:52Z

src/pool/pool_disjoint_internal.h

+
+// Represents the allocated memory block of size 'slab_min_size'
+// Internally, it splits the memory block into chunks. The number of
+// chunks depends of the size of a Bucket which created the Slab.


ldorau · 2024-12-16T07:50:16Z

src/pool/pool_disjoint_internal.h

+    size_t slab_size;
+
+    // Represents the current state of each chunk: if the bit is set then the
+    // chunk is allocated, and if the chunk is free for allocation otherwise


or the chunk is free ...

ldorau · 2024-12-16T07:52:58Z

src/pool/pool_disjoint_internal.h

+    slab_list_item_t *available_slabs;
+    size_t available_slabs_num;
+
+    // Linked list of slabs with 0 available chunk.


... chunks.

ldorau · 2024-12-16T08:04:01Z

src/pool/pool_disjoint_internal.h

+    // When a slab becomes entirely free we have to decide whether to return it
+    // to the provider or keep it allocated. A simple check for size of the
+    // Available list is not sufficient to check whether any slab has been
+    // pooled yet.We would have to traverse the entire Available listand check


listand -> list and

ldorau · 2024-12-16T08:04:03Z

src/pool/pool_disjoint_internal.h

+    // For buckets used in chunked mode, a counter of slabs in the pool.
+    // For allocations that use an entire slab each, the entries in the Available
+    // list are entries in the pool.Each slab is available for a new
+    // allocation.The size of the Available list is the size of the pool.
+    // For allocations that use slabs in chunked mode, slabs will be in the
+    // Available list if any one or more of their chunks is free.The entire slab
+    // is not necessarily free, just some chunks in the slab are free. To
+    // implement pooling we will allow one slab in the Available list to be
+    // entirely empty. Normally such a slab would have been freed. But
+    // now we don't, and treat this slab as "in the pool".
+    // When a slab becomes entirely free we have to decide whether to return it
+    // to the provider or keep it allocated. A simple check for size of the
+    // Available list is not sufficient to check whether any slab has been
+    // pooled yet.We would have to traverse the entire Available listand check
+    // if any of them is entirely free. Instead we keep a counter of entirely
+    // empty slabs within the Available list to speed up the process of checking
+    // if a slab in this bucket is already pooled.


Please use space after dot .

ldorau · 2024-12-16T08:41:14Z

src/pool/pool_disjoint.c

+}
+
+bool slab_has_avail(const slab_t *slab) {
+    return slab->num_allocated != slab->num_chunks;


return slab->num_allocated < slab->num_chunks;

ldorau · 2024-12-16T08:45:50Z

src/pool/pool_disjoint.c

+    // points to the last byte of slab data
+    void *slab_addr = slab_get(slab);
+    ASSERT_IS_ALIGNED((uintptr_t)slab_addr, bucket_slab_min_size(bucket));
+    LOG_DEBUG("slab: %p, start: %p", (void *)slab, slab_addr);


LOG_DEBUG("registering slab: %p, ...

ldorau · 2024-12-16T08:48:44Z

src/pool/pool_disjoint.c

+    int ret = critnib_insert(slabs, (uintptr_t)slab_addr, slab, 0);
+    if (ret == ENOMEM) {
+        LOG_ERR("register failed because of out of memory!");
+    } else if (ret == EEXIST) {
+        LOG_ERR("register failed because the address is already registered!");
+    }


This function returns void, so the caller does not know if registering failed or succeeded.

ldorau · 2024-12-16T08:49:54Z

src/pool/pool_disjoint.c

+
+    void *slab_addr = slab_get(slab);
+    ASSERT_IS_ALIGNED((uintptr_t)slab_addr, bucket_slab_min_size(bucket));
+    LOG_DEBUG("slab: %p, start: %p", (void *)slab, slab_addr);


LOG_DEBUG("unregistering slab: %p, ...

ldorau · 2024-12-16T08:50:28Z

src/pool/pool_disjoint.c

+    utils_write_lock(lock);
+
+#ifndef NDEBUG
+    // debug only


lukaszstolarczuk · 2024-12-16T13:36:25Z

README.md

@@ -258,10 +257,6 @@ To enable this feature, the `UMF_BUILD_SHARED_LIBRARY` option needs to be turned

 TODO: Add a description


could you pls add some description here?

and also mention that it is (part of libumf) now

lplewa

Not sure about all debug_logs. Imho there are litte too much logs for production code, so maybe we should try to reduce them to maximum 1-2 logs per API call.

lplewa · 2024-12-13T16:40:52Z

src/utils/utils_windows_concurrency.c

-    if (mutex_internal->lock.RecursionCount > 1) {
-        LeaveCriticalSection(&mutex_internal->lock);
+    if (mutex->lock.RecursionCount > 1) {
+        LeaveCriticalSection(&mutex->lock);
        /* deadlock detected */


This is a critical error and it should abort program

lplewa · 2024-12-13T16:45:53Z

src/pool/pool_disjoint.c

+// Temporary solution for disabling memory poisoning. This is needed because
+// AddressSanitizer does not support memory poisoning for GPU allocations.
+// More info: https://github.com/oneapi-src/unified-memory-framework/issues/634
+#ifndef POISON_MEMORY
+#define POISON_MEMORY 0
+#endif
+
+static void annotate_memory_inaccessible(void *ptr, size_t size) {
+    (void)ptr;
+    (void)size;
+#if (POISON_MEMORY != 0)
+    utils_annotate_memory_inaccessible(ptr, size);
+#endif
+}
+
+static void annotate_memory_undefined(void *ptr, size_t size) {
+    (void)ptr;
+    (void)size;
+#if (POISON_MEMORY != 0)
+    utils_annotate_memory_undefined(ptr, size);
+#endif
+}


Suggested change

// Temporary solution for disabling memory poisoning. This is needed because

// AddressSanitizer does not support memory poisoning for GPU allocations.

// More info: https://github.com/oneapi-src/unified-memory-framework/issues/634

#ifndef POISON_MEMORY

#define POISON_MEMORY 0

#endif

static void annotate_memory_inaccessible(void *ptr, size_t size) {

(void)ptr;

(void)size;

#if (POISON_MEMORY != 0)

utils_annotate_memory_inaccessible(ptr, size);

#endif

}

static void annotate_memory_undefined(void *ptr, size_t size) {

(void)ptr;

(void)size;

#if (POISON_MEMORY != 0)

utils_annotate_memory_undefined(ptr, size);

#endif

}

// Temporary solution for disabling memory poisoning. This is needed because

// AddressSanitizer does not support memory poisoning for GPU allocations.

// More info: https://github.com/oneapi-src/unified-memory-framework/issues/634

#ifndef POISON_MEMORY

#undef __SANITIZE_ADDRESS__

#endif

#include "utils_sanitizers.h"

}

lplewa · 2024-12-13T16:49:55Z

src/pool/pool_disjoint.c

+    slab->bucket = bucket;
+
+    slab->iter =
+        (slab_list_item_t *)umf_ba_global_alloc(sizeof(slab_list_item_t));


This is no longer c++. Cast is not needed
sizeof(slab->iter )

lplewa · 2024-12-13T16:50:08Z

src/pool/pool_disjoint.c

+slab_t *create_slab(bucket_t *bucket, bool full_size) {
+    assert(bucket);
+
+    slab_t *slab = umf_ba_global_alloc(sizeof(slab_t));


sizeof(*slab)

lplewa · 2024-12-13T16:51:09Z

src/pool/pool_disjoint.c

+        slab->chunks = umf_ba_global_alloc(sizeof(bool) * slab->num_chunks);
+        if (slab->chunks == NULL) {
+            LOG_ERR("allocation of slab chunks failed!");
+            umf_ba_global_free(slab->iter);


please use goto to handle errors

lplewa · 2024-12-16T16:36:00Z

src/pool/pool_disjoint.c

+#ifndef NDEBUG
+    // debug check
+    bucket_t *bucket = pool->buckets[calculated_idx];
+    assert(bucket->size >= size);
+    (void)bucket;
+
+    if (calculated_idx > 0) {
+        bucket_t *bucket_prev = pool->buckets[calculated_idx - 1];
+        assert(bucket_prev->size < size);
+        (void)bucket_prev;
+    }
+#endif // NDEBUG


If buckets are created on pool creation, do we need to check their sizes and order each time that we are looking for correct one?

lplewa · 2024-12-16T16:43:29Z

src/pool/pool_disjoint.c

+void bucket_print_stats(bucket_t *bucket, bool *title_printed,
+                        const char *label) {
+    if (bucket->alloc_count) {
+        if (!*title_printed) {
+            LOG_DEBUG("\"%s\" pool memory statistics", label);
+            LOG_DEBUG("%14s %12s %12s %18s %20s %21s", "Bucket Size", "Allocs",
+                      "Frees", "Allocs from Pool", "Peak Slabs in Use",
+                      "Peak Slabs in Pool");
+            *title_printed = true;
+        }
+        LOG_DEBUG("%14zu %12zu %12zu %18zu %20zu %21zu", bucket->size,
+                  bucket->alloc_count, bucket->free_count,
+                  bucket->alloc_pool_count, bucket->max_slabs_in_use,
+                  bucket->max_slabs_in_pool);
+    }
+}
+
+void disjoint_pool_print_stats(disjoint_pool_t *pool, bool *title_printed,


Instead of bool *title_printed, create function print_title and just print once, without extra pointer argument passed to all functions.

lplewa · 2024-12-16T16:44:15Z

src/pool/pool_disjoint.c

+
+        disjoint_pool_print_stats(hPool, &title_printed, &high_bucket_size,
+                                  &high_peak_slabs_in_use, name);
+        if (title_printed) {


It will be allways printed, so not sure why we have if here

lplewa · 2024-12-16T16:51:10Z

src/pool/pool_disjoint.c

+    }
+
+    disjoint_pool_t *disjoint_pool =
+        (disjoint_pool_t *)umf_ba_global_alloc(sizeof(struct disjoint_pool_t));


not needed cast

sizeof(*disjoint_pool)

lplewa · 2024-12-16T16:51:46Z

src/pool/pool_disjoint.c

+
+umf_result_t disjoint_pool_initialize(umf_memory_provider_handle_t provider,
+                                      void *params, void **ppPool) {
+    if (!provider) {


check other arguments

PatKamin · 2024-12-19T09:00:23Z

src/pool/pool_disjoint_internal.h

+
+    umf_disjoint_pool_shared_limits_handle_t shared_limits;
+
+    // For buckets used in chunked mode, a counter of slabs in the pool.


Mixing description of chunked slabs with non-chunked brings confusion in this comment. Please separate these cases, i.e. by creating two blocks of text, one for each case.

PatKamin · 2024-12-19T09:06:13Z

src/pool/pool_disjoint_internal.h

+    // Available list if any one or more of their chunks is free.The entire slab
+    // is not necessarily free, just some chunks in the slab are free. To
+    // implement pooling we will allow one slab in the Available list to be
+    // entirely empty. Normally such a slab would have been freed. But


I think it's better to describe what API does, not what it doesn't do, to avoid confusion. Normally(..) sentence could be dropped. Same below before Instead(...).

PatKamin · 2024-12-19T09:11:13Z

src/pool/pool_disjoint_internal.h

+    // Protects the bucket and all the corresponding slabs
+    utils_mutex_t bucket_lock;
+
+    // Reference to the allocator context, used access memory allocation


used to access ?

PatKamin · 2024-12-19T11:50:12Z

src/pool/pool_disjoint.c

+umf_result_t
+umfDisjointPoolParamsDestroy(umf_disjoint_pool_params_handle_t hParams) {
+    // NOTE: dereferencing hParams when BA is already destroyed leads to crash
+    if (hParams && !umf_ba_global_is_destroyed()) {


Shall there be some warning when base allocator is already destroyed?

PatKamin · 2024-12-19T12:04:52Z

src/pool/pool_disjoint.c

+        disjoint_pool_print_stats(hPool, &title_printed, &high_bucket_size,
+                                  &high_peak_slabs_in_use, name);
+        if (title_printed) {
+            LOG_DEBUG("current pool size: %zu",


current -> destroyed ?

PatKamin · 2024-12-19T12:05:26Z

src/pool/pool_disjoint.c

+        if (title_printed) {
+            LOG_DEBUG("current pool size: %zu",
+                      disjoint_pool_get_limits(hPool)->total_size);
+            LOG_DEBUG("suggested setting=;%c%s:%zu,%zu,64K",


If we are destroying a pool, what do we need suggested settings for?

PatKamin · 2024-12-19T12:07:45Z

src/pool/pool_disjoint.c

+        umf_result_t ret =
+            umfMemoryProviderAlloc(pool->provider, size, 0, &ptr);
+        if (ret != UMF_RESULT_SUCCESS) {
+            TLS_last_allocation_error = ret;


Log a failure?

PatKamin · 2024-12-19T12:36:45Z

src/pool/pool_disjoint.c

+    int i = 0;
+    Size1 = ts1;
+    Size2 = ts2;
+    for (; Size2 < CutOff; Size1 *= 2, Size2 *= 2, i += 2) {


The last Size1 bucket won't be created in case 1.5 * Size1 < CutOff even if Size1 < CutOff. This should be either created or clearly commented somewhere that this is the case.

PatKamin · 2024-12-19T12:48:56Z

src/pool/pool_disjoint.c

+
+void *disjoint_pool_malloc(void *pool, size_t size) {
+    disjoint_pool_t *hPool = (disjoint_pool_t *)pool;
+    void *ptr = disjoint_pool_allocate(hPool, size);


Check for pool == NULL

PatKamin · 2024-12-19T12:54:38Z

src/pool/pool_disjoint.c

+        // This allocation will be served from a Bucket which size is multiple
+        // of Alignment and Slab address is aligned to provider_min_page_size
+        // so the address will be properly aligned.
+        aligned_size = (size > 1) ? ALIGN_UP(size, alignment) : alignment;


ALIGN_UP_SAFE?

PatKamin · 2024-12-19T13:07:00Z

src/pool/pool_disjoint.c

+    while (chunk != slab->chunks + slab->num_chunks) {
+        // false means not used
+        if (*chunk == false) {
+            size_t idx = (chunk - slab->chunks) / sizeof(bool);


bool * pointer arithmetic will already provide the index number. Remove the division by sizeof(bool).

bratpiorka requested a review from a team as a code owner November 14, 2024 13:30

lukaszstolarczuk marked this pull request as draft November 14, 2024 13:39

bratpiorka force-pushed the rrudnick_disjoint_c branch 3 times, most recently from 97807f1 to 3521be5 Compare November 18, 2024 10:14

lplewa mentioned this pull request Nov 19, 2024

Helgrind failures on concurrent critnib write and read #905

Open

bratpiorka force-pushed the rrudnick_disjoint_c branch 8 times, most recently from 17d00a2 to 9b22721 Compare November 27, 2024 09:57

github-advanced-security bot found potential problems Nov 27, 2024

View reviewed changes

src/pool/pool_disjoint.c Fixed Show fixed Hide fixed

src/pool/pool_disjoint.c Fixed Show fixed Hide fixed

vinser52 mentioned this pull request Nov 27, 2024

Fix failure with destructor order in CUDA params #940

Merged

3 tasks

bratpiorka force-pushed the rrudnick_disjoint_c branch 14 times, most recently from cdad0f1 to 4a8935a Compare December 6, 2024 15:03

bratpiorka force-pushed the rrudnick_disjoint_c branch 3 times, most recently from 1bf955a to 0fa2440 Compare December 10, 2024 14:07

igchor reviewed Dec 10, 2024

View reviewed changes

src/pool/pool_disjoint.c Show resolved Hide resolved

bratpiorka force-pushed the rrudnick_disjoint_c branch 3 times, most recently from 706aa96 to b2d07ad Compare December 11, 2024 14:22

bratpiorka force-pushed the rrudnick_disjoint_c branch 3 times, most recently from 0d2e129 to c87e492 Compare December 12, 2024 14:07

bratpiorka added 3 commits December 12, 2024 15:11

do nothing in ba_global_free if ba is destroyed

119dd34

add utils min/max functions

ef970b0

add utils_compare_exchange function

68d217d

bratpiorka force-pushed the rrudnick_disjoint_c branch from c87e492 to 5764115 Compare December 12, 2024 14:32

make disjoint pool a C structure

e8bc871

bratpiorka force-pushed the rrudnick_disjoint_c branch from 5764115 to e8bc871 Compare December 12, 2024 14:34

ldorau requested changes Dec 16, 2024

View reviewed changes

lukaszstolarczuk reviewed Dec 16, 2024

View reviewed changes

lplewa requested changes Dec 16, 2024

View reviewed changes

PatKamin reviewed Dec 19, 2024

View reviewed changes

		@@ -258,10 +257,6 @@ To enable this feature, the `UMF_BUILD_SHARED_LIBRARY` option needs to be turned

		TODO: Add a description


		umf_disjoint_pool_shared_limits_handle_t shared_limits;

		// For buckets used in chunked mode, a counter of slabs in the pool.

make disjoint pool a C structure #898

Are you sure you want to change the base?

make disjoint pool a C structure #898

Conversation

bratpiorka commented Nov 14, 2024 • edited Loading

bratpiorka commented Dec 11, 2024

ldorau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lplewa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bratpiorka commented Nov 14, 2024 •

edited

Loading