Skip to content

Add a Doxygen note about max IO size to API calls#206

Open
derobins wants to merge 1 commit intodevelopfrom
derobins/max_size_io_docs
Open

Add a Doxygen note about max IO size to API calls#206
derobins wants to merge 1 commit intodevelopfrom
derobins/max_size_io_docs

Conversation

@derobins
Copy link
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
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.

Comment on lines 532 to 533
* hipFileRead() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
* returning the number of bytes actually transferred.
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The doc now states both a fixed max transfer size (0x7ffff000) and then expands \max_io_size_note (“2^31 - PAGE_SIZE”). These can diverge (e.g., non-4K PAGE_SIZE) and read as contradictory. Consider replacing the hard-coded 0x7ffff000 sentence with the alias (or adjust the alias to match the constant used by hipFile) so there is a single authoritative statement of the limit.

Suggested change
* hipFileRead() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
* returning the number of bytes actually transferred.
* hipFileRead() will transfer at most the maximum I/O size described in
* \max_io_size_note, returning the number of bytes actually transferred.

Copilot uses AI. Check for mistakes.
Comment on lines 555 to +559
* hipFileWrite() will transfer at most 0x7ffff000 (2,147,479,552) bytes,
* returning the number of bytes actually transferred.
*
* \max_io_size_note
*
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Same issue as hipFileRead(): the comment includes a fixed max (0x7ffff000) and also \max_io_size_note (“2^31 - PAGE_SIZE”). To avoid potentially conflicting documentation, prefer one source of truth (either use the alias in place of the hard-coded value, or make the alias explicitly reflect the constant hipFile enforces).

Copilot uses AI. Check for mistakes.
# @} or use a double escape (\\{ and \\})

ALIASES = hipFileError_t="A hipFileError_t struct that holds both hipFile and HIP error values"
ALIASES += max_io_size_note="@note The maximum IO size is determined by the Linux kernel and is currently 2^31 - PAGE_SIZE"
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This alias claims the max IO size is “determined by the Linux kernel” and “currently 2^31 - PAGE_SIZE”, but hipFile appears to hard-code MAX_RW_COUNT to 0x7ffff000 (see src/amd_detail/backend.h). On systems with non-4K PAGE_SIZE, the kernel’s MAX_RW_COUNT differs, so this note could be misleading. Suggest rewording to match what hipFile actually enforces (e.g., explicitly say hipFile caps requests to 0x7ffff000 / Linux MAX_RW_COUNT on supported platforms), or update the implementation to derive the limit from PAGE_SIZE if that’s the intended behavior.

Suggested change
ALIASES += max_io_size_note="@note The maximum IO size is determined by the Linux kernel and is currently 2^31 - PAGE_SIZE"
ALIASES += max_io_size_note="@note hipFile currently caps individual IO requests to 0x7ffff000 bytes (matching Linux MAX_RW_COUNT on systems with 4 KiB pages)."

Copilot uses AI. Check for mistakes.
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.

3 participants