Initial memspace and memory_target APIs #53

igchor · 2023-12-01T23:33:51Z

This is an initial draft of APIs for memspace and memory_target (without any actual implementation).

A few things to discuss:

Should memory_target_ops be a public API? Exposing it, would allow us to implement support for some targets outside of this repo. E.g. we could implement support for UR directly where it's needed.
Where should the logic for creating pool/provider from a memspace reside? In this PR I proposed to make it a part of memory_target implementation. This won't work if a memspace consists of memory_targets of different_types (but we haven't explored this too much yet anyway)
Do we want to have type-specific queries (.e.g node_id for numa memory_target)?

vinser52 · 2023-12-04T13:33:31Z

What you described makes sense but to break it down into several independent steps I suggest the following:

Expose only predefined Memspaces for CPU (e.g. Highest Bandwidth, Highest Capacity, etc.). Keep the ops structure and underlying implementation internal. The first step allows us to define API for pool creation from the memspace and replace Memkind.
Think about what we need to expose to support UR.
Think about what we need to expose to support user-defined memory spaces and memory targets.

Perhaps, steps 2 & 3 are dependant.

Let's expose only required APIs for each step but design the implementation with all future use cases in mind.

include/umf/memspaces/memspace_numa.h

igchor · 2023-12-05T18:35:29Z

Implementation for NUMA is here: igchor#1

include/umf/memspace.h

include/umf/memspaces/memspace_numa.h

include/umf/memspace_policy.h

src/memory_target_ops.h

src/memspace.c

igchor · 2023-12-13T00:10:02Z

Depends on: #69

We can also not implement umfPoolCreateFromMemspace in this PR if you think we need to discuss the approach in #69 more.

include/umf/memory_pool.h

include/umf/memspace.h

src/memory_pool_internal.h

src/memory_target_ops.h

include/umf/memspace.h

src/memory_target.c

include/umf/memspace.h

include/umf/memspaces/memspace_numa.h

src/memory_target.h

src/memory_target_ops.h

src/memory_targets/memory_target_numa.c

src/memspace.c

bratpiorka · 2023-12-21T10:20:32Z

@vinser52 I think we could go with this code - I know it still needs some changes in the future, but I think it is a good basis for introducing the memspace API

vinser52 · 2023-12-21T12:09:14Z

include/umf/memspace.h

+/// \brief Destroys memspace
+/// \param hMemspace handle to memspace
+///
+void umfMemspaceDestroy(umf_memspace_handle_t hMemspace);


Where is the corresponding create function?

Another question: if we consider only predefined memspaces as a first step do we need Create/Destroy functions for memspaces in a public API?

Right now we only have umfMemspaceCreateFromNumaArray that is defined in memspace_numa.h

We could move that function to memspace.h for now but in the long term, I think those create functions should be in separate headers (since we might want to have the memspaces in separate libs).

Another question: if we consider only predefined memspaces as a first step do we need Create/Destroy functions for memspaces in a public API?

Ok, good point. @bratpiorka for openMP, we won't need umfMemspaceCreateFromNumaArray right? If that's the case I can make it internal for now.

Done. I've made those functions private for now.

This PR does not expose the predefined memspaces but this will be added in next PR (it requires integrating with hwloc).

bratpiorka · 2024-01-09T15:59:00Z

@igchor please rebase
@vinser52 can we merge this PR? I think we could implement the next steps in separate PRs.

igchor · 2024-01-09T16:07:04Z

@igchor please rebase @vinser52 can we merge this PR? I think we could implement the next steps in separate PRs.

Rebased.

igchor · 2024-01-10T23:04:28Z

@vinser52 please take a look. We'd like to unblock next step which is implementing HOST_MEMSPACE

vinser52 · 2024-01-11T18:28:25Z

src/memory_target_ops.h

