Clear tracker for the current pool on destroy #759

ldorau · 2024-09-26T09:22:14Z

Description

Clear tracker for the current pool when destroying the pool
and clear the whole tracker when destroying the UMF library.
Print error messages about unfreed allocations left in the tracker
only if provider supports the free() operation.

Checklist

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

lukaszstolarczuk · 2024-09-26T10:01:55Z

do we have any place where this was failing (without your change)?

ldorau · 2024-09-26T10:16:28Z

do we have any place where this was failing (without your change)?

We do not parse logs in our CI tests.

vinser52 · 2024-09-26T15:17:29Z

I do not understand why this logic is needed. I thought that free is a mandatory operation for all memory providers, isn't it?

ldorau · 2024-09-26T15:59:15Z

I do not understand why this logic is needed. I thought that free is a mandatory operation for all memory providers, isn't it?

The DevDax memory provider:
https://github.com/oneapi-src/unified-memory-framework/blob/main/README.md#devdax-memory-provider-linux-only

and the file memory provider:
https://github.com/oneapi-src/unified-memory-framework/blob/main/README.md#file-memory-provider-linux-only-yet

do not support the free() operation - they both free whole available memory in the destroy() operation.

vinser52 · 2024-09-26T16:32:40Z

I do not understand why this logic is needed. I thought that free is a mandatory operation for all memory providers, isn't it?

The DevDax memory provider: https://github.com/oneapi-src/unified-memory-framework/blob/main/README.md#devdax-memory-provider-linux-only

and the file memory provider: https://github.com/oneapi-src/unified-memory-framework/blob/main/README.md#file-memory-provider-linux-only-yet

do not support the free() operation - they both free whole available memory in the destroy() operation.

I missed that (probably because I was on vacation), what is the reason for such a limitation? Does it mean that only jemalloc pool can be used today with these memory providers?

ldorau · 2024-09-26T16:52:55Z

Does it mean that only jemalloc pool can be used today with these memory providers?

