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

[Michael Halkenhäuser via Phabricator via Openmp-commits]

mhalk added a comment.

Thanks for checking and letting me know!



================
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:
> 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;
>     }
> ```
Ok, great -- will leave this as it is, for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154523