Skip to content

Commit

Permalink
Fixes #639: Simplify access-permissions-related code:
Browse files Browse the repository at this point in the history
* Renamed: `access_permissions` -> `permissions` and `access_permissions_t` to `permissions_t`, everwhere
* Dropped the boolean enum in favor of parameter-less some named constructor idioms
* Dropped the static methods in favor of named constructor idioms
* No user code needs to worry about pairs-of-booleans; but they can aggregate-initialize if they really want to
  • Loading branch information
eyalroz committed Apr 29, 2024
1 parent d023df4 commit 58686c0
Show file tree
Hide file tree
Showing 10 changed files with 129 additions and 142 deletions.
18 changes: 9 additions & 9 deletions examples/by_api_module/memory_pools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,21 @@ void copy_through_pool_allocation(
}
}

cuda::memory::access_permissions_t
cuda::memory::permissions_t
try_forbidding_same_device_access(int device_id, cuda::memory::pool_t &pool)
{
cuda::memory::access_permissions_t permissions;
cuda::memory::permissions_t permissions;
permissions.read = true;
permissions.write = false;
bool got_expected_exception = false;
try {
pool.set_access_permissions(cuda::device::get(device_id), permissions);
pool.set_permissions(cuda::device::get(device_id), permissions);
}
catch(std::invalid_argument&) {
got_expected_exception = true;
}
if (not got_expected_exception) {
die_("Unexpected success in setting a device's access permissions to a pool using"
die_("Unexpected success in setting a device's access get_permissions to a pool using"
"that device; it should have failed");
}
return permissions;
Expand All @@ -155,12 +155,12 @@ void try_writing_to_pool_allocation_without_permission(
const cuda::stream_t& stream,
cuda::memory::pool_t& pool,
cuda::memory::region_t& pool_allocated_region,
cuda::memory::access_permissions_t& permissions,
cuda::memory::permissions_t& permissions,
cuda::device_t& peer)
{
permissions.read = false;
permissions.write = false;
pool.set_access_permissions(peer, permissions);
pool.set_permissions(peer, permissions);
std::string str{"hello world"};
stream.synchronize();
auto stream_on_peer = peer.create_stream(cuda::stream::async);
Expand All @@ -184,12 +184,12 @@ void try_writing_to_pool_allocation_without_permission(
void try_reading_from_pool_allocation_without_permission(
cuda::memory::pool_t& pool,
cuda::memory::region_t& pool_allocated_region,
cuda::memory::access_permissions_t& permissions,
cuda::memory::permissions_t& permissions,
cuda::device_t& peer)
{
permissions.read = false;
permissions.write = false;
pool.set_access_permissions(peer, permissions);
pool.set_permissions(peer, permissions);
auto host_buffer_uptr = std::unique_ptr<char[]>(new char[region_size]); // replace this with make_unique in C++14
auto host_buffer = cuda::span<char>{host_buffer_uptr.get(), region_size};
std::fill_n(host_buffer.begin(), host_buffer.size()-1, 'a');
Expand Down Expand Up @@ -223,7 +223,7 @@ int main(int argc, char** argv)
play_with_attributes(pool, stream);
copy_through_pool_allocation(stream, pool_allocated_region);

cuda::memory::access_permissions_t permissions = try_forbidding_same_device_access(device_id, pool);
cuda::memory::permissions_t permissions = try_forbidding_same_device_access(device_id, pool);

auto maybe_peer_id = maybe_get_p2p_peer_id(device_id);
if (maybe_peer_id) {
Expand Down
3 changes: 1 addition & 2 deletions examples/modified_cuda_samples/memMapIPCDrv/child.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ std::vector<memory_mapping_t> import_and_map_allocations(
});

// Retain peer access and map all chunks to mapDevice
virtual_mem::set_access_mode(mappings_region, device,
cuda::memory::access_permissions_t::read_and_write());
virtual_mem::set_permissions(mappings_region, device, cuda::memory::permissions::read_and_write());
return mappings;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,9 @@ static void childProcess(int index_in_shared_devices)
auto shared_pool_handle = shared_pool_handles[i];
auto pool_device = cuda::device::get(shm->devices[i]);
auto pool = cuda::memory::pool::ipc::import<shared_handle_kind>(pool_device, shared_pool_handle);
auto permissions = pool.access_permissions(device);
auto permissions = pool.permissions(device);
if (not (permissions.read and permissions.write)) {
pool.set_access_permissions(device, cuda::memory::read_enabled, cuda::memory::write_enabled);
pool.set_permissions(device, cuda::memory::permissions::read_and_write());
}

// Import the allocations from each memory pool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,7 @@ int memPoolP2PCopy()

auto stream2 = p2pDevices.second.create_stream(cuda::stream::async);
auto output_on_device = cuda::span<int>(stream2.enqueue.allocate(nelem * sizeof(int)));
memPool.set_access_permissions(p2pDevices.second,
cuda::memory::read_enabled, cuda::memory::write_enabled);
memPool.set_permissions(p2pDevices.second, cuda::memory::permissions::read_and_write());

std::cout << "> copyP2PAndScale kernel running ...\n";
auto launch_config = cuda::launch_config_builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,13 @@ setup_virtual_memory(
);

#ifndef _MSC_VER
virtual_mem::set_access_mode(reserved_range.region(), mapping_devices,
cuda::memory::access_permissions_t::read_and_write());
virtual_mem::set_permissions(reserved_range.region(), mapping_devices, cuda::memory::permissions::read_and_write());
#else
// MSVC, at least as of 2019, can't handle template-template parameters with variadcs properly;
// so let's go manual:
for(const auto& mapping_device : mapping_devices) {
virtual_mem::set_access_mode(reserved_range.region(), mapping_device,
cuda::memory::access_permissions_t::read_and_write());
virtual_mem::set_permissions(reserved_range.region(), mapping_device,
cuda::memory::permissions::read_and_write());
}
#endif

Expand Down
54 changes: 22 additions & 32 deletions src/cuda/api/memory_pool.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -159,21 +159,21 @@ pool_t wrap(cuda::device::id_t device_id, pool::handle_t handle, bool owning) no

namespace detail_ {

inline access_permissions_t access_permissions(cuda::device::id_t device_id, pool::handle_t pool_handle)
inline permissions_t get_permissions(cuda::device::id_t device_id, pool::handle_t pool_handle)
{
CUmemAccess_flags access_flags;
auto mem_location = pool::detail_::create_mem_location(device_id);
auto status = cuMemPoolGetAccess(&access_flags, pool_handle, &mem_location);
throw_if_error_lazy(status,
"Determining access information for " + cuda::device::detail_::identify(device_id)
+ " to " + pool::detail_::identify(pool_handle));
return access_permissions_t::from_access_flags(access_flags);
return permissions::detail_::from_flags(access_flags);
}

inline void set_access_permissions(span<cuda::device::id_t> device_ids, pool::handle_t pool_handle, access_permissions_t permissions)
inline void set_permissions(span<cuda::device::id_t> device_ids, pool::handle_t pool_handle, permissions_t permissions)
{
if (permissions.write and not permissions.read) {
throw ::std::invalid_argument("Memory pool access permissions cannot be write-only");
throw ::std::invalid_argument("Memory pool access get_permissions cannot be write-only");
}

CUmemAccess_flags flags = permissions.read ?
Expand All @@ -192,14 +192,14 @@ inline void set_access_permissions(span<cuda::device::id_t> device_ids, pool::ha

auto status = cuMemPoolSetAccess(pool_handle, descriptors.data(), descriptors.size());
throw_if_error_lazy(status,
"Setting access permissions for " + ::std::to_string(descriptors.size())
"Setting access get_permissions for " + ::std::to_string(descriptors.size())
+ " devices to " + pool::detail_::identify(pool_handle));
}

inline void set_access_permissions(cuda::device::id_t device_id, pool::handle_t pool_handle, access_permissions_t permissions)
inline void set_permissions(cuda::device::id_t device_id, pool::handle_t pool_handle, permissions_t permissions)
{
if (permissions.write and not permissions.read) {
throw ::std::invalid_argument("Memory pool access permissions cannot be write-only");
throw ::std::invalid_argument("Memory pool access get_permissions cannot be write-only");
}

CUmemAccessDesc desc;
Expand All @@ -212,16 +212,16 @@ inline void set_access_permissions(cuda::device::id_t device_id, pool::handle_t
desc.location = pool::detail_::create_mem_location(device_id);
auto status = cuMemPoolSetAccess(pool_handle, &desc, 1);
throw_if_error_lazy(status,
"Setting access permissions for " + cuda::device::detail_::identify(device_id)
"Setting access get_permissions for " + cuda::device::detail_::identify(device_id)
+ " to " + pool::detail_::identify(pool_handle));
}

} // namespace detail_

access_permissions_t access_permissions(const cuda::device_t& device, const pool_t& pool);
void set_access_permissions(const cuda::device_t& device, const pool_t& pool, access_permissions_t permissions);
permissions_t get_permissions(const cuda::device_t& device, const pool_t& pool);
void set_permissions(const cuda::device_t& device, const pool_t& pool, permissions_t permissions);
template <typename DeviceRange>
void set_access_permissions(DeviceRange devices, const pool_t& pool_handle, access_permissions_t permissions);
void get_permissions(DeviceRange devices, const pool_t& pool_handle, permissions_t permissions);

namespace pool {

Expand Down Expand Up @@ -302,48 +302,38 @@ class pool_t {
set_attribute<CU_MEMPOOL_ATTR_RELEASE_THRESHOLD>(threshold);
}

access_permissions_t access_permissions(const cuda::device_t& device)
permissions_t permissions(const cuda::device_t& device)
{
return memory::access_permissions(device, *this);
return memory::get_permissions(device, *this);
}

/**
* Set read and write permissions from a device to the allocations from
* Set read and write get_permissions from a device to the allocations from
* this pool
*
* @param device the device the kernels running on which are governed by these permissions
* @param permissions new read and write permissions to use
* @param device the device the kernels running on which are governed by these get_permissions
* @param permissions new read and write get_permissions to use
*
* @note This affects both future _and past_ allocations from this pool.
*/
///@{
/**
* @param device the device the kernels running on which are governed by this new setting
* @param permissions new read and write permissions to use
* @param permissions new read and write get_permissions to use
*/
void set_access_permissions(const cuda::device_t& device, access_permissions_t permissions)
void set_permissions(const cuda::device_t& device, permissions_t permissions)
{
return memory::set_access_permissions(device, *this, permissions);
}

/**
* @param device the device the kernels running on which are governed by this new setting
* @param read_permission true if kernels are allowed to read from memory allocated by this pool
* @param write_permission true if kernels are allowed to write to memory allocated by this pool
*/
void set_access_permissions(const cuda::device_t& device, bool read_permission, bool write_permission)
{
set_access_permissions(device, access_permissions_t{read_permission, write_permission});
return memory::set_permissions(device, *this, permissions);
}

/**
* @param device the devices the kernels running on which are governed by this new setting
* @param permissions new read and write permissions to use
* @param permissions new read and write get_permissions to use
*/
template <typename DeviceRange>
void set_access_permissions(DeviceRange devices, access_permissions_t permissions)
void set_permissions(DeviceRange devices, permissions_t permissions)
{
return memory::set_access_permissions(devices, *this, permissions);
return memory::set_permissions(devices, *this, permissions);
}
///@}

Expand Down
15 changes: 7 additions & 8 deletions src/cuda/api/multi_wrapper_impls/memory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -512,29 +512,28 @@ inline pool::ipc::imported_ptr_t pool_t::import(const memory::pool::ipc::ptr_han
return pool::ipc::import_ptr(*this, exported_handle);
}

inline access_permissions_t access_permissions(const cuda::device_t& device, const pool_t& pool)
inline permissions_t get_permissions(const cuda::device_t& device, const pool_t& pool)
{
return cuda::memory::detail_::access_permissions(device.id(), pool.handle());
return cuda::memory::detail_::get_permissions(device.id(), pool.handle());
}

inline void set_access_permissions(const cuda::device_t& device, const pool_t& pool, access_permissions_t permissions)
inline void set_permissions(const cuda::device_t& device, const pool_t& pool, permissions_t permissions)
{
if (pool.device_id() == device.id()) {
throw ::std::invalid_argument("Cannot change the access permissions to a pool of the device "
throw ::std::invalid_argument("Cannot change the access get_permissions to a pool of the device "
"on which the pool's memory is allocated (" + cuda::device::detail_::identify(device.id()) + ')');
}
cuda::memory::detail_::set_access_permissions(device.id(), pool.handle(), permissions);
cuda::memory::detail_::set_permissions(device.id(), pool.handle(), permissions);
}

template <typename DeviceRange>
void set_access_permissions(DeviceRange devices, const pool_t& pool, access_permissions_t permissions)
void set_permissions(DeviceRange devices, const pool_t& pool, permissions_t permissions)
{
// Not depending on unique_span here :-(
auto device_ids = ::std::unique_ptr<cuda::device::id_t[]>(new cuda::device::id_t[devices.size()]);
auto device_to_id = [](device_t const& device){ return device.id(); };
::std::transform(::std::begin(devices), ::std::end(devices), device_ids.get(), device_to_id);
cuda::memory::detail_::set_access_permissions(
{ device_ids.get(), devices.size() }, pool.handle(), permissions);
cuda::memory::detail_::set_permissions( { device_ids.get(), devices.size() }, pool.handle(), permissions);
}
#endif // #if CUDA_VERSION >= 11020

Expand Down
Loading

0 comments on commit 58686c0

Please sign in to comment.