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

[cc]

Summary

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

[cc]

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]

sys/netinet/cc/cc_htcp.c

393

Could you double check the indentation? It looks strange in this tool.

[cc]

fix the indentation issue

[cc]

sys/netinet/cc/cc_htcp.c

393

Thanks for catching this. fixed

[tuexen]

What about the RACK and BBR stack?

[cc]

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);

[tuexen]

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?

[cc]

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.

[cc]

In D59305#1360844, @cc wrote:

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.

To be clear, my current concern is if the check condition if (tp->snd_cwnd < tp->snd_ssthresh) below can be safely removed.

static void
rack_post_recovery(struct tcpcb *tp, uint32_t th_ack)
{
	struct tcp_rack *rack;
	uint32_t orig_cwnd;

	orig_cwnd = tp->snd_cwnd;
	INP_WLOCK_ASSERT(tptoinpcb(tp));
	rack = (struct tcp_rack *)tp->t_fb_ptr;
	/* only alert CC if we alerted when we entered */
	if (CC_ALGO(tp)->post_recovery != NULL) {
		tp->t_ccv.curack = th_ack;
		CC_ALGO(tp)->post_recovery(&tp->t_ccv);
		if (tp->snd_cwnd < tp->snd_ssthresh) {           << check condition redundant?
			/*
			 * Rack has burst control and pacing
			 * so lets not set this any lower than
			 * snd_ssthresh per RFC-6582 (option 2).
			 */
			tp->snd_cwnd = tp->snd_ssthresh;
		}
	}
...

[cc]

After a second thought, I created D59357 for better elaborating.

[cc]

I am in favor of D59357, therefore, drop this change.