docs(core): drop incorrect handle_type requirement for cuMemRetainAllocationHandle#2418
Open
aryanputta wants to merge 1 commit into
Open
docs(core): drop incorrect handle_type requirement for cuMemRetainAllocationHandle#2418aryanputta wants to merge 1 commit into
aryanputta wants to merge 1 commit into
Conversation
…ocationHandle Defect 4 of NVIDIA#2388. Signed-off-by: Aryan <aryansputta@gmail.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses defect 4 of #2388.
The
handle_typeentry onVirtualMemoryResourceOptionstold users that"posix_fd"is "required for cuMemRetainAllocationHandle". The precondition as written is wrong, on two independent pieces of evidence in this repo:cuMemRetainAllocationHandle(cuda_bindings/cuda/bindings/driver.pyx) states only that the address must have been previously mapped bycuMemMap. It places no requirement on the allocation's export handle type.VirtualMemoryResource.deallocatecallscuMemRetainAllocationHandle(ptr)unconditionally, underraise_if_driver_error, on every buffer it frees. If an exportable POSIX file descriptor were genuinely required, closing anyhandle_type=Nonebuffer would raise, and it does not.The cost of the sentence is that it steers users into requesting an exportable handle they do not need, which constrains the allocation for no benefit.
To be careful about what is being claimed: the clause may have been shorthand for the fact that you need an exportable handle to do something useful with a retained allocation across processes. That is a different and defensible statement, but it is not what the docstring said, so this removes the incorrect precondition rather than asserting that handle types are irrelevant to IPC.
Scope: the rest of the entry is accurate and is kept, so this removes the parenthetical and reflows the three lines it left ragged. One thing I noticed and deliberately did not touch is that
handle_typealready defaults toVirtualMemoryHandleType.POSIX_FD, which makes the surviving "useposix_fdif you plan to import/export" advice read a little oddly against its own default. That is a pre-existing wording question rather than a correctness one, and rewriting the whole entry felt like scope creep on a defect-4 fix. Happy to fold it in here if you would prefer.No behavior change and no rendered API-surface change, so no release note.
The other defects in #2388 are not covered here. Defect 2 is #2407. Defects 1 and 3 overlap the
Transactionrework in #2235 and the guard restructure in #2237, so I am holding those until those two land rather than building on a moving target.I cannot set a label or milestone as an outside contributor, so
pr-metadata-checkwill be red until a maintainer applies them.documentationpluscuda.core nextlook right.