[SYCL] Guard access to memory objects with a mutex #5231

againull · 2021-12-27T23:14:12Z

No description provided.

smaslov-intel · 2022-02-23T17:39:43Z

sycl/plugins/level_zero/pi_level_zero.hpp

@@ -839,6 +839,10 @@ struct _pi_mem : _pi_object {
    size_t Size;
  };

+  // Protects accesses to all the non-const member variables.  Exclusive access
+  // is required to modify any of these members.
+  std::shared_mutex Mutex;


NIT: I think eventually we may want to move the Mutex to base _pi_object as I expect every PI object needs this.

Moved to pi_object.

smaslov-intel · 2022-02-23T17:41:59Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -403,7 +403,6 @@ static bool PiDriverModuleProgramExtensionFound = false;
 // write (as it is OK for multiple threads to read the map without locking it).

 pi_result _pi_mem::addMapping(void *MappedTo, size_t Offset, size_t Size) {


Update comments in the header that the "this->Mutex" should be locked before calling this.
Maybe, in fact, inline these functions and remove these methods

Inlined these functions.

smaslov-intel · 2022-02-23T18:05:07Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -6001,6 +6008,10 @@ pi_result piEnqueueMemBufferCopy(pi_queue Queue, pi_mem SrcBuffer,
                                 pi_event *Event) {
  PI_ASSERT(SrcBuffer && DstBuffer, PI_INVALID_MEM_OBJECT);
  PI_ASSERT(Queue, PI_INVALID_QUEUE);
+
+  std::scoped_lock Lock(Queue->PiQueueMutex, SrcBuffer->Mutex,


The "std::scoped_lock" will lock all mutex for exclusive use, which looks like unnecessary many times.
Can we adopt a more fine-grain approach, like this:

std::shared_lock L1(SrcBuffer->Mutex, std::defer_lock); std::unique_lock L2(DstBuffer->Mutex, std::defer_lock); std::unique_lock L3(Queue->Mutex, std::defer_lock); std::scoped_lock lockall(L1, L2, L3);

?

Good call, thanks, fixed.

smaslov-intel · 2022-02-23T18:17:50Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -3595,13 +3593,15 @@ pi_result piMemGetInfo(pi_mem Mem,
  (void)ParamValueSize;
  (void)ParamValue;
  (void)ParamValueSizeRet;
+  std::shared_lock Guard(Mem->Mutex);


Do we even need this lock?

Probably you are right, removed for now. Can be added if necessary.

smaslov-intel · 2022-02-23T18:32:50Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -5762,6 +5770,7 @@ pi_result piEnqueueMemBufferRead(pi_queue Queue, pi_mem Src,
  PI_ASSERT(Src, PI_INVALID_MEM_OBJECT);
  PI_ASSERT(Queue, PI_INVALID_QUEUE);

+  std::scoped_lock Lock(Queue->PiQueueMutex, Src->Mutex);


we can only lock "Src" for shared use, right?

Good call, fixed.

smaslov-intel · 2022-02-23T19:59:57Z

sycl/plugins/level_zero/pi_level_zero.cpp

+// TODO:: In the following 2 methods we may want to distinguish read access vs.
 // write (as it is OK for multiple threads to read the map without locking it).


Remove comment

smaslov-intel · 2022-02-23T20:03:59Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -5805,8 +5792,6 @@ static pi_result enqueueMemCopyHelper(pi_command_type CommandType,
                                      pi_event *Event, bool PreferCopyEngine) {
  PI_ASSERT(Queue, PI_INVALID_QUEUE);
  PI_ASSERT(Event, PI_INVALID_EVENT);
-  // Lock automatically releases when this goes out of scope.
-  std::lock_guard<std::mutex> lock(Queue->PiQueueMutex);


Adjust the comment at line 5798

smaslov-intel · 2022-02-23T20:04:41Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -5868,9 +5853,6 @@ static pi_result enqueueMemCopyRectHelper(
  PI_ASSERT(Region && SrcOrigin && DstOrigin && Queue, PI_INVALID_VALUE);
  PI_ASSERT(Event, PI_INVALID_EVENT);

-  // Lock automatically releases when this goes out of scope.
-  std::lock_guard<std::mutex> lock(Queue->PiQueueMutex);


adjust the comment at 5859

smaslov-intel · 2022-02-23T20:06:51Z

sycl/plugins/level_zero/pi_level_zero.cpp

+  std::shared_lock SrcLock(SrcBuffer->Mutex, std::defer_lock);
+  std::unique_lock DstLock(DstBuffer->Mutex, std::defer_lock);
+  std::unique_lock QueueLock(Queue->Mutex, std::defer_lock);
+  std::scoped_lock LockAll(SrcLock, DstLock, QueueLock);


This is shorter, although I am not sure if it reads better:

Suggested change

std::shared_lock SrcLock(SrcBuffer->Mutex, std::defer_lock);

std::unique_lock DstLock(DstBuffer->Mutex, std::defer_lock);

std::unique_lock QueueLock(Queue->Mutex, std::defer_lock);

std::scoped_lock LockAll(SrcLock, DstLock, QueueLock);

std::shared_lock SrcLock(SrcBuffer->Mutex, std::defer_lock);

std::scoped_lock LockAll(SrcLock, DstBuffer->Mutex, Queue->Mutex);

Thanks, used shorter version.

smaslov-intel · 2022-02-23T20:09:40Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -6317,7 +6337,7 @@ pi_result piEnqueueMemUnmap(pi_queue Queue, pi_mem MemObj, void *MappedPtr,

      {
        // Lock automatically releases when this goes out of scope.
-        std::lock_guard<std::mutex> lock(Queue->PiQueueMutex);
+        std::scoped_lock lock(Queue->Mutex);


Suggested change

std::scoped_lock lock(Queue->Mutex);

std::shared_lock lock(Queue->Mutex);

I agree, fixed.

smaslov-intel · 2022-02-23T20:11:09Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -6326,6 +6346,7 @@ pi_result piEnqueueMemUnmap(pi_queue Queue, pi_mem MemObj, void *MappedPtr,
      }
    }

+    std::shared_lock Guard(MemObj->Mutex);


The "memcpy" below potentially modifies the data of the MemObj, right?

Suggested change

std::shared_lock Guard(MemObj->Mutex);

std::scoped_lock Guard(MemObj->Mutex);

I agree, fixed.

smaslov-intel · 2022-02-23T20:16:44Z

sycl/plugins/level_zero/pi_level_zero.hpp

@@ -206,6 +206,10 @@ struct _pi_object {
  // Level Zero doesn't do the reference counting, so we have to do.
  // Must be atomic to prevent data race when incrementing/decrementing.
  std::atomic<pi_uint32> RefCount;
+
+  // Protects accesses to all the non-const member variables.  Exclusive access
+  // is required to modify any of these members.


Please add examples here how to properly lock things (including multiple objects, and read/write), so that people use the same syntax more or less.

I think we should add a mode where single-threaded app doesn't pay the overhead of working with many mutexes. Please see how this can get organized. Maybe wrap the "std::shared_mutex" into a class that would redefine mutex operations to no-op under some env var.

Added examples and single-threaded mode.

smaslov-intel

Few more comments

smaslov-intel · 2022-02-24T15:45:24Z

sycl/plugins/level_zero/pi_level_zero.hpp

+// A single-threaded app has an opportunity to enable this mode to avoid
+// overhead from mutex locking.
+static const bool SingleThreadMode = [] {
+  return std::getenv("SYCL_PI_LEVEL_ZERO_SINGLE_THREAD_MODE") != nullptr;


I suggest that we distinguish "0" as disabling ST mode (it it convenient to switch between 0 and 1 when debugging)

smaslov-intel

LGTM. Just few comments about new env var

againull · 2022-02-24T19:45:40Z

Failed Tests (1): SYCL :: Assert/assert_in_simultaneous_kernels.cpp

Where can I find the logs?

I was able to reproduce this failure by running make check-sycl-all multiple times, it can't be reproduced by running a single test or by single make check run.
Log is here: https://github.com/intel/llvm/runs/5312116716?check_suite_focus=true

againull · 2022-02-25T00:56:36Z

Please don't merge this PR yet, I am double checking SYCL :: Assert/assert_in_simultaneous_kernels.cpp failure.

againull · 2022-02-25T05:53:13Z

Actually, it looks like I can reproduce Assert/assert_in_simultaneous_kernels.cpp failure only with this PR. But it requires running make check-sycl-all many times. I have no idea how to analyze such type of failure. Please let me know if you have any ideas.

It doesn't look to me that these changes themselves can cause a failure. More likely these changes change timing and this makes some existing problem to appear more often. I don't have an idea how to prove that.

smaslov-intel · 2022-02-25T15:12:23Z

Actually, it looks like I can reproduce Assert/assert_in_simultaneous_kernels.cpp failure only with this PR. But it requires running make check-sycl-all many times. I have no idea how to analyze such type of failure. Please let me know if you have any ideas.

It doesn't look to me that these changes themselves can cause a failure. More likely these changes change timing and this makes some existing problem to appear more often. I don't have an idea how to prove that.

I suggest you try https://www.intel.com/content/www/us/en/developer/tools/oneapi/inspector.html to pinpoint the issue,

bader · 2022-03-03T11:52:24Z

Moved to a draft - drafts can't be merged. Artur, please, convert to a pull request when it's ready for merge.

againull · 2022-03-08T21:34:18Z

Fixed Assert/assert_in_simultaneous_kernels.cpp regression by this commit: 786dc4b

againull · 2022-03-09T00:40:50Z

@intel/dpcpp-doc-reviewers Could you please review doc changes.

pvchupin

doc ok

This reverts commit 3321141.

[SYCL] Guard access to memory objects with a mutex

ef17116

againull marked this pull request as ready for review January 4, 2022 20:45

againull requested a review from a team as a code owner January 4, 2022 20:45

againull added 2 commits February 22, 2022 15:43

Merge remote-tracking branch 'origin/sycl' into mem_mutex

988e14f

Use shared_lock to separate shared read and exclusive read/write access

00d4aa3

againull requested a review from smaslov-intel February 23, 2022 17:06

smaslov-intel reviewed Feb 23, 2022

View reviewed changes

Inline addMapping/removeMapping and move mutex to the pi_object class

c8c8564

smaslov-intel reviewed Feb 23, 2022

View reviewed changes

againull added 2 commits February 23, 2022 11:19

Use more fine-grained approach to lock mutexes

2d3ad92

Replace PiQueueMutex with Mutex

acb3bce

againull requested a review from smaslov-intel February 23, 2022 19:40

smaslov-intel reviewed Feb 23, 2022

View reviewed changes

Remove comment

20fc7fb

againull requested a review from smaslov-intel February 23, 2022 20:03

smaslov-intel reviewed Feb 23, 2022

View reviewed changes

smaslov-intel suggested changes Feb 23, 2022

View reviewed changes

againull added 4 commits February 23, 2022 13:15

Adjust comments

071d1bf

Use shorter style to lock multiple mutexes and add examples

3b66b4b

Add single-threaded mode

8d150ac

Fix to exclusive access in piEnqueueMemUnmap

cbcca19

againull requested a review from a team as a code owner February 23, 2022 23:32

smaslov-intel reviewed Feb 24, 2022

View reviewed changes

smaslov-intel suggested changes Feb 24, 2022

View reviewed changes

Turn env variable into an integer

f543be3

againull requested a review from smaslov-intel February 24, 2022 20:16

smaslov-intel previously approved these changes Feb 24, 2022

View reviewed changes

Include header for std::stoi

61b1cbc

againull dismissed smaslov-intel’s stale review via 61b1cbc February 24, 2022 21:26

smaslov-intel previously approved these changes Feb 24, 2022

View reviewed changes

againull changed the title ~~[SYCL] Guard access to memory objects with a mutex~~ [DO NOT MERGE][SYCL] Guard access to memory objects with a mutex Feb 25, 2022

bader marked this pull request as draft March 3, 2022 11:43

bader changed the title ~~[DO NOT MERGE][SYCL] Guard access to memory objects with a mutex~~ [SYCL] Guard access to memory objects with a mutex Mar 3, 2022

againull added 2 commits March 3, 2022 15:59

Merge remote-tracking branch 'origin/sycl' into mem_mutex

addfc7d

Fix missing queue locks

786dc4b

againull dismissed smaslov-intel’s stale review via 786dc4b March 7, 2022 22:28

againull added 2 commits March 8, 2022 13:32

Fix formatting

2c0e864

Merge remote-tracking branch 'origin/sycl' into mem_mutex_publish

43f534a

againull marked this pull request as ready for review March 8, 2022 21:33

smaslov-intel approved these changes Mar 8, 2022

View reviewed changes

pvchupin approved these changes Mar 9, 2022

View reviewed changes

againull merged commit 3321141 into intel:sycl Mar 9, 2022

bader mentioned this pull request Mar 11, 2022

[CI] Revert w/a for FPGA emu bug #5775

Closed

againull added a commit to againull/llvm that referenced this pull request Mar 11, 2022

Revert "[SYCL] Guard access to memory objects with a mutex (intel#5231)"

92b7259

This reverts commit 3321141.

againull deleted the mem_mutex branch December 3, 2022 00:14

		@@ -403,7 +403,6 @@ static bool PiDriverModuleProgramExtensionFound = false;
		// write (as it is OK for multiple threads to read the map without locking it).

		pi_result _pi_mem::addMapping(void *MappedTo, size_t Offset, size_t Size) {

		// TODO:: In the following 2 methods we may want to distinguish read access vs.
		// write (as it is OK for multiple threads to read the map without locking it).

	std::scoped_lock lock(Queue->Mutex);
	std::shared_lock lock(Queue->Mutex);

	std::shared_lock Guard(MemObj->Mutex);
	std::scoped_lock Guard(MemObj->Mutex);

[SYCL] Guard access to memory objects with a mutex #5231

[SYCL] Guard access to memory objects with a mutex #5231

Uh oh!

Conversation

againull commented Dec 27, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaslov-intel left a comment

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!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

againull commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

againull commented Feb 25, 2022

Uh oh!

againull commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smaslov-intel commented Feb 25, 2022

Uh oh!

bader commented Mar 3, 2022

Uh oh!

againull commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

againull commented Mar 9, 2022

againull commented Feb 24, 2022 •

edited

Loading

againull commented Feb 25, 2022 •

edited

Loading

againull commented Mar 8, 2022 •

edited

Loading