tcp cc: move the cc relevant part of cc_post_recovery() into cc modules.
AcceptedPublic
Actions

Authored by cc on Tue, Sep 1, 1:54 PM.

Details

Reviewers

rscheff
tuexen

Group Reviewers

transport

Summary

No functional change intended.
Improves each individual cc's responsibility for congestion control.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 76381
Build 73264: arc lint + arc unit

Event Timeline

cc created this revision.Tue, Sep 1, 1:54 PM

Herald added a reviewer: transport. · View Herald TranscriptTue, Sep 1, 1:54 PM

Herald added subscribers: glebius, melifaro, imp. · View Herald Transcript

cc requested review of this revision.Tue, Sep 1, 1:54 PM

Harbormaster completed remote builds in B76376: Diff 185560.Tue, Sep 1, 1:54 PM

cc added inline comments.Tue, Sep 1, 1:58 PM

sys/netinet/cc/cc.c
392	Many other cc modules' post_recovery() actually calls this `newreno_cc_post_recovery()`, except for `cubic` and `htcp`.

tuexen added inline comments.Tue, Sep 1, 2:16 PM

sys/netinet/cc/cc_htcp.c
393	Could you double check the indentation? It looks strange in this tool.

fix the indentation issue

Harbormaster completed remote builds in B76381: Diff 185567.Tue, Sep 1, 2:24 PM

cc marked an inline comment as done.Tue, Sep 1, 2:25 PM

cc added inline comments.

sys/netinet/cc/cc_htcp.c
393	Thanks for catching this. fixed

tuexen accepted this revision.Tue, Sep 1, 6:52 PM

This revision is now accepted and ready to land.Tue, Sep 1, 6:52 PM

What about the RACK and BBR stack?

In D59305#1360805, @tuexen wrote:

What about the RACK and BBR stack?

They don't seem to be impacted or relevant.

static void rack_post_recovery(struct tcpcb *tp, uint32_t th_ack);

static void bbr_post_recovery(struct tcpcb *tp);

In D59305#1360809, @cc wrote:
In D59305#1360805, @tuexen wrote:

What about the RACK and BBR stack?

They don't seem to be impacted or relevant.
static void rack_post_recovery(struct tcpcb *tp, uint32_t th_ack);

static void bbr_post_recovery(struct tcpcb *tp);

I guess BBR is fine, but rack_post_recovery() calls CC_ALGO(tp)->post_recovery(), which you are changing. Doesn't it needs the corresponding changes?

In D59305#1360814, @tuexen wrote:
In D59305#1360809, @cc wrote:
In D59305#1360805, @tuexen wrote:

What about the RACK and BBR stack?

They don't seem to be impacted or relevant.
static void rack_post_recovery(struct tcpcb *tp, uint32_t th_ack);

static void bbr_post_recovery(struct tcpcb *tp);
I guess BBR is fine, but rack_post_recovery() calls CC_ALGO(tp)->post_recovery(), which you are changing. Doesn't it needs the corresponding changes?

I think rack_post_recovery() also calls rack_exit_recovery() at the end, which is a dup after this patch. The only concern, in rack_post_recovery(), will be can snd_cwnd be larger than snd_ssthresh after calling CC_ALGO(tp)->post_recovery() before this patch? If so, then it seems like a bug introduced by commit 506e3e30a43cc04a21aa65a423bbd1cc4e0543f8. Or as commit 506e3e30a43cc04a21aa65a423bbd1cc4e0543f8 indicates, "Set cwnd to ssthresh post recovery. (RFC 9937 4)" is more necessary. Then, "cwnd shall be set to ssthresh post recovery" for all seems like a simple ruling.