[ChannelGenerator] Assign the channel only for circuit flow #1013
Conversation
Yu-Zhewen
requested review from
makslevental,
MaheshRavishankar,
nirvedhmeshram,
yzhang93,
Abhishek-Varma and
jtuyls
as code owners
Yu-Zhewen
changed the title
Yu-Zhewen
changed the title
|
|
||
| /// Returns its next usable producer channel. | ||
| std::optional<uint8_t> getAndAssignProducerDMAChannel() { | ||
| for (uint8_t i = 0; i < numProducerChannels; i++) { | ||
| std::optional<uint8_t> getAndAssignProducerDMAChannel(bool isPacketFlow) { |
There was a problem hiding this comment.
getAndAssignProducerDMAChannel indicates that a channel is retrieved and assigned. Not assigning it based on some internal condition will lead to confusion and potential bugs, so instead, I think we should have separate getDMAChannel and assignChannel (already exists) functions which can be called by a user (AssignChannels) when appropriate. We can keep getAndAssignProducerDMAChannel around if it is still useful and call getDMAChannel and assignChannel under the hood.
| numConsumerChannels(numConsumerChannels) { | ||
| assert(numProducerChannels > 0 && numConsumerChannels > 0 && | ||
| "Invalid number of producer/consumer channels."); | ||
| // Initialize to the last channel for round-robin usage. |
There was a problem hiding this comment.
I think it makes sense to have an AssignmentMode similar to the ChannelBdIdGenerator:
ChannelAssignmentMode {
FirstAvailable,
RoundRobin
}
Or if the logic differs quite a bit, use specialized child classes instead: RoundRobinChannelGenerator, FirstAvailableChannelGenerator.
| DenseSet<uint8_t> assignedProducerChannels; | ||
| DenseSet<uint8_t> assignedConsumerChannels; | ||
| // Tracks the last used channel, for both circuit and packet flows. |
There was a problem hiding this comment.
There might be a bit of unclarity about what 'using' a DMA channel means, so I think 'lastRetrievedDMAChannel' might be a more clear name. And maybe document this as well.
No description provided.