[TEST CI] Run IPC tests with the proxy library #868
Conversation
ldorau
force-pushed
the
Run_IPC_tests_with_the_proxy_library
branch
10 times, most recently
from
b26565d
to
9c8c488
Compare
ldorau
force-pushed
the
Run_IPC_tests_with_the_proxy_library
branch
5 times, most recently
from
5842644
to
0ed63c4
Compare
ldorau
changed the title
trackingFree() for providers not supporting free() op and enable all IPC tests
ldorau
force-pushed
the
Run_IPC_tests_with_the_proxy_library
branch
from
0ed63c4
to
057c819
Compare
|
@vinser52 please review |
src/provider/provider_tracking.c
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Do you mean umfMemoryProviderFree() above ? - inside trackingFree() in line no. 382 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, the file provider and the devdax provider work exactly as as you wrote - they were designed to work in this way.
There was a problem hiding this comment.
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 was a problem hiding this comment.
... but we should not just return "success" here because the memory is actually not released. Maybe this fix is wrong... We should fix only the case when the pool that uses devdax/file providers is destroyed. Only then we should remove the entries from the tracker as we also unmap the memory.
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).
There was a problem hiding this comment.
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
umfMemoryProviderFreeis 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed (code change). It will be fixed in another way.
Done.
src/provider/provider_tracking.c
Outdated
| @@ -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"); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Removed (code change). It will be fixed in another way.
Done.
Signed-off-by: Lukasz Dorau <[email protected]>
ldorau
force-pushed
the
Run_IPC_tests_with_the_proxy_library
branch
from
057c819
to
2058774
Compare
ldorau
changed the title
trackingFree() for providers not supporting free() op and enable all IPC testsSigned-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]>
ldorau
force-pushed
the
Run_IPC_tests_with_the_proxy_library
branch
from
2058774
to
efef7d0
Compare
Description
[TEST CI] Run IPC tests with the proxy library.
Ref: #864
Checklist