Implementation of TCP Low Priority in ns-3

TCP Low Priority (TCP-LP) [1] is a Less than Best Effort (LBE) algorithm.

This patch provides an implementation of TCP-LP in ns-3. It is a port of ns-2 TCP-LP model.

The patch includes examples, documentation and test suite for TCP-LP.

Please let us know your views on this patch.

Regards,
Charitha Sangaraju

[1] Aleksandar Kuzmanovic, Edward W. Knightly.(2003).TCP-LP: A Distributed Algorithm for Low Priority Data Transfer.IEEE INFOCOM 2003.

[n.p, 2017-02-02 09:29:03]

There are some overflow errors. Please take care of them.

Overall, the quality of the code is good. Be sure about the 80 chars limit, and
check with ./utils/check-style.py all your files.

https://codereview.appspot.com/319240043/diff/1/src/internet/doc/tcp.rst
File src/internet/doc/tcp.rst (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/doc/tcp.rst#newc...
src/internet/doc/tcp.rst:675: TCP-Low priority is a delay based congestion
control protocol in which the low priority data utilizes only the excess
bandwidth available on an end-to-end path.TCP-LP uses one way delay measurements
as an indicator of congestion as it does not influence cross-traffic in the
reverse direction.
Limit the string length to 80 chars (applies on everything)

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc
File src/internet/model/tcp-lp.cc (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:85: 
all other functions miss the NS_LOG_FUNCTION statement, please add it everywhere

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:101: }
owd is an unsigned variable. What is the sense of such conditional statement and
the following assignment ?

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:111: 
This conditional statement is always true

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:124: }
This could be moved at the beginning, avoiding the calculation of Owd

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:185: {
possible overflow in timestamp - m_lastDrop if there is a wraparound of the
values of timestamp (remember how is calculated)

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:1574: {
Add this code in which the timestamp option is already evaluated to avoid
another check on m_timestampEnabled

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.h (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:169: 
Please use the ns-3 naming convention, and also be more descriptive in the
description. Add the declaration near the other timestamp values

[Charitha Sangaraju, 2017-02-04 12:21:39]

Hi Natale,

Thank you for reviewing the code.

I've uploaded a new patch with corrections. Some minor queries are in line.
Please let me know your views.

Thanks and Regards,
Charitha

https://codereview.appspot.com/319240043/diff/1/src/internet/doc/tcp.rst
File src/internet/doc/tcp.rst (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/doc/tcp.rst#newc...
src/internet/doc/tcp.rst:675: TCP-Low priority is a delay based congestion
control protocol in which the low priority data utilizes only the excess
bandwidth available on an end-to-end path.TCP-LP uses one way delay measurements
as an indicator of congestion as it does not influence cross-traffic in the
reverse direction.
On 2017/02/02 09:29:03, n.p wrote:
> Limit the string length to 80 chars (applies on everything)

Done.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc
File src/internet/model/tcp-lp.cc (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:85: 
On 2017/02/02 09:29:03, n.p wrote:
> all other functions miss the NS_LOG_FUNCTION statement, please add it
everywhere

Done.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:101: }
On 2017/02/02 09:29:03, n.p wrote:
> owd is an unsigned variable. What is the sense of such conditional statement
and
> the following assignment ?

Done.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:111: 
On 2017/02/02 09:29:03, n.p wrote:
> This conditional statement is always true

I thought when timestamps are disabled, there is a possibility for owd = 0. So I
kept this condition. I would be glad to know your opinion.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:124: }
On 2017/02/02 09:29:03, n.p wrote:
> This could be moved at the beginning, avoiding the calculation of Owd

m_flag is set in OwdCalculator, so I think this should be fine here.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-lp.cc#...
src/internet/model/tcp-lp.cc:185: {
On 2017/02/02 09:29:03, n.p wrote:
> possible overflow in timestamp - m_lastDrop if there is a wraparound of the
> values of timestamp (remember how is calculated)

I believe single wraparound of uint might not affect the subtraction. Can you
please suggest a better way to do this? Thanks for your help.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.cc:1574: {
On 2017/02/02 09:29:03, n.p wrote:
> Add this code in which the timestamp option is already evaluated to avoid
> another check on m_timestampEnabled

Done.

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
File src/internet/model/tcp-socket-base.h (right):

https://codereview.appspot.com/319240043/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:169: 
On 2017/02/02 09:29:03, n.p wrote:
> Please use the ns-3 naming convention, and also be more descriptive in the
> description. Add the declaration near the other timestamp values

Changed the naming convention to fit ns-3 style, but retained them here as they
are used in TcpSocketState

[n.p, 2017-02-04 19:51:57]

I'm satisfied with some of your answer. Please, take a look at the additional
questions I come up with (at first sight I missed them). The most important is
on the timestamp values that are calculated in 32 bit but then compared with all
the 64-bit value. There could be the possibility that something goes wrong.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp.cc
File src/internet/model/tcp-lp.cc (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:119: uint32_t mowd = OwdCalculator (tcb);
Based on what you answered, I took a closer look and it seems good. However, the
fact is that in one code branch (when (m_flag & LP_VALID_OWD) is false) we have
an unused variable (mowd). So, can't you move all the OwdCalculator code inside
this function adding a proper conditional statement? If the Linux code (there is
a Linux code of that?) is structured like this, don't touch anything, it's good.

I'm thinking like

uint32_t owd = ... value - reply;

if (owd == 0)
{
  return;
}

... do all the other things, setting flags, record the next min owd ....

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:177: uint64_t timestamp = (uint64_t) Simulator::Now
().GetSeconds ();
Please avoid C-style casts. But I think the following code is wrong, more on
that later.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:182: }
The timestamp value is calculated like this (TcpOptionTs class):

136   uint64_t now = (uint64_t) Simulator::Now ().GetMilliSeconds ();
137 
138   // high: (now & 0xFFFFFFFF00000000ULL) >> 32;
139   // low: now & 0xFFFFFFFF
140   return (now & 0xFFFFFFFF);

They are the latest 32-bit of the Now () converted (in a C-style way, I know) to
uint64_t. In that code, you are checking a 32-bit value vs *all* the 64 bits...
I'm not sure this is totally correct.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:229: m_lastDrop = timestamp;
This is not a drop time... a segment has been acked, that's it. Or we mean
different thing for dropped ? I am sorry, I didn't read the paper yet.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp.h
File src/internet/model/tcp-lp.h (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.h:88: enum TcpLpState
I would prefer to have this enum called State, because its complete name is
TcpLp::TcpLpState right now, which is a bit redundant

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:3424: m_tcb->m_rcvTimestampEchoReply =
ts->GetEcho();
The space between Echo and ()

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.h (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.h:154: TracedValue<uint32_t>  m_cWnd;        
         //!< Congestion window
Are these added tab? Or added spaces?

[Charitha Sangaraju, 2017-02-06 15:07:30]

Hi Natale,

I have uploaded a new patch based on your suggestions.

Please let me know your views.

Thanks for taking out time to review.

Regards,
Charitha Sangaraju

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp.cc
File src/internet/model/tcp-lp.cc (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:119: uint32_t mowd = OwdCalculator (tcb);
On 2017/02/04 19:51:57, n.p wrote:
> Based on what you answered, I took a closer look and it seems good. However,
the
> fact is that in one code branch (when (m_flag & LP_VALID_OWD) is false) we
have
> an unused variable (mowd). So, can't you move all the OwdCalculator code
inside
> this function adding a proper conditional statement? If the Linux code (there
is
> a Linux code of that?) is structured like this, don't touch anything, it's
good.
> 
> I'm thinking like
> 
> uint32_t owd = ... value - reply;
> 
> if (owd == 0)
> {
>   return;
> }
> 
> ... do all the other things, setting flags, record the next min owd ....

Yes, this is the way it is done in Linux code.

Linux code link: http://lxr.free-electrons.com/source/net/ipv4/tcp_lp.c

I also observed that they have used signed int for owd, so I brought back my
previous code which checks whether owd < 0 and made owd as a signed int. Is that
alright? Please suggest.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:177: uint64_t timestamp = (uint64_t) Simulator::Now
().GetSeconds ();
On 2017/02/04 19:51:57, n.p wrote:
> Please avoid C-style casts. But I think the following code is wrong, more on
> that later.

Done.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:182: }
On 2017/02/04 19:51:57, n.p wrote:
> The timestamp value is calculated like this (TcpOptionTs class):
> 
> 136   uint64_t now = (uint64_t) Simulator::Now ().GetMilliSeconds ();
> 137 
> 138   // high: (now & 0xFFFFFFFF00000000ULL) >> 32;
> 139   // low: now & 0xFFFFFFFF
> 140   return (now & 0xFFFFFFFF);
> 
> They are the latest 32-bit of the Now () converted (in a C-style way, I know)
to
> uint64_t. In that code, you are checking a 32-bit value vs *all* the 64
bits...
> I'm not sure this is totally correct.

Done.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:229: m_lastDrop = timestamp;
On 2017/02/04 19:51:57, n.p wrote:
> This is not a drop time... a segment has been acked, that's it. Or we mean
> different thing for dropped ? I am sorry, I didn't read the paper yet.

m_lastDrop indicates the last time congestion window was reduced. I used this
name to keep the code in line with Linux and ns-2. Please let me know if this is
fine.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp.h
File src/internet/model/tcp-lp.h (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.h:88: enum TcpLpState
On 2017/02/04 19:51:57, n.p wrote:
> I would prefer to have this enum called State, because its complete name is
> TcpLp::TcpLpState right now, which is a bit redundant

Done.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:3424: m_tcb->m_rcvTimestampEchoReply =
ts->GetEcho();
On 2017/02/04 19:51:57, n.p wrote:
> The space between Echo and ()

Done.

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.h (right):

https://codereview.appspot.com/319240043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.h:154: TracedValue<uint32_t>  m_cWnd;        
         //!< Congestion window
On 2017/02/04 19:51:57, n.p wrote:
> Are these added tab? Or added spaces?

Tabs

[n.p, 2017-02-06 16:43:18]

Very good, the last concern is about the use of Time class whenever possible.
Please don't add tabs.
Everything else is fine. Good job.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp.cc
File src/internet/model/tcp-lp.cc (right):

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:177: uint32_t timestamp = Simulator::Now
().GetMilliSeconds ();
Please, try to use Time class as much as possible. Linux uses timestamp as uint
because, well, they are real. We have the time in the simulator.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:180: {
The only point where a uint timestamp is needed is here. For all the rest,
please use Time class.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp.h
File src/internet/model/tcp-lp.h (right):

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.h:100: uint32_t            m_lastDrop;   //!<Time when
last active drop
If this is a time, then please use the Time class

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.h (left):

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.h:158: 
Please avoid these corrections on the existing code

[Charitha Sangaraju, 2017-02-07 10:14:37]

Hi Natale,

Thank you for your suggestions. 

I have made the changes as per your comments and uploaded a new patch. Please
let me know if there are any further concerns.

Thanks and Regards,
Charitha Sangaraju.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp.cc
File src/internet/model/tcp-lp.cc (right):

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:177: uint32_t timestamp = Simulator::Now
().GetMilliSeconds ();
On 2017/02/06 16:43:18, n.p wrote:
> Please, try to use Time class as much as possible. Linux uses timestamp as
uint
> because, well, they are real. We have the time in the simulator.

Done.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.cc:180: {
On 2017/02/06 16:43:18, n.p wrote:
> The only point where a uint timestamp is needed is here. For all the rest,
> please use Time class.

Done.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp.h
File src/internet/model/tcp-lp.h (right):

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-lp...
src/internet/model/tcp-lp.h:100: uint32_t            m_lastDrop;   //!<Time when
last active drop
On 2017/02/06 16:43:18, n.p wrote:
> If this is a time, then please use the Time class

Done.

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.h (left):

https://codereview.appspot.com/319240043/diff/40001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.h:158: 
On 2017/02/06 16:43:18, n.p wrote:
> Please avoid these corrections on the existing code

Done.

[n.p, 2017-02-08 08:43:13]

On 2017/02/07 10:14:37, Charitha Sangaraju wrote:
> Hi Natale,
> 
> Thank you for your suggestions. 
> 
> I have made the changes as per your comments and uploaded a new patch. Please
> let me know if there are any further concerns.
> 
> Thanks and Regards,
> Charitha Sangaraju.
> 


Please don't rush the code... the testcase should be reworked in order to
schedule what is happening in the DoRun function at appropriate time. It is not
enough to set the rcvTimeStamp values to what you would like... it should be
done in the simulator. In fact, if you run your test in valgrind you will see
errors.

Nat

[Charitha Sangaraju, 2017-03-09 17:04:17]

Hi Natale,

Thanks for your feedback. 

I did not completely get your point, but I believe it is something similar to
the changes that you made to TCP LEDBAT test suite.

So I have changed the TCP-LP test suite by looking at the changes you made to
LEDBAT test suite, and uploaded a new patch.

I have not tried running the test in valgrind yet. I will try to do that.

Regards,
Charitha Sangaraju

[Charitha Sangaraju, 2018-01-31 09:56:59]

On 2017/03/09 17:04:17, Charitha Sangaraju wrote:
> Hi Natale,
> 
> Thanks for your feedback. 
> 
> I did not completely get your point, but I believe it is something similar to
> the changes that you made to TCP LEDBAT test suite.
> 
> So I have changed the TCP-LP test suite by looking at the changes you made to
> LEDBAT test suite, and uploaded a new patch.
> 
> I have not tried running the test in valgrind yet. I will try to do that.
> 
> Regards,
> Charitha Sangaraju

Thanks Natale for merging TCP-LP in ns-3-dev

Your help is much appreciated.

Regards,
Charitha Sangaraju