Skip to content
Merged
1 change: 1 addition & 0 deletions .codespellrc
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
[codespell]
check-hidden = true
skip = ./.git
ignore-words-list = hsa,
10 changes: 6 additions & 4 deletions hipfile/examples/aiscp/aiscp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,16 @@ main(int argc, char *argv[])
}

nbytes = hipFileRead(src_handle, devbuf, file_size, 0, 0);
if (-1 == nbytes || file_size != static_cast<size_t>(nbytes)) {
fprintf(stderr, "Could not read from %s (%zd) (%s)\n", src_path, nbytes, strerror(errno));
if (nbytes < 0 || file_size != static_cast<size_t>(nbytes)) {
fprintf(stderr, "Could not read from %s (%zd) (%s)\n", src_path, nbytes,
IS_HIPFILE_ERR(nbytes) ? HIPFILE_ERRSTR(nbytes) : strerror(errno));
goto free_devbuf;
}

nbytes = hipFileWrite(dst_handle, devbuf, file_size, 0, 0);
if (-1 == nbytes || file_size != static_cast<size_t>(nbytes)) {
fprintf(stderr, "Could not write to %s (%zd) (%s)\n", dst_path, nbytes, strerror(errno));
if (nbytes < 0 || file_size != static_cast<size_t>(nbytes)) {
fprintf(stderr, "Could not write to %s (%zd) (%s)\n", src_path, nbytes,
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message prints "src_path" but this is the write operation for "dst_path". This will confuse users when a write error occurs, as the error message will report the wrong file. Change src_path to dst_path in this error message.

Copilot uses AI. Check for mistakes.
IS_HIPFILE_ERR(nbytes) ? HIPFILE_ERRSTR(nbytes) : strerror(errno));
goto free_devbuf;
}

Expand Down
1 change: 1 addition & 0 deletions rocfile/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ set(ROCFILE_SOURCES
async.cpp
backend/asyncop-fallback.cpp
backend/fallback.cpp
backend/fastpath.cpp
batch/batch.cpp
buffer.cpp
context.cpp
Expand Down
158 changes: 158 additions & 0 deletions rocfile/src/backend/fastpath.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/* Copyright (c) Advanced Micro Devices, Inc. All rights reserved.
*
* SPDX-License-Identifier: MIT
*/

#include "buffer.h"
#include "context.h"
#include "fastpath.h"
#include "file.h"
#include "hip.h"
#include "io.h"

#include <cstdint>
#include <fcntl.h>
#include <hip/hip_runtime_api.h>
#include <stdexcept>

using namespace rocFile;
using namespace std;

/* The fastpath backend is used when:
* - The file has been opened with the O_DIRECT flag
* - file_offset is 4KiB aligned
* - buffer_offset is 4KiB aligned
* - size is a multiple of 4KiB
* - The buffer type is hipMemoryTypeDevice
*
* When using the fastpath the IO flows through the following
* - rocFile HIP wrapper (userspace)
* - repository: hipFile
* - file: rocfile/hip.cpp
* - methods: Hip::hipAmdFileRead(...), Hip::hipAmdFileWrite(...)
* - throws
* - std::system_error if status is non-zero (set by kfd)
* - Hip::runtime_error if hip runtime does not return hipSuccess or
* if rocFile was unable to find hipAmdFileRead/hipAmdFileWrite
* - HIP Runtime (userspace)
* - repository: rocm-systems
* - file: projects/clr/hipamd/src/hip_storage.cpp
* - functions: hipAmdFileRead(...), hipAmdFileWrite(...)
* - hipAmdFileRead & hipAmdFileWrite are not exposed through a public
* header so hipGetProcAddress() is used to lookup each function's function
* pointer
* - returns
* - hipSuccess if size is zero
* - This check should be removed
* - hipErrorInvalidDevice if unable to lookup current device
* - hipErrorUnknown if rocr runtime does not return success
* - ROCR Runtime (userspace)
* - repository: rocm-systems
* - file: projects/clr/rocclr/device/rocm/rocdevice.cpp
* - methods: Device::amdFileRead(...), Device::amdFileWrite(...)
* - Does not perform any input validation
* - Logs on every IO failure
* - size_copied and status are never modified
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "size_copied" is misspelled as "size_coppied".

Copilot uses AI. Check for mistakes.
* - returns error if HSA does not return HSA_STATUS_SUCCESS
* - HSA (userspace)
* - repository: rocm-systems
Comment thread
kurtmcmillan marked this conversation as resolved.
* - file: projects/rocr-runtime/runtime/hsa-runtime/core/runtime/hsa_ext_amd.cpp
* - hsa_amd_ais_file_read(...), hsa_amd_ais_file_write(...)
* - status and size_coppied untouched
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "size_copied" is misspelled as "size_coppied".

Copilot uses AI. Check for mistakes.
* - returns
* - HSA_STATUS_ERROR_INVALID_ARGUMENT if size is zero
* - This check should be be removed
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "be be removed" has a duplicated "be".

Copilot uses AI. Check for mistakes.
* - HSA_STATUS_ERROR_INVALID_ARGUMENT if devicePtr is NULL
* - This check should be be removed
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "be be removed" has a duplicated "be".

Copilot uses AI. Check for mistakes.
* - HSA_STATUS_ERROR if hsaKmtAisReadWriteFile(...) does not return success
* - Thunk (userspace)
* - repository: rocm-systems
* - file: projects/rocr-runtime/libhsakmt/src/ais.c
* - functions: hsaKmtAisReadWriteFile(...),
* - If the buffer pointer, and io_size combination are invalid, return HSAKMT_STATUS_INVALID_PARAMETER
* - converts buffer pointer + io_size to a buffer handle (no alignment check)
* - size_copied and status are untouched
* - returns
* - HSAKMT_STATUS_INVALID_PARAMETER if
* - the buffer pointer + size combination are invalid
* - if AisFlags (read/write) are invalid
* - HSAKMT_STATUS_ERROR if the kfd ioctl does not return success
* - KFD (chardev) (kernel)
* - repository: ROCm
* - file: amdgpu/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
* - function: kfd_ioctl_ais(...)
* - returns
* - -EINVAL if op != READ or WRITE
* - -EINVAL if fd < 0
* - -EINVAL if unable to lookup device
* - -NODEV if AIS is not initialized on the device
* - -ESRCH if unable to bind the process to the device
* - -EINVAL if the buffer's domain is not AMDGPU_GEM_DOMAIN_VRAM
* - an error if it is unable to pin the buffer object
* - error is from amdgpu_amdkfd_gpuvm_pin_bo(...)
* - the result of the call to kfd_ais_rw_file(...)
* - the value returned is also written to the status variable
* - KFD (kernel)
* - repository: ROCm
* - file: amdgpu/drivers/gpu/drm/amd/amdkfd/kfd_ais.c
* - function: kfd_ais_rw_file(...)
* - Calls dev_dbg on every successful IO
* - May want to remove this
* - returns
* - -EINVAL if file_offset or size are not page aligned (4K)
* - This check should be removed /if/ the IO stack checks for alignment.
* - -EBADF unable to lookup the file object
* - -ENODEV if unable to lookup the storage device
* - -ENODEV if pcie_p2pdma_distance < 0
* - This should be changed to return -EREMOTE
* - -EINVAL if buffer object domain is not AMDGPU_GEM_DOMAIN_VRAM
* - an error if unable to create a sg_table for the buffer object
* - error is from amdgpu_amdkfd_gpuvm_get_sg_table(...)
* - -ENOMEM if unable to init a bvec
* - -EIO on short reads
* - Need to confirm & fix
*/

int
Fastpath::score(shared_ptr<IFile> file, shared_ptr<IBuffer> buffer, size_t size, hoff_t file_offset,
hoff_t buffer_offset) const
{
bool accept_io{true};

accept_io &= !!(file->getStatusFlags() & O_DIRECT);

accept_io &= buffer->getType() == hipMemoryTypeDevice;

accept_io &= 0 <= file_offset;
accept_io &= 0 <= buffer_offset;

accept_io &= !(size & 0xFFF);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alignment check !(size & 0xFFF) accepts size=0 since 0 is 4KiB-aligned. However, according to the extensive comment block above (lines 45-46 and 63-64), the HIP runtime and HSA layers return errors for size=0. While these checks "should be removed" according to the comments, accepting size=0 here may lead to unnecessary calls that will fail downstream. Consider explicitly rejecting size=0 until the stack is fixed, or add a comment explaining why size=0 is acceptable despite known downstream issues.

Copilot uses AI. Check for mistakes.
accept_io &= !(file_offset & 0xFFF);
accept_io &= !(buffer_offset & 0xFFF);

return accept_io ? 100 : -1;
}

ssize_t
Fastpath::io(IoType type, shared_ptr<IFile> file, shared_ptr<IBuffer> buffer, size_t size, hoff_t file_offset,
hoff_t buffer_offset)
{
void *devptr{reinterpret_cast<void *>(reinterpret_cast<intptr_t>(buffer->getBuffer()) + buffer_offset)};
hipAmdFileHandle_t handle{};
size_t nbytes{};

handle.fd = file->getFd();

switch (type) {
case IoType::Read:
nbytes = Context<Hip>::get()->hipAmdFileRead(handle, devptr, size, file_offset);
break;
case IoType::Write:
nbytes = Context<Hip>::get()->hipAmdFileWrite(handle, devptr, size, file_offset);
break;
default:
throw std::runtime_error("Invalid IoType");
}

return static_cast<ssize_t>(nbytes);
}
36 changes: 36 additions & 0 deletions rocfile/src/backend/fastpath.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/* Copyright (c) Advanced Micro Devices, Inc. All rights reserved.
*
* SPDX-License-Identifier: MIT
*/

#pragma once

#include "backend.h"
#include "hipfile-types.h"

#include <memory>
Comment thread
kurtmcmillan marked this conversation as resolved.
#include <sys/types.h>

namespace rocFile {
class IBuffer;
}
namespace rocFile {
class IFile;
}
namespace rocFile {
enum class IoType;
}

namespace rocFile {

struct Fastpath : public Backend {
virtual ~Fastpath() override = default;

int score(std::shared_ptr<IFile> file, std::shared_ptr<IBuffer> buffer, size_t size, hoff_t file_offset,
hoff_t buffer_offset) const override;

ssize_t io(IoType type, std::shared_ptr<IFile> file, std::shared_ptr<IBuffer> buffer, size_t size,
hoff_t file_offset, hoff_t buffer_offset) override;
};

}
5 changes: 5 additions & 0 deletions rocfile/src/rocfile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <memory>
#include <stdexcept>
#include <vector>
#include <system_error>

using namespace rocFile;

Expand Down Expand Up @@ -276,6 +277,10 @@ catch (const Sys::RuntimeError &e) {
errno = e.error;
return -1;
}
catch (const std::system_error &e) {
errno = e.code().value();
return -1;
}
catch (...) {
return -rocFileInternalError;
}
Expand Down
13 changes: 12 additions & 1 deletion rocfile/src/state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
*/

#include "backend/fallback.h"
#include "backend/fastpath.h"
#include "batch/batch.h"
#include "buffer.h"
#include "file.h"
#include "hip.h"
#include "state.h"
#include "stream.h"

Expand All @@ -26,7 +28,7 @@ struct Backend;

namespace rocFile {

DriverState::DriverState() : ref_count{0}, backends{std::shared_ptr<Backend>(new Fallback{})}
DriverState::DriverState() : ref_count{0}
{
this->file_map = std::make_unique<FileMap>();
this->batch_map = std::make_unique<BatchContextMap>();
Expand Down Expand Up @@ -275,6 +277,15 @@ DriverState::ensureInitialized()
std::vector<std::shared_ptr<Backend>>
DriverState::getBackends() const
{
static bool once = [&]() {
if (getHipAmdFileReadPtr() && getHipAmdFileWritePtr()) {
backends.emplace_back(new Fastpath{});
}
backends.emplace_back(new Fallback{});
return true;
}();
Comment on lines +280 to +286
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda capture [&] is dangerous here because it captures the 'this' pointer by reference, but the lambda is stored in a static variable that persists beyond the lifetime of any single DriverState instance. Since DriverState is a singleton accessed through Context, this may work in practice, but the code is technically undefined behavior if the singleton is ever destroyed and recreated.

Additionally, the lambda modifies the mutable 'backends' member without any synchronization. While function-local static initialization is thread-safe in C++11+, multiple threads could call getBackends() concurrently after initialization, potentially causing a data race when reading the 'backends' vector while it's being modified by emplace_back.

Consider either: (1) protecting the initialization with std::call_once or a mutex, or (2) moving the initialization to the constructor where it can be properly synchronized, or (3) using a non-capturing lambda and accessing the singleton instance explicitly within the lambda.

Copilot uses AI. Check for mistakes.
(void)once;

return backends;
}
}
2 changes: 1 addition & 1 deletion rocfile/src/state.h
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ class DriverState {
std::unique_ptr<StreamMap> stream_map;

// Backends available to service IO requests
const std::vector<std::shared_ptr<Backend>> backends;
mutable std::vector<std::shared_ptr<Backend>> backends;
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making 'backends' mutable and non-const allows lazy initialization in a const method, but this pattern can hide thread-safety issues. The vector is being modified in a const method (getBackends) without proper synchronization, which is a code smell. Consider either making getBackends() non-const (which better reflects its behavior), or using a more thread-safe initialization pattern like std::call_once with a std::once_flag member.

Copilot uses AI. Check for mistakes.

/// Mutex to protect the state
mutable std::shared_mutex state_mutex;
Expand Down
1 change: 1 addition & 0 deletions rocfile/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ set(TEST_SOURCE_FILES
driver.cpp
handle.cpp
hip.cpp
fastpath.cpp
io.cpp
misc.cpp
mountinfo.cpp
Expand Down
Loading
Loading