In D7986#1184580, @kbowling wrote:

@imp if I recall this was something we were looking at on my team at LLNW, if it were in Bugzilla I'd say "Close - OBE" is appropriate.

@cc since you've been working on CUBIC lately, the only thing possibly still relevant here is turning the first level if into the early returns.. it removes one level of indention which might help justify a couple other nearby style(9) fixes but it's not worth holding this review open.

Thanks for the following improvements:

I am ok with this patch. But I want to be sure how snd_recover shrinks. Is it because tp->snd_recover = tp->snd_recover_prev in CC_RTO_ERR? But that shall not be in SACK recovery.

Other than the fd is not used in the test code, I am good with this change.

Thanks for the elaboration. Looks good to me now.

Looks I am missing the context. Given the fact that the socket is in TCPS_TIME_WAIT while switching to the default stack, was this assert a day one issue that was not test covered before? Or was this panic caused by recent changes?

This is for the fast path fix. Just want to confirm with your that a second patch may be around to improve both of the fast and slow path?

Didn't see any surprising regression from my test result for this patch:
testD49047

code update based on Richard's comments

re-base, add a missing part and update based on Richard's comment in the meeting

In D49047#1118719, @glebius wrote:

Question: why does this function returns signed value? And why some of the sackhint counters are signed?

There are several calls to tcp_compute_pipe() that did not check the V_tcp_do_newsack: 2 in tcp_output and 1 in tcp_input. Their behavior will change with revised SACK turned off. Commit message should explain why this is okay.

Why not move the old_method: label above the stack variables' declaration? I think it may be cleaner to read.
Like this:

Additional comment:

update:
Looks this patch has some significant reduction on fragment (data_size % MSS) > 0 out of TSO data chunks: testD47474
TSO not enabled:

In D47474#1084985, @rscheff wrote:

Well, I had the same thought - the full MSS (including options) is less frequently used, that the mss without options...

So, it would certainly be more efficient to store the MSS exkluding options in tcpcb, and calculating the MSS inkl. option space only where that is needed...

But that should a different Diff IMHO, as this one is more in the break/fix category.

I think you meant the title be:
tcp: consistently set CWND to MSS => tcp: consistently set CWND to 1
in case of SYN/SYN ACK retransmissions => in case of SYN retransmissions

OK. I am approving it now as my test in https://wiki.freebsd.org/chengcui/testD43470 shows some improvement. Any bug related observations can be fixed later.

Also, please correct the SUMMARY section:

In D30155#1076291, @kbowling wrote:

In D30155#1075233, @cc wrote:

From my test result in testD30155, I didn't find any significant improvement under my eyes:

• no significant difference in ping latency
• no significant iperf3 performance improvement due to bad performance (3.x Gbps) in FreeBSD 15-current vs. (9.x Gbps) in stock Linux kernel 5.15.

Thanks for the results @cc. Something seems very strange with the throughput there, the main system I am testing is a xeon-d that is much less than 1/4th as powerful and can line rate both directions no issues and I also have an older 2x Xeon E5-2695 v2 (two NUMA domains) without throughput limitations. I will see if I can find my emulab credentials and take a look there, it seems like these might be 4-way NUMA machines but it is not expected to me that that would cause this magnitude of throughput issues, especially at the 10gbit data rate.

update code based on discussion

From my test result in testD30155, I didn't find any significant improvement under my eyes:

Better now. But it can be cleaner.

Add the __inline keyword to avoid overhead when possible.

My current concern is that the definition and the usage of the super set macro TH_FLAGS or TCPF_ALL are inconsistent. For example, TH_ECE is in TH_FLAGS, but TH_ECN is in TCPF_ALL.

In D30155#1073639, @kbowling wrote:

Ok this is a bit messy code and comment wise but I have the new algorithm working in what I believe to be the correct way with some bug fixes versus the origin and would like some data to see how to proceed before tidying everything up.
@cc it looks like emulab has ix(4) on d820s nodes, would you be willing to take a look at these 3 options similar to the e1000 test?

• Default in HEAD/STABLE: sysctl dev.ix.<N>.enable_aim=0
• New algorithm (on by default with this patch) sysctl dev.ix.<N>.enable_aim=1
• Old algorithm (FreeBSD <10) sysctl dev.ix.<N>.enable_aim=2

I current concern is that new code for the TH_AE shall be in a separate patch, so that this patch can be a pure big non-functional change.

Need code update.

Because of commit 440f4ba18e3a, please re-base.

