[Openmp-commits] [PATCH] D154523: [OpenMP][AMDGPU] Tracking of busy HSA queues

[Kevin Sala via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1478-1481
+    std::unique_lock<std::mutex> Lock(Mutex);
+    (*Resource).Queue->removeUser();
+    Lock.unlock();
+    GenericDeviceResourceManagerTy::returnResource(Resource);
----------------
mhalk wrote:
> Rather unhappy with this particular piece of code.
> Now there's a chance for interleaving.
> I don't know if that's good, "ok" or bad, but if I had to guess: the latter.
> 
> But without unlocking, this will cause a deadlock.
> (Could also revert to a `lock_guard` and call it's dtor, if preferred.)
> 
> Thoughts and comments on this would be highly appreciated.
Yes, this seems correct, but I agree that it may produce unnecessary overhead.

To cover this use case, we could extend the generic `getResource` and `returnResource` to accept an (optional) template functor that processes the element before returning (i.e., with the lock acquired). Something like:

```
class GenericDeviceResourceManagerTy {
protected:
    template <typename Func>
    ResourceRef getResourceAndProcess(Func processor)
    {
        ResourceRef ref = ...;
        processor(ref);
        return ref;
    }

public:
    virtual ResourceRef getResource()
    {
        return getResource([](ResourceRef &) { /* do nothing */ });
    }
}
```

And your `getResource` function would look like something like:

```
ResourceRef getResource() override {
    return GenericDeviceResourceManagerTy::getResource(
        [this](ResourceRef &ref) {
            assignNextQueue(ref);
        });
}
```

I will implement this change in another patch. For the moment, this look fine to me. Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154523/new/

https://reviews.llvm.org/D154523