Page MenuHomePhabricator

[OpenMP][Plugin] Introduce generic resource pool
ClosedPublic

Authored by tianshilei1992 on Oct 16 2021, 4:34 PM.

Details

Summary

Currently CUDA streams are managed by StreamManagerTy. It works very well. Now
we have the need that some resources, such as CUDA stream and event, will be
hold by libomptarget. It is always good to buffer those resources. What's more
important, given the way that libomptarget and plugins are connected, we cannot
make sure whether plugins are still alive when libomptarget is destroyed. That
leads to an issue that those resouces hold by libomptarget might not be
released correctly. As a result, we need an unified management of all the resources
that can be shared between libomptarget and plugins.

ResourcePoolTy is designed to manage the type of resource for one device.
It has to work with an allocator which is supposed to provide create and
destroy. In this way, when the plugin is destroyed, we can make sure that
all resources allocated from native runtime library will be released correctly,
no matter whether libomptarget starts its destroy.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Oct 16 2021, 4:34 PM
tianshilei1992 requested review of this revision.Oct 16 2021, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2021, 4:34 PM
ye-luo added inline comments.Oct 16 2021, 5:31 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
195

Too many managers. How about ResourcePoolTy

259

allocate and deallocate are really meant for pointers. How about create/destroy for single object.

330–333

I think we can avoid StreamManagerTy abstraction and have
std::vector<std::unique_ptr<RMTy>> StreamPools;
directly here.

DeviceData, Modules, DeviceAllocators, MemoryManagers don't need additional layer.

ye-luo added inline comments.Oct 16 2021, 5:35 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
246

You may consider acquire/release instead of put/get. I'm open to suggestions.

ye-luo added inline comments.Oct 16 2021, 5:41 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
207

Whe CUDA call fails. Allocator.allocate() returns nullptr and the code continues running. This doesn't make sense.

259

Error handling and returning the object needs to be separate.

tianshilei1992 added inline comments.Oct 16 2021, 6:17 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
207

ResourceManagerTy doesn't know what is a valid return value. Ty can be pointer, object, etc. Of course, for CUDA, everything is pointer, but it doesn't sound right to have if (Ty == nullptr)`.

259

create and destroy sound good.

Error handling and returning the object needs to be separate.

I didn't follow.

ye-luo added inline comments.Oct 16 2021, 7:39 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
207

Allocator.allocate(Ty&) should return OFFLOAD_SUCCESS, OFFLOAD_FAIL.
You have AllocatorTy which knows what Ty is. Indeed, Ty should be removed, it can be derived from AllocatorTy

259

I mean int32_t Allocator.create(Ty& res)
So the return is OFFLOAD_SUCCESS or OFFLOAD_FAIL.
The created resource is returned via res.

rebase and fix comments

tianshilei1992 marked 6 inline comments as done.Dec 26 2021, 3:02 PM

use create/destroy

reduce indirection

tianshilei1992 marked 2 inline comments as done.Dec 26 2021, 3:26 PM
tianshilei1992 edited the summary of this revision. (Show Details)

Free stream pool and allocator explicitly to avoid CUcontext is destroyed before them

ye-luo added inline comments.Dec 26 2021, 4:15 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
191

What does it mean a resource is copyable? Did you mean a resource handle is copyable?

207

If Ty can be accessed from AllocatorTy, we'd better avoid passing Ty as a template argument.

tianshilei1992 marked 2 inline comments as done.Dec 26 2021, 7:08 PM
tianshilei1992 added inline comments.
openmp/libomptarget/plugins/cuda/src/rtl.cpp
207

I did it in another way: to avoid passing AllocatorTy as a template argument.

ye-luo added inline comments.Dec 26 2021, 7:42 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
239

How about making this a member rather than a reference and remove StreamAllocator vector?
ResourcePoolTy(AllocatorTy<T> &&A, size_t Size = 0)

tianshilei1992 marked an inline comment as done.

fix comments

tianshilei1992 marked an inline comment as done.Dec 27 2021, 7:49 AM
tianshilei1992 retitled this revision from [OpenMP][Plugin] Introduce resouce manager to [OpenMP][Plugin] Introduce generic resource pool.
ye-luo accepted this revision.Dec 27 2021, 8:05 AM
This revision is now accepted and ready to land.Dec 27 2021, 8:05 AM

fixed error caused by the order of destructions

This revision was landed with ongoing or failed builds.Dec 27 2021, 8:32 AM
This revision was automatically updated to reflect the committed changes.
ye-luo added inline comments.Dec 27 2021, 8:32 AM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
565–566

Probabaly S.reset() is more C++ way.

tianshilei1992 marked an inline comment as done.Dec 27 2021, 8:33 AM
tianshilei1992 added inline comments.
openmp/libomptarget/plugins/cuda/src/rtl.cpp
565–566

Will make the change in another patch.