Add a host-pinned memory resource that can be used as upstream for pool_memory_resource.

[harrism]

Description

Depends on #1417

Adds a new host_pinned_memory_resource that implements the new cuda::mr::memory_resource and cuda::mr::async_memory_resource concepts which makes it usable as an upstream MR for rmm::mr::device_memory_resource.

Also tests a pool made with this new MR as the upstream.

Note that the tests explicitly set the initial and maximum pool sizes as using the defaults does not currently work. See #1388 .

Closes #618

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[miscco]

I have some minor suggestions

I am a bit dismayed about the amount of documentation boilerplate.

Maybe we could work with defaulted arguments rather than redefining the function each time?

[harrism]

Maybe we could work with defaulted arguments rather than redefining the function each time?

I am able to consolidate the non-async functions into a single allocate and deallocate (eliminating two functions). But for the async versions, we have existing calls to allocate_async(bytes, stream) that will fail if we just have allocate_async(bytes, alignment=default_alignment, stream=default_stream. So I can't consolidate these yet.

Also, suppose we did consolidate these. What should we use for default_alignment? A device memory resource should use the default CUDA memory alignment (256 bytes). But a host memory resource should probably use alignof(std::max_align_t). And a user who wants to not care about alignment will have to decide what to pass for alignment because they need it in order to use an explicit stream.

Actually, this default alignment problem applies to the non-async allocate/deallocate functions too. Because when we convert something like pool_memory_resource to the cuda::memory_resource concept what should it use if we need to provide default alignments? The underlying memory for a pool could be device or host memory. I think the pool needs to be able to figure out what its default alignment should be...

Thoughts?

[miscco]

Actually, this default alignment problem applies to the non-async allocate/deallocate functions too.

We could define a free helper function that return 256 on device and alignof(std::max_align_t) on host. I am not sure whether this is something that we really want

[wence-]

Actually, this default alignment problem applies to the non-async allocate/deallocate functions too. Because when we convert something like pool_memory_resource to the cuda::memory_resource concept what should it use if we need to provide default alignments? The underlying memory for a pool could be device or host memory. I think the pool needs to be able to figure out what its default alignment should be...

What is a minimum safe value (given we don't know the type of the pointer we're allocating for)? On host, I believe the answer is alignof(std::max_align_t), on device I think it's alignof(double4) == 16 bytes (as per https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#vector-types-alignment-requirements-in-device-code). This is smaller than the 256 byte alignment guaranteed by allocations from the cudamalloc family, so I am not sure if that is pertinent.

Should memory resources that perform concrete allocations advertise their default alignment as a property and then wrapping resources can query that?

[miscco]

Should memory resources that perform concrete allocations advertise their default alignment as a property and then wrapping resources can query that?

We can make this a property. The one potential design issue that our memory_resource has is that it pushes those properties into APIs. We cannot querry it for a property that we did not specify in the template arguments

[harrism]

We can make this a property. The one potential design issue that our memory_resource has is that it pushes those properties into APIs. We cannot querry it for a property that we did not specify in the template arguments

Can you explain a bit more? Maybe an example so I can understand what you mean?

[miscco]

Can you explain a bit more? Maybe an example so I can understand what you mean?

The issue is that properties are awesome if you have them around. But resource_ref is type erased, so we essentially drop all properties that are not stored in the resource_ref in the bin.

Its the difference between:

template<class T>
    requires resource_with<cuda::mr::device_accessible>
void* special_allocate(T& memory_resource, size_t size)

void* special_allocate(cuda::mr::resource_ref<cuda::mr::device_accessible>& memory_resource, size_t size)

The latter is a streamlined implementation that reduces binary size considerably and generally simplifies the interfaces a lot. However, there is no T there anymore. In the former function I can querry T with cuda::mr::has_property<T, some_property> and get a result. In the latter it is impossible because resource_ref itself does not hold that property unless we tell it so explicitly.

Its definitely a wart

[harrism]

Also the former requires C++20, right?

[miscco]

Also the former requires C++20, right?

Yes, but you can also just write

template<class T, cuda::std::enable_if_t<resource_with<cuda::mr::device_accessible>, int> = 0>
void* special_allocate(T& memory_resource, size_t size)

[abellina]

This LGTM, the alignment parameters not being used obviously means we will need to align on our own. Our pinned code looks to be using std::max_align_t right now, just FYI.

[harrism]

@abellina you are right we should probably fix this to actually align. I think cudaHostAlloc leaves alignment up to the caller.

[harrism]

@abellina you are right we should probably fix this to actually align. I think cudaHostAlloc leaves alignment up to the caller.

Done.

[harrism]

/ok to test

[harrism]

/ok to test

[wence-]

Minor documentation nits, otherwise looks great!

[wence-]

Suggested change

	* @return True if the input a power of two with non-negative exponent, false otherwise.
	* @return True if the input is a power of two with non-negative exponent, false otherwise.

[wence-]

Double-nit (no need to act on it) non-negative integer exponent (all integers can be expressed as powers of two if we admit real exponents).

[harrism]

/ok to test

[abellina]

For me this still looks good. I was able to replace our pinned pool with a pool_memory_resource<pinned_host_memory_resource> and use the PTDS stream for all allocations. Perf overall is unchanged from our java-base one, except for one of the NDS queries (83) where this shows a small improvement. I would like to confirm why that is, but my guess is that it has to do with fragmentation in the old pool.

[bdice]

Two questions, then I'm happy to approve.

[bdice]

This docstring implies it's possible to compare with another type of resource and get false, but the implementation doesn't allow that. Do we need to update the implementation or the docstrings?

[wence-]

Oh yeah, I had that thought. Is there a blanket "false" implementation in the base class somehow?

[harrism]

I think this is how comparison works with cuda::mr. Basically if you try to compare with another type of resource, compilation will fail. Note that refactoring to cuda::mr will necessitate changing the semantics RMM currently (mostly) has for MR equality comparison. #1402

[harrism]

Note also that pinned_host_memory_resource is NOT a device_memory_resource. It simply implements the cuda::mr::memory_resource and cuda::mr::async_memory_resource concepts.

[harrism]

(also note there is no base class)

[harrism]

I changed the docstring so it doesn't say that false can be returned. Note that we should probably followup with more explicit tests of this MR and future MRs like it. Right now, though, our test machinery for MRs assumes they are all device_memory_resource, so while I can pass a pool_memory_resource<pinned_host_memory_resource> to all the MR tests, I can't pass just pinned_host_memory_resource currently. (It does get tested as the upstream in the former case though, including its operator==).

[bdice]

Okay. If there's no base class, I've just lost track of how the class hierarchy works. I don't have any further comments here but I'll need to refresh myself on how things are supposed to work someday.

[harrism]

/ok to test

[bdice]

@harrism asked me to merge once I approve, so I'll do that.

[bdice]

/merge