[SYCL] Remove unnecessary mutex locks from release and retain functions #6192

againull · 2022-05-25T17:16:53Z

Commit removes mutex locks from PI release and retain functions. They
were added under impression that 2 threads working simultaneously on
releasing resources can both reach ref count 0 and cause double free.
But since our ref count is atomic and decrement is an atomic operation
only single thread will execute code under if (--(PiObj->RefCount) == 0).
There can't be 2 threads which can reach ref count 0.

Behavior is undefined if DPCPP runtime somehow uses deleted pi object
(with ref count 0) or provides it as an argument to any of the pi functions.

Commit removes mutex locks from PI release and retain functions. They were added under impression that 2 threads working simultaneously on releasing resources can both reach ref count 0 and cause double free. But since our ref count is atomic and decrement is an atomic operation only single thread will execute code under if (--(PiObj->RefCount) == 0). There can't be 2 threads which can reach ref count 0. Behavior is undefined if DPCPP runtime calls somehow uses deeleted pi object (with ref count 0) or provides it as an argument to any of the pi functions.

aelovikov-intel · 2022-05-25T18:12:21Z

sycl/plugins/level_zero/pi_level_zero.cpp

+  // of the Level Zero plugin. Behavior is undefined if DPCPP runtime calls
+  // piQueueRelease on a queue with external ref count 0 or provides such queue
+  // as an argument to any of the pi functions.
+  if (--(Queue->RefCountExternal) == 0) {


nit: I'd personally use fetch_sub interface as not every reader is aware (I certainly wasn't one of them) that operator-- provides the guarantees and it's more likely they'll "cppreference" the member function than an overloaded operator.

aelovikov-intel · 2022-05-25T18:39:19Z

sycl/plugins/level_zero/pi_level_zero.cpp

