Explanation of new feature Added TCP Congestion control support. -C, --linux-congestion <algo> set TCP congestion control algorithm (Linux only)

Attached patch

Thanks, Susant

thanks for the patch!

Susant: I went ahead and made you a committer, so you can add this if you'd like.

Just a couple of comments on the patch: - The references to test->congestion do not need to be ifdeffed. - The references to TCP_CONGESTION in iperf_tcp.c probably do need to be ifdeffed. - Make sure the flag gives an error on on-linux systems; it's ifdeffed in longopts but not in getopt_long or the switch cases. - The IESETCONGESTION case in iperf_error.c should probably set perr = 1. - Don't forget to add it to the man page, and the wiki copy of the man page.

Couple more notes on this change: - Not sure we want to transmit the setting from client to server, the other alternative is to specify it separately on the two sides. - If we do want to transmit it, we need to strdup the value client side as well as server side, and free it. Also we should set client_flag=1. - Not sure sending sizeof(test->congestion) to setsockopt is right - that's the size of the pointer. Shouldn't it be strlen instead?

Modified the patch

• The references to test->congestion do not need to be ifdeffed. Done
• The references to TCP_CONGESTION in iperf_tcp.c probably do need to be ifdeffed. Done
• Make sure the flag gives an error on on-linux systems; it's ifdeffed in longopts but not in getopt_long or the switch cases. Done

• The IESETCONGESTION case in iperf_error.c should probably set perr = 1. Not sure .. setting perr =1 resulting in wrong error code .. "File not found"

• Not sure we want to transmit the setting from client to server, the other alternative is to specify it separately on the two sides.

  If it's about JSON setting then The API itself doing a strdup void cJSON_AddItemToObject( cJSON *object, const char *string, cJSON *item ) { if ( ! item ) return; if ( item->string ) cJSON_free( item->string ); item->string = cJSON_strdup( string ); cJSON_AddItemToArray( object, item ); }

• Not sure sending sizeof(test->congestion) to setsockopt is right - that's the size of the pointer. Shouldn't it be strlen instead? Modified

Thanks.

The cJSON library does do a strdup, but then it frees the item when cJSON_Delete is called, so you can't save that copy and have to make your own. So, get_parameters should strdup congestion, therefore iperf_free_test should free it, and therefore iperf_parse_arguments should strdup it too. That last part is the only thing still missing in this version of the patch.

I'm not sure what the problem is with perr. The setsockopt man page says it sets errno so that should work. Oh wait, you're calling close - that probably overwrites errno even if it succeeds. I guess all the other setsockopt calls (and the bind call) have the same problem and no one ever noticed.

Oh and the flag still needs to be added to the man page.

Thanks for reviewing . iperf_parse_arguments it's doing a malloc

I believe I added the free part

iperf_free_test

• if(test->congestion)
• free(test->congestion);

Oh right, iperf_parse_arguments is doing a malloc and strncpy. My eyes were looking for strdup and missed that. Ok.

Although this is assigned to Bruce I will probably do it later tonight. Now that we have hashed it out, it's just a matter of applying the patch and testing, and I want to log another hour for this week.

Ok, added.

Someone needs to test whether it's actually working though! First step would be to find a list of valid congestion control algorithm names.

I put in code to save & restore errno across the close() calls. However it's still giving "file not found". I put in a printf to be sure - yep, that's really the errno. Is it possible that's the actual error code returned when you ask for an algorithm that doesn't exist?

Thanks for committing .

Here is a doc speaks about the list . http://linuxgazette.net/135/pfeiffer.html

In My Fedora 20 cubic is default : $ cat /proc/sys/net/ipv4/tcp_congestion_control cubic Also with RHEL 6.X

I think It can be added to wiki how to figure out what are the available congestion control algo . This is what I could pick from the kernel makefile

sus@max ipv4]$ uname -a Linux max 3.11.10-300.fc20.x86_64 #1 SMP Fri Nov 29 19:16:48 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

obj-$(CONFIG_TCP_CONG_BIC) += tcp_bic.o obj-$(CONFIG_TCP_CONG_CUBIC) += tcp_cubic.o obj-$(CONFIG_TCP_CONG_WESTWOOD) += tcp_westwood.o obj-$(CONFIG_TCP_CONG_HSTCP) += tcp_highspeed.o obj-$(CONFIG_TCP_CONG_HYBLA) += tcp_hybla.o obj-$(CONFIG_TCP_CONG_HTCP) += tcp_htcp.o obj-$(CONFIG_TCP_CONG_VEGAS) += tcp_vegas.o obj-$(CONFIG_TCP_CONG_VENO) += tcp_veno.o obj-$(CONFIG_TCP_CONG_SCALABLE) += tcp_scalable.o obj-$(CONFIG_TCP_CONG_LP) += tcp_lp.o obj-$(CONFIG_TCP_CONG_YEAH) += tcp_yeah.o obj-$(CONFIG_TCP_CONG_ILLINOIS) += tcp_illinois.o

Cool. I confirmed that my test hosts accept cubic and htcp. Don't know how to verify that they actually do anything different. Perhaps we just have to accept that part on faith.

To check which algorithms are supported by the running kernel, the following command can be used :

$ cat /boot/config-uname -r |grep CONFIG_TCP_CONG

CONFIG_TCP_CONG_ADVANCED=y CONFIG_TCP_CONG_BIC=m CONFIG_TCP_CONG_CUBIC=y CONFIG_TCP_CONG_WESTWOOD=m CONFIG_TCP_CONG_HTCP=m CONFIG_TCP_CONG_HSTCP=m CONFIG_TCP_CONG_HYBLA=m CONFIG_TCP_CONG_VEGAS=m CONFIG_TCP_CONG_SCALABLE=m CONFIG_TCP_CONG_LP=m CONFIG_TCP_CONG_VENO=m CONFIG_TCP_CONG_YEAH=m CONFIG_TCP_CONG_ILLINOIS=m

The default is usually cubic as Susant pointed out.

$ cat /proc/sys/net/ipv4/tcp_congestion_control cubic

-Vivek

can folks help test this? Does it work as is should?

Seems to work fine, but I wonder if we could have a more user-friendly error message.

Currently its this: iperf3: error - unable to set TCP_CONGESTION: No such file or directory

better might be this: iperf3: error - unable to set TCP_CONGESTION: Congestion control XXX not supported on this host

The error number ENOENT is getting set when the congestion algo not found .
~~~

define ENOENT 2 /* No such file or directory */

~~~ This is the reason why it's not giving correct error. So the saved error number actually carrying the same value what is set by setsockopt failure irrespective of the close() success.

To print the CC algo while it failed a struct iperf_test object is required to access in iperf_strerror(). I think it's bit better than file not found . In this case can't trust on perror which would be confusion for end user.

"Supplied congestion control algorithm not supported on this host"

This is a way to test CC

http://www.linuxfoundation.org/collaborate/workgroups/networking/tcp_testing#Congestion_Control

I dint figured out there was a ; by mistake . I changed it now .

Committed the patch from Comment 19 in 74da0cfee476, thanks!

As far as I can tell from reading through the issue comments, all the work for this enhancement is done. Susant and Brian, do you agree? If so I'll close this issue.

Yep, I agree it can be closed. Susant?

Yes please . Thank you !

By consensus we're declaring victory.