+        umf_memspace_policy_handle_t policy, umf_memory_pool_handle_t *pool);
+
+    umf_result_t (*memory_provider_create_from_memspace)(
+        umf_memspace_handle_t memspace, void **memoryTargets, size_t numTargets,


what if memspace consists of multiple memory_targets? How we will choose which memory_target should we ask to create a pool or provider from memspace?

That's why we only support memory targets of the same type for now. See here:

unified-memory-framework/src/memspace.c

Line 68 in 9c4e3da

assert(verifyMemTargetsTypes(memspace) == UMF_RESULT_SUCCESS);

I understand that today we support only memory targets of the same type (same ops structure). My question is mostly about the architecture/design, not the current implementation. if we are creating pool/provider from memspace perhaps the current design is wrong and memspace should be responsible for pool/provider creation? In the future I can imagine memspace which consists of GPU memory target and numa memory target - in that case I expect that we will choose GPU memory provider (L0 or cuda) and create USM shared pool.

What do you think?

src/memspaces/memspace_numa.c

igchor force-pushed the memspace_api branch from 4273771 to 45266eb Compare December 2, 2023 02:27

igchor force-pushed the memspace_api branch 4 times, most recently from 402c168 to c61e914 Compare December 4, 2023 21:05

igchor commented Dec 4, 2023

View reviewed changes

include/umf/memspaces/memspace_numa.h Outdated Show resolved Hide resolved

igchor force-pushed the memspace_api branch 2 times, most recently from 1b501c9 to b818231 Compare December 4, 2023 21:15

bratpiorka reviewed Dec 6, 2023

View reviewed changes

include/umf/memspace.h Outdated Show resolved Hide resolved

bratpiorka reviewed Dec 6, 2023

View reviewed changes

include/umf/memspace.h Outdated Show resolved Hide resolved

bratpiorka reviewed Dec 6, 2023

View reviewed changes

include/umf/memspaces/memspace_numa.h Outdated Show resolved Hide resolved

include/umf/memspace_policy.h Outdated Show resolved Hide resolved

src/memory_target_ops.h Outdated Show resolved Hide resolved

src/memspace.c Outdated Show resolved Hide resolved

igchor force-pushed the memspace_api branch 3 times, most recently from d78c8fc to ad09620 Compare December 13, 2023 00:08

igchor marked this pull request as ready for review December 13, 2023 00:10

igchor requested a review from a team as a code owner December 13, 2023 00:10

igchor requested review from vinser52 and bratpiorka December 13, 2023 00:10

vinser52 reviewed Dec 13, 2023

View reviewed changes

kswiecicki force-pushed the memspace_api branch from ad09620 to 0740413 Compare December 13, 2023 17:03

igchor force-pushed the memspace_api branch 2 times, most recently from b81f25d to 978584d Compare December 18, 2023 18:24

igchor requested a review from vinser52 December 18, 2023 18:24

igchor force-pushed the memspace_api branch from 978584d to cf50fda Compare December 20, 2023 00:58

bratpiorka requested changes Dec 20, 2023

View reviewed changes

igchor force-pushed the memspace_api branch 2 times, most recently from 4b99aea to 801276c Compare December 20, 2023 17:52

igchor force-pushed the memspace_api branch from 801276c to 9f8ea03 Compare December 20, 2023 17:57

bratpiorka approved these changes Dec 21, 2023

View reviewed changes

vinser52 reviewed Dec 21, 2023

View reviewed changes

igchor force-pushed the memspace_api branch from 9f8ea03 to a19c2b1 Compare December 21, 2023 16:33

bratpiorka requested a review from vinser52 January 3, 2024 14:52

igchor force-pushed the memspace_api branch from 88ecca1 to 74b0ddd Compare January 9, 2024 16:06

igchor force-pushed the memspace_api branch from 74b0ddd to 8e4b060 Compare January 9, 2024 16:27

igchor and others added 5 commits January 9, 2024 19:55

Initial memspace and memory_target APIs

c6a13e6

Add provider/pool create from memspace functions

e946061

Add memspace create from numa array

895d918

Add numa memspace tests

285e557

Make memspace creation/destruction functions private

9c4e3da

igchor force-pushed the memspace_api branch from 8e4b060 to 9c4e3da Compare January 9, 2024 18:55

vinser52 reviewed Jan 11, 2024

View reviewed changes

src/memspaces/memspace_numa.c Show resolved Hide resolved

vinser52 approved these changes Jan 11, 2024

View reviewed changes

vinser52 merged commit da4ae27 into oneapi-src:main Jan 11, 2024

Initial memspace and memory_target APIs #53

Initial memspace and memory_target APIs #53

Uh oh!

Conversation

igchor commented Dec 1, 2023

Uh oh!

vinser52 commented Dec 4, 2023

Uh oh!

Uh oh!

igchor commented Dec 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igchor commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bratpiorka commented Dec 21, 2023

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!

bratpiorka commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igchor commented Jan 9, 2024

Uh oh!

igchor commented Jan 10, 2024

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!

Uh oh!

Uh oh!

bratpiorka commented Jan 9, 2024 •

edited

Loading