By the way based on my test, I didn't find this statement In addition, cwnd used to be 1 MSS right after RTO, increasing to 2 MSS more recently. to be true in your SUMMARY section. Also Address this by setting up snd_recover just in cc_cong_signal. needs to be revised.

With the provided packetdrill scripts before/after the fix, my test result is in my wiki: testD43355.

I have no problem with this patch after testing it in Emulab. The test result is in my above comment.

If I recall these machines are Pentium 4 era and pretty CPU constrained. You can try the tunable 'hw.em.unsupported_tso=1' and then enable TSO on the interface to get some more bulk bandwidth, they are stable with TSO.

Are you able to detect any improvements or regressions otherwise? ping-pong time at low packet rate between two systems both set with enable_aim=0,1,2 would be interesting.

In D46768#1069027, @kbowling wrote:

In D46768#1069015, @cc wrote:

In D46768#1067607, @kbowling wrote:

@cc this code works well in my testing. There are now some quality of life improvements, at runtime you can now switch in the middle of a test. I run a tmux session with three splits, one of systat -vmsat, one of the benchmark (iperf3 or whatever), and one to either toggle sysctl dev.{em,igb}.<interface number>.enable_aim=<N> where <N> description which follows. You can also do something like sysctl dev.igb.0 | grep _rate to see the current queue values.

Existing static 8000 int/s behavior (how the driver is in main):

sysctl dev.igb.0.enable_aim=0

Suggested new default, you will boot in this mode with this patch:

sysctl dev.igb.0.enable_aim=1

Low latency option of above algorithm (up to 70k ints/s):

sysctl dev.igb.0.enable_aim=2

ixl(4) algorithm bodged in that would need to be cleaned up:

sysctl dev.igb.0.enable_aim=3

I would be curious to know what you find with these different options in an array of testing and I will use the results to ready this for actual use.

I didn't find any rate change by the sysctl. Please let me know if the hardware does not support this new change.

root@s1:~ # sysctl dev.em.2.enable_aim=0
dev.em.2.enable_aim: 0 -> 0
root@s1:~ # sysctl dev.em.2 | grep _rate
dev.em.2.queue_rx_0.interrupt_rate: 20032
dev.em.2.queue_tx_0.interrupt_rate: 20032
root@s1:~ # sysctl dev.em.2.enable_aim=1
dev.em.2.enable_aim: 0 -> 1
root@s1:~ # sysctl dev.em.2 | grep _rate
dev.em.2.queue_rx_0.interrupt_rate: 20032
dev.em.2.queue_tx_0.interrupt_rate: 20032
root@s1:~ # sysctl dev.em.2.enable_aim=2
dev.em.2.enable_aim: 1 -> 2
root@s1:~ # sysctl dev.em.2 | grep _rate
dev.em.2.queue_rx_0.interrupt_rate: 20032
dev.em.2.queue_tx_0.interrupt_rate: 20032
root@s1:~ #

This looks to me like it is working, the algorithm is dynamic and 20k would be latency reducing idle queue. At enable_aim=0, you would see 8000. 20k looks right for an idle queue, what happens if you place a bulk load through it like iperf3? It should drop down to 4k.

In D46824#1068983, @tuexen wrote:

In D46824#1068981, @jhb wrote:

I can fix the type mismatch during commit. I have not looked to see if other stacks are affected.

Fixing the type mismatch would be good. I think other stacks are not affected, since I think they
do not send a FIN before any outstanding data is ACKed and nothing is buffered anymore.

In D46768#1067607, @kbowling wrote:

@cc this code works well in my testing. There are now some quality of life improvements, at runtime you can now switch in the middle of a test. I run a tmux session with three splits, one of systat -vmsat, one of the benchmark (iperf3 or whatever), and one to either toggle sysctl dev.{em,igb}.<interface number>.enable_aim=<N> where <N> description which follows. You can also do something like sysctl dev.igb.0 | grep _rate to see the current queue values.

Existing static 8000 int/s behavior (how the driver is in main):

sysctl dev.igb.0.enable_aim=0

Suggested new default, you will boot in this mode with this patch:

sysctl dev.igb.0.enable_aim=1

Low latency option of above algorithm (up to 70k ints/s):

sysctl dev.igb.0.enable_aim=2

ixl(4) algorithm bodged in that would need to be cleaned up:

sysctl dev.igb.0.enable_aim=3

I would be curious to know what you find with these different options in an array of testing and I will use the results to ready this for actual use.

I think this change also applies to the bbr and rack stacks.

Looks good to me. Thanks for removing the goto label skip_alloc that improves reading.