Skip to content

Update and document max IO size handling#206

Merged
derobins merged 6 commits intodevelopfrom
derobins/max_size_io_docs
Apr 1, 2026
Merged

Update and document max IO size handling#206
derobins merged 6 commits intodevelopfrom
derobins/max_size_io_docs

Conversation

@derobins
Copy link
Copy Markdown
Collaborator

@derobins derobins commented Mar 3, 2026

No description provided.

Copilot AI review requested due to automatic review settings March 3, 2026 18:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a reusable Doxygen note describing the maximum single-call I/O size limit, and inserts that note into the public API documentation for read/write calls.

Changes:

  • Introduce a new Doxygen alias (max_io_size_note) describing the max I/O size limit.
  • Reference \max_io_size_note in the documentation blocks for hipFileRead, hipFileWrite, hipFileReadAsync, and hipFileWriteAsync.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
include/hipfile.h Adds \max_io_size_note to multiple API doc blocks (sync + async read/write).
docs/Doxyfile.in Defines the max_io_size_note Doxygen alias used by the header docs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings March 17, 2026 04:01
@derobins derobins force-pushed the derobins/max_size_io_docs branch from 5dbe70c to ca7c174 Compare March 17, 2026 04:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

include/hipfile.h:536

  • The doc comment still hard-codes the max transfer size as 0x7ffff000 (2,147,479,552) for hipFileRead/hipFileWrite, but the new implementation/note makes this dependent on the system page size (e.g., 64KiB pages yield a different limit). To avoid incorrect docs on non-4KiB systems, please remove or update these hard-coded values and rely on the new \max_io_size_note (or make the text page-size-aware).
 * hipFileRead() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
 * returning the number of bytes actually transferred.
 *
 * @param [in] fh            \hipfile_handle_param
 * @param [in] buffer_base   \buffer_base_param
 * @param [in] size          Number of bytes that should be read
 * @param [in] file_offset   Offset into the file that should be read from
 * @param [in] buffer_offset Offset of the GPU buffer that that the data should be written to
 *
 * \max_io_size_note
 *
 * @return if >= 0: Number of bytes read
 * @return if -1:   System error (check `errno` for the specific error)
 * @return else:    Negative value of the related hipFileOpError_t
 */
HIPFILE_API
ssize_t hipFileRead(hipFileHandle_t fh, void *buffer_base, size_t size, hoff_t file_offset,
                    hoff_t buffer_offset);

/*!
 * @brief Synchronously write data from a GPU buffer to a file
 * @ingroup file
 *
 * hipFileWrite() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
 * returning the number of bytes actually transferred.
 *

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@derobins derobins force-pushed the derobins/max_size_io_docs branch from ca7c174 to 39a2918 Compare April 1, 2026 16:04
derobins and others added 2 commits April 1, 2026 12:09
Since PAGE_SIZE is not the same on all plaforms, mirror the kernel's
computation of MAX_RW_COUNT so that hipFile doesn't use an incorrect value.
Copilot AI review requested due to automatic review settings April 1, 2026 18:10
@derobins derobins force-pushed the derobins/max_size_io_docs branch from 39a2918 to 015c09f Compare April 1, 2026 18:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@derobins derobins changed the title Add a Doxygen note about max IO size to API calls Update and document max IO size handling Apr 1, 2026
derobins added 2 commits April 1, 2026 13:44
The max IO size constants are initialized using a lambda that:

* Occurs in every TU where it's used
* Can throw on errors during static initialization

The solution is to replace the globals + lambda with
inline functions that have static variables.
Copilot AI review requested due to automatic review settings April 1, 2026 19:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* Minor change to Doxygen macro
* _SC_PAGE_SIZE --> _SC_PAGESIZE
@derobins derobins merged commit 9e40f2f into develop Apr 1, 2026
39 checks passed
@derobins derobins deleted the derobins/max_size_io_docs branch April 1, 2026 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants