[DRAFT] Enable IPC tests with the proxy library #868

ldorau · 2024-11-05T11:46:30Z

Description

Enable IPC tests with the proxy library.

Fixes: #864

Requires:

Add the coarse library #931

Checklist

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

ldorau · 2024-11-14T12:01:29Z

@vinser52 please review

vinser52 · 2024-11-14T13:28:27Z

src/provider/provider_tracking.c

@@ -382,6 +382,12 @@ static umf_result_t trackingFree(void *hProvider, void *ptr, size_t size) {
    ret = umfMemoryProviderFree(p->hUpstream, ptr, size);
    if (ret != UMF_RESULT_SUCCESS) {
        LOG_ERR("upstream provider failed to free the memory");
+        // Do not add memory back to the tracker,
+        // if the provider does not support the free() op.
+        if (ret == UMF_RESULT_ERROR_NOT_SUPPORTED) {


I agree that we should not add memory back to the tracker in case of memory provider does not support free.

But why is pool implementation tries to call umfMemoryProviderFree() at all?

Do you mean umfMemoryProviderFree() above ? - inside trackingFree() in line no. 382 ?

With such fix we get a memory leak.

Imagine the pool calls umfMemoryProviderFree() for the address range [a. b). After that pool calls umfMemoryProviderAlloc() my understanding is that memory provider will never return the range [a. b) as a result of malloc, but it will return the next chunk [b,c). If the pool will call umfMemoryProviderFree() for the [b,c) range it also will be lost and never be used again.

Yes, the file provider and the devdax provider work exactly as as you wrote - they were designed to work in this way.

I agree with @ldorau - this is how these providers work. And because of this, in the real-life scenarios they should be used either with a pool that never calls free() or with CoarseProvider

There is also an additional way fix this bug - free() in devdax/file could do the unmap but we could keep the offset unchanged - this way the free() would be supported and entries would be correctly removed from the tracker while we still keep file/devdax provider logic simple (so the memory would be still unavailable after free).

I agree that at least we are not wasting VA space (but it is not a problem), but since we never decrement the offset in devdax and file providers by design we still have a kind of memory leak. Because if the umfMemoryProviderAlloc and umfMemoryProviderFree are called multiple times, after some time umfMemoryProviderAlloc will return OOM because offset reached the end of file/devdax.

We discussed this with @ldorau offline. My main comment that currently this PR eliminates the issue:

today we have a memory leak if umfMemoryProviderFree is called. If we do not remove the range from the tracker our diagnostic in DEBUG mode will detect it and report.

with this PR the memory leak still there but our diagnostic is not able to detect it and report.

I agree with @ldorau - this is how these providers work. And because of this, in the real-life scenarios they should be used either with a pool that never calls free() or with CoarseProvider

Just to clarify, I am not challenging how devdax or file providers work. we agreed on that. My concern is about this particular flow when some pool calls umfmemoryProviderFree for devdax or file providers. This situation should not happens or should be handled correctly. Today looks like there is a memory leak in that case and this PR does not fix the leak.

Removed (code change). It will be fixed in another way.
Done.

I was saying that providers without free should have a null ptr to function, and we should have an API that pool can check if free function is supported if not - if it's not supported it should never call providerFree().

Or as another option - pool informs umf if it can support provider without free - if pool cannot handle it, we always wrap it with coarse provider.

Code changed.
Done.

bratpiorka · 2024-11-14T14:36:47Z

src/provider/provider_tracking.c

@@ -382,6 +382,12 @@ static umf_result_t trackingFree(void *hProvider, void *ptr, size_t size) {
    ret = umfMemoryProviderFree(p->hUpstream, ptr, size);
    if (ret != UMF_RESULT_SUCCESS) {
        LOG_ERR("upstream provider failed to free the memory");


in case the free() is not supported we should call LOG_ERR. Please do not nest ifs here and use switch or if/else if

Removed (code change). It will be fixed in another way.
Done.

Code changed.
Done.

Add the coarse library that will replace the coarse provider. Signed-off-by: Lukasz Dorau <[email protected]>

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

Remove free_not_supp from the ipcTestParams tuple. It is not needed any more. Signed-off-by: Lukasz Dorau <[email protected]>

Fix two error messages and add some new ones. Signed-off-by: Lukasz Dorau <[email protected]>

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

ldorau mentioned this pull request Nov 5, 2024

IPC API does not work with the proxy library #864

Open

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch 10 times, most recently from b26565d to 9c8c488 Compare November 12, 2024 14:14

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch 5 times, most recently from 5842644 to 0ed63c4 Compare November 14, 2024 11:51

ldorau changed the title ~~[TEST CI] Run IPC tests with the proxy library~~ Fix trackingFree() for providers not supporting free() op and enable all IPC tests Nov 14, 2024

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch from 0ed63c4 to 057c819 Compare November 14, 2024 12:00

ldorau marked this pull request as ready for review November 14, 2024 12:01

ldorau requested a review from a team as a code owner November 14, 2024 12:01

ldorau requested review from vinser52 and bratpiorka November 14, 2024 12:01

vinser52 reviewed Nov 14, 2024

View reviewed changes

bratpiorka reviewed Nov 14, 2024

View reviewed changes

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch from 057c819 to 2058774 Compare November 14, 2024 21:33

ldorau marked this pull request as draft November 14, 2024 21:34

ldorau changed the title ~~Fix trackingFree() for providers not supporting free() op and enable all IPC tests~~ [TEST CI] Run IPC tests with the proxy library Nov 14, 2024

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch 2 times, most recently from efef7d0 to 3805ad8 Compare November 15, 2024 08:58

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch 4 times, most recently from fc98d1d to 30fc257 Compare November 18, 2024 10:03

ldorau changed the title ~~[TEST CI] Run IPC tests with the proxy library~~ Enable IPC tests with the proxy library Nov 18, 2024

ldorau requested review from lplewa, vinser52 and bratpiorka November 18, 2024 11:16

ldorau marked this pull request as ready for review November 18, 2024 11:20

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch 2 times, most recently from f49d417 to e9102ad Compare November 19, 2024 07:29

ldorau marked this pull request as draft November 19, 2024 12:11

ldorau changed the title ~~Enable IPC tests with the proxy library~~ [DRAFT] Enable IPC tests with the proxy library Nov 19, 2024

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch 2 times, most recently from 9f3d43b to 42d02ec Compare November 21, 2024 17:16

ldorau added 10 commits November 25, 2024 13:22

Add the coarse library

9016be2

Add the coarse library that will replace the coarse provider. Signed-off-by: Lukasz Dorau <[email protected]>

Add tests for the coarse library

bf3610b

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

Use libcoarse in the file provider

6501325

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

Add free() op to the file provider

3d0c368

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

Use libcoarse in the devdax provider

7781830

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

Add free() to the devdax provider

b7e6f36

Remove free_not_supp from the ipcTestParams tuple

b17a69f

Remove free_not_supp from the ipcTestParams tuple. It is not needed any more. Signed-off-by: Lukasz Dorau <[email protected]>

Fix and add error messages

07264bf

Fix two error messages and add some new ones. Signed-off-by: Lukasz Dorau <[email protected]>

Enable all IPC tests

097331c

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

DEBUG CI

a25b910

ldorau force-pushed the Run_IPC_tests_with_the_proxy_library branch from 42d02ec to a25b910 Compare November 25, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Enable IPC tests with the proxy library #868

[DRAFT] Enable IPC tests with the proxy library #868

ldorau commented Nov 5, 2024 •

edited

Loading

ldorau commented Nov 14, 2024

vinser52 Nov 14, 2024

ldorau Nov 14, 2024

vinser52 Nov 14, 2024

ldorau Nov 14, 2024

bratpiorka Nov 14, 2024

vinser52 Nov 14, 2024

vinser52 Nov 14, 2024

ldorau Nov 14, 2024

lplewa Nov 15, 2024

ldorau Nov 18, 2024

bratpiorka Nov 14, 2024

ldorau Nov 14, 2024

ldorau Nov 18, 2024

[DRAFT] Enable IPC tests with the proxy library #868

Are you sure you want to change the base?

[DRAFT] Enable IPC tests with the proxy library #868

Conversation

ldorau commented Nov 5, 2024 • edited Loading

Description

Checklist

ldorau commented Nov 14, 2024

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 5, 2024 •

edited

Loading