router: Priority forwarding queues#4866
Conversation
The udp/ip underlay provider now uses priority queues to send the received packets. This is done by reading first all of the priority queue, and only then reading the best-effort one. Also, modify the layout of the Packet struct to automatically align to 64 bytes, instead of manually adding padding (which failed for zero padding bytes).
| case v, ok := <-queue[0]: | ||
| return v, ok | ||
| case v, ok := <-queue[1]: | ||
| return v, ok |
There was a problem hiding this comment.
This might break priority order because select implements uniform pseudo-random selection in case more than one communication becomes available at the same time. I guess we are fine with this edge case, right?
There was a problem hiding this comment.
For most of the cases, it should not break the priority contract. At the point where we do the select on the two channels (right after the comment // Block until any queue has a value.), there was no packet in the priority queue and we are waiting for any packet to arrive. If two packets arrive simultaneously, this thread will be awaken by the first unlocked channel. This is the common case.
The less common case involves a race between this reader blocked or about to be blocked on the select and a sender (or more) sending two packets, one to the priority channel and one to the best-effort one.
Although possible (this reader routine could be parked for enough cycles for the writers to write two packets to the channels), I consider this case to happen seldom enough to not have a real impact.
| // of _pad). See notes for the Packet struct. | ||
| _ [_pad]byte | ||
| // Priority forwarding label: packets with more priority are forwarded first | ||
| QueueIndex pr.PriorityLabel |
There was a problem hiding this comment.
Doesn't this result in packets having QueueIndex == WithPriority by default, if not explicitly set? This would be somewhat counterintuitive, right? As far as I can see, there's the tension between two design choices: We want to have highest priority at index 0 in router/priority/priority.go and uninitialized fields in packets should default to 'best effort' instead of highest priority here.
There was a problem hiding this comment.
This is a good catch. The original assumption is that every processor would determine the priority for the packet, and this is not happening at the moment: the QueueIndex field is not reset.
I will add this in Packet.reset() now.
There was a problem hiding this comment.
Additionally, I am also renaming QueueIndex to PriorityLabel, as I think it is a more intuitive name.
|
As already discussed in the open source contributors call, we're in an unfortunate dilemma here. On the one hand, we don't want to block progress towards the implementation of Hummingbird while on the other hand intermediate steps like this priority queue mechanism introduce additional complexity with unclear benefits: at this point, differentiation only applies on egress. Ingress and packet processing use simple FIFO queueing. A high-priority packet arriving behind best-effort packets will still wait for them to be processed first, resulting in weak end-to-end QoS guarantees when bottlenecks are elsewhere (ingress saturation, processor contention). I'd welcome additional opinions on how to proceed with this in a constructive way. |
juagargi
changed the title
juagargi
changed the title
|
It is decided to merge against a Netsec local version of the border router instead of |
2 participants
This PR introduces priority forwarding queues at the border router.
The main changes are:
Packetdata structure now contains aQueueIndexfield, that denotes the priority of the packet. It can be set toWithPriorityor toWithBestEffort.udpip.gothat reads the packets from the packet processor output channel, and sends them viaWriteBatchhas changed, mainly inside the functionreadUpTo, where priority packets are always read before anything else.sendandreadUpTouse now circular iterators (ring buffer iterators). See clarification below.Because we are now reading from two distinct Go channels (in priority order), the amount of time needed to send N packets with the new feature is greater than before.
To improve performance, circular iterators are used in the function
udpConnection.send.Accompanying benchmarks are in the PR, mainly to ease the discussion on how not to impact performance (too much) while still having priority queues.