Bugzilla – Full Text Bug Listing |
Summary: | TCP ScheduleNow() for data transfer | ||
---|---|---|---|
Product: | ns-3 | Reporter: | Tom Henderson <tomh> |
Component: | tcp | Assignee: | natale.patriciello |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | ns-bugs, tommaso.pecorella |
Priority: | P5 | ||
Version: | pre-release | ||
Hardware: | PC | ||
OS: | Linux | ||
See Also: |
https://www.nsnam.org/bugzilla/show_bug.cgi?id=2067 https://www.nsnam.org/bugzilla/show_bug.cgi?id=2111 |
||
Attachments: |
change Schedule (TimeStep(1)) to ScheduleNow()
Patch to have the step only when application is pushing down the data |
Description
Tom Henderson
2015-11-11 23:43:08 EST
Hi, with this patch I think we will drop performance on TCP. Why? Let's analyze it, picking one little sketch: @@ -2313,7 +2307,10 @@ m_retxEvent.Cancel (); } // Try to send more data - SendPendingData (m_connected); + if (!m_sendPendingDataEvent.IsRunning ()) + { + m_sendPendingDataEvent = Simulator::Schedule ( TimeStep (1), &TcpSocketBase::SendPendingData, this, m_connected); + } } If we replace SendPendingData scheduled at now () + Timestep (1) with now (), we are basically reverting the situation (since, after these line, there is nothing and the function returns). Why Tommaso added these Timestep in first place? Well, it is clearly explained here: http://mailman.isi.edu/pipermail/ns-developers/2015-February/012577.html and in the following mail. (In reply to natale.patriciello from comment #1) > Hi, with this patch I think we will drop performance on TCP. Why? Let's > analyze it, picking one little sketch: > > @@ -2313,7 +2307,10 @@ > m_retxEvent.Cancel (); > } > // Try to send more data > - SendPendingData (m_connected); > + if (!m_sendPendingDataEvent.IsRunning ()) > + { > + m_sendPendingDataEvent = Simulator::Schedule ( TimeStep (1), > &TcpSocketBase::SendPendingData, this, m_connected); > + } > } > > If we replace SendPendingData scheduled at now () + Timestep (1) with now > (), we are basically reverting the situation (since, after these line, there > is nothing and the function returns). > > Why Tommaso added these Timestep in first place? Well, it is clearly > explained here: > > http://mailman.isi.edu/pipermail/ns-developers/2015-February/012577.html > > and in the following mail. I think we should audit these (should we add a TimeStep to each of these three cases?) and add a test to prevent regressions; our current tests are not catching the subtle issue that Tommaso pointed out. Aleksandr pointed out the possible weakness of this approach in bug 2111. I have conducted some experiment, and I believe that the only points where we need this event is when the application is pushing down some data. As a result, I removed the sendpendingdata event from parts that, inside the TCP socket, call SendPendingData. Patch is in queue with TCP SACK. Patched added Created attachment 2762 [details]
Patch to have the step only when application is pushing down the data
fixed in 12648:2200388888c5 |