Add threshold to proxy lib to call system allocator #883

ldorau · 2024-11-08T13:17:20Z

Description

Add threshold to proxy lib to call system allocator
when a size is less than the given threshold (Linux only yet).

Ref: #894

Requires:

enable tracker in ProxyLib by default #761

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

bratpiorka · 2024-11-12T11:57:27Z

.github/workflows/reusable_proxy_lib.yml

+          UMF_PROXY="size.threshold=128"
+          UMF_LOG="level:debug;flush:debug;output:stderr;pid:yes"
+          LD_PRELOAD=./lib/libumf_proxy.so
+          ctest --output-on-failure -E provider_file_memory_ipc


we should have a test where you use umfPoolByPtr() to check allocs smaller than threshold were not registered in the tracker

I will add a test for that

@bratpiorka tests added
Done

@bratpiorka ping

src/proxy_lib/proxy_lib.c

src/libumf.c

lplewa · 2024-11-08T15:10:46Z

src/proxy_lib/proxy_lib.c

-    if (Proxy_pool) {
+    if (!was_called_from_umfPool && Proxy_pool &&
+        (umfPoolByPtr(ptr) == Proxy_pool)) {
        was_called_from_umfPool = 1;
        void *new_ptr = umfPoolRealloc(Proxy_pool, ptr, size);
        was_called_from_umfPool = 0;
        return new_ptr;
    }

-    assert(0);
+    if (threshold_value) {
+        return system_realloc(ptr, size);
+    }
+


Please add test
ptr = Malloc(threshold_value + 1)
realloc(ptr, threshold_value - 1)

and vice versa.
ptr = Malloc(threshold_value - 1)
realloc(ptr, threshold_value + 1)

Both cases check then umfPoolByPtr if it returns expected value.

(atm in this PR we are not supporting this correctly - if we increasing or decrease size of allocation thru realoc we should "realloc this allocation" to correct allocator.

@lplewa Tests added.
Done

@lplewa If you would like to change an allocator in realloc() depending on a size of an allocation, where do you get from the size of the current allocation coming from glibc ? You have to know the original size to do memcopy() inside realloc() ...

.github/workflows/reusable_proxy_lib.yml

lukaszstolarczuk · 2024-11-14T10:03:50Z

src/libumf.c

+        if (utils_is_running_in_proxy_lib_with_size_threshold()) {
+            // We cannot destroy the TRACKER nor the base allocator
+            // when we are running in the proxy library with a size threshold,
+            // because it could lead to calling the system free() with an invalid pointer


I meant "It could lead to something", so "calling ..." , not "call ..."

I believe lead to should be followed by either something or to <verb>
you could write it somehow differently e.g. it could result in calling, or lead to system free call with ...

that's my understanding, but you know... I'm not native ;)

I'd re-phrase with it could result in calling ... and a segfault.

Replaced with it could result in calling ...
Done.

now the last part as a result is redundant.

test/CMakeLists.txt

src/proxy_lib/proxy_lib.c

lukaszstolarczuk · 2024-11-14T12:53:45Z

src/proxy_lib/proxy_lib.c

+        *end = '\0';
+    }
+
+    size_t int_threshold = (size_t)atoi(str_threshold);


won't coverity complain about int -> size_t conversion?

Can you check that? I cannot. We will see ...

heh, ok, let me re-phrase the question - should we worry if the user pass size.threshold=-1 (e.g. by mistake)

Fixed.
Done.

src/proxy_lib/proxy_lib.c

test/test_proxy_lib.cpp

lukaszstolarczuk · 2024-11-14T13:27:52Z

src/proxy_lib/proxy_lib.c

+        // replace ';' with '\0' to mark end of the string
+        *end = '\0';
+    }
+


we could have a test for checking this parsing

I would have to move this function to utils, it cannot be tested here as a part of proxy lib.

we could have a script or part of CI, that will run multiple configurations, like:

UMF_PROXY="size.threshold=XYZ;<other var>" LD_PRELOAD=./lib/libumf_proxy.so ctest --output-on-failure UMF_PROXY="<other var>;size.threshold=ABC" LD_PRELOAD=./lib/libumf_proxy.so ctest --output-on-failure etc...

we could have a script or part of CI, that will run multiple configurations, like:

UMF_PROXY="size.threshold=XYZ;<other var>" LD_PRELOAD=./lib/libumf_proxy.so ctest --output-on-failure UMF_PROXY="<other var>;size.threshold=ABC" LD_PRELOAD=./lib/libumf_proxy.so ctest --output-on-failure etc...

I am afraid it would take too much time ...

lukaszstolarczuk · 2024-11-14T13:28:49Z

test/test_proxy_lib.cpp

