Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
ff5d648 to
123f2f5
Compare
e94e43d to
d61c94c
Compare
This comment has been minimized.
This comment has been minimized.
| //! @brief Wrapper for retrieving the open mode. | ||
| [[nodiscard]] static _CCCL_HOST_API cufile_open_mode __open_mode(native_handle_type __native_handle) | ||
| { | ||
| int __oflags = ::fcntl(__native_handle, F_GETFL); |
There was a problem hiding this comment.
do we really need to retrieve the opening mode?
I'm a bit worried on non-standard things in the bindings for when eventually cufile is windows supported.
| _CCCL_HOST_API void open(const char* __filename, cufile_open_mode __open_mode) | ||
| { | ||
| if (is_open()) | ||
| { | ||
| ::cuda::std::__throw_runtime_error("File is already opened."); | ||
| } | ||
|
|
||
| __native_handle_ = __open_file(__filename, __make_oflags(__open_mode)); | ||
|
|
||
| try | ||
| { | ||
| __cufile_handle_ = __register_cufile_handle(__native_handle_); | ||
| } | ||
| catch (...) | ||
| { | ||
| __close_file(::cuda::std::exchange(__native_handle_, __invalid_native_handle)); | ||
| throw; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure we need this... its two ways of doing the same (this vs constructor) and this actually is forbidden if the constructor was already called. I'd vote to remove this one.
There was a problem hiding this comment.
But for example std::fstream also implements the .open(...) method. I'd like to stay as close as what is common in the standard as possible
There was a problem hiding this comment.
but we can always implement it later if needed... I don't see why this is needed; we are not trying to stay close to fstream either, are we?
There was a problem hiding this comment.
That's true, but I'd like to provide tools users are used to have. Consider this:
cuda::cufile file;
// ...
file = cuda::cufile{filename, open_mode}; // open the file
// ...
file = cuda::cufile{}; // close the fileYou have a situation like this, when you want to have the object in not opened state. Then, to open the file, you must create another object and move assign it to the original object. And if you want to close the file with detection whether it was closed or not, you must (again) move assign a new default constructed object.
In my opinion this is not a good design. I'd like to do:
cuda::cufile file;
// ...
file.open(filename, open_mode); // open the file
// ...
file.close(); // close the file|
Fair enough, those are valid points. I was unsure we needed it but as you said the easier of use and known methods are totally valid and I think it's good to have!
Enviado desde Outlook para Android<https://aka.ms/AAb9ysg>
________________________________
From: David Bayer ***@***.***>
Sent: Thursday, October 16, 2025 9:15:39 PM
To: NVIDIA/cccl ***@***.***>
Cc: Javier Vera ***@***.***>; Comment ***@***.***>
Subject: Re: [NVIDIA/cccl] Implement `cudax::cufile` (PR #6122)
@davebayer commented on this pull request.
________________________________
In cudax/include/cuda/experimental/__cufile/cufile.cuh<#6122 (comment)>:
+ _CCCL_HOST_API void open(const char* __filename, cufile_open_mode __open_mode)
+ {
+ if (is_open())
+ {
+ ::cuda::std::__throw_runtime_error("File is already opened.");
+ }
+
+ __native_handle_ = __open_file(__filename, __make_oflags(__open_mode));
+
+ try
+ {
+ __cufile_handle_ = __register_cufile_handle(__native_handle_);
+ }
+ catch (...)
+ {
+ __close_file(::cuda::std::exchange(__native_handle_, __invalid_native_handle));
+ throw;
+ }
+ }
That's true, but I'd like to provide tools users are used to have. Consider this:
cuda::cufile file;
// ...
file = cuda::cufile{filename, open_mode}; // open the file
// ...
file = cuda::cufile{}; // close the file
You have a situation like this, when you want to have the object in not opened state. Then, to open the file, you must create another object and move assign it to the original object. And if you want to close the file with detection whether it was closed or not, you must (again) move assign a new default constructed object.
—
Reply to this email directly, view it on GitHub<#6122 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BSUF2R623U6KWV3NPWPNYAL3X7VFXAVCNFSM6AAAAACIGCYCK2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNBWGYZDKOJZGA>.
You are receiving this because you commented.Message ID: ***@***.***>
|
This comment has been minimized.
This comment has been minimized.
cudax/test/cufile/common.h
Outdated
|
|
||
| void test_check_fd_is_valid(int fd) | ||
| { | ||
| CUDAX_REQUIRE(fcntl(fd, F_GETFD) == 0); |
There was a problem hiding this comment.
Could correct flags be non-zero here? Internet says to do:
fcntl(fd, F_GETFD) != -1 || errno != EBADF;
For this use case probably just the first part is ok
There was a problem hiding this comment.
Oh yeah, you are right
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
acc2cf1 to
f683170
Compare
This comment has been minimized.
This comment has been minimized.
f683170 to
4f0b244
Compare
As discussed internally, we will just build the tests for now, we will run those tests once the support is added. I tested it locally with CUDA 12.9 and 13.0 and both compiled & ran fine. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 12h 12m: Pass: 100%/261 | Total: 3d 16h | Max: 1h 41m | Hits: 99%/379106See results here. |
This PR implements wrappers of
CUfileHandle_tand related APIs.It introduces:
cudax::cufiletype which handles opening/closing the native file handle and registering/deregistering of the cuFile file handle. It owns both of these resources.cudax::cufile_reftype that is a lightweight non-owning wrapper for the cuFile file handle. It doesn't own the resources (with exception below).cuda::cufile_driver.[de]register_native_handle(...) -> cuda::cufile_refthat can be used for manual registration of the native file handle. It has basically the same semantics asnew/delete.So, right now these are the scenarios are covered:
CUfileHandle_t-> he can usecuda::cufile_refwith our APIs.cuda::cufile_driver.register_native_handle(...).cuda::cufileWe might introduce a third option:
cuda::scoped_cufileor something similar, that would be constructible fromcuda::cufile_refand would deregister the handle when going out of scope, but I would wait with it.