TCP congestion control algorithm support for client

[GoogleCodeExporter]

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

 Attached patch

Thanks,
Susant

Original issue reported on code.google.com by susant.sahani on 5 Dec 2013 at 5:38

Attachments:

• tcp_congestion.patch

[GoogleCodeExporter]

thanks for the patch!

Original comment by bltier...@es.net on 6 Dec 2013 at 2:12

[GoogleCodeExporter]

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

Original comment by bltier...@es.net on 10 Dec 2013 at 2:39

[GoogleCodeExporter]

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.

Original comment by jef.posk...@gmail.com on 10 Dec 2013 at 2:53

[GoogleCodeExporter]

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?

Original comment by jef.posk...@gmail.com on 10 Dec 2013 at 11:26

[GoogleCodeExporter]

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

Original comment by susant%redhat.com@gtempaccount.com on 12 Dec 2013 at 10:22

Attachments:

• tcp_congestion.patch

[GoogleCodeExporter]

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.

Original comment by jef.posk...@gmail.com on 12 Dec 2013 at 4:37

[GoogleCodeExporter]

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

Original comment by susant%redhat.com@gtempaccount.com on 12 Dec 2013 at 4:54

[GoogleCodeExporter]

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

Original comment by jef.posk...@gmail.com on 12 Dec 2013 at 4:58

[GoogleCodeExporter]

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.

Original comment by jef.posk...@gmail.com on 14 Dec 2013 at 1:23

[GoogleCodeExporter]

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?

Original comment by jef.posk...@gmail.com on 14 Dec 2013 at 4:04

[GoogleCodeExporter]

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

Original comment by susant%redhat.com@gtempaccount.com on 14 Dec 2013 at 5:06

[GoogleCodeExporter]

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.

Original comment by jef.posk...@gmail.com on 14 Dec 2013 at 5:14

[GoogleCodeExporter]

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

Original comment by vdasgupt...@gtempaccount.com on 14 Dec 2013 at 6:24

[GoogleCodeExporter]

Original comment by bltier...@es.net on 18 Dec 2013 at 9:49

• Added labels: Milestone-3.0-Release

[GoogleCodeExporter]

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

Original comment by bltier...@es.net on 18 Dec 2013 at 9:50

[GoogleCodeExporter]

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

Original comment by bltier...@es.net on 20 Dec 2013 at 11:18

[GoogleCodeExporter]

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"

Original comment by susant%redhat.com@gtempaccount.com on 22 Dec 2013 at 12:21

Attachments:

• cc_err.diff

[GoogleCodeExporter]

This is a way to test CC 

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

Original comment by susant%redhat.com@gtempaccount.com on 22 Dec 2013 at 2:26

[GoogleCodeExporter]

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

Original comment by susant%redhat.com@gtempaccount.com on 23 Dec 2013 at 6:57

Attachments:

• cc_err.diff

[GoogleCodeExporter]

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

Original comment by bmah@es.net on 2 Jan 2014 at 9:51

[GoogleCodeExporter]

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.

Original comment by bmah@es.net on 3 Jan 2014 at 9:31

[GoogleCodeExporter]

Yep, I agree it can be closed. Susant?

Original comment by bltier...@gmail.com on 4 Jan 2014 at 3:32

[GoogleCodeExporter]

Yes please . Thank you !

Original comment by susant%redhat.com@gtempaccount.com on 4 Jan 2014 at 4:59

[GoogleCodeExporter]

By consensus we're declaring victory.

Original comment by bmah@es.net on 5 Jan 2014 at 4:21

• Changed state: Fixed