Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ACCECN fallback for Prague and BBR2 #20

Merged
merged 53 commits into from
Sep 14, 2023
Merged

Conversation

minuscat
Copy link
Collaborator

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

  • BEFORE this commit, CCA & ECN will act in the following case

    • Reverse mode (Client initiated SYN, Server sends data to Client)
      image

    • Normla mode (Client initiated SYN, Client sends data to Server)
      image

  • AFTER this commit, CCA & ECN will act in the following case

    • Reverse mode (Client initiated SYN, Server sends data to Client)
      image

    • Normla mode (Client initiated SYN, Client sends data to Server)
      image

@bbriscoe
Copy link
Contributor

bbriscoe commented Jun 20, 2023

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 ECT(0)? Because, I don't think a bbr2 client offers ECN feedback unless ecn_tcp=3.
  • 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.
  • 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

@minuscat
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

minuscat commented Jul 1, 2023

Update README.md file, and please review again.

@minuscat minuscat merged commit 4708fab into L4STeam:testing Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants