[SYCL] USM shared memory allocator for L0 plugin #2366

ilyastepykin · 2020-08-25T09:39:53Z

In L0 each allocation results in at least a memory page
no matter how small the size was requested. This adds
significant overhead when an app does many small allocations.

This patch adds a memory allocator on top of L0 USM API in
order to solve the problem.

High level description:
There is a predefine list of bucket sizes. When allocation function
is called, the best fitted bucket is found for the requested size.
If there is a free chunk in the bucket then it's returned, otherwise
a new slab of size 64k requested from the system via L0 API.
This slab is split into chunks of the size of corresponding bucket.
Now there is a free chunk in the slab which is returned.
When a chunk is returned it's marked as non-free until the user
call free with a corresponding pointer.

In addition to that add an environment variable SYCL_PI_LEVEL0_DISABLE_USM_ALLOCATOR
which disables the allocator and returns back to the original behavior.

Signed-off-by: Ilya Stepykin ilya.stepykin@intel.com

sycl/plugins/level_zero/pi_level_zero.hpp

sycl/plugins/level_zero/pi_level_zero.cpp

sycl/plugins/level_zero/pi_level_zero.hpp

sycl/plugins/level_zero/pi_level_zero.cpp

sycl/plugins/level_zero/pi_level_zero.hpp

sycl/plugins/level_zero/usm_allocator.cpp

romanovvlad · 2020-09-03T15:58:11Z

sycl/plugins/level_zero/usm_allocator.cpp

+    return Sizes;
+  }
+
+  static const std::vector<size_t> BucketSizes = generateBucketSizes();


It seems values of this vector can be computed at compile time. Can we replace it with std::array then?
Non trivial global objects can cause problems during destruction.

Done, unfortunately I had to hardcode all the sizes instead of writing the loop due to some limitations of std::array(e.g. lack of constexpr operator[] for non-const std::array in C++14 , etc).

sycl/plugins/level_zero/usm_allocator.cpp

In L0 each allocation results in at least a memory page no matter how small the size was requested. This adds significant overhead when an app does many small allocations. This patch adds a memory allocator on top of L0 USM API in order to solve the problem. High level description: There is a predefine list of bucket sizes. When allocation function is called, the best fitted bucket is found for the requested size. If there is a free chunk in the bucket then it's returned, otherwise a new slab of size 64k requested from the system via L0 API. This slab is split into chunks of the size of corresponding bucket. Now there is a free chunk in the slab which is returned. When a chunk is returned it's marked as non-free until the user call free with a corresponding pointer. In addition to that add an environment variable SYCL_PI_LEVEL0_DISABLE_USM_ALLOCATOR which disables the allocator and returns back to the original behavior. Signed-off-by: Ilya Stepykin <ilya.stepykin@intel.com>

smaslov-intel · 2020-09-07T06:57:57Z

@ilyastepykin , because of you "force-pushed" the update changes the incremental review is not possible, please avoid doing so in future.

smaslov-intel · 2020-09-07T07:09:34Z

sycl/plugins/level_zero/pi_level_zero.hpp

@@ -174,6 +228,12 @@ struct _pi_context : _pi_object {
  // and destroy the pool if there are no alive events.
  ze_result_t decrementAliveEventsInPool(ze_event_pool_handle_t pool);

+  // Store USM allocator context(internal allocator structures)
+  // for USM shared/host and device allocations. There is 1 allocator context
+  // per each pair of (context, device) per each memory type.


Why there should be separate allocators for device and shared USM?

They have different allocation function: zeMemAllocShared vs zeMemAllocDevice.
Technically, it's possible to have a single allocator for both of them, but it requires to differentiate and keep track of allocation type. It basically makes the core logic more complex without any advantages of the current approach where we keep allocator per allocation type.

OK, it make sense.

Yes, keeping track of them separately is good.

smaslov-intel · 2020-09-07T07:14:46Z

The support of multiple devices was added: https://github.com/intel/llvm/pull/2343/files, though, it was tested on OpenCL only. @smaslov-intel, for the purposes of LIT tests, can Level Zero emulate multiple devices?

@dm-vodopyanov : the #2343 does not add the support for contexts with multiple devices to Level-Zero plugin, so no, it is not possible to test that with Level-Zero yet.

smaslov-intel

LGTM

There are issues with running these tests on OpenCL on windows. The primary goal of these tests is to test USM allocator which is currently emabled only for Level zero, so disable them on OpenCL for now.

Due to incorrect pointer alignment in case when allcation alignment > size we returned to the user and incorrect chunk(i.e. next to the one which was marked as non-free) which is later detected as "double free" error. Also corrected the existing testcases to test this scenario

jbrodman · 2020-09-08T17:53:44Z

LGTM, but there are a couple of places @romanovvlad wanted initialization to zero that haven't been changed yet.

sycl/doc/EnvironmentVariables.md

pvchupin · 2020-09-08T21:34:11Z

Ok for docs.

pvchupin

Please fix typo.

sycl/doc/EnvironmentVariables.md

Co-authored-by: Pavel Chupin <45979248+pvchupin@users.noreply.github.com>

kbobrovs

Doc part OK

Disable CFI checks when ASAN is specified

ilyastepykin requested review from smaslov-intel and a team as code owners August 25, 2020 09:39

pvchupin requested a review from jbrodman August 25, 2020 14:48

jbrodman previously approved these changes Aug 25, 2020

View reviewed changes

smaslov-intel reviewed Sep 2, 2020

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.hpp Outdated Show resolved Hide resolved

smaslov-intel reviewed Sep 2, 2020

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.hpp Outdated Show resolved Hide resolved

smaslov-intel suggested changes Sep 2, 2020

View reviewed changes

dm-vodopyanov reviewed Sep 3, 2020

View reviewed changes

romanovvlad reviewed Sep 3, 2020

View reviewed changes

ilyastepykin dismissed jbrodman’s stale review via ea836e3 September 3, 2020 19:33

ilyastepykin force-pushed the istepyki/usm-allocator branch from 49a6201 to ea836e3 Compare September 3, 2020 19:33

ilyastepykin requested review from kbobrovs and pvchupin as code owners September 3, 2020 19:33

ilyastepykin force-pushed the istepyki/usm-allocator branch from ea836e3 to d31de69 Compare September 3, 2020 20:00

ilyastepykin changed the title ~~[WIP][SYCL] USM shared memory allocator for L0 plugin~~ [SYCL] USM shared memory allocator for L0 plugin Sep 3, 2020

ilyastepykin force-pushed the istepyki/usm-allocator branch from d31de69 to bf4ab17 Compare September 3, 2020 23:16

ilyastepykin force-pushed the istepyki/usm-allocator branch from bf4ab17 to 769d3b9 Compare September 4, 2020 21:52

smaslov-intel reviewed Sep 7, 2020

View reviewed changes

smaslov-intel previously approved these changes Sep 7, 2020

View reviewed changes

[SYCL] Restrict USM allocators tests to Level zero

3cfe30f

There are issues with running these tests on OpenCL on windows. The primary goal of these tests is to test USM allocator which is currently emabled only for Level zero, so disable them on OpenCL for now.

ilyastepykin dismissed smaslov-intel’s stale review via 3cfe30f September 7, 2020 14:42

smaslov-intel reviewed Sep 8, 2020

View reviewed changes

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved

[SYCL] Minor fix and wording change

0fbffa4

pvchupin approved these changes Sep 8, 2020

View reviewed changes

pvchupin previously approved these changes Sep 8, 2020

View reviewed changes

sycl/doc/EnvironmentVariables.md Outdated Show resolved Hide resolved

Fix a typo

d9dc92b

Co-authored-by: Pavel Chupin <45979248+pvchupin@users.noreply.github.com>

ilyastepykin dismissed pvchupin’s stale review via d9dc92b September 8, 2020 22:09

smaslov-intel approved these changes Sep 9, 2020

View reviewed changes

kbobrovs approved these changes Sep 9, 2020

View reviewed changes

pvchupin merged commit db5037c into intel:sycl Sep 9, 2020

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2366 from RossBrunton/ross/asancfi

4ff27f2

Disable CFI checks when ASAN is specified

[SYCL] USM shared memory allocator for L0 plugin #2366

[SYCL] USM shared memory allocator for L0 plugin #2366

Uh oh!

Conversation

ilyastepykin commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

romanovvlad Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

ilyastepykin Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel commented Sep 7, 2020

Uh oh!

smaslov-intel Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

ilyastepykin Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

smaslov-intel Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

jbrodman Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

smaslov-intel commented Sep 7, 2020

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

jbrodman commented Sep 8, 2020

Uh oh!

Uh oh!

pvchupin commented Sep 8, 2020

Uh oh!

pvchupin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilyastepykin commented Aug 25, 2020 •

edited

Loading