+#endif
+}
+
+TEST_F(test, proxyLib_size_threshold_malloc) {


we could have a test with multiple allocs, not just 2

What would it change?

we would have more tests 😄

Only 2 are needed in tests of the size threshold: one for the default system allocator (size < threshold) and another one for the proxy library pool UMF pool allocator (size >= threshold). There is no need for an extra one.

test/test_proxy_lib.cpp

Signed-off-by: Lukasz Dorau <[email protected]>

Do not assert(ptr) in umfMemoryTrackerGetAllocInfo(), return UMF_RESULT_ERROR_INVALID_ARGUMENT instead. Replace LOG_WARN() with LOG_DEBUG().

Add utils_env_var_get_str() to utils_common. Use utils_env_var_get_str() inside utils_env_var_has_str() and utils_is_running_in_proxy_lib(). Signed-off-by: Lukasz Dorau <[email protected]>

Signed-off-by: Lukasz Dorau <[email protected]>

Add a size threshold to proxy lib to call system allocator when the size is less than the given threshold (Linux only yet). Signed-off-by: Lukasz Dorau <[email protected]>

This WA for the issue: oneapi-src#894 It protects us from a recursion in malloc_usable_size() when the JEMALLOC proxy_lib_pool is used. TODO: remove this WA when the issue is fixed. Signed-off-by: Lukasz Dorau <[email protected]>

The proxyLib_size_threshold_* tests test the size threshold of the proxy library (Linux only yet). The size threshold is set to 64 bytes in this test, so all allocations of: 1) size < 64 go through the default system allocator and (umfPoolByPtr(ptr_size < 64) == nullptr) 2) size >= 64 go through the proxy lib allocator and (umfPoolByPtr(ptr_size >= 64) != nullptr). Ref: oneapi-src#894 Signed-off-by: Lukasz Dorau <[email protected]>

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch 10 times, most recently from 8bfade2 to 2a5fb9e Compare November 12, 2024 08:47

ldorau changed the title ~~[WIP] Add threshold to proxy lib to call system allocator~~ Add threshold to proxy lib to call system allocator Nov 12, 2024

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch from 2a5fb9e to 4cdba02 Compare November 12, 2024 08:57

ldorau marked this pull request as ready for review November 12, 2024 08:57

ldorau requested a review from a team as a code owner November 12, 2024 08:57

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch 2 times, most recently from 0d3f0c7 to 7c781ef Compare November 12, 2024 11:34

ldorau requested review from bratpiorka and KFilipek November 12, 2024 11:37

bratpiorka reviewed Nov 12, 2024

View reviewed changes

bratpiorka requested changes Nov 12, 2024

View reviewed changes

src/proxy_lib/proxy_lib.c Outdated Show resolved Hide resolved

src/libumf.c Show resolved Hide resolved

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch from 7c781ef to db760b2 Compare November 12, 2024 13:32

lplewa mentioned this pull request Nov 12, 2024

jemalloc_pool should not require user to link with jemalloc #891

Open

ldorau mentioned this pull request Nov 13, 2024

malloc_usable_size() segfaults under the proxy library based on the jemalloc pool #894

Open

lplewa requested changes Nov 13, 2024

View reviewed changes

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch from db760b2 to 4ac6b3e Compare November 14, 2024 09:02

ldorau requested review from bratpiorka and lplewa November 14, 2024 09:06

lukaszstolarczuk reviewed Nov 14, 2024

View reviewed changes

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch from 4ac6b3e to 846f30b Compare November 14, 2024 15:14

ldorau requested a review from lukaszstolarczuk November 14, 2024 15:16

ldorau added 6 commits November 14, 2024 17:15

Fix format string

8cbe831

Signed-off-by: Lukasz Dorau <[email protected]>

Fix checks in umfMemoryProviderAllocationSplit/Merge()

f074fc1

Signed-off-by: Lukasz Dorau <[email protected]>

Do not assert(ptr) in umfMemoryTrackerGetAllocInfo()

c99b876

Do not assert(ptr) in umfMemoryTrackerGetAllocInfo(), return UMF_RESULT_ERROR_INVALID_ARGUMENT instead. Replace LOG_WARN() with LOG_DEBUG().

Add utils_env_var_get_str() to utils_common

cf64635

Add utils_env_var_get_str() to utils_common. Use utils_env_var_get_str() inside utils_env_var_has_str() and utils_is_running_in_proxy_lib(). Signed-off-by: Lukasz Dorau <[email protected]>

Do not destroy tracker in umfTearDown() under the proxy library

2172179

Signed-off-by: Lukasz Dorau <[email protected]>

Add size threshold to proxy lib to call system allocator (Linux only)

bb7d311

Add a size threshold to proxy lib to call system allocator when the size is less than the given threshold (Linux only yet). Signed-off-by: Lukasz Dorau <[email protected]>

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch from 846f30b to 653bd3d Compare November 14, 2024 19:56

ldorau added 2 commits November 14, 2024 22:19

Add WA for the issue

f99b905

This WA for the issue: oneapi-src#894 It protects us from a recursion in malloc_usable_size() when the JEMALLOC proxy_lib_pool is used. TODO: remove this WA when the issue is fixed. Signed-off-by: Lukasz Dorau <[email protected]>

ldorau force-pushed the Add_threshold_to_proxy_lib_to_call_system_allocator branch from 653bd3d to ec89615 Compare November 14, 2024 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add threshold to proxy lib to call system allocator #883

Add threshold to proxy lib to call system allocator #883

ldorau commented Nov 8, 2024 •

edited

Loading

bratpiorka Nov 12, 2024

ldorau Nov 13, 2024

ldorau Nov 14, 2024

ldorau Nov 14, 2024

lplewa Nov 8, 2024 •

edited

Loading

ldorau Nov 14, 2024 •

edited

Loading

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

lukaszstolarczuk Nov 14, 2024

ldorau Nov 14, 2024

Add threshold to proxy lib to call system allocator #883

Are you sure you want to change the base?

Add threshold to proxy lib to call system allocator #883

Conversation

ldorau commented Nov 8, 2024 • edited Loading

Description

Checklist

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 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

ldorau Nov 14, 2024 • 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

ldorau commented Nov 8, 2024 •

edited

Loading

lplewa Nov 8, 2024 •

edited

Loading

ldorau Nov 14, 2024 •

edited

Loading