Only the jemalloc pool or the course provider (#715 - hopefully it will be merged soon).

ldorau · 2024-09-26T17:22:59Z

I missed that (probably because I was on vacation), what is the reason for such a limitation?

The reason is to keep these providers as simple as possible. The course provider will take over the memory management for them.

vinser52 · 2024-09-27T10:10:02Z

The reason is to keep these providers as simple as possible. The course provider will take over the memory management for them.

I look from the user perspective, does "keep these providers as simple as possible" mean that we simplify things for us (UMF developers) but complicate it for the users?

Another question regarding correctness: when memory tracker is cleared? When the memory provider allocates a memory region it is added to the memory tracker, but if the pool implementation does not release memory back and relies on the fact that the memory provider implementation will release virtual memory mapping who will cleanup the memory tracker? If it is not done then it is an error, because after the memory provider is destroyed the virtual memory address range might be re-used by other memory providers and the same virtual memory region might be added to the tracker and it will fail because the same virtual memory region already presented in the tracker.

ldorau · 2024-09-27T12:01:48Z

Another question regarding correctness: when memory tracker is cleared? When the memory provider allocates a memory region it is added to the memory tracker, but if the pool implementation does not release memory back and relies on the fact that the memory provider implementation will release virtual memory mapping who will cleanup the memory tracker? If it is not done then it is an error, because after the memory provider is destroyed the virtual memory address range might be re-used by other memory providers and the same virtual memory region might be added to the tracker and it will fail because the same virtual memory region already presented in the tracker.

Right. So I replaced it with clearing the tracker for the current pool - please re-review

ldorau · 2024-09-30T06:49:31Z

Clear tracker for the current pool when destroying the pool
and clear the whole tracker when destroying the UMF library.
Print error messages about unfreed allocations left in the tracker
only if provider supports the free() operation.

vinser52 · 2024-09-30T10:33:38Z

Another question regarding correctness: when memory tracker is cleared? When the memory provider allocates a memory region it is added to the memory tracker, but if the pool implementation does not release memory back and relies on the fact that the memory provider implementation will release virtual memory mapping who will cleanup the memory tracker? If it is not done then it is an error, because after the memory provider is destroyed the virtual memory address range might be re-used by other memory providers and the same virtual memory region might be added to the tracker and it will fail because the same virtual memory region already presented in the tracker.

Right. So I replaced it with clearing the tracker for the current pool - please re-review

I still think that not supporting free operation in the memory provider is weird:

First, it means that not every pool manager can be used with such memory provider or user needs to use coarse provider on top of upstream provider that does not support free operation.
Second, in this PR you need to iterate via the tracker (that contains records from all pools) and filter only records for the corresponding pool. It might be a performance impact in real life.

Another consideration, I am working on the cache implementation for opened IPC handles, the cache is global and I need to remove entries from the IPC cache when the corresponding pool is destroyed. It looks like a similar problem. BUt I still think that in your case it is better to require free implementation in all providers.

ldorau · 2024-10-01T06:57:34Z

I still think that not supporting free operation in the memory provider is weird:

First, it means that not every pool manager can be used with such memory provider or user needs to use coarse provider on top of upstream provider that does not support free operation.

Second, in this PR you need to iterate via the tracker (that contains records from all pools) and filter only records for the corresponding pool. It might be a performance impact in real life.

Another consideration, I am working on the cache implementation for opened IPC handles, the cache is global and I need to remove entries from the IPC cache when the corresponding pool is destroyed. It looks like a similar problem. BUt I still think that in your case it is better to require free implementation in all providers.

@vinser52 OK, let's move this discussion to #769.

This PR is about clearing the tracker. Currently it is not cleared and this patch adds clearing the tracker.

@vinser52 You have written recently:

Another question regarding correctness: when memory tracker is cleared? When the memory provider allocates a memory region it is added to the memory tracker, but if the pool implementation does not release memory back and relies on the fact that the memory provider implementation will release virtual memory mapping who will cleanup the memory tracker? If it is not done then it is an error, because after the memory provider is destroyed the virtual memory address range might be re-used by other memory providers and the same virtual memory region might be added to the tracker and it will fail because the same virtual memory region already presented in the tracker.

So now the question is: should we clear the tracker or not?

pbalcer · 2024-10-01T12:04:46Z

src/provider/provider_tracking.c

-            }
+#ifndef NDEBUG
+    // print error messages only if provider supports the free() operation
+    if (n_items && !upstreamDoesNotFree) {


is upstreamDoesNotFree needed for anything other than debug printing?

No, currently it is not. The tracker is cleared from all left non-freed pointers regardless of value of upstreamDoesNotFree.

vinser52 · 2024-10-01T12:12:22Z

So now the question is: should we clear the tracker or not?

The general rule is that the tracker should always be cleared when some chunk is deallocated. Otherwise the state of the tracker is incorrect.

Now regarding this PR: since file and DAX providers are already upstreamed to the main branch, this PR fixes the issue with the tracker. Yesterday we discussed this with @bratpiorka and agreed that while I disagree with the general design and we need design discussion on free API implementation, we need to merge this PR as a temporary workaround to fix the main branch.

ldorau · 2024-10-01T12:57:30Z

So now the question is: should we clear the tracker or not?

The general rule is that the tracker should always be cleared when some chunk is deallocated. Otherwise the state of the tracker is incorrect.

Now regarding this PR: since file and DAX providers are already upstreamed to the main branch, this PR fixes the issue with the tracker. Yesterday we discussed this with @bratpiorka and agreed that while I disagree with the general design and we need design discussion on free API implementation, we need to merge this PR as a temporary workaround to fix the main branch.

Yes, I agree.

So you can approve or request changes to this PR.

vinser52 · 2024-10-01T15:06:14Z

So you can approve or request changes to this PR.

I approved, but maybe you can add a TODO comment to note that it is a temporary solution.

ldorau · 2024-10-02T08:40:04Z

So you can approve or request changes to this PR.

I approved, but maybe you can add a TODO comment to note that it is a temporary solution.

@vinser52 It seems that after the yesterday's discussion (free() is an optional op) it is not a temporary solution,
so TODO is not needed?

vinser52 · 2024-10-02T08:50:47Z

So you can approve or request changes to this PR.

I approved, but maybe you can add a TODO comment to note that it is a temporary solution.

@vinser52 It seems that after the yesterday's discussion (free() is an optional op) it is not a temporary solution, so TODO is not needed?

free() is optional, but on the meeting we agreed that in case some provider does not define free() UMF will wrap such provider with coarse grain provider that supports free(). So my initial comment was about your solution to cleanup the tracker: we do not want to iterate over memory tracker, and we are doing it right now temporary to fix the main branch.

Clear tracker for the current pool on destroy. Do not print error messages if provider does not support the free() operation. Signed-off-by: Lukasz Dorau <[email protected]>

ldorau · 2024-10-02T09:02:54Z

@vinser52 TODO added.

Clearing the tracker was a temporary solution and should be removed. The tracker should be cleared using the provider's free() operation. Replace clear_tracker_for_the_pool() with check_if_tracker_is_empty(). This patch reverts commit 2766a21 Ref: oneapi-src#759 Signed-off-by: Lukasz Dorau <[email protected]>

ldorau requested a review from a team as a code owner September 26, 2024 09:22

ldorau force-pushed the Check_if_the_provider_supports_the_free_operation branch from e14fcbd to abf8876 Compare September 26, 2024 10:32

ldorau requested a review from lukaszstolarczuk September 26, 2024 16:23

ldorau requested review from vinser52, PatKamin and bratpiorka September 26, 2024 17:24

ldorau changed the title ~~Check if the provider supports the free() operation~~ Call check_if_tracker_is_empty() conditionally Sep 27, 2024

ldorau force-pushed the Check_if_the_provider_supports_the_free_operation branch from abf8876 to 34197ef Compare September 27, 2024 07:09

ldorau changed the title ~~Call check_if_tracker_is_empty() conditionally~~ Clear tracker for the pool if provider does not support free() operation Sep 27, 2024

ldorau force-pushed the Check_if_the_provider_supports_the_free_operation branch from 34197ef to b147c91 Compare September 27, 2024 11:59

ldorau force-pushed the Check_if_the_provider_supports_the_free_operation branch 2 times, most recently from 120b06d to 3d3adf1 Compare September 28, 2024 16:37

ldorau changed the title ~~Clear tracker for the pool if provider does not support free() operation~~ Clear tracker for the current pool on destroy Sep 28, 2024

ldorau force-pushed the Check_if_the_provider_supports_the_free_operation branch 2 times, most recently from d7d4afd to 04a55e3 Compare September 30, 2024 06:44

ldorau mentioned this pull request Oct 1, 2024

Can some memory providers not support the free() operation or not? #769

Closed

pbalcer reviewed Oct 1, 2024

View reviewed changes

vinser52 approved these changes Oct 1, 2024

View reviewed changes

Clear tracker for the current pool on destroy

2766a21

Clear tracker for the current pool on destroy. Do not print error messages if provider does not support the free() operation. Signed-off-by: Lukasz Dorau <[email protected]>

ldorau force-pushed the Check_if_the_provider_supports_the_free_operation branch from 04a55e3 to 2766a21 Compare October 2, 2024 09:02

bratpiorka approved these changes Oct 2, 2024

View reviewed changes

ldorau merged commit 3e4f65f into oneapi-src:main Oct 2, 2024
72 checks passed

ldorau deleted the Check_if_the_provider_supports_the_free_operation branch October 2, 2024 13:38

ldorau mentioned this pull request Feb 18, 2025

Check if tracker is empty instead of clearing it #1104

Merged

3 tasks

Clear tracker for the current pool on destroy #759

Clear tracker for the current pool on destroy #759

Uh oh!

Conversation

ldorau commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

lukaszstolarczuk commented Sep 26, 2024

Uh oh!

ldorau commented Sep 26, 2024

Uh oh!

vinser52 commented Sep 26, 2024

Uh oh!

ldorau commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinser52 commented Sep 26, 2024

Uh oh!

ldorau commented Sep 26, 2024

Uh oh!

ldorau commented Sep 26, 2024

Uh oh!

vinser52 commented Sep 27, 2024

Uh oh!

ldorau commented Sep 27, 2024

Uh oh!

ldorau commented Sep 30, 2024

Uh oh!

vinser52 commented Sep 30, 2024

Uh oh!

ldorau commented Oct 1, 2024

Uh oh!

pbalcer Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

ldorau Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinser52 commented Oct 1, 2024

Uh oh!

ldorau commented Oct 1, 2024

Uh oh!

vinser52 commented Oct 1, 2024

Uh oh!

ldorau commented Oct 2, 2024

Uh oh!

vinser52 commented Oct 2, 2024

Uh oh!

ldorau commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

ldorau commented Sep 26, 2024 •

edited

Loading

ldorau commented Sep 26, 2024 •

edited

Loading

ldorau Oct 1, 2024 •

edited

Loading