Bug 829 - TCP unbound memory problem (pending data)
TCP unbound memory problem (pending data)
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: internet
ns-3-dev
All All
: P5 major
Assigned To: Josh Pelkey
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-03 06:11 EST by Gustavo J. A. M. Carneiro
Modified: 2010-03-27 19:16 EDT (History)
4 users (show)

See Also:


Attachments
patch (2.58 KB, patch)
2010-03-03 10:55 EST, Gustavo J. A. M. Carneiro
Details | Diff
cleaned up and simplified patch (1.47 KB, patch)
2010-03-04 07:32 EST, Gustavo J. A. M. Carneiro
Details | Diff
move logic to PendingData classs (3.21 KB, patch)
2010-03-27 18:42 EDT, Tom Henderson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo J. A. M. Carneiro 2010-03-03 06:11:56 EST
In my simulations, I am seeing TCP consume lots of memory.  It keeps around tens of thousands of packets _per socket_ in the m_pendingData structure.  There is a fundamental design problem with the way pending data is freed.  The only place pending data is freed is here:

  // See if all pending ack'ed; if so we can delete the data
  if (m_pendingData)
    { // Data exists, see if can be deleted
      if (m_pendingData->SizeFromSeq (m_firstPendingSequence, m_highestRxAck) == 0)
        { // All pending acked, can be deleted
          m_pendingData->Clear ();
          delete m_pendingData;
          m_pendingData = 0;
          // Insure no re-tx timer
          NS_LOG_LOGIC (this<<" Cancelled ReTxTimeout event which was set to expire at "
                    << (Simulator::Now () + 
                        Simulator::GetDelayLeft (m_retxEvent)).GetSeconds());
          m_retxEvent.Cancel ();
        }
    }

Basically, if ever the TCP socket reaches a state when all pending data has been ACKed, then all of that pending data is freed at once.  However, this is only one possible scenario, which does not always happen.  In my case, I try to estimate the network capacity with many TCP flows, each one sending much more data than the network can transport.  In this case, there is _always_ pending data, and so the condition "m_pendingData->SizeFromSeq (m_firstPendingSequence, m_highestRxAck) == 0" is never true until the end of the simulation, causing all my packets to be stored in memory for the duration of the simulation.

I need to fix this ASAP, so I'll probably submit a patch later.

Note: I use NS 3.2 but I already checked ns-3-dev still uses the same basic design.
Comment 1 Gustavo J. A. M. Carneiro 2010-03-03 10:55:23 EST
Created attachment 775 [details]
patch

I made this patch.  It had limited testing yet, but seems to work.  I'll keep testing, but in the mean time the TCP maintainer may comment.
Comment 2 Josh Pelkey 2010-03-03 12:55:25 EST
Aside from the commented code, patch seems good.  One small question: Shouldn't the timer be cancelled in the added "else delete just a portion" section?
Comment 3 Gustavo J. A. M. Carneiro 2010-03-03 13:16:14 EST
(In reply to comment #2)
> Aside from the commented code, patch seems good.  One small question: Shouldn't
> the timer be cancelled in the added "else delete just a portion" section?

I'm not 100% sure, still not very familiar with the TCP code in spite of the patch. However it seems to me that the retransmit timer is there to ensure we keep trying to transmit more data at a later time.  In that branch, some data is left in the pending data structure, which means we need to keep the timer to attempt to transmit it later.
Comment 4 Gustavo J. A. M. Carneiro 2010-03-04 07:32:34 EST
Created attachment 777 [details]
cleaned up and simplified patch
Comment 5 Tom Henderson 2010-03-27 18:42:32 EDT
Created attachment 802 [details]
move logic to PendingData classs

This patch is a bit more encapsulated, since the data member in PendingData should probably not be public.  Otherwise, it is similar to the previous patch.

I suggest that Josh decide which patch to commit and push one of them.
Comment 6 Josh Pelkey 2010-03-27 19:16:31 EDT
(In reply to comment #5)
> Created an attachment (id=802) [details]
> move logic to PendingData classs
> 
> This patch is a bit more encapsulated, since the data member in PendingData
> should probably not be public.  Otherwise, it is similar to the previous patch.
> 
> I suggest that Josh decide which patch to commit and push one of them.

changeset cdabef59da08

Thanks all :)