L4STeam / linux

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

tcp_prague.c: Prague CC and ECT1 enabled if RFC3168 signaling negotiated #10

Open nealcardwell opened 2 years ago

nealcardwell commented 2 years ago

In the tcp_prague.c code, in prague_init() it disables Prague if no ECN support has been negotiated:

    if (!tcp_ecn_mode_any(tp) &&
        sk->sk_state != TCP_LISTEN && sk->sk_state != TCP_CLOSE) {
            prague_release(sk);
            LOG(sk, "Switching to pure reno [ecn_status=%u,sk_state=%u]",
                tcp_ecn_mode_any(tp), sk->sk_state);
            inet_csk(sk)->icsk_ca_ops = &prague_reno;
            return;
    }

    tp->ecn_flags |= TCP_ECN_ECT_1;
    ...

However, AFAICT that means if RFC3168 support is negotiated then Prague CC stays enabled and enables ECT1.

Probably this should be checking for AccECN support instead:

    if (!tcp_ecn_mode_accecn(tp) &&
      ....

That would match more closely the logic in Ilpo's patch for bbr2.c:

git show 4b75165fb450b commit 4b75165fb450bc5fa79385ed47771c43b719573e Author: Ilpo Järvinen ilpo.jarvinen@cs.helsinki.fi Date: Mon Jun 28 11:07:23 2021 +0300

l4s: make BBR v2 want ECT(1)

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>

diff --git a/net/ipv4/tcp_bbr2.c b/net/ipv4/tcp_bbr2.c index 5510adc92bbb4..2b4fc9abb1dbc 100644 --- a/net/ipv4/tcp_bbr2.c +++ b/net/ipv4/tcp_bbr2.c @@ -2471,6 +2471,8 @@ static void bbr2_init(struct sock *sk) bbr->alpha_last_delivered_ce = 0;

    tp->fast_ack_mode = min_t(u32, 0x2U, bbr_fast_ack_mode);

This issue is in the current tcp_prague.c from:

3cc3851880a1b (tag: testing-build, l4steam/testing, l4s/testing) github workflows

7d7c8a92714b4 (l4steam/tcp_prague, l4s/tcp_prague) tcp: Optionally pace IW

koen0607 commented 1 year ago

I recall that at some point Prague also supported or still supports something in between RFC3168 and ACC_ECN: DCTCP-type of Classic ECN, so that only CE packets are ECEed and that receiving a non-CE packet would clear the ECE instead of waiting for CWR.

Not sure if this still works and is needed/wanted because otherwise this issue would indeed need to be fixed.

As there are no DCTCP-type Echo-CE Clients/servers on the Internet, it seems to be useless, and even dangerous if the other side is a plain RFC3168 receiver, as Prague will get a full RTT of ECEs for every CE. We never further explored sending consistent/frequently CWRs per RTT to see if we still would get some cleared ECE throughput from a Classic ECN receiver.

Any opinions?

nealcardwell commented 1 year ago

I agree with your statement: "As there are no DCTCP-type Echo-CE Clients/servers on the Internet, it seems to be useless, and even dangerous if the other side is a plain RFC3168 receiver, as Prague will get a full RTT of ECEs for every CE."

So I would still suggest that if the connection has not negotiated AccECN then it should fall back to Reno:

   if (!tcp_ecn_mode_accecn(tp) &&
      ....
      inet_csk(sk)->icsk_ca_ops = &prague_reno;
cpaasch commented 1 year ago

+1 on @nealcardwell 's suggestion.

And beyond that, it would be great to have a configurable fallback CC. Meaning, instead of falling back to Reno after negotiation, fall back to Cubic/BBR/... whatever would be the system's "secondary" default (probably needs yet another sysctl..).

minuscat commented 1 year ago

@nealcardwell and @cpaasch A new pull https://github.com/L4STeam/linux/pull/20 request is made to fix ACCECN fallback for Prague and BBR2. This is to fix the both Prague and BBR2 on the response when ACCECN is not successfully negotiated and how the downgrade shall be have, note that the operations of Prague and BBR2 are different. Please check the figures shared in that pull request for more details and comments are welcomed.