[SYCL] Usm allocator refactor #7785

igchor · 2022-12-14T20:28:48Z

This commit makes USMAllocator unaware of the memory type on which it is operating. The user of USMAllocator is responsible
for setting appropriate options for each instance of USMAllocator.

This is the first step to making USMAllocator generic. In the next step, I'd like to start integration with Unified Memory Allocation (oneMemory). USMAllocator would be used as a heap manager by UMA.

smaslov-intel · 2022-12-16T03:13:55Z

@igchor : rather than adding new stuff into L0 Plugin, could you first move the USM allocator into Unified Runtime please?

igchor · 2022-12-16T17:57:43Z

@igchor : rather than adding new stuff into L0 Plugin, could you first move the USM allocator into Unified Runtime please?

Sure, done: #7820
I've also moved some move utils from level_zero to UR so that we can implement ur_context, etc: #7821

smaslov-intel

Update after #7820 is merged

igchor · 2022-12-19T21:06:53Z

Update after #7820 is merged

Done.

smaslov-intel · 2023-01-19T00:21:46Z

@rdeodhar : please help review the change to USM allocator

jandres742 · 2023-01-24T06:07:15Z

sycl/plugins/unified_runtime/ur/usm_allocator.cpp


    // Generate buckets sized such as: 64, 96, 128, 192, ..., CutOff.
    // Powers of 2 and the value halfway between the powers of 2.
-    auto Size1 = USMSettings.MinBucketSize[MemHandle->getMemType()];
+    auto Size1 = params.MinBucketSize;


would this affect performance during this period of transition, if MinBucketSize may be different depending on the memory type? See here

llvm/sycl/plugins/unified_runtime/ur/usm_allocator.cpp

Line 101 in 8759d07

MinBucketSize[MemType::Host] = 64;

No, in fact, my goal was to keep the original behavior intact. This assignment is just in a different place: https://github.com/intel/llvm/blob/51609155370086670b74ba99f3724d3d8d340852/sycl/plugins/unified_runtime/ur/usm_allocator_config.cpp

In the original implementation, those variables (MinBucketSize, Capacity, itp.) were stored in a single global structure and indexed using [getMemType()]. I have moved those variables to a separate class: USMAllocatorParameters and I'm now passing instance of this class to the USMAllocContext as you can see here: https://github.com/intel/llvm/blob/51609155370086670b74ba99f3724d3d8d340852/sycl/plugins/level_zero/pi_level_zero.cpp#L844

Basically, instead of having Structure of Arrays:

class SetLimits() { ... size_t MinBucketSize[MemType::All]; };

we now have Array of Structs:

class USMAllocatorParameters { ... size_t MinBucketSize; } configs[MemType::All];

The goal of this is to split the pool management logic from reading/setting the configuration since this might differ for other adapters.

thanks @igchor.

jandres742 · 2023-01-24T06:07:49Z

sycl/plugins/unified_runtime/ur/usm_allocator.cpp

-  size_t SlabMinSize() {
-    return USMSettings.SlabMinSize[(*MemHandle).getMemType()];
-  };
+  size_t SlabMinSize() { return params.SlabMinSize; };


same comment about performance.

Please see my previous answer

jandres742 · 2023-01-24T06:09:26Z

sycl/plugins/unified_runtime/ur/usm_allocator.cpp

