data segment aggregation limit
ClosedPublic
Actions

Authored by sepherosa_gmail.com on Feb 4 2016, 8:44 AM.

Details

Reviewers

gallatin
howard0su_gmail.com
honzhan_microsoft.com
decui_microsoft.com
• hselasky
np
delphij
royger
adrian

Group Reviewers

network
transport

Commits

rS295740: hyperv/hn: Set the TCP ACK/data segment aggregation limit
rS295739: tcp/lro: Allow drivers to set the TCP ACK/data segment aggregation limit

Summary

It's ACK aggregation limit is append count based, while the TCP data segment aggregation is length based. Unless the network driver sets these two limits, its an NO-OP.

For hn(4):

Set TCP ACK append limit to 1, i.e. aggregate 2 ACKs at most. Aggregate anything more than 2 hurts TCP sending performance in hyperv. This significantly improves the TCP sending performance when the number of concurrent connetion is low (2~8). And greatly stabilize the TCP sending performance in other cases.
Set TCP data segments length limit to 37500. Without this limitation, hn(4) could aggregate ~45 TCP data segments for each connection (even at 64 or more connections) before dispatching them to socket code; large aggregation slows down ACK sending and eventually hurts/destabilizes TCP reception performance. This setting stabilizes and improves TCP reception performance for >4 concurrent connections significantly.

Make them sysctls so they could be adjusted.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

sepherosa_gmail.com updated this revision to Diff 12995.Feb 4 2016, 8:44 AM

sepherosa_gmail.com retitled this revision from to tcp/lro: Allow network drivers to set the limit for TCP ACK/data segment aggregation limit.

sepherosa_gmail.com updated this object.

sepherosa_gmail.com edited the test plan for this revision. (Show Details)

sepherosa_gmail.com added reviewers: network, adrian, delphij, royger, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, gallatin, • hselasky, np.

sepherosa_gmail.com added subscribers: freebsd-net-list, freebsd-virtualization-list.

Herald added a reviewer: transport. · View Herald TranscriptFeb 4 2016, 8:44 AM

sepherosa_gmail.com updated this object.Feb 4 2016, 8:46 AM

sepherosa_gmail.com edited edge metadata.

It might be nice to make these general tunables that could be done centrally and apply to all drivers, but that's probably outside the scope of the review.

sys/netinet/tcp_lro.c
655	Can you just initialize ack_append_limit to the max value for whatever type it is and eliminate the check for a 0 ack_append_limit? That would eliminate one clause from this conditional.
682	Rather than adding more clauses to this condition, how would to feel about setting an append limit in bytes, and replacing the hard-coded 65535 with this new limit? The default lro init would initialize the new limit to 65535. And hn(4) would initialize it in terms of multiples of its MTU.

adrian added inline comments.Feb 4 2016, 4:02 PM

sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
459	this should be a separate commit

sepherosa_gmail.com added inline comments.Feb 5 2016, 1:08 AM

sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c
459	OK, I will split it out.

I will adjust the patch accordingly.

sys/netinet/tcp_lro.c
655	Sure :)
682	Sounds fine to me. I did the byte limit before (https://reviews.freebsd.org/D4825). But it turns out the ACKs need seperate limit (append count based). To make them consistent, I changed the original patch to use append count too.

Address gallatin and adrian's concern.

Herald edited edge metadata. · View Herald TranscriptFeb 5 2016, 3:00 AM

Thanks for addressing my concerns.. Does anybody else want to comment?

Nice! Thanks for all this work!

• hselasky added inline comments.Feb 5 2016, 4:47 PM

sys/netinet/tcp_lro.h
94	Might be worth set this limit to unsigned instead of unsigned short. Technically we can LRO more than 64KBytes worth of data!

sepherosa_gmail.com added inline comments.Feb 6 2016, 1:12 AM

sys/netinet/tcp_lro.h
94	My intention here is too keep the size of lro_ctrl unchanged on amd64 (I think there is an implicit 4 bytes padding after lro_mbuf_max :). But I am fine to change them into unsigned int. Does anyone know any drawbacks to change these two fields into unsigned int? If not, I would change them into unsigned int after Chinese New Year :)

The size of lro_ctrl already changed when the statistics was made 64-bit. Just remember to bump the FreeBSD_version. Might not be possible to MFC.

In D5185#110952, @hselasky wrote:

The size of lro_ctrl already changed when the statistics was made 64-bit. Just remember to bump the FreeBSD_version. Might not be possible to MFC.

OK, let's wait for others inputs (I will be away next week). If no objection comes, I will change the limits to unsigned int.