Wait for MPTCP GSoC news.

Matt posted a recent patch for this:
https://codereview.appspot.com/277240043

I just rebased on master & updated the patch (style issues).

It fails for the tests ns3-tcp-loss & ns3-tcp-state, I don't know why, I think it might be related to the Pcap handling, I had to change file.good() to file.is_open() else test would crash even if pcaps are here.

./src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood3-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood1-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state8-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus4-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno0-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state0-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno1-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus2-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus3-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-interop-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood2-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus0-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno2-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood0-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-Westwood4-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno3-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state3-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-WestwoodPlus1-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state1-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state6-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state4-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state7-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state5-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-state2-response-vectors.pcap
./src/test/ns3tcp/response-vectors/ns3tcp-loss-NewReno4-response-vectors.pcap

Hi Matt, 

please attach the patch here so I can test and push it. THanks!

Created attachment 2214 [details]
patch

I just uploaded the patch sent on
https://codereview.appspot.com/download/issue277240043_20001.diff

Uhm, I'm testing it. I'm writing here the progresses.

Found out that, with the patches, we have another SND.NXT value:

1.29313s  [node 0] TcpSocketBase:ForwardUp(): Socket 0x9d8f20 forward up 10.1.2.2:50000 to 10.1.3.1:49153
1.29313s  [node 0] TcpSocketBase:ReadOptions(0x9d8f20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:UpdateWindowSize(0x9d8f20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:UpdateWindowSize(): Received (scaled) window is 131072 bytes
1.29313s  [node 0] TcpSocketBase:ProcessEstablished(0x9d8f20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:ReceivedAck(0x9d8f20, 0x9877e0, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:ReceivedAck(0x9d8f20, 14001)
1.29313s  [node 0] TcpSocketBase:ReceivedAck():  Bytes acked: 0 Segments acked: 0 bytes left: 0
1.29313s  [node 0] TcpSocketBase:ReceivedAck(): ACK of 14001 SND.UNA=14001 SND.NXT=15001

This is the original one:

1.29313s  [node 0] TcpSocketBase:ForwardUp(): Socket 0x9dff20 forward up 10.1.2.2:50000 to 10.1.3.1:49153
1.29313s  [node 0] TcpSocketBase:ReadOptions(0x9dff20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:UpdateWindowSize(0x9dff20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:UpdateWindowSize(): Received (scaled) window is 131072 bytes
1.29313s  [node 0] TcpSocketBase:ProcessEstablished(0x9dff20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:ReceivedAck(0x9dff20, 50000 > 49153 [ACK] Seq=1 Ack=14001 Win=32768)
1.29313s  [node 0] TcpSocketBase:ReceivedAck():  Bytes acked: 0 Segments acked: 0 bytes left: 0
1.29313s  [node 0] TcpSocketBase:ReceivedAck(): ACK of 14001 SND.UNA=14001 SND.NXT=30001

In the original, SND.NXT is 30001, while in the patched it is 15001.

and, after a bit of surfing, the error is there:

   // Retransmit a data packet: Call SendDataPacket
-  uint32_t sz = SendDataPacket (m_txBuffer->HeadSequence (), m_tcb->m_segmentSize, true);
+  NS_LOG_LOGIC ("TcpSocketBase " << this << " retxing seq " << FirstUnackedSeq ());
+  uint32_t sz = SendDataPacket (FirstUnackedSeq(), GetSegSize(), true);
   // In case of RTO, advance m_nextTxSequence
-  m_nextTxSequence = std::max (m_nextTxSequence.Get (), m_txBuffer->HeadSequence () + sz);
-
-  NS_LOG_DEBUG ("retxing seq " << m_txBuffer->HeadSequence ());
+  m_nextTxSequence = std::max (FirstUnackedSeq (), FirstUnackedSeq () + sz);
+//  m_firstTxUnack = m_nextTxSequence;
+
+  NS_LOG_DEBUG ("retxing seq " << FirstUnackedSeq ());


Overall, I'm not satisfied with this patch; but I strongly encourage moving all TCP-related traces to TcpSocketBase, so we need to work on this.
A non-intrusive way to do this is to make a variable that follows which happens inside Tcp{Tx,Rx}Buffer, less or more what is done with TcpSocketState for tracing cWnd and ssTh. This would resolve the issues you have in MPTCP?

An important thing is to make sure that "following" traced values does not impact the performance, but this is core-related and I don't have an answer for that. It would be awesome to check before the release of ns-3.25 ..

Btw, unprotecting Socket members isn't good; do you really need these as public for MPTCP?

Thanks!

Nat

Created attachment 2217 [details]
Decoupling TCP attributes

Ok, there is my proposal, which contains only the decoupling part. Here, I add two API:

public:
SequenceNumber32 GetFirstUnackedSeq () const;

protected:
void UpdateTxBuffer (SequenceNumber32 seq);

with the meaning you intended; to provide two additional trace sources (NextRxSequence and UnackSequence) I used the chaining methods we used for ssth and cwnd, adding two little "chain" functions.

Would that be enough for your purpose?

Nat

I mixed up different features in this commit so you are right not to be satisfied. I've splitted into different patches I'll send by email.

Your modifications are needed but not enough. 
1/ The "UpdateTxBuffer" allows to keep the data even when it's available, as MPTCP does, so thanks for keeping it.
2/ Everytime ns3 needs a sequence number, it passes the TcpHeader as a parameter. Problem is that in MPTCP, the sequence number comes from a TCP option so having prototypes directly accepting a sequencenumber would be best IMO.
This is for this reason that I changed the ReceivedAck prototype:
-TcpSocketBase::ReceivedAck (Ptr<Packet> packet, const TcpHeader& tcpHeader)
+TcpSocketBase::ReceivedAck (Ptr<Packet> packet, SequenceNumber32 ack)

>Btw, unprotecting Socket members isn't good; do you really need these as public for MPTCP?
My approach is this: ns3 advantage compared to real world systems is that it should be easier to work with and fiddle with internals. The functions I made public are all const so the user can't break the application even when they are public and they provide information that can be useful to the user.
In my case I have a meta mptcp TCP socket that fiddle with (MP)TCP subflows and they need to access their states one another. I can make some of it protected but it may break some things when those classes are inherited.

As for the virtual calls, there may be a 1% performance hit but we would not code in C++ if we cared about that ?

When I write
>As for the virtual calls, there may be a 1% performance hit but we would not code in C++ if we cared about that ?
It it because the MptcpSocket may inherit MpTcpSocketBase and thus will try to override some behaviors.

I will try to look at the SACK patch since it should share some concerns with this issue.

(In reply to Matthieu Coudron from comment #8)
> I mixed up different features in this commit so you are right not to be
> satisfied. I've splitted into different patches I'll send by email.

I have merged two of them, changeset 11802 and 11803. They have no problems, and if they can help, it's a pleasure to merge these simple fixes. For the last two, see below.

> Your modifications are needed but not enough. 
> 2/ Everytime ns3 needs a sequence number, it passes the TcpHeader as a
> parameter. Problem is that in MPTCP, the sequence number comes from a TCP
> option so having prototypes directly accepting a sequencenumber would be
> best IMO.
> This is for this reason that I changed the ReceivedAck prototype:
> -TcpSocketBase::ReceivedAck (Ptr<Packet> packet, const TcpHeader& tcpHeader)
> +TcpSocketBase::ReceivedAck (Ptr<Packet> packet, SequenceNumber32 ack)

We are in progress of SACK development, and I don't want that these two goes in conflict. It is clear that ReceivedAck will need the entire header to process these options, so I would like to ask the following: would be an API change useful, in order to reduce the length of functions involved in ACK management, and to better divide the assignment? Like this:

-> ReceivedAck (Ptr<Packet>, const TcpHeader&)
-> OptionsManagement (Ptr<Packet>, const TcpHeader&)
-> SlidingWindowManagement (SequenceNumber32&)
-> NewAck (SequenceNumber32)

And then, you could override any of these.

> >Btw, unprotecting Socket members isn't good; do you really need these as public for MPTCP?
> My approach is this: ns3 advantage compared to real world systems is that it
> should be easier to work with and fiddle with internals. The functions I
> made public are all const so the user can't break the application even when
> they are public and they provide information that can be useful to the user.
> In my case I have a meta mptcp TCP socket that fiddle with (MP)TCP subflows
> and they need to access their states one another. I can make some of it
> protected but it may break some things when those classes are inherited.
> 
> As for the virtual calls, there may be a 1% performance hit but we would not
> code in C++ if we cared about that ?

Yes but this isn't the point; it is the original design of the Socket class, which keeps them protected. The values accessible to the users are meant to be shared through TracedValues; and in my conceptual opinion for this use case, MpTcpSocketBase needs access to protected members of all TcpSocketBase objects, and then MpTcpSocketBase needs to be a friend of TcpSocketBase.

Nat

Thanks for merging changeset 11802 and 11803.

As for your patch about SequenceNumber32 GetFirstUnackedSeq () const; in its current state, it could already unblock me for https://www.nsnam.org/bugzilla/show_bug.cgi?id=2264 .

ReceivedAck is a huge function I would not like to duplicate. In the current state, 'tcpHeader' is just used to retrieve the sequence number but that may change with TCP SACK. I suppose for now my best course of action is to build fake TcpHeader with MPTCP sequence numbers. We can revise this later, once SACK is merged.


>Yes but this isn't the point; it is the original design of the Socket class, >which keeps them protected. The values accessible to the users are meant to be >shared through TracedValues; and in my conceptual opinion for this use case, >MpTcpSocketBase needs access to protected members of all TcpSocketBase objects, >and then MpTcpSocketBase needs to be a friend of TcpSocketBase.
The problem with friendship is that it is not inherited and thus subclassing MpTcpSocketBase may lead to artifically constrained situations (the member are const-qualified). I can live with friend (-ship) for now so let's leave this problem aside.

A last problem I have is that MpTcpSocketBase being a subclass of TcpSocketBase (this may not be the idea design but inheriting directly from TcpSocket would have created other problems), it needs to override some members. I would propose to add "virtual" in front of every TcpSocketBase member ?