Make it possible to create Fixed Provider from pointer of UMF memory pool #1121

ldorau · 2025-02-21T10:12:11Z

Description

Make it possible to create Fixed-size Memory Provider
from a pointer belonging to a UMF memory pool.
This pointer is already tracked in the UMF tracker,
so we have to remove this memory range
<pointer, pointer + length) from the tracker.

Checklist

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

include/umf/providers/provider_fixed_memory.h

src/provider/provider_fixed_memory.c

src/provider/provider_tracking.c

vinser52 · 2025-02-21T12:40:30Z

src/provider/provider_tracking.c

@@ -267,6 +267,58 @@ static umf_result_t trackingAllocationSplit(void *hProvider, void *ptr,
    return ret;
 }

+// shrink (or remove) an entry in the tracker and return the totalSize of the original entry
+umf_result_t trackerShrinkEntry(void *hProvider, void *ptr, size_t shrinkSize,


I do not think it is a good idea for several reasons:

Tracker contains info about actual memory chunks that correspond to the coarse grain allocations by the upstream memory providers. This function violates this rule.

We have split and merge functions but they notify the upstream memory provider first and work only if the upstream provider supports split/merge functionality.

Have you considered how IPC flow is affected by this logic? it might break IPC functionality.

If we could restrict this functionality to using only the whole allocations, splitting and merging would not be needed. I will prepare an additional commit with those changes.

@vinser52 I have prepared one commit with this change (only the whole allocation can be used) - on the https://github.com/ldorau/unified-memory-framework/tree/Replace_trackerShrinkEntry_with_trackerRemoveEntry branch.

@vinser52 Just a brief explanation to paint the background for the later discussion:

This new functionality is needed for async API in UR/SYCL/LLVM (oneapi-src/unified-runtime#2180). We need to create a native pool based on the Fixed-size Memory Provider created from a user-provided buffer that comes from a USM pool based on Disjoint Pool with the L0 memory provider (see here) - @kswiecicki please correct me if I am wrong.

Using Disjoint Pool we cannot guarantee that we will receive from umfPoolMalloc() the whole allocation saved in the tracker and received from the L0 memory provider, so we have to be able to split/merge this allocation in the tracker, but L0 memory provider does not support splitting and merging, so we cannot notify the upstream provider (L0 memory provider in this case) and we have to add a new API to the tracker for this purpose ...

I do not think it is a good idea for several reasons:

Tracker contains info about actual memory chunks that correspond to the coarse grain allocations by the upstream memory providers. This function violates this rule.

We have to create a nested UMF pool here, what causes that we have to add the same entry to the tracker for the second time. This seems to be the simplest and the fastest solution of this issue.

We have split and merge functions but they notify the upstream memory provider first and work only if the upstream provider supports split/merge functionality.

The upstream provider is L0 memory provider that does not support splitting and merging.

Have you considered how IPC flow is affected by this logic? it might break IPC functionality.

This feature is enabled only in the Fixed-size memory provider that does not support IPC at all yet.

Also, the pool handle param seems to be optional here: https://github.com/intel/llvm/pull/17117/files#diff-b4c22134fb720af1413e8c0d1a8c737092bae2a747ce73ca4d803773249eb81bR242.

In case of NULL pool handle the default pool is used. And UR knows the default pool handle

There is also a matter of UR queries: urUSMGetMemAllocInfo with https://github.com/intel/llvm/blob/sycl/unified-runtime/include/ur_api.h#L4230 I'm not sure if the spec for async says anything about this, but I think returning the underlying pool here would be a misleading.

@vinser52

Tracker contains info about actual memory chunks that correspond to the coarse grain allocations by the upstream memory providers. This function violates this rule.

We can't implement this feature (if we indeed need tracking capabilities) without changing the tracker behavior somehow. I do agree that we lose some information in the tracker with this approach. Now, only the fixed_provider knows everything about the original allocation, and to support IPC, I think the fixed_provider would have to forward calls to the original provider. (this might also apply to other functions, like get_recommended_page_size()).

If we extended the provider to allow multiple values to be kept on a stack for every key, as @lplewa proposed, we would keep all the information, but then the IPC implementation would need to have some extra logic to extract the correct provider. Also, merge/split implementation might become a bit complicated.

By the way, in the current implementation instead of adding trackerShrinkEntry/trackerShrinkGrow I think we could just add extra argument to trackerSplit/trackerMerge to specify whether to call upstream provider split/merge and use them.

We can't implement this feature (if we indeed need tracking capabilities) without changing the tracker behavior somehow.

I understand that we have to change somehow our tracking mechanism. Don't get me wrong I am not against of any changes, I just try to figure out the right way.

Now, only the fixed_provider knows everything about the original allocation, and to support IPC, I think the fixed_provider would have to forward calls to the original provider. (this might also apply to other functions, like get_recommended_page_size()).

That is what I am worried about. Today we are implementing the Fixed provider and we are UMF experts. Imagine tomorrow some external people want to implement a provider similar to the Fixed provider do we want to propose them that way or should we think of a solution that exposes as little as possible UMF internals (memory tracker, IPC implementation, etc.).

If we extended the provider to allow multiple values to be kept on a stack for every key, as @lplewa proposed, we would keep all the information, but then the IPC implementation would need to have some extra logic to extract the correct provider. Also, merge/split implementation might become a bit complicated.

If this extra logic that you have in mind will be encapsulated inside UMF it is fine, what we should avoid/decrease is delegating the challenges we have to the pool/provider implementers.

By the way, in the current implementation instead of adding trackerShrinkEntry/trackerShrinkGrow I think we could just add extra argument to trackerSplit/trackerMerge to specify whether to call upstream provider split/merge and use them.

I did not get the idea, did you mean trackingAllocationSplit/trackingAllocationMerge functions? If so, these functions implement the provider interface (umf_memory_provider_ops_t structure). To add a new parameter to these functions we have to change the interface of the memory provider.

vinser52 · 2025-02-21T12:41:27Z

Before merging this PR I think we should discuss it and agree on the design/arch at the UMF tech meeting

ldorau · 2025-02-21T13:07:13Z

@pbalcer @bratpiorka please review

Add umfFixedMemoryProviderParamsSetFlags() to set flags of Fixed-size Memory Provider. Add the UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR flag that makes it possible to create Fixed-size Memory Provider from a pointer belonging to a UMF memory pool. This pointer is already tracked in the UMF tracker, so we will have to remove this memory range <base, base+size) from the tracker. Signed-off-by: Lukasz Dorau <[email protected]>

Add umfPoolGetTrackingProvider() to get the tracking provider of the original memory pool. Signed-off-by: Lukasz Dorau <[email protected]>

Add trackerShrinkEntry() that shrinks (or removes) an entry in the tracker and returns the totalSize of the original entry. Add trackerGrowEntry() that grows (or adds) an entry in the tracker to its original size. Signed-off-by: Lukasz Dorau <[email protected]>

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

ldorau · 2025-02-24T12:14:31Z

@igchor please review

ldorau · 2025-02-24T14:35:51Z

Converting to draft, because it does not support all possible split/merge combinations yet (found by @lplewa). The missing implementation will be added after the discussion if splitting/merging is accepted.

igchor · 2025-02-24T16:50:57Z

test/provider_fixed_memory.cpp

+    umfPoolDestroy(proxyFixedPool);
+}
+
+TEST_P(FixedProviderTest, pool_from_ptr_half_size_success) {


should we also test a case where we create multiple pools from a single allocation?

I think we should generalize the tests and run all the tests that we do today for other providers.

igchor · 2025-02-24T17:01:31Z

src/provider/provider_tracking.c

@@ -267,6 +267,58 @@ static umf_result_t trackingAllocationSplit(void *hProvider, void *ptr,
    return ret;
 }

+// shrink (or remove) an entry in the tracker and return the totalSize of the original entry
+umf_result_t trackerShrinkEntry(void *hProvider, void *ptr, size_t shrinkSize,


There is also a matter of UR queries: urUSMGetMemAllocInfo with https://github.com/intel/llvm/blob/sycl/unified-runtime/include/ur_api.h#L4230 I'm not sure if the spec for async says anything about this, but I think returning the underlying pool here would be a misleading.

vinser52 · 2025-02-24T20:36:23Z

test/provider_fixed_memory.cpp

Could you please instantiate ipcFixture.cpp tests with Fixed provider

I cannot, because the Fixed provider does not support IPC at all.

Do we have plans to implement IPC support? It is hard to say right now (without implementation experience) if the proposed approach works.

@bratpiorka Do we have plans to implement IPC support in Fixed provider? @vinser52 Does anyone need it now?

ldorau · 2025-02-25T10:32:23Z

@vinser52 @igchor @bratpiorka @lplewa @kswiecicki Please review also an alternative solution: #1134 - without splitting and merging tracker entries.

ldorau · 2025-02-26T10:37:52Z

It will be implemented in another way soon.

ldorau requested a review from a team as a code owner February 21, 2025 10:12

ldorau requested review from kswiecicki and bratpiorka February 21, 2025 10:12

kswiecicki reviewed Feb 21, 2025

View reviewed changes

include/umf/providers/provider_fixed_memory.h Show resolved Hide resolved

src/provider/provider_fixed_memory.c Show resolved Hide resolved

src/provider/provider_tracking.c Outdated Show resolved Hide resolved

src/provider/provider_tracking.c Show resolved Hide resolved

ldorau force-pushed the Make_it_possible_to_create_Fixed_Provider_from_pointer_of_UMF_memory_pool branch from d1c6e1c to 0939cb3 Compare February 21, 2025 11:19

ldorau requested a review from kswiecicki February 21, 2025 11:19

kswiecicki approved these changes Feb 21, 2025

View reviewed changes

vinser52 reviewed Feb 21, 2025

View reviewed changes

ldorau requested a review from pbalcer February 21, 2025 13:07

ldorau added 4 commits February 21, 2025 14:22

Add umfPoolGetTrackingProvider()

b4d91c3

Add umfPoolGetTrackingProvider() to get the tracking provider of the original memory pool. Signed-off-by: Lukasz Dorau <[email protected]>

Add tests for pool from pointer

d6d8f60

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

ldorau force-pushed the Make_it_possible_to_create_Fixed_Provider_from_pointer_of_UMF_memory_pool branch 2 times, most recently from 7e4d37a to d6d8f60 Compare February 21, 2025 16:58

ldorau requested review from vinser52 and igchor February 24, 2025 10:43

ldorau marked this pull request as draft February 24, 2025 14:36

igchor reviewed Feb 24, 2025

View reviewed changes

vinser52 reviewed Feb 24, 2025

View reviewed changes

ldorau mentioned this pull request Feb 25, 2025

Stack the same values in the tracker #1134

Closed

3 tasks

ldorau closed this Feb 26, 2025

ldorau deleted the Make_it_possible_to_create_Fixed_Provider_from_pointer_of_UMF_memory_pool branch March 18, 2025 08:06

Make it possible to create Fixed Provider from pointer of UMF memory pool #1121

Make it possible to create Fixed Provider from pointer of UMF memory pool #1121

Uh oh!

Conversation

ldorau commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau Feb 21, 2025 • 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!

ldorau Feb 21, 2025 • 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!

ldorau Feb 21, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinser52 commented Feb 21, 2025

Uh oh!

ldorau commented Feb 21, 2025

Uh oh!

ldorau commented Feb 24, 2025

Uh oh!

ldorau commented Feb 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau commented Feb 25, 2025

Uh oh!

ldorau commented Feb 26, 2025

Uh oh!

Uh oh!

ldorau commented Feb 21, 2025 •

edited

Loading

ldorau Feb 21, 2025 •

edited

Loading

ldorau Feb 21, 2025 •

edited

Loading

ldorau Feb 21, 2025 •

edited

Loading