Add flags to the core queue interface for device-side ring buf/queue descriptor allocation #284
Conversation
This file contains 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
|
FYI @benvanik this is still WIP, but let me know if this works for you and if the cacheable flag works/has benefits. |
dayatsin-amd
requested changes
Jan 23, 2025
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dayatsin-amd
force-pushed
the
amd-staging
branch
from
ea66d58 to
9971e7b
Compare
atgutier
force-pushed
the
atgutier/queue-struct-dev-mem
branch
from
76a3db2 to
fe3307c
Compare
atgutier
commented
Jan 24, 2025
| @@ -1272,7 +1272,7 @@ void BlitKernel::PopulateQueue(uint64_t index, uint64_t code_handle, void* args, | |||
| std::atomic_thread_fence(std::memory_order_acquire); | |||
| queue_buffer[index & queue_bitmask_] = packet; | |||
| std::atomic_thread_fence(std::memory_order_release); | |||
| if (core::Runtime::runtime_singleton_->flag().dev_mem_queue() && !queue_->needsPcieOrdering()) { | |||
| if (queue_->IsDeviceMem() && !queue_->needsPcieOrdering()) { | |||
There was a problem hiding this comment.
@saleelk I'm curious if the logic is correct for the original needsPcieOrdering() method you added. Shouldn't this really be:
queue_->needsPcieOrdering()
Meaning we need to change the logic of that call internally?
atgutier
force-pushed
the
atgutier/queue-struct-dev-mem
branch
from
fe3307c to
6bbccb0
Compare
atgutier
force-pushed
the
atgutier/queue-struct-dev-mem
branch
from
6bbccb0 to
436b64a
Compare
dayatsin-amd
approved these changes
Jan 28, 2025
atgutier
force-pushed
the
atgutier/queue-struct-dev-mem
branch
9 times, most recently
from
51dc622 to
4533c8d
Compare
saleelk
reviewed
Jan 29, 2025
| * The queue packet buffer and the queue struct should be allocated in | ||
| * the agent's device memory. | ||
| */ | ||
| HSA_AMD_QUEUE_FLAG_DEVICE_MEM = (1 << 0), |
There was a problem hiding this comment.
We should explicitly mention whether its cached on uncached. Like HSA_AMD_QUEUE_HOST = 0, HSA_AMD_QUEUE_DEV_UNCACHED = 1 << 0,
There was a problem hiding this comment.
Removed the cache flag for now.
saleelk
reviewed
Jan 29, 2025
| * Used to indicate if the queue created in device memory should be | ||
| * cacheable. | ||
| */ | ||
| HSA_AMD_QUEUE_FLAG_CACHEABLE = (1 << 1), |
There was a problem hiding this comment.
We should probably discuss if this is even doable at the moment, to flush L2 is costly and if we are writing a packet (aka 64bytes), we can probably also have false sharing issues/
atgutier
force-pushed
the
atgutier/queue-struct-dev-mem
branch
from
4533c8d to
47357bf
Compare
atgutier
changed the title
This builds on a prior change that allowed for allocating a user-mode queue's packet buffer in device memory to also allocate the queue struct in device memory. This provides additional latency benefits particularly for cases where dispatches are performed from the GPU itself. Flags are added to support the various use cases.
atgutier
force-pushed
the
atgutier/queue-struct-dev-mem
branch
from
47357bf to
b601fbc
Compare
Adds a bool to the GPU agent and a public member method to check if the GPU supports large BAR. This is needed so we can check if large BAR is supported when a user tries to allocate an AQL queue in device memory on a given GPU agent. Also adds an exception to the AQL queue if device-side AQL queues are requested and the GPU owner of the AQL doesn't support large BAR. Otherwise, ROCr will currently allow device-side queues that can cause faults when the user tries to touch their ring buffers and the user will not know why the faults are occuring. This relies on the fact that the KFD does not exposed any links from the CPU to the GPU if large BAR is not enabled (though links from the GPU to the CPU may still be exposed by the KFD).
|
I was going to test this but I noticed there is no longer any such function as |
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.
Add flags to the queue interface to choose whether or not to allocate it in device memory.
Now the queue struct is also allocated in device memory.
Fixes a bug due to missing the executable flag when allocating the ring buffer in device memory.