[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);
----------------
kevinsala wrote:
> 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
Fix to my previous comment:
```
    template <typename Func>
    ResourceRef getResource(Func processor)
    {
        std::lock_guard<std::mutex> Lock(Mutex);
        ResourceRef ref = ...;
        processor(ref);
        return ref;
    }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154523