Copy the packet before adding the header.
Try to enqueue the copied packet.

T.

Yeah, copying the packet works, thanks for the advice. I am only concerned about the overhead introduced by copying every single transmitted packet. Maybe methods 1) and 2) I proposed are more efficient from a computational point of view.

Stefano

ps I found (by chance) the comments you left on github regarding my traffic control work. I am currently addressing them. Huge thanks!

Hi Stefano,

probably 1 or 2 are slightly more efficient, but only when there are dropped packets. Packet drops are something that should be avoided (nobody likes to be in a congestion state), so the performance hit is secondary.
The packet copy technique is used in many places, and it's quite clear to understand.
Hence... I'd avoid to optimize this point (the rule is: don't optimize what is not a bottleneck - first find the bottlenecks, then optimize them).

Cheers,

T.

PS: I'm glad you found my comments, I thought you wanted them there... :)

(In reply to Stefano Avallone from comment #2)
> Yeah, copying the packet works, thanks for the advice. I am only concerned
> about the overhead introduced by copying every single transmitted packet.
> Maybe methods 1) and 2) I proposed are more efficient from a computational
> point of view.
> 
> Stefano
> 
> ps I found (by chance) the comments you left on github regarding my traffic
> control work. I am currently addressing them. Huge thanks!

PS: Do you want that I commit this fix ?

T.

(In reply to Tommaso Pecorella from comment #3)
> Hi Stefano,
> 
> probably 1 or 2 are slightly more efficient, but only when there are dropped
> packets. Packet drops are something that should be avoided (nobody likes to
> be in a congestion state), so the performance hit is secondary.
> The packet copy technique is used in many places, and it's quite clear to
> understand.
> Hence... I'd avoid to optimize this point (the rule is: don't optimize what
> is not a bottleneck - first find the bottlenecks, then optimize them).
> 
> Cheers,
> 
> T.
> 
> PS: I'm glad you found my comments, I thought you wanted them there... :)
> 
> (In reply to Stefano Avallone from comment #2)
> > Yeah, copying the packet works, thanks for the advice. I am only concerned
> > about the overhead introduced by copying every single transmitted packet.
> > Maybe methods 1) and 2) I proposed are more efficient from a computational
> > point of view.
> > 
> > Stefano
> > 
> > ps I found (by chance) the comments you left on github regarding my traffic
> > control work. I am currently addressing them. Huge thanks!

Maybe I am missing something. I thought you meant something like this:

  //
  // Stick a point to point protocol header on the packet in preparation for
  // shoving it out the door.
  //
  Ptr<Packet> copy = packet->Copy ();
  AddHeader (copy, protocolNumber);

  m_macTxTrace (copy);

  //
  // We should enqueue and dequeue the packet to hit the tracing hooks.
  //
  if (m_queue->Enqueue (copy))
    {
      //
      // If the channel is ready for transition we send the packet right now
      // 
      if (m_txMachineState == READY)
        {
          packet = m_queue->Dequeue ()->GetPacket ();
          m_snifferTrace (packet);
          m_promiscSnifferTrace (packet);
          return TransmitStart (packet);
        }
      return true;
    }

  // Enqueue may fail (overflow)
  m_macTxDropTrace (copy);
  return false;

where the copy is done for all the packets. Please note that the header cannot be added after a successful enqueue because that would invalidate the statistics held by m_queue in terms of queued bytes (I tried and actually an assert in droptail fails).

Anyway, I agree with your observations on performance efficiency. In the light of them, we might consider option 3), i.e., remove the PPP header only for dropped packets:

  // Enqueue may fail (overflow)
  m_macTxDropTrace (packet);
  PppHeader ppp;
  packet->RemoveHeader (ppp);
  return false;

Though, maybe removing the header is more costly than copying a packet?

Stefano


(In reply to Tommaso Pecorella from comment #3)
> Hi Stefano,
> 
> probably 1 or 2 are slightly more efficient, but only when there are dropped
> packets. Packet drops are something that should be avoided (nobody likes to
> be in a congestion state), so the performance hit is secondary.
> The packet copy technique is used in many places, and it's quite clear to
> understand.
> Hence... I'd avoid to optimize this point (the rule is: don't optimize what
> is not a bottleneck - first find the bottlenecks, then optimize them).
> 
> Cheers,
> 
> T.
> 
> PS: I'm glad you found my comments, I thought you wanted them there... :)

Here is the signature of this NetDevice() method.

  /**
   * \param packet packet sent from above down to Network Device
   * \param dest mac address of the destination (already resolved)
   * \param protocolNumber identifies the type of payload contained in
   *        this packet. Used to call the right L3Protocol when the packet
   *        is received.
   *
   *  Called from higher layer to send packet into Network Device
   *  to the specified destination Address
   *
   * \return whether the Send operation succeeded
   */
  virtual bool Send (Ptr<Packet> packet, const Address& dest, uint16_t protocolNumber) = 0;


It isn't clear from the doxygen whether packet is modified by Send() but since a non-const pointer is passed, one has to assume that it is modified.

It seems to me the right thing to do here is to leave PointToPointNetDevice alone and have the tc layer copy the packet before giving it to the device, so that if Send() returns false, the copy is still available.

By the way, here is some background to read on the cost of Packet operations:

https://www.nsnam.org/docs/release/3.24/models/html/packets.html#copy-on-write-semantics

I think the bug isn't related to TC. Perhaps we have never "seen" it, but the problem can be there.

