Skip to content

Conversation

@TommyPec
Copy link
Member

Sorry but I'm new to GitHub, and I wasn't able to comment the code in-line.
here are some comments:

  1. Please change PacketWithHeader to PacketWithV4Header and PacketWithV6Header (less confusion).
  2. ARP callbacks (and probably many more) had packets with headers. Now they are without. Restore the previous behavior, e.g., m_dropTrace (pending.first);
  3. Why the changes to Ipv4Header::DscpTypeToString (DscpType dscp) ?
  4. Ipv4Header::GetVersion (void) const - don't add zombies. An Ipv4Header version must be 4, the assert is there for a reason.
    There's no need to add a function for this (unless it's virtual, and we could discuss also about this). Anyway, it's never used, so you can remove it.
  5. Why the changes to Ipv4L3Protocol::m_txTrace, m_rxTrace, etc. ? Are they necessary ? (I suspect that the same traces have been changed to v6).
  6. TypeId NetDeviceQueue::GetTypeId (void) - missing SetConstructor
  7. NetDevice and NetDeviceQueue::DoDispose should be updated to unlink each other (circular Ptr are not good). Check with Valgrind
  8. Wake -> WakeUp (e.g., "Wake Up Little Susie")
  9. PointToPointNetDevice::Send - Ptr txq = GetTxQueue (0); - why it is hardcoded ? Is it possible to have more TxQueues ?
    9b) Why the txq is stopped if it couldn't enqueue packets ?
  10. Documentation: until now ns-3 only had Tx queues. Clarify the role and the effect of rx queues in TC.
  11. QueueDiscContainer - const_iterator Iterator; <- change the name, if it's const call it ConstIterator (or something similar).
  12. PfifoFastQueueDisc::GetTypeId (void) - missing SetGroup (check also others).
  13. PfifoFastQueueDisc::Classify - Ipv6 code should be practically identical... copy paste it :P
  14. PfifoFastQueueDisc (Doxygen in header) - same comments as I did to Tom's proposal about QoS marking: RFC is outdated and bits are wrong (they're 6, not 7).
  15. QueueDiscItem::Print (std::ostream& os) const - explicit casts to int of m_protocol and m_txq. The ostram operator can (and will) try to print 'em as characters, messing up the output.
  16. DoDispose is missing from many classes.

General comment: Did I miss something or the only NetDevice taking advantage of the new TC is PointToPoint so far ?

natale-p and others added 21 commits January 15, 2016 14:33
This patch adds a NetDeviceQueue class to store information about a single
transmission queue of a network device. This class is meant to store the
state of a transmission queue (i.e., whether the queue is stopped or not)
and some data used by techniques such as Byte Queue Limits. Also, multi-queue
aware queue discs can aggregate a child queue disc to an object of this class.
These features (excluding BQL) are added in subsequent commits.
The NetDevice class maintains a vector of NetDeviceQueue pointers, one for
each transmission queue. A NetDevice constructor is added which creates a
single transmission queue by default for every device. The number of transmission
queues can be modified (by child classes) by calling NetDevice::SetTxQueuesN.
Two public methods, GetTxQueue and GetTxQueuesN, are also added to the NetDevice class
to return the i-th NetDeviceQueue and the number of transmission queues, respectively.
…the installed MAC

For WifiNetDevices, the number of transmission queues is set when installing the MAC (in
WifiNetDevice::SetMac), given that the number of queues is 4 if the MAC supports
QoS and 1 otherwise.
Add public methods to the NetDeviceQueue class to start, stop or wake
a transmission queue. These methods are primarily called by NetDevices,
but can also be invoked by the network stack once techniques like BQL
are implemented. An IsStopped method is also added to allow queue discs to
check the current status of a transmission queue. A wake callback is provided,
so that upper layers (queue discs) can be solicited to send packets down.
The (unique) transmission queue is stopped when enqueuing a packet fails, so
that upper layers do not send other packets down. When a packet transmission
is completed, if the queue is empty then wake the upper layers. If the queue
was stopped, there is now room for another packet and hence wake the upper
layers as well.
… a given packet

This commit just adds a GetSelectedQueue method to the NetDevice class. This
method is called by the Traffic Control layer before enqueuing a packet in the
queue disc to determine in which transmission queue the device will enqueue the
packet. Such information is exploited by multi-queue aware queue discs.
By default, this function returns 0, so that single-queue devices do not need
to be modified.
A QueueItem base class is introduced to represent the items stored
in a Queue. The base class only contains a Ptr<Packet>. Derived classes
can store additional information. DropTailQueue, RedQueue and CodelQueue,
along with their examples and testsuits, have been adapted. Objects using
such queues have been adapted too.
QueueDisc is a base class providing the interface and implementing
the operations common to all the queueing disciplines. Child classes
need to implement the methods used to enqueue a packet (DoEnqueue),
dequeue a single packet (DoDequeue), get a copy of the next packet
to extract (DoPeek), plus methods to classify enqueued packets in
case they manage multiple queues internally.
...aggregated to the outgoing network device, if any. Otherwise,
packets are sent directly to the device.
…m the queue disc

When a packet is passed to the Traffic Control layer, the IPv{4,6} header has not
been added yet. It will be added when the packet is dequeued from the queue disc.
Linux pfifo_fast is the default priority queue enabled on Linux
systems.  Packets are enqueued in three FIFO droptail queues according
to three priority bands based on their Type of Service bits or DSCP bits.
Signed-off-by: Stefano Avallone <[email protected]>
@stavallo stavallo deleted the queue-disc branch February 25, 2016 22:58
@TommyPec TommyPec closed this Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants