Patch for GSOC Project : Framework for TCP Prague simulations in ns-3

Patch for GSOC Project : Framework for TCP Prague simulations in ns-3

[n.p, 2017-09-10 09:31:14]

I am sorry to spam ns-developers, but I would like to have the input of everyone
that maybe do not follow ns-3-reviews regularly.

The code quality is high; there are small issues that can be easily solved.
However, I have one big concern regarding the design. I am (unless sound
explications are made) against the 'friend' keyword on the TcpSocketBase. The
overall idea behind the interface between congestion control and TCP socket is
that we should have a clear way to made the two communicate. By adding the
friend keyword, we are just circumventing that idea, allowing the congestion
control to do whatever with the socket.

I see two ways to overcome this:

1) Move the variables needed by the algorithm into the TCB (e.g., no access
through the friend keyword)
2) Add a set of optional functions to congestion control that can be invoked by
the TCP to alter its behavior.

While the point 1 is easy, the biggest thing is the point 2, because it requires
the coder to think about a suitable interface that can be reused (maybe) by
other congestion control algorithms. How to do this? Well, it's a good exercise,
but requires time. The GSoC has ended, and I would have liked a talk about this
design issues on the mailing list. However, there is a pseudo algorithm that can
help in this case:

- Identify, at high level, the functionalities required by the congestion
control algorithm (e.g., I can see that the algorithm requires ECN enabled..
maybe it has more requirements, I don't know)
- Identify, at a high level, the functionalities that the algorithm should
modify in the TCP socket (e.g., I can see that the algorithm should be able to
duplicate an ACK with some flags).

Then, the first step can be solved by adding a small layer that specifies the
requirements of the congestion control algorithm, defining (for example) an enum
in TcpCongestionControl, and each time an algorithm is set checking that all the
requirements are met. If the TCP has not these options (e.g., ECN disabled) it
fails gracefully to set the congestion control algorithm.
The second step is a bit more complicated. Since we have the problem that a
public method in TCP socket base can be called by anyone, and we have a clear
separation between TCP socket and algorithm, the only viable way is (in my
opinion) to put callbacks inside the TCB, that the congestion control algorithm
can call. This maintains a clear separation, and allow concept reuse between
algorithms. I mean, if another algorithm wants to send a crafted ACK, it can do
it without adding 'friend TcpOtherAlgorithm' in the definition of the
TcpSocketBase.


Another issue is related to code merging. I am not in favor of merging this big
patch as-is (even with the comments solved) because it is, as I said, big. It
breaks the possibility of bisecting errors. I would prefer a patch series
composed by smaller pieces, with a description, and in which each patch adds
parts that, together, form the original patch. 
You have git, so the tool allows you to do this (I have made a wiki entry
explaining how to rebase and work over the git repo). Since everything is
connected, you can prepare a git branch with this patch series, including also
the work that is not yours but that is necessary.
If you have any question, please let me know!

Have a beautiful day!
Nat

https://codereview.appspot.com/328380044/diff/1/examples/tcp/dctcp-example.cc
File examples/tcp/dctcp-example.cc (right):

https://codereview.appspot.com/328380044/diff/1/examples/tcp/dctcp-example.cc...
examples/tcp/dctcp-example.cc:40: std::stringstream filePlotQueue2;
Objects like these should be declared static

https://codereview.appspot.com/328380044/diff/1/examples/tcp/dctcp-example.cc...
examples/tcp/dctcp-example.cc:43: PrintPayload ( Ptr< const Packet > p)
C functions should be declared static in order to avoid linking pollution.

https://codereview.appspot.com/328380044/diff/1/src/internet/model/ipv4-queue...
File src/internet/model/ipv4-queue-disc-item.cc (right):

https://codereview.appspot.com/328380044/diff/1/src/internet/model/ipv4-queue...
src/internet/model/ipv4-queue-disc-item.cc:104: return false;
It's not blocking, but in these situations, you can directly return the result
of the comparison:

return (!m_headerAdded && m_header.GetEcn () == Ipv4Header::ECN_ECT1);

https://codereview.appspot.com/328380044/diff/1/src/internet/model/ipv6-queue...
File src/internet/model/ipv6-queue-disc-item.cc (right):

https://codereview.appspot.com/328380044/diff/1/src/internet/model/ipv6-queue...
src/internet/model/ipv6-queue-disc-item.cc:104: }
Same comment from ipv4 version applies here.

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-conges...
File src/internet/model/tcp-congestion-ops.h (right):

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-conges...
src/internet/model/tcp-congestion-ops.h:149: const TcpSocketState::TcpCaEvent_t
event)
Indentation problem

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-conges...
src/internet/model/tcp-congestion-ops.h:169: * \param tsb
Missing parameter documentation

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-conges...
src/internet/model/tcp-congestion-ops.h:173: }
The method breaks the interface we have created between TCP and a congestion
control.

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

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-header...
src/internet/model/tcp-header.cc:369: m_flags = field & 0xFF;
Please document this change.

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

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:161: ECN_CWR_SENT      /**< This state
indicates that the sender has reduced the congestion window, and sent a packet
These red points indicate trailing spaces. It is better to avoid them.

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:550: * \param [in] interface
Why the documentation has changed but the code has not?

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:590: friend class TcpDctcp;
I'm against this design choice. We have an interface between congestion control
and the TCP; why don't expand this interface????

https://codereview.appspot.com/328380044/diff/1/src/internet/model/tcp-socket...
src/internet/model/tcp-socket-base.h:722: virtual uint32_t SendDataPacket
(SequenceNumber32 seq, uint32_t maxSize, bool withAck);
The virtual keyword is not allowed in the TcpSocketBase, it should only be
requested by test (and properly documented)

[tomh_tomh.org, 2017-09-10 22:29:17]

On 9/10/17 2:31 AM, natale.patriciello@gmail.com wrote:
> I am sorry to spam ns-developers, but I would like to have the input of
> everyone that maybe do not follow ns-3-reviews regularly.
>
> The code quality is high; there are small issues that can be easily
> solved. However, I have one big concern regarding the design. I am
> (unless sound explications are made) against the 'friend' keyword on the
> TcpSocketBase. The overall idea behind the interface between congestion
> control and TCP socket is that we should have a clear way to made the
> two communicate. By adding the friend keyword, we are just circumventing
> that idea, allowing the congestion control to do whatever with the
> socket.

I had a look at this code and it seems that there is only one method 
(SendEmptyPacket()) that cannot be accomplished using the existing 
public API of TcpSocketBase, so perhaps we could just figure out a 
suitable public API extension for that piece, and avoid the friend keyword.

- Tom