Integrated Codel into DRR #476

TheJStone · 2017-05-25T18:54:39Z

Uses the added functions in the commit Extended Utility Codel to allow the use of locklessqueue and Codel in DRR for each flow's queue. Main changes:

change calls to queue's within drr to match Queue template
add the ability to specify queuetype and arguments within protobuf message and construct that queue in drr.

Note: this pr will fail build until the Extended Utility Codel commit is merged because it is dependent on the methods in that pr.

apanda

Some minor comments. I think drr.cc could use a few more comments.

apanda · 2017-05-31T17:06:21Z

core/modules/drr.cc

@@ -274,21 +303,21 @@ void DRR::AddNewFlow(bess::Packet* pkt, FlowId id, int* err) {
  Flow* f = new Flow(id);

  // TODO(joshua) do proper error checking
-  f->queue = AddQueue(static_cast<int>(kFlowQueueSize), err);
+  f->queue = generator_();


Given that there are a fixed number of flows, I wonder if it might be worth considering preallocating these queues, avoiding some of the initial allocation costs.

Given that there are only two types of Queue, I am somewhat concerned that the generator/factory mechanism might be a little over-engineered. Can we just simply do something like this...?

if (queue type is codel) { f->queue = Codel(codel params); } if (queue type is llring) { f->queue = LockLessQueue(llring params); }

This is less fancy, but shorter and simpler.

ah, I guess the main point of the factory is that it binds the parameters to the generator so that DRR doesn't have to have a place to store each queue type's parameters. I could just store these parameters and use this code if you think that is better? but I could also help simplify things down as panda suggested and have the factory take the protobuf message instead?

apanda · 2017-05-31T17:09:51Z

core/modules/drr.cc

-    if (*err != 0) {
+        RoundToPowerTwo(f->queue->Size() * kQueueGrowthFactor);
+    *err = f->queue->Resize(slots);
+    if (*err) {


The code from the last PR did not indicate that the queue needed to survive a failed Resize operation. In general I would have assumed a queue for which Resize failed is no longer valid, in which case you should remove the flow to avoid bad accesses further down the line. Also it might be useful to log something so we know things have gone bad.

In my current implementation of LockLessQueue and Codel, the old queue is not effected if the resize fails. is that too restrictive of an assumption to make? I can put it in resize method description of queue.h

apanda · 2017-05-31T17:13:08Z

core/modules/drr.cc

  if (err_num != 0) {
    return CommandFailure(-err_num);
  }

+  /* get flow queue generator */


It might help readability if you split out the code for creating a generator from DRRArg::Codel and DRRArg::Llring to different functions -- perhaps they could be in static functions in codel.h and lock_less_queue.h.

// for comments ;)

sangjinhan · 2017-05-31T19:02:50Z

core/modules/drr.cc

@@ -274,21 +303,21 @@ void DRR::AddNewFlow(bess::Packet* pkt, FlowId id, int* err) {
  Flow* f = new Flow(id);

  // TODO(joshua) do proper error checking
-  f->queue = AddQueue(static_cast<int>(kFlowQueueSize), err);
+  f->queue = generator_();


Given that there are only two types of Queue, I am somewhat concerned that the generator/factory mechanism might be a little over-engineered. Can we just simply do something like this...?

if (queue type is codel) { f->queue = Codel(codel params); } if (queue type is llring) { f->queue = LockLessQueue(llring params); }

This is less fancy, but shorter and simpler.

sangjinhan · 2017-05-31T19:17:39Z

protobuf/module_msg.proto

+    uint64 window = 2; /// window in microseconds between changing states in codel 
+  }
+
+  QueueType type = 4; /// which queuetype is used for each flow


Instead of having QueueType, we can do

oneof params { Llring ring = 4; Codel codel = 5; }

This is more semantically correct (only either of them can be set), and simpler (neither Enum nor type indicator is required).

In my opinion, Llring ring should be renamed DropTailQueue or something like that. The fact that the underlying queue is lockless is just internal implementation details; external users need to know what it does (plain old queue without active queue management), not how it is implemented.

I don't see much value for multiple_producer and multiple_consumer parameters. These parameters should be hidden from users; the system must not crash even if some parameters are not correctly set. Also, given that the module is not thread-safe, there is no point for mult-producer/consumer parameters set.

ah ok. I will make the respective changes.

sangjinhan · 2017-05-31T23:20:10Z

core/modules/drr.cc

@@ -41,7 +47,7 @@ DRR::~DRR() {
  std::free(flow_ring_);


Shouldn't it be delete flow_ring_, since it was allocated via new?

yes, I missed this in converting from llring.

sangjinhan · 2017-05-31T23:32:08Z

core/modules/drr.h

@@ -237,7 +220,8 @@ class DRR final : public Module {

  // state map used to reunite packets with their flow
  CuckooMap<FlowId, Flow*, Hash, EqualTo> flows_;
-  llring* flow_ring_;   // llring used for round robin.
+  LockLessQueue<Flow*>* flow_ring_;   // queue used for round robin.


While it is not part of this PR, I am not quite convinced that this module needs LockLessQueue to maintain a list of flows. All operations are single-item enqueue/dequeues so it would not give you any performance advantage (actually it will hurt). I'd rather recommend using std::deque to keep the code less foreign.

That makes sense. I will do this.

barath · 2017-06-20T15:28:27Z

What's the status of this PR?

TheJStone · 2017-06-21T01:03:03Z

I pushed the changes requested in the review and these chnages are awaiting further review. The code should also be run again now that the codel update has been merged.

apanda reviewed May 31, 2017

View reviewed changes

sangjinhan reviewed May 31, 2017

View reviewed changes

sangjinhan added the notready label Jun 5, 2017

TheJStone force-pushed the drr_extend branch from 82d9680 to 3d87c6e Compare June 14, 2017 06:14

Integrated Codel into DRR

aac0ccc

TheJStone force-pushed the drr_extend branch from 3d87c6e to aac0ccc Compare September 6, 2017 04:37

barath mentioned this pull request Nov 14, 2017

Possible bug in weighted_fair when resouce is 'bit' #717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrated Codel into DRR #476

Integrated Codel into DRR #476

TheJStone commented May 25, 2017

apanda left a comment

apanda May 31, 2017

sangjinhan May 31, 2017

TheJStone Jun 2, 2017

apanda May 31, 2017

TheJStone Jun 2, 2017

apanda May 31, 2017

sangjinhan May 31, 2017

sangjinhan May 31, 2017

sangjinhan May 31, 2017

TheJStone Jun 2, 2017

sangjinhan May 31, 2017

TheJStone Jun 2, 2017

sangjinhan May 31, 2017

TheJStone Jun 2, 2017

barath commented Jun 20, 2017

TheJStone commented Jun 21, 2017

Integrated Codel into DRR #476

Are you sure you want to change the base?

Integrated Codel into DRR #476

Conversation

TheJStone commented May 25, 2017

apanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barath commented Jun 20, 2017

TheJStone commented Jun 21, 2017