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

ldorau

...

src/pool/pool_disjoint_internal.h

src/pool/pool_disjoint.c

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

what change do you suggest?

As i said Abort(). It's critical issue in our library, it should never happen.

Or add error handling to all mutex_locks, but i prefer first one

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)

no, no, no. * slab looks like dereferencing an pointer which is invalid at the time I pass it to sizeof(). I understand this will compile but let's say I got a function that allocates mem and returns void * and I don't know the "name" of the results pointer - what should I then pass to sizeof? passing variable name instead of type in such simple cases looks strange and makes things more complex to understand

If i cannot convince you let me show some references (by searching for them i did not found single one preferring sizeof(type) instead of sizeof(variable)):
1.

The preferred form for passing a size of a struct is the following:
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and introduces an opportunity for a bug when the pointer variable type is changed but the corresponding sizeof that is passed to a memory allocator is not.
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#allocating-memory

I perfer sizeof(variable) over sizeof(type). Consider:
int a1;
float a2;
memset(&a1,0,sizeof(a1));
memset(&a2,0,sizeof(a2));

vs.

int a1;
float a2;
memset(&a1,0,sizeof(int));
memset(&a2,0,sizeof(float));

In the first case, it's easy to verify that the right sizes are being passed to memset. In the second case, you need to constantly review top and bottom sections to make sure you are consistent.
https://softwareengineering.stackexchange.com/questions/201104/sizeof-style-sizeoftype-or-sizeof-variable

Prefer sizeof(varname) to sizeof(type).

Use sizeof(varname) when you take the size of a particular variable. sizeof(varname) will update appropriately if someone changes the variable type either now or later.
https://google.github.io/styleguide/cppguide.html#sizeof

IMO a = malloc(sizeof(int)); is poor, and worse than a = (int *)malloc(sizeof(int));, because you cannot tell just from looking at this line whether or not it is correct.
a = malloc(sizeof(*a)); is obviously correct.
https://www.reddit.com/r/C_Programming/comments/6e008m/sizeof_type_vs_sizeof_variable/

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

src/pool/pool_disjoint_internal.h

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

this is already done in umfPoolMalloc

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).

ldorau

Issues resolved

All issues fixed

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 2 times, most recently from 5764115 to e8bc871 Compare December 12, 2024 14:34

ldorau previously 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

src/pool/pool_disjoint_internal.h Show resolved Hide resolved

PatKamin reviewed Dec 19, 2024

View reviewed changes

bratpiorka force-pushed the rrudnick_disjoint_c branch 7 times, most recently from 6f24a0e to 6237f6c Compare January 14, 2025 17:27

bratpiorka force-pushed the rrudnick_disjoint_c branch from 6237f6c to 64fdb4e Compare January 15, 2025 12:22

bratpiorka added 4 commits January 15, 2025 14:06

do nothing in ba_global_free if ba is destroyed

4a155c1

add utils min/max functions

0e50064

add utils_compare_exchange function

ead0506

make disjoint pool a C structure

26e4737

bratpiorka force-pushed the rrudnick_disjoint_c branch from 64fdb4e to 26e4737 Compare January 15, 2025 13:11

ldorau reviewed Jan 16, 2025

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

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

ldorau left a comment • edited Loading

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

bratpiorka Jan 14, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

bratpiorka commented Nov 14, 2024 •

edited

Loading

ldorau left a comment •

edited

Loading

bratpiorka Jan 14, 2025 •

edited

Loading