search

L4STeam / linux

Kernel tree containing patches for TCP Prague and the dualpi2 qdisc
Other
47 stars 16 forks source link

Fix ACCECN fallback for Prague and BBR2 #20

Closed minuscat closed 10 months ago

minuscat commented 1 year ago

This commit includes changes to fix ACCECN fallback for Prague and BBR2.

bbriscoe commented 1 year ago

I'm reviewing the tables of goals (rather than the code). I agree with the 'after commit' tables, except for one query and a couple of clarification points:

minuscat commented 1 year ago

Please see my inline comments below:

I'm reviewing the tables of goals (rather than the code). I agree with the 'after commit' tables, except for one query and a couple of clarification points:

  • In the 'Reverse mode' table, where the bbr2 ecn_tcp=1 row intersects cubic ecn_tcp={3,2,1}, shouldn't the three cells be Non-ECT, not ECN(0)? Because, I don't think a bbr2 client offers ECN feedback unless ecn_tcp=3.

[Chia-Yu] Here the meaning of ECT(0) in that cell is to say data is sent with '10' in the ECN fields of IP header, but this does not directly imply legacy ECN feedback is provided. See my comment in the last bullet point.

  • Given these tables are likely to be used as explanation for others, I suggest changing the headings of each (assuming my interpretation is correct?), because reverse/normal are surely not /modes/:
    • Reverse mode (Client initiated SYN, Server sends data to Client) → Server as Data Sender
    • Normla mode (Client initiated SYN, Client sends data to Server) → Client as Data Sender
  • It would help with immediate visualization if the axes were in tcp_ecn={3,1,2,0} order and prague, bbr2, cubic.

[Chia-Yu] Sure these two points I will fix the script to create the tables and later create new actions Github Actions

  • It would help to first show what feedback mode each cell enters, which then helps explain why each end sends the ECN field it does, and why it uses the CCA it does. Given the feedback mode should be the same for both ends, it could be shown in a table common to both directions, as below: 20230620NegotiationAccECN

[Chia-Yu] We can add the table as the 1st table, let me think how to make it as an Github Action, and following modes I agree with you but I would say we will need further fix for following cases to make how data is sent and how ECN action is done in sync.

  1. "Cubic tcp_ecn=3" + "BBR2, tcp_ecn=2/1" --> ECN(0) is sent from client to server, but in real is Non-ECN feedback mode
  2. "Cubic tcp_ecn=1" + "BBR2, tcp_ecn=3/2/1" --> ECN(0) is sent from client to server now, but in real is Non-ECN feedback mode
  3. "BBR2, tcp_ecn=3" + "Cubic tcp_ecn=2/1" --> ECN(0) is sent from server to client, but in fact is Nno-ECN feedback mode
  4. "BBR2, tcp_ecn=1" + "Cubic tcp_ecn=3/2/1" --> ECN(0) is sent from server to client, but in fact is Nno-ECN feedback mode
bbriscoe commented 1 year ago

I understood that ECTX is what is set in the ECN field in one direction, and doesn't directly infer the feedback mode. But when I was trying to reverse engineer the feedback mode that bbr2 negotiates, I just didn't pay careful enough attention to every clue in the table. Now you've pointed out the other rows where I was wrong (tcp_ecn = {3,1} for cubic & bbr2), I think I've worked it out. Here's my corrected feedback mode table:

20230621NegotiationAccECN

minuscat commented 1 year ago

Update README.md file, and please review again.