Conversation
|
The first commit 3d73420 keeps the current prioritization but fixes the equality case. We can see in the TGen benchmark that the TGen performance is still mostly the same as the nightly before the change.
The second commit 10db06a changes the prioritization so that we do not modify the socket priority after it has already been pushed into the queue, meaning that the priority is set at the time it enters the queue and stays static until the socket is dequeued. We can see in the TGen benchmark that the TGen performance gets slightly worse because we are no longer able to move the socket ahead in priority when it needs to send control packets (ACKS) in response to incoming packets. That makes TCP slightly less responsive. This performance matches the Rust impl in #3480.
|
|
@stevenengler do we want to merge one or both of these commits before we merge #3480? I think it would be good to have especially 10db06a in the git history since that is the minimal change that is responsible for the performance change, so I think I would lean toward merging this before #3480, even though we're basically going to remove the code in #3480 anyway. |
stevenengler
left a comment
There was a problem hiding this comment.
Nice find! I'm glad there's a good explanation for the difference in graphs.
This allows a socket that had only bad priority packets when it was initially pushed into the queue to move up in priority when it suddenly has new packets with better priority.
Ah I didn't realize (or forgot) that this is what the C version did. I'm not sure which behaviour we want, but I think the new one is more realistic?
@stevenengler do we want to merge one or both of these commits before we merge #3480?
I agree, I think it's good to merge this first.
10db06a to
99197fd
Compare
I think the "old" design is "better" in the sense that it produces a result that is likely closer to a true |
Our fifo qdisc violates the assumption that a queued item's priority does not change while the item is enqueued. This allows a socket that had only bad priority packets when it was initially pushed into the queue to move up in priority when it suddenly has new packets with better priority. This case can happen often in the legacy TCP stack because it assigns priority=0 to control packets (ACKS) without data. This means we can be more responsive to TCP controls.
This PR does two things:
If we only apply the first commit, the performance should be identical to nightly. If we apply both commits, the performance should be identical to #3480.