-  if (RefCountZero)
+  // Only single thread can reach ref count equal to 0. This means that
+  // the code under if is executed by single thread.
+  if (--(Sampler->RefCount) == 0) {


nit: I'd do an early return here for consistency of the pattern.

aelovikov-intel · 2022-05-25T18:41:38Z

sycl/plugins/level_zero/pi_level_zero.cpp

-// on the Queue that is passed in.
-inline static void piQueueRetainNoLock(pi_queue Queue) { Queue->RefCount++; }
+// This helper function increments the internal reference counter of the Queue.
+inline static void piQueueRetainInternal(pi_queue Queue) { Queue->RefCount++; }


Why can't we just inline it?

aelovikov-intel · 2022-05-25T18:43:32Z

sycl/plugins/level_zero/pi_level_zero.cpp

  Queue->RefCountExternal++;
-  piQueueRetainNoLock(Queue);
+  piQueueRetainInternal(Queue);


Do you know why we have to counters? Any chance that it's because we assumed we needed a whole lock for one but not for another? If so, we might be able to just merge them in this PR.

If I don't miss anything, we can't merge these ref counts. External ref count equal to 0 means that there are no external references anymore and we synchronize the queue and reset command lists in this queue. It is possible that at this point internal ref count is still not zero. After all internal references to this queue disappear, i.e. internal ref count = 0 then we can delete the queue.
Shortly, external ref count is used to detect the point when we can synchronize and internal ref count is used to detect the point when we can delete.

aelovikov-intel · 2022-05-25T18:43:56Z

sycl/plugins/level_zero/pi_level_zero.cpp

+  // of the Level Zero plugin. Behavior is undefined if DPCPP runtime calls
+  // piQueueRelease on a queue with external ref count 0 or provides such queue
+  // as an argument to any of the pi functions.
+  if (--(Queue->RefCountExternal) == 0) {


Early return please.

aelovikov-intel · 2022-05-25T18:45:12Z

sycl/plugins/level_zero/pi_level_zero.cpp

  return PI_SUCCESS;
 }

-static pi_result QueueRelease(pi_queue Queue, pi_queue LockedQueue) {
+static pi_result piQueueReleaseInternal(pi_queue Queue, pi_queue LockedQueue) {
  PI_ASSERT(Queue, PI_INVALID_QUEUE);
  PI_ASSERT(Queue->RefCount, PI_INVALID_QUEUE);


I think you've removed a similar assert somewhere else in this patch. Why not here?

smaslov-intel · 2022-05-26T06:24:52Z

Behavior is undefined if DPCPP runtime somehow uses deleted pi object
(with ref count 0) or provides it as an argument to any of the pi functions.

But SYCL RT does not have visibility of ref-count or when the object is destroyed. It is true, however, that SYCL RT cannot use PI objects that it had released it's own last reference to (there can be other references to it, e.g. from other PI objects).

There can't be 2 threads which can reach ref count 0.

This sounds right to me, only one thread can get to 0. But is this change just a performance optimization or something more? What if we pretend the ref-count is not atomic and continue to guard it's access, would it be still correct?

smaslov-intel · 2022-05-31T03:54:28Z

sycl/plugins/level_zero/pi_level_zero.hpp

@@ -239,13 +239,46 @@ class pi_shared_mutex : public std::shared_mutex {
  }
 };

+// This wrapper around std::atomic is created to limit operations with reference
+// counter and to make allowed operations more transparent in terms of
+// thread-safety in the plugin.


Please explain in comment that these operations do not need a mutex guard around them since the underlying data is already atomic (plus extra semantics that is explained for "decrement_and_count")

Added comment.

sycl/plugins/level_zero/pi_level_zero.hpp

sycl/plugins/level_zero/pi_level_zero.cpp

HaohaiWen · 2022-06-01T01:58:45Z

sycl/plugins/level_zero/pi_level_zero.cpp

  return PI_SUCCESS;
 }

-static pi_result QueueRelease(pi_queue Queue, pi_queue LockedQueue) {
+static pi_result piQueueReleaseInternal(pi_queue Queue, pi_queue LockedQueue) {


LockedQueue has no user. Consider to delete it or add (void)LockedQueue?
error: unused parameter 'LockedQueue'

intel#6192 removed the need for the LockedQueue parameter in piQueueReleaseInternal. These changes remove the parameter from the function. Signed-off-by: Larsen, Steffen <[email protected]>

againull requested a review from a team as a code owner May 25, 2022 17:16

againull requested review from smaslov-intel and aelovikov-intel May 25, 2022 17:17

aelovikov-intel reviewed May 25, 2022

View reviewed changes

againull mentioned this pull request May 25, 2022

[SYCL] Prework before protecting access to the event objects with a mutex #6179

Merged

Address review comments

edf513c

Introduce wrapper around atomic variable

5f71565

smaslov-intel reviewed May 31, 2022

View reviewed changes

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

smaslov-intel reviewed May 31, 2022

View reviewed changes

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

smaslov-intel reviewed May 31, 2022

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Show resolved Hide resolved

againull added 2 commits May 31, 2022 10:01

[NFC] Address review comments

b055130

Rename method

bf0c596

smaslov-intel approved these changes May 31, 2022

View reviewed changes

againull merged commit b545c60 into intel:sycl May 31, 2022

HaohaiWen reviewed Jun 1, 2022

View reviewed changes

steffenlarsen mentioned this pull request Jun 1, 2022

[SYCL][L0] Remove unused argument from piQueueReleaseInternal #6226

Closed

againull deleted the remove_lock_from_release branch December 3, 2022 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Remove unnecessary mutex locks from release and retain functions #6192

[SYCL] Remove unnecessary mutex locks from release and retain functions #6192

Uh oh!

againull commented May 25, 2022 •

edited

Loading

Uh oh!

aelovikov-intel May 25, 2022

Uh oh!

aelovikov-intel May 25, 2022

Uh oh!

aelovikov-intel May 25, 2022

Uh oh!

aelovikov-intel May 25, 2022

Uh oh!

againull May 25, 2022 •

edited

Loading

Uh oh!

aelovikov-intel May 25, 2022

Uh oh!

aelovikov-intel May 25, 2022

Uh oh!

smaslov-intel commented May 26, 2022

Uh oh!

smaslov-intel May 31, 2022

Uh oh!

againull May 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HaohaiWen Jun 1, 2022

Uh oh!

Uh oh!

[SYCL] Remove unnecessary mutex locks from release and retain functions #6192

[SYCL] Remove unnecessary mutex locks from release and retain functions #6192

Uh oh!

Conversation

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

Uh oh!

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!

againull May 25, 2022 • 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!

smaslov-intel commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

againull commented May 25, 2022 •

edited

Loading

againull May 25, 2022 •

edited

Loading