Bugzilla – Bug 2058
TCP window update can shrink the Left Edge of the window which is a bug
Last modified: 2015-05-05 03:33:42 EDT
Hi, I was looking at the TCP source code and apparently the remote window is set with every new packet (see TcpSocketBase::DoForwardUp): m_rWnd = tcpHeader.GetWindowSize(); In case of reordering in the network (maybe this never happens in ns3, but maybe there are other cases too), I believe this may have some effect. For instance 2 packets are sent from A to B, the 2nd one advertising a bigger window. If the 2nd packet arrives at B before the first then you advance B's right edge of the RCV.WND and when the first packet sent arrives it reduces B's right edge of the window which is not advised by the RFC 793: "Once the TCP takes responsibility for the data it advances RCV.NXT over the data accepted, and adjusts RCV.WND as appropriate to the current buffer availability. The total of RCV.NXT and RCV.WND should not be reduced." So I would propose to replace: m_rWnd = tcpHeader.GetWindowSize(); with /* receiver MUST NOT shrink the right edge of the receive window (i.e., DATA_ACK + receive window) */ if(tcpHeader.GetAckNumber() + tcpHeader.GetWindowSize() >= m_rxBuffer.NextRxSequence() + m_rWnd ) { m_rWnd = tcpHeader.GetWindowSize(); } "Once the TCP takes responsibility for the data it advances RCV.NXT over the data accepted" also means ns3 should wait and update the receivewindow only if it processes the packet. See http://tools.ietf.org/html/rfc793 p 74.
Created attachment 1993 [details] add checking the reordering of packets for control receive window-size There are some ways of fixing it: 1. Create new field in the class TCPSocketBase, where will containe previous TS. Or bool that met or no re-ordering. 2. Create the same variable in DoForwardUp(...), for checking tre-ordering - I implement this way, because i think it is not a good idea of increasing the volume of class, but I sacrificed a little performance.
(In reply to Evgeny Evstropov from comment #1) > Created attachment 1993 [details] > add checking the reordering of packets for control receive window-size > > There are some ways of fixing it: > 1. Create new field in the class TCPSocketBase, where will containe previous > TS. Or bool that met or no re-ordering. > 2. Create the same variable in DoForwardUp(...), for checking tre-ordering - > I implement this way, because i think it is not a good idea of increasing > the volume of class, but I sacrificed a little performance. The bug report is valid but I don't think the proposed patch is sufficient; it does not cover the case in which timestamps are disabled. Timestamps don't need to be used; this detection can be done with sequence numbers. BSD TCP uses these conditions to prevent old segments from updating the window: update the send window with the advertised window in the receive segment if one of three conditions exist: 1) segment contains new data (i.e. is an in-sequence packet advancing the right edge of the receive buffer) 2) segment does not contain new data but the segment acks new data (i.e. the highest sequence number acked advances) 3) segment contains neither new data nor new acknowledgment, but the advertised window is larger than the current send window BSD implementations maintain two internal state variables snd_wl1 and snd_wl2 to record the sequence number or acknowledgment number of the last segment used to update the send window, respectively. It would be nice also to write a test that confirms this behavior.
I correctly understand, that i can add new two fields(snd_wl1 and snd_wl2) in this class? For implementation this mechanism Because i didn't find the same fields in TCPSocketBase.
(In reply to Evgeny Evstropov from comment #3) > I correctly understand, that i can add new two fields(snd_wl1 and snd_wl2) > in this class? For implementation this mechanism > Because i didn't find the same fields in TCPSocketBase. It seems like that would be the most straightforward way to do it; it may be the case that the conditions can be inferred from existing state variables but I am not sure. I have not looked at how Linux implementation handles this either.
Created attachment 1998 [details] not change winSize when re-order. h-file I add two new fields: for the biggest sequence and ack numbers respectively. And new function - where determining that occur re-ordering of packets or not. And upgrade (if necessary) Window Size.
Created attachment 1999 [details] not change winSize when re-order. cc-file
Created attachment 2000 [details] test for not updating WinSize when re-order packets. for run this test must be accepted some changes in Ponit-To-Pointe - realization: 1. in class PointToPointHelper set field as protected: protected: ObjectFactory m_queueFactory; //!< Queue Factory ObjectFactory m_channelFactory; //!< Channel Factory ObjectFactory m_remoteChannelFactory; //!< Remote Channel Factory ObjectFactory m_deviceFactory; //!< Device Factory 2. in class PointToPointNetDevice set method as virtual: virtual void Receive (Ptr<Packet> p);
(In reply to Evgeny Evstropov from comment #5) > Created attachment 1998 [details] > not change winSize when re-order. h-file > > I add two new fields: > for the biggest sequence and ack numbers respectively. > And new function - where determining that occur re-ordering of packets or > not. And upgrade (if necessary) Window Size. In general, this approach looks fine to me. - Do not use GetValue () to compare sequence number values. This sequence number class has operator < defined in it already. In fact, that is the whole point of the class, to define operators for comparing sequence numbers for when they wrap around. - I might suggest to use the term 'highest' instead of 'biggest'. - there should be an NS_LOG_FUNCTION (this << header); at the top of the method - there should be an NS_LOG_DEBUG ("updating rWnd to " << rWnd); statement in both branches of code where rWnd is actually updated. The debug statement could also include the m_highestSeqNo and m_highestAckNo. - there could also be an NS_LOG_DEBUG ("Detected reordering; not updating rWnd") statement added for when reorder == true. - Spelling needs to be corrected in the comments.
(In reply to Evgeny Evstropov from comment #7) > Created attachment 2000 [details] > test for not updating WinSize when re-order packets. > > for run this test must be accepted some changes in Ponit-To-Pointe - > realization: > > 1. in class PointToPointHelper set field as protected: > protected: > ObjectFactory m_queueFactory; //!< Queue Factory > ObjectFactory m_channelFactory; //!< Channel Factory > ObjectFactory m_remoteChannelFactory; //!< Remote Channel Factory > ObjectFactory m_deviceFactory; //!< Device Factory > > 2. in class PointToPointNetDevice set method as virtual: > virtual void Receive (Ptr<Packet> p); I like that you wrote a test, thank you for taking this step. The test needs a bit of work before merging; I can help with it. A few initial comments: : TestSuite ("tcp-reorder", SYSTEM) this should be a UNIT test There is too much new code introduced; this can be reworked to use existing code. The reordering model is fine to introduce (I would call it a ReorderModel not a ReorderErrorModel since ErrorModel is already in use in ns-3 and does not behave similarly). However, the use of PointToPoint and the use of many of these new derived classes can be avoided. See the classes SimpleNetDevice, SimpleChannel. We do not need to change PointToPoint classes. The ReorderModel could perhaps be implemented as a ReorderModel : public SimpleChannel? Also, be aware that NS_LOG_UNCOND is not typically used. By default, NS_LOG is disabled in the test suite. It can be used when running tests through 'test-runner' program directly, but log statements should avoid UNCOND and should be DEBUG level.
(In reply to Tom Henderson from comment #9) > (In reply to Evgeny Evstropov from comment #7) > One last comment in the tests and in general. Prefer use of C++ containers to C-style arrays, and avoid use of raw pointers in the APIs. Instead of raw pointer to array, pass a std::vector either by value or by const reference.
Created attachment 2009 [details] with some correction according suggestions. h correct spelling + change biggest->highest
Created attachment 2010 [details] with some correction according suggestions. cpp correct spelling and logging
Created attachment 2014 [details] patch (without test) to fix I made a few changes to Evgeny's proposal: - renamed the variables to align with other variable names in this implementation, and converted them to TracedValues, since it is conceivable that people may want to plot these values. - since m_rWnd is a traced value, this kind of code (which was a pre-existing bug): m_rWnd = tcpHeader.GetWindowSize (); m_rWnd <<= m_rcvWindowScale; would lead to two traced value updates, which would be confusing to anyone trying to trace this. So m_rWnd is only updated once to the scaled value. - Matthieu suggested deferring the window update until after the segment was accepted for processing. This meant moving the update to after the check for out-of-range segments. - do not update window for first SYN received (i.e. check for the ACK flag set on the segment); it was noted in the BSD code that some implementations do not populate the window field in the first SYN - fully implement the three conditions to test that I mentioned earlier in this bug report - various formatting changes and rename RefreshWindowSize to UpdateWindowSize All tests pass with this check. It would be preferable to also have a new test case that is currently broken but that this patch fixes, but such a new test may involve some work (implementing some forced reordering).
> > All tests pass with this check. It would be preferable to also have a new > test case that is currently broken but that this patch fixes, but such a new > test may involve some work (implementing some forced reordering). I think that Evgeny's test listed in the attachments (and modified based on my previous comments) might form the basis for this new test.
pushed in changeset 11362 917793d13817