Skip to content

Fix double send in SimpleNetDevice when queue length is 1 packet

Greg Steinbrecher requested to merge steinbrecher/ns-3-dev:fix-simple-net-device into master

I was testing some things with queues on NetDevices and wanted to see if they would function correctly with a single packet output queue.

It turns out they don't, with a DropTailQueue of MaxSize=StringValue("1p"), we get double sends.

This seems to be because SimpleNetDevice::TransmitComplete checks to see if m_queue contains any packets and, if it doesn't, skips scheduling TransmitCompleteEvent. This means that, in SimpleNetDevice::SendFrom (which gets called after this), the check for !TransmitComplete.IsRunning() fails (because it wasn't scheduled), so it doesn't know there's already a packet in flight.

This seems to only come up when the queue length is 1, probably because with a longer queue length, the check in TransmitComplete passes, so the event gets scheduled, at least in the context of the test (where the QueueDisc has all the packet scheduled) at time 0.

Not 100% sure this couldn't crop up with longer queue lengths though, and it's obviously a bug with a length of 1 packet, so worth fixing.

Replication:

I built a test case that's a simple modification of tc-flow-control-test-suite.cc, which transmits 10 packets and then checks that flow control is working properly. I modified it to use a queue on the SimpleNetDevice of length 1 packet, and observed the double sends. With NS_LOG="SimpleChannel":

Without the fix in this commit:

@"+0.000000000s -1 SimpleChannel:SimpleChannel(0x108c19c60)\r\n"
@"+0.000000000s -1 SimpleChannel:Add(0x108c19c60, 0x108c0e100)\r\n"
@"+0.000000000s -1 SimpleChannel:Add(0x108c19c60, 0x108c315f0)\r\n"
@"+0.000000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c4c2c0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.008000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c4c2c0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.008000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c366f0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.016000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c33560, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.016000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c162c0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.024000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c15310, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.024000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c14ad0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.032000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c0b3a0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.032000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c09cc0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"
@"+0.040000000s -1 SimpleChannel:Send(0x108c19c60, 0x108c0efc0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108c315f0)\r\n"

With the fix in this commit:

@"+0.000000000s -1 SimpleChannel:SimpleChannel(0x108f04640)\r\n"
@"+0.000000000s -1 SimpleChannel:Add(0x108f04640, 0x108f047e0)\r\n"
@"+0.000000000s -1 SimpleChannel:Add(0x108f04640, 0x108f04d60)\r\n"
@"+0.000000000s -1 SimpleChannel:Send(0x108f04640, 0x108f054f0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.008000000s -1 SimpleChannel:Send(0x108f04640, 0x108f054f0, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.016000000s -1 SimpleChannel:Send(0x108f04640, 0x108f05c10, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.024000000s -1 SimpleChannel:Send(0x108f04640, 0x108f05d10, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.032000000s -1 SimpleChannel:Send(0x108f04640, 0x108f05de0, 0, 00:00:00:00:00:00, 00:00:00:0"
@"0:00:02, 0x108f04d60)\r\n"
@"+0.040000000s -1 SimpleChannel:Send(0x108f04640, 0x108f05ef0, 0, 00:00:00:00"
@":00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.048000000s -1 SimpleChannel:Send(0x108f04640, 0x108f06000, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.056000000s -1 SimpleChannel:Send(0x108f04640, 0x108f06110, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.064000000s -1 SimpleChannel:Send(0x108f04640, 0x108f06220, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"
@"+0.072000000s -1 SimpleChannel:Send(0x108f04640, 0x108f06330, 0, 00:00:00:00:00:00, 00:00:00:00:00:02, 0x108f04d60)\r\n"

New test cases:

To cover this against regression, I modified the QueueSizeUnit::PACKET case of tc-flow-control-test-suite.cc to take the size of the device queue and the total # of packets sent as parameters, and added checks for a few edge cases:

  • totalPackets = 10, deviceQueueLen = 1
  • totalPackets = 10, deviceQueueLen = 5 (this is the original test case)
  • totalPackets = 10, deviceQueueLen = 9
  • totalPackets = 10, deviceQueueLen = 10
  • totalPackets = 10, deviceQueueLen = 11
  • totalPackets = 10, deviceQueueLen = 15
  • totalPackets = 1, deviceQueueLen = 1
  • totalPackets = 1, deviceQueueLen = 2
  • totalPackets = 1, deviceQueueLen = 5

Merge request reports