Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Added pinned memory resource #141

Closed

Conversation

UmashankarTriforce
Copy link

This PR aims to add pinned_memory_resource. Following the specs posted to this issue.

Fixes #136

@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@jrhemstad
Copy link
Contributor

ok to test

@jrhemstad
Copy link
Contributor

We may need to hold off on merging this.

@harrism I realized that using cudaHostAlloc means pinned_memory_resource is no longer a "device" memory resource. Specifically, there are tests for the MRs that explicitly check to ensure the memory is device memory and those will fail.

How do we want to handle this? Work around the test? Introduce a host_memory_resource base class?

@UmashankarTriforce
Copy link
Author

I think introducing host_memory_resource should work out, because not only will it support pinned memory resource, but also for all future methods that would make use of this

@harrism
Copy link
Member

harrism commented Oct 3, 2019

Do we need to distinguish host vs. device at the base class level at all? Can they just all be memory_resource classes with attributes of some sort?

@UmashankarTriforce
Copy link
Author

If we follow that route, do we have any added benefits?

@jrhemstad
Copy link
Contributor

jrhemstad commented Oct 3, 2019

Do we need to distinguish host vs. device at the base class level at all? Can they just all be memory_resource classes with attributes of some sort?

I suspect we'd have a memory_resource base from which host_ and device_ memory resources are derived.

The larger question is, do we want RMM to become a host memory manager as well? This is currently in the README:

RMM is not:
A replacement allocator for host memory (malloc, new, cudaMallocHost, cudaHostRegister).

@harrism
Copy link
Member

harrism commented Oct 3, 2019

That statement from the readme is true until we make it false. It was never intended to be a philosophical statement of value. :) I think if our users need a suballocator for pinned host memory for use in CUDA, then it makes sense to support it.

@UmashankarTriforce
Copy link
Author

Agreed!

I suspect we'd have a memory_resource base from which host_ and device_ memory resources are derived.

That's a wiser approach to take as the common elements will not have to be repeated, and moreover it makes sense to branch this way

@jrhemstad
Copy link
Contributor

That statement from the readme is true until we make it false. It was never intended to be a philosophical statement of value. :)

Sure, I just want to make sure we've thought through all the implications. For example:

  • Would rmm::alloc be renamed to rmm::device_alloc and we add a rmm::host_alloc?
  • Does a host memory resource accept streams for alloc/free? If not, then host/device_memory_resource cannot share the same base class.
  • Do we enforce using RMM host memory resources anywhere host memory is being allocated in the same way we do with device memory? (e.g., are we going to provide a rmm::host_vector to replace std::vector? rmm::host_buffer?)
  • Do host allocations require the same allocation alignment (256B)?
  • We'll have to provide separate mechanisms to set/get the default host vs default device memory resource.
  • rmmInitialize will need options for selecting host vs. device options

It's not as simple as just adding a pinned_memory_resource and calling it a day. In order for people to actually be able to use it, there's a bunch of design and machinery that needs to be built up as well.

@UmashankarTriforce
Copy link
Author

I guess let's put this PR on hold atm and maybe raise a discussion on the mechanisms and blueprint for this modification. Plan it out right with timeline and get it deployed layer by layer. Sounds good?

@harrism
Copy link
Member

harrism commented Oct 3, 2019

To update this, in a meeting, Jake and I discussed that we could provide host memory_resource types that are not necessarily exposed through the rmm::alloc API, but are available to users. But until we have demand for it, we should wait.

@UmashankarTriforce
Copy link
Author

Alright. What do we do with this PR?

@harrism
Copy link
Member

harrism commented Oct 4, 2019

Leave it open.

@j-ieong
Copy link

j-ieong commented Oct 30, 2019

For the cudf-IO readers/writers, we would like a pinned host memory resource.

We currently manually call cudaMallocHost/Free for allocs used for packed parsing/decoding/encoding metadata that we transfer between CPU and GPU.

@jrhemstad
Copy link
Contributor

For the cudf-IO readers/writers, we would like a pinned host memory resource.

We currently manually call cudaMallocHost/Free for allocs used for packed parsing/decoding/encoding metadata that we transfer between CPU and GPU.

How would you like to be able to use it though? As a resource for a device_buffer? Or as a resource for some new data structure like a rmm::host_buffer?

@j-ieong
Copy link

j-ieong commented Oct 30, 2019

For the cudf-IO readers/writers, we would like a pinned host memory resource.
We currently manually call cudaMallocHost/Free for allocs used for packed parsing/decoding/encoding metadata that we transfer between CPU and GPU.

How would you like to be able to use it though? As a resource for a device_buffer? Or as a resource for some new data structure like a rmm::host_buffer?

Probably the latter host_buffer or as a possible backing for an allocator to thrust::host_vector...?

@kkraus14
Copy link
Contributor

FWIW we could utilize this nicely on the Python side as well where we know when a user is explicitly copying data from device to host and we're always responsible for allocating that memory and somewhat have control over that allocation.

@harrism harrism changed the base branch from branch-0.10 to branch-0.12 December 6, 2019 04:14
@jakirkham
Copy link
Member

Is this still needed now that PR ( #272 ) is in?

@kkraus14
Copy link
Contributor

Closing this as it's been made obsolete by #272

@kkraus14 kkraus14 closed this Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants