Issue 121 in iperf: TCP congestion control algorithm support for client

[ip...@googlecode.com]

Status: New
Owner: ----
Labels: Type-Enhancement

New issue 121 by susant.sahani: TCP congestion control algorithm support
for client
http://code.google.com/p/iperf/issues/detail?id=121

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

Attached patch

Thanks,
Susant


Attachments:
tcp_congestion.patch 6.2 KB

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[ip...@googlecode.com]

Updates:
Owner: b...@es.net

Comment #1 on issue 121 by bltier...@es.net: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

thanks for the patch!

[ip...@googlecode.com]

Comment #2 on issue 121 by bltier...@es.net: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #3 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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.

[ip...@googlecode.com]

Comment #4 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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?

[Brian Tierney]

The CC algorithm is only relevant to the sender, so in 'reverse' mode I think it would be good to send it across.

> You received this message because you are subscribed to the Google Groups "iperf-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to iperf-dev+...@googlegroups.com.
> To post to this group, send email to iper...@googlegroups.com.
> Visit this group at http://groups.google.com/group/iperf-dev.
> For more options, visit https://groups.google.com/groups/opt_out.

[Jon Dugan]

Are we asking Susant to update these patches and resubmit?  Is Susant clear that is what we are asking?

And also, I agree it should be strlen(test->congestion) rather than sizeof.

Jon

[Jef Poskanzer]

No need to update the patch. Her version plus my notes should
suffice when someone gets around to implementing it.
---
Jef

Jef Poskanzer j...@mail.acme.com http://acme.com/jef/

[Susant Sahani]

Jef,

     I am guy :) .. Will  update the patch ... 

Thanks,

Susant

[ip...@googlecode.com]

Comment #5 on issue 121 by ssa...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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



Attachments:
tcp_congestion.patch 6.6 KB

[ip...@googlecode.com]

Comment #6 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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.

[ip...@googlecode.com]

Comment #7 on issue 121 by ssa...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #8 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #9 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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.

[ip...@googlecode.com]

Comment #10 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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?

[ip...@googlecode.com]

Comment #11 on issue 121 by ssa...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #12 on issue 121 by jef.posk...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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.

[ip...@googlecode.com]

Comment #13 on issue 121 by vdasg...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Updates:
Labels: Milestone-3.0-Release

Comment #14 on issue 121 by bltier...@es.net: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

(No comment was entered for this change.)

[ip...@googlecode.com]

Comment #15 on issue 121 by bltier...@es.net: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #16 on issue 121 by bltier...@es.net: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #17 on issue 121 by ssa...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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"





Attachments:
cc_err.diff 1.4 KB

[ip...@googlecode.com]

Comment #18 on issue 121 by ssa...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

This is a way to test CC

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

[ip...@googlecode.com]

Comment #19 on issue 121 by ssa...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

Attachments:
cc_err.diff 1.3 KB

[ip...@googlecode.com]

Comment #20 on issue 121 by bm...@es.net: TCP congestion control algorithm

support for client
http://code.google.com/p/iperf/issues/detail?id=121

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

[ip...@googlecode.com]

Comment #21 on issue 121 by bm...@es.net: TCP congestion control algorithm

support for client
http://code.google.com/p/iperf/issues/detail?id=121

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.

[ip...@googlecode.com]

Comment #22 on issue 121 by bltier...@gmail.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

Yep, I agree it can be closed. Susant?

[ip...@googlecode.com]

Comment #23 on issue 121 by ssah...@redhat.com: TCP congestion control

algorithm support for client
http://code.google.com/p/iperf/issues/detail?id=121

Yes please . Thank you !

[ip...@googlecode.com]

Updates:
Status: Fixed

Comment #24 on issue 121 by bm...@es.net: TCP congestion control algorithm

support for client
http://code.google.com/p/iperf/issues/detail?id=121

By consensus we're declaring victory.