Unify batch and stream API check #271
Conversation
madsbk
added
improvement
madsbk
force-pushed
the
unify_batch_and_stream_api_check
branch
from
109835e to
f4f71ab
Compare
madsbk
force-pushed
the
unify_batch_and_stream_api_check
branch
from
f4f71ab to
4dc2cee
Compare
madsbk
force-pushed
the
unify_batch_and_stream_api_check
branch
from
0d5f440 to
e4a4622
Compare
madsbk
force-pushed
the
unify_batch_and_stream_api_check
branch
from
e4a4622 to
5358b35
Compare
cpp/examples/basic_io.cpp
Outdated
| check(cudaStreamSynchronize(stream) == cudaSuccess); | ||
| // Note, `bytes_done` might be negative, which indicate an IO error thus we | ||
| // use `CUFILE_CHECK_STREAM_IO` to check for errors. | ||
| CUFILE_CHECK_STREAM_IO(bytes_done); |
There was a problem hiding this comment.
Wow, this is a garbage error reporting api.
cpp/examples/basic_io.cpp
Outdated
| cout << "Performing async I/O using file handle" << endl; | ||
| off_t f_off{0}; | ||
| off_t d_off{0}; | ||
| ssize_t bytes_done{0}; |
There was a problem hiding this comment.
I think this is incorrect, per the documentation for cuFileWriteAsync:
The bytes_written pointer should be allocated with
cuMemHostAllocor registered withcuMemHostRegister, and the pointer to access that memory from the device can be obtained by usingcuMemHostGetDevicePointer.
Which we are not doing here.
cpp/include/kvikio/error.hpp
Outdated
| if (_nbytes < 0) { \ | ||
| throw(_exception_type){std::string{"cuFile error at: "} + __FILE__ + ":" + \ | ||
| KVIKIO_STRINGIFY(__LINE__) + ": " + \ | ||
| cufileop_status_error((CUfileOpError)(CUFILEOP_BASE_ERR - _nbytes))}; \ |
There was a problem hiding this comment.
So the documentation says that this returns:
- The number of bytes successfully written. or;
- -1 on IO errors. or;
- All other errors will return a negative integer value of the
CUfileOpErrorenum value.
But this code suggests that it returns -(some_cufile_op_error - CUFILEOP_BASE_ERR). Which can't be right given the above because it value-puns CU_FILE_DRIVER_NOT_INITIALIZED with an IO error.
There was a problem hiding this comment.
I don't think the docs are correct. I now just print the raw value of _nbytes. Let's fix this in a follow-up when we know the intended behavior :)
…o unify_batch_and_stream_api_check
| * until execution time. | ||
| * @param bytes_read Pointer to the bytes read from file. This pointer should be a non-NULL value | ||
| * and *bytes_read set to 0. The bytes_read memory should be allocated with cuMemHostAlloc/malloc/ | ||
| * mmap or registered with cuMemHostRegister. |
There was a problem hiding this comment.
I know you have just copied the cufile docs here, but this sentence makes no sense to me. AIUI, just plain malloc and mmap are completely unknown to the driver. So does this mean use cuMemHostAlloc or cuMemAlloc or cuMemMap? Or am I fully even more confused than I thought?
There was a problem hiding this comment.
I guess it means that it has to be allocated on the heap (not stack)?
Co-authored-by: Lawrence Mitchell <[email protected]>
|
Thanks @robertmaynard and @wence- |
|
/merge |
We now use the
cuFileReadAsyncsymbol to check for availability of both the stream and batch API of cuFile.We used to look for the mangled symbol
_ZTS13CUfileOpErrorto determine the availability of the batch API. However, since mangling is compiler specific, we prefer usingcuFileReadAsynceven though it means that the batch API isn't available until CUDA version 12.2.Additionally, this PR also implements
CUFILE_CHECK_STREAM_IO(), which is used to check async IO errors.cc. @tell-rebanta