Could you produce a patch for this ?

Created an attachment (id=498) [details]
Solution

I took a look at this and it is a bit more complicated, I think.

There are two Attribute setters that interact when setting a third (initial)
congestion window variable.  We have to worry about consistency of: m_cWnd,
m_initialCWnd and m_segmentSize.

The only time that the segment size can be changed is from (socket) object
creation up to the SYN of a connection which can provide an MSS option.  TCP
specs say no, can't do it afterward.  Ns-3 TCP seems to ignore the MSS in the
SYN, so as it stands we just have to deal with the static-initialization-by
attribute problem. 

To make this work, to a first approximation all we have to do is to add:

    m_cWnd = m_initialCWnd * m_segmentSize;

to the Attribute setters for both "InitialCwnd" and "SegmentSize."  Then
setting either attribute will set the initial m_cWnd correctly.

However, the TCP protocol is in charge of changing m_cWnd and it isn't going to
expect some other entity to mess with it, so I think we must not allow changes
to m_segmentSize or m_initialCWnd after a connection leaves the CLOSED state
unless TCP does it (modifies m_Cwnd according to the protocol, or sets
m_segmentSize according to the MSS in a SYN).

So I think the right thing to do is to also change the attribute setters to
restrict the times (states) during which they can be used to preclude having a
user shoot him or herself in the foot:

void
TcpSocketImpl::SetSegSize (uint32_t size)
{
  m_segmentSize = size;
  NS_ABORT_MSG_UNLESS (m_state == CLOSED, 
    "TcpSocketImpl::SetSegSize(): Cannot change segment size dynamically.");
  m_cWnd = m_initialCWnd * m_segmentSize;
}

and 

void
TcpSocketImpl::SetInitialCwnd (uint32_t cwnd)
{
  m_initialCWnd = cwnd;
  NS_ABORT_MSG_UNLESS (m_state == CLOSED, 
    "TcpSocketImpl::SetInitialCwnd(): Cannot change initial cwnd
dynamically.");
  m_cWnd = m_initialCWnd * m_segmentSize;
}

If we ever teach ns-3 TCP to repect an initial MSS, it will need to do:

  m_segmentSize = segmentSizeFromSynOption;
  m_cWnd = m_initialCWnd * m_segmentSize;

outside of the Attribute setters.  I think this covers all of the bases.

Does this sound right?  










As it stands now, you can set m_segmentSize to anything you want at any time
you want.  

> void
> TcpSocketImpl::SetSegSize (uint32_t size)
> {
>   m_segmentSize = size;
>   NS_ABORT_MSG_UNLESS (m_state == CLOSED, 
>     "TcpSocketImpl::SetSegSize(): Cannot change segment size dynamically.");
>   m_cWnd = m_initialCWnd * m_segmentSize;
> }
> 
> and 
> 
> void
> TcpSocketImpl::SetInitialCwnd (uint32_t cwnd)
> {
>   m_initialCWnd = cwnd;
>   NS_ABORT_MSG_UNLESS (m_state == CLOSED, 
>     "TcpSocketImpl::SetInitialCwnd(): Cannot change initial cwnd
> dynamically.");
>   m_cWnd = m_initialCWnd * m_segmentSize;
> }
> 
> If we ever teach ns-3 TCP to repect an initial MSS, it will need to do:
> 
>   m_segmentSize = segmentSizeFromSynOption;
>   m_cWnd = m_initialCWnd * m_segmentSize;
> 
> outside of the Attribute setters.  I think this covers all of the bases.
> 
> Does this sound right?  

I agree that this patch is more robust and support checking it now in if it
tests well.  In the future, someone could add the MSS option support, and allow
TCP to set the cwnd to the outbound interface MTU - 40 bytes when the initial
SYN is sent.  But for now, this should solve Pavel's problem.

(In reply to comment #3)

  Solution by Craig looks pretty good so I close this bug as fixed. Thank you.

> I took a look at this and it is a bit more complicated, I think.
> 
> There are two Attribute setters that interact when setting a third (initial)
> congestion window variable.  We have to worry about consistency of: m_cWnd,
> m_initialCWnd and m_segmentSize.
> 
> The only time that the segment size can be changed is from (socket) object
> creation up to the SYN of a connection which can provide an MSS option.  TCP
> specs say no, can't do it afterward.  Ns-3 TCP seems to ignore the MSS in the
> SYN, so as it stands we just have to deal with the static-initialization-by
> attribute problem. 
> 
> To make this work, to a first approximation all we have to do is to add:
> 
>     m_cWnd = m_initialCWnd * m_segmentSize;
> 
> to the Attribute setters for both "InitialCwnd" and "SegmentSize."  Then
> setting either attribute will set the initial m_cWnd correctly.
> 
> However, the TCP protocol is in charge of changing m_cWnd and it isn't going to
> expect some other entity to mess with it, so I think we must not allow changes
> to m_segmentSize or m_initialCWnd after a connection leaves the CLOSED state
> unless TCP does it (modifies m_Cwnd according to the protocol, or sets
> m_segmentSize according to the MSS in a SYN).
> 
> So I think the right thing to do is to also change the attribute setters to
> restrict the times (states) during which they can be used to preclude having a
> user shoot him or herself in the foot:
> 
> void
> TcpSocketImpl::SetSegSize (uint32_t size)
> {
>   m_segmentSize = size;
>   NS_ABORT_MSG_UNLESS (m_state == CLOSED, 
>     "TcpSocketImpl::SetSegSize(): Cannot change segment size dynamically.");
>   m_cWnd = m_initialCWnd * m_segmentSize;
> }
> 
> and 
> 
> void
> TcpSocketImpl::SetInitialCwnd (uint32_t cwnd)
> {
>   m_initialCWnd = cwnd;
>   NS_ABORT_MSG_UNLESS (m_state == CLOSED, 
>     "TcpSocketImpl::SetInitialCwnd(): Cannot change initial cwnd
> dynamically.");
>   m_cWnd = m_initialCWnd * m_segmentSize;
> }
> 
> If we ever teach ns-3 TCP to repect an initial MSS, it will need to do:
> 
>   m_segmentSize = segmentSizeFromSynOption;
>   m_cWnd = m_initialCWnd * m_segmentSize;
> 
> outside of the Attribute setters.  I think this covers all of the bases.
> 
> Does this sound right?  
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> As it stands now, you can set m_segmentSize to anything you want at any time
> you want.  
> 

By the way, I have one more confusing 200 ms delay between the first and the
second packets in TCP session.

This time the reason is delayed ack -- receiver doesn't reply on the first
packet waiting for a) the second one OR b) 200 ms (delayed ack timeout). But
the sender has CWND = (segment size) by default and doesn't send the second
segment waiting for an ack. So both wait for 200 ms in vain.

Setting ns3::TcpSocket::InitialCwnd attribute to "2" solves this problem.
Should we set this as default (sorry, I don't know what TCP spec says about
this)?