(In reply to Tom Henderson from comment #6)
> Here is the signature of this NetDevice() method.
> 
>   /**
>    * \param packet packet sent from above down to Network Device
>    * \param dest mac address of the destination (already resolved)
>    * \param protocolNumber identifies the type of payload contained in
>    *        this packet. Used to call the right L3Protocol when the packet
>    *        is received.
>    *
>    *  Called from higher layer to send packet into Network Device
>    *  to the specified destination Address
>    *
>    * \return whether the Send operation succeeded
>    */
>   virtual bool Send (Ptr<Packet> packet, const Address& dest, uint16_t
> protocolNumber) = 0;
> 
> 
> It isn't clear from the doxygen whether packet is modified by Send() but
> since a non-const pointer is passed, one has to assume that it is modified.
> 
> It seems to me the right thing to do here is to leave PointToPointNetDevice
> alone and have the tc layer copy the packet before giving it to the device,
> so that if Send() returns false, the copy is still available.

(In reply to Tommaso Pecorella from comment #8)
> I think the bug isn't related to TC. Perhaps we have never "seen" it, but
> the problem can be there.

can you state again what the bug is?  Modifying the Packet despite returning false?

(In reply to Tom Henderson from comment #9)
> can you state again what the bug is?  Modifying the Packet despite returning
> false?

if the xxxNetDevice::Send method adds the MAC header to a packet and *then* checks whether it can enqueue the packet, it turns out, in case it can not, that it returns false and the packet has the MAC header added.

If the object which passed the packet to the xxxNetDevice::Send method retries to send the packet until xxxNetDevice::Send returns true (which is the case of TC), the second time the packet is passed to the device, the packet already has a MAC header added and the Send method adds another MAC header to the packet, thus creating a malformed packet.

So, this problem has been disclosed by TC, since TC retries to send a packet, and must be clearly fixed when we introduce TC. Please note that this is not only an issue with the point-to-point device. All the devices that add the MAC header despite returning false trigger this problem (e.g., csma).

I think that having the TC layer copy every packet sent to the device is costly (the AddHeader performed by Send is a dirty operation). In my opinion, it would be better that the device removes the header (non-dirty operation) only from the packets that cannot enqueue.

If we agree on a solution, I can check which devices need to be "fixed" and include the patch in the upcoming TC review request.

Thanks,
Stefano

(In reply to Stefano Avallone from comment #10)
> I think that having the TC layer copy every packet sent to the device is
> costly (the AddHeader performed by Send is a dirty operation). In my
> opinion, it would be better that the device removes the header (non-dirty
> operation) only from the packets that cannot enqueue.

Well, maybe costs should be evaluated more carefully...

> 
> I think that having the TC layer copy every packet sent to the device is
> costly (the AddHeader performed by Send is a dirty operation). In my
> opinion, it would be better that the device removes the header (non-dirty
> operation) only from the packets that cannot enqueue.
> 
> If we agree on a solution, I can check which devices need to be "fixed" and
> include the patch in the upcoming TC review request.
> 

I'm not convinced yet about what to do.

We could change NetDevice::Send () to take a Ptr<const Packet> and force the NetDevice to copy the Packet before modifying; this approach is taken elsewhere in the stack.  This may appear costly (extra copies) but it seems to me that whenever AddHeader() is called, usually somewhere in the code still has a pointer to the original packet, and there may be no real difference.  Packets are designed to have inexpensive copies.  As Tommaso suggested, it could be directly measured with a benchmark without speculating.

We could also change the API documentation to state that if Send() returns false, packet must not be modified.  This will just tend to move the copy from the tc layer to the NetDevice layer.

In general, most devices need to be modified to provide flow control or feedback on whether Send() succeeded.

I think that you can proceed for the moment by copying the packet at tc layer in case Send() fails, and later look at an optimization to remove this copy, perhaps in the context of more widespread NetDevice changes to better support tc.

(In reply to Tom Henderson from comment #12)
> I think that you can proceed for the moment by copying the packet at tc
> layer in case Send() fails, and later look at an optimization to remove this
> copy, perhaps in the context of more widespread NetDevice changes to better
> support tc.

This looks reasonable to me. I just added to TrafficControlLayer::Transmit the following:

      // send a copy of the packet because the device might add the
      // MAC header even if the transmission is unsuccessful (see BUG 2284)
      Ptr<Packet> copy = item->GetPacket ()->Copy ();
      ret = m_device->Send (copy, item->GetAddress (), item->GetProtocol ());

Thanks,
Stefano

Marking as needinfo; need to wait to see if traffic control layer requires any changes related to this.

Hi, sorry for being late on that.

I agree with Stefano and I think the option 3 is the most reasonable. It was the thing I done in early stage of my work (https://github.com/kronat/ns-3-dev-git/blob/netdevice-backpressure/src/point-to-point/model/point-to-point-net-device.cc).

Send should take a non-const pointer to Packet, because it is probably going to modify it; but if it fails (it does not happen often) it should "roll-back" the packet to the original one. It can be defined in the interface, which should state clearly this strategy.

Then, NetDevices have two possibilities: making themselves a copy, and returning the original in case of failure, or rolling back the header addition. Probably these two can be employed on different netdevices, e.g. P2P could revert the header addition, while some others, like LTE, which performs complex operations on packets (encapsulation) could use copy.

Fixed with changeset 12124:b38f758e87a8