+      if (OwnAllocCtx.getParams().limits->TotalSize.compare_exchange_strong(
+              PoolSize, NewPoolSize)) {
+        if (chunkedBucket)
+          ++chunkedSlabsInPool;


how is this protected now that the lock at the beginning of this function was removed?

My assumption was that the PoolLock was here only to protect CurPoolSize update (since this is a global variable, shared between all instances of USMAllocContext. This would match the comment: https://github.com/intel/llvm/blob/sycl/sycl/plugins/unified_runtime/ur/usm_allocator.cpp#L62 Now, instead of this lock I just rely on CAS to update the value.

As for the remaining variables (e.g. chunkedSlabsInPool) they should be protected by BucketLock which must be taken before calling CanPool

thanks @igchor. if that's the case, then yes, we would be ok.

jandres742 · 2023-01-24T06:10:13Z

sycl/plugins/unified_runtime/ur/usm_allocator.cpp

@@ -885,11 +622,11 @@ size_t Bucket::Capacity() {
  if (getSize() <= ChunkCutOff())
    return 1;
  else
-    return USMSettings.Capacity[getMemType()];
+    return OwnAllocCtx.getParams().Capacity;


how we will handle different values for Capacity?

Please see my previous answer

This is the first step to make USMAllocator generic and usable for different plugins/adapters. This commit makes USMAllocator unaware of memory type on which it is operating. User of USMAllocator is responsible for setting appropriate options for each instance of USMAllocator.

all memory types when MemType::All is specified. Previously, SharedReadyOnly was not updated in MemParser.

since TotalSize is now updated atomically, there is no need for additional lock. All other (per-bucket) stats are protected by a per-bucket lock.

igchor · 2023-01-30T22:29:20Z

@rdeodhar : please help review the change to USM allocator

@rdeodhar would you please take a look?

igchor · 2023-02-03T16:59:04Z

@intel/llvm-gatekeepers @againull @smaslov-intel is there something pending to merge this?

bader · 2023-02-03T17:46:09Z

@intel/llvm-gatekeepers @againull @smaslov-intel is there something pending to merge this?

@smaslov-intel requested changes. He must approve before it can be merged.

smaslov-intel

LGTM

smaslov-intel · 2023-02-06T17:24:22Z

@intel/llvm-gatekeepers @againull @smaslov-intel is there something pending to merge this?

@smaslov-intel requested changes. He must approve before it can be merged.

I think this is good to go in.

bader · 2023-02-06T17:39:43Z

Approval from @intel/llvm-reviewers-runtime team is required.

igchor requested a review from a team as a code owner December 14, 2022 20:28

againull requested review from smaslov-intel and rdeodhar December 16, 2022 02:01

smaslov-intel suggested changes Dec 19, 2022

View reviewed changes

igchor force-pushed the usm_allocator_refactor branch from d84b2d9 to a78068d Compare December 19, 2022 20:54

igchor requested a review from a team as a code owner December 19, 2022 20:54

igchor requested a review from KseniyaTikhomirova December 19, 2022 20:54

igchor requested review from smaslov-intel and removed request for rdeodhar and KseniyaTikhomirova January 18, 2023 21:33

smaslov-intel requested a review from rdeodhar January 19, 2023 00:21

igchor mentioned this pull request Jan 23, 2023

Move Unified Memory Allocation sources to UR-common oneapi-src/unified-runtime#182

Merged

jandres742 reviewed Jan 24, 2023

View reviewed changes

igchor added 3 commits January 30, 2023 22:16

[SYCL] usm_allocator: set properties for pools of

0e28d38

all memory types when MemType::All is specified. Previously, SharedReadyOnly was not updated in MemParser.

[SYCL] remove spin lock from usm_allocator

a684627

since TotalSize is now updated atomically, there is no need for additional lock. All other (per-bucket) stats are protected by a per-bucket lock.

igchor force-pushed the usm_allocator_refactor branch from 5160915 to e684a78 Compare January 30, 2023 22:24

igchor temporarily deployed to aws January 30, 2023 22:29 — with GitHub Actions Inactive

igchor force-pushed the usm_allocator_refactor branch from e684a78 to a684627 Compare January 30, 2023 22:36

igchor temporarily deployed to aws January 30, 2023 23:01 — with GitHub Actions Inactive

igchor temporarily deployed to aws January 30, 2023 23:32 — with GitHub Actions Inactive

smaslov-intel approved these changes Feb 6, 2023

View reviewed changes

steffenlarsen approved these changes Feb 6, 2023

View reviewed changes

igchor mentioned this pull request Feb 7, 2023

[ur] Implement UMA memory pool: usmPool oneapi-src/unified-runtime#217

Closed

bader merged commit edd9002 into intel:sycl Feb 7, 2023

igchor deleted the usm_allocator_refactor branch February 7, 2023 18:15

[SYCL] Usm allocator refactor #7785

[SYCL] Usm allocator refactor #7785

Uh oh!

Conversation

igchor commented Dec 14, 2022

Uh oh!

smaslov-intel commented Dec 16, 2022

Uh oh!

igchor commented Dec 16, 2022

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

igchor commented Dec 19, 2022

Uh oh!

smaslov-intel commented Jan 19, 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!

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!

igchor commented Jan 30, 2023

Uh oh!

igchor commented Feb 3, 2023

Uh oh!

bader commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

smaslov-intel commented Feb 6, 2023

Uh oh!

bader commented Feb 6, 2023

Uh oh!

Uh oh!

bader commented Feb 3, 2023 •

edited

Loading