Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RED AQM. #122

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add RED AQM. #122

wants to merge 3 commits into from

Conversation

@squidarth
Copy link

@squidarth squidarth commented Jul 26, 2018

Hey there,

@vsrinivas and I were working on experimenting with different congestion control schemes and wanted to try out our congestion control schemes w/ different queueing disciplines. We were particularly interested in RED, and wrote an implementation in mahimahi to try out.

There's a high level overview of the algorithm in this RFC, and more detail in this paper.

Algorithm & Parameters

The general idea behind RED is that it drops packets probabilistically, where that probability is determined by an exponentially weighted average of the depth of the queue.

If the queue is empty, we decay the weighted average by the amount of time the queue has been empty to prevent the case where the queue is empty but the weighted average remains high because of the lower number of enqueues that happen (this is outlined in the paper as well).

There are three parameters:

  • wq: This is a weight that governs how heavily to weight the the current depth of the queue when updating the weighted average. The higher this is, the more reactive RED is to random bursts of packets
  • minthresh: This is the minimum threshold to drop packets (as a % of the queue size). If the queue depth is is below min_thresh, no packets are dropped
  • maxthresh: This is the point at which all packets are dropped (as a % of the queue size)

The paper I link to above has a guide for how to tune these parameters.

Thanks for putting the simulator on github--it's been super helpful in learning how this congestion control stuff works, and let me know if you'd like to make any changes here.

@squidarth squidarth force-pushed the ss-add-red-queuing branch from 5530f3c to ad00a23 Jul 27, 2018
Copy link
Collaborator

@keithw keithw left a comment

Thank you for implementing this! In general it looks good -- comments below.

Loading

{ "uplink-queue-args", required_argument, nullptr, 'a' },
{ "downlink-queue", required_argument, nullptr, 'w' },
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks like it might be a spurious change -- could you please rebase to keep it as-is (or maybe I'm missing a subtle change?).

Loading

@@ -7,5 +7,6 @@ libpacket_a_SOURCES = packetshell.hh packetshell.cc queued_packet.hh \
abstract_packet_queue.hh dropping_packet_queue.hh dropping_packet_queue.cc infinite_packet_queue.hh \
drop_tail_packet_queue.hh drop_head_packet_queue.hh \
codel_packet_queue.cc codel_packet_queue.hh \
red_packet_queue.cc red_packet_queue.hh \
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this match the indentation level of the other lines.

Loading

@@ -99,11 +99,11 @@ string DroppingPacketQueue::to_string( void ) const
return ret;
}

unsigned int DroppingPacketQueue::get_arg( const string & args, const string & name )
string DroppingPacketQueue::parse_number_arg(const string & args, const string & name, bool isfloat)
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I realize you need floating-point parameters for RED, but I don't totally love the design here of adding a (const?) bool isfloat as another parameter to the (already complicated) get_arg method. I think we either need to do the argument parsing "properly" (which probably means splitting the parsing up into two stages -- one to build up a string->string dictionary of arguments that happens early, and then a set of const methods that take a key and then extract argument values of various types [string, int, float] from the dictionary, with error checks if the argument doesn't parse as whatever was expected).

Or the other alternative might be to express the wq, minthresh, and maxthresh as integers somehow...

Loading

if (packet_limit_) {
return packet_limit_;
} else if (byte_limit_ ) {
return byte_limit_ / PACKET_SIZE;
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm... this seems a little counter to user expectations. If the user specifies a byte limit, they probably really do want a byte limit. (What if the queue has a bunch of small packets in it? It seems counterintuitive to treat them all as MTU-sized for purposes of calculating the RED dropping probability.)

Maybe you want to just use size_bytes() directly (and measure the instantaneous queue size, and the moving average of the queue size, in bytes) in the case where the user has specified a byte limit? Or if you don't think users want that and you only want RED to be able to operate with a packet limit, how about we just bomb out with an exception if the user has specified a byte limit instead? I'd rather bomb out with an exception than make the hidden assumption that all packets are MTU-sized.

Loading


auto ratio = (weighted_average_)/max_queue_depth_packets();
std::default_random_engine generator (0);
std::uniform_real_distribution<double> distribution (min_thresh_, max_thresh_);
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing these objects (the RNG and the distribution) seems pretty expensive to do every time a packet is enqueued -- could you make them members and initialize them once in the constructor? Check out how LossQueue does it. (Also please consider seeding the RNG with random_device()() as done in loss_queue.cc)

You have using namespace std here so no need for the std:: prefix.

Loading

std::default_random_engine generator (0);
std::uniform_real_distribution<double> distribution (min_thresh_, max_thresh_);
double threshold = distribution(generator);
if (ratio > max_thresh_) {
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why these lines are necessary -- if ratio > max_thresh_, won't threshold never be greater than ratio anyway? (Since max_thresh_ is the upper limit of the uniform_real_distribution.) It took me a while to understand the code here! Some comments might help, but also simplifying if it can indeed be simplified...

Loading

}
if (ratio < min_thresh_) {
ratio = 0;
}
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit expensive to have to compute a new 64-bit floating-point random number for every enqueued packet. Can you tell me how much CPU this ends up consuming for, e.g., traffic at 200 Mbps? Do we have to use this much precision? (And what do you think about the algorithm in the RED93 paper that only requires a new random number every time a packet is dropped or the moving average is updated?)

Loading

@@ -112,21 +112,52 @@ unsigned int DroppingPacketQueue::get_arg( const string & args, const string & n

/* make sure next char is "=" */
if ( args.substr( offset, 1 ) != "=" ) {
throw runtime_error( "could not parse queue arguments: " + args );
throw runtime_error( "could not parse queue arguments: " + args);
Copy link
Collaborator

@keithw keithw Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks like it might be a spurious change -- could you please rebase to keep it as-is (or maybe I'm missing a subtle change?).

Loading

@squidarth
Copy link
Author

@squidarth squidarth commented Jul 31, 2018

@keithw awesome, thanks for looking at this & the feedback! I resolved most of your comments except for the argument parsing stuff, which I need to think about a little bit. min and max thresholds could certainly be read in as ints, but wq is a little bit tougher, so I might change this to parse arguments "properly".

Couple things I changed that are worth pointing out:

  • I had earlier hard-coded s, a variable for managing decaying the weighted average. This should be a parameter to the queuing algorithm, so I added that
  • I saw your comment about computing the random number more efficiently, and decided to switch out the implementation to match the one in the paper more closely (scroll down to the "Implementation" section & the pseudo-code if you're curious).

Thanks again for taking a look at this!

Loading

@squidarth squidarth force-pushed the ss-add-red-queuing branch from bf4f66a to 8e1457f Aug 7, 2018
@squidarth
Copy link
Author

@squidarth squidarth commented Aug 7, 2018

@keithw this is ready for another look! I updated the argument parsing to first convert the arguments into a map<string, string> map first, and then passes that into each queue. DroppingPacketQueue has simple get_int_arg and get_float_arg that work properly.

Note: I copied the tokenize.hh file that I saw you were using in the http module into util. If you're happy with this change I can remove tokenize.hh from the util module and add util to the -I of src/http/Makefile.am.

Error Catching

I tested this with this command:

$ mm-link 2.64mbps-poisson.trace 2.64mbps-poisson.trace --downlink-queue=red  --downlink-queue-args=packets=1000,wq=0.002,minthresh=0.2,maxthresh=5,transmission_time=5

Messing with the args to not use "=" properly results in:

$ mm-link 2.64mbps-poisson.trace 2.64mbps-poisson.trace --downlink-queue=red  --downlink-queue-args=packets1000,wq=0.002,minthresh=0.2,maxthresh=5,transmission_time=5

Died on std::runtime_error: Queue args passed in wrong format

Passing in an incorrect int results in:

$ mm-link 2.64mbps-poisson.trace 2.64mbps-poisson.trace --downlink-queue=red  --downlink-queue-args=packets=x1000,wq=0.002,minthresh=0.2,maxthresh=5,transmission_time=5

Died on std::runtime_error: Invalid integer: x1000

And passing in an incorrect float results in:

$ mm-link 2.64mbps-poisson.trace 2.64mbps-poisson.trace --downlink-queue=red  --downlink-queue-args=packets=1000,wq=x0.002,minthresh=0.2,maxthresh=5,transmission_time=5

Died on std::runtime_error: Invalid floating-point number: x0.002

Let me know what you think! Thanks again for looking at this

Loading

@squidarth squidarth force-pushed the ss-add-red-queuing branch from 8e1457f to f2f5e7d Aug 7, 2018
@keithw
Copy link
Collaborator

@keithw keithw commented Sep 1, 2018

Looks really good! (And sorry for the delayed review...) This is almost ready pending:

  1. Yes, if you want to use tokenize.hh outside of http, please just move it to util and remove the copy in http. src/http/Makefile.am already has a -I flag for the util directory in AM_CPPFLAGS so I don't think a lot needs to change...

  2. On the argument parsing: I'm nervous about the implicit "0" default value in the implementations of get_int_arg and get_float_arg (or that these are static methods that take the argument map by const reference). I think this could lead to user confusion (which leads to emails requesting help with mahimahi, which we are trying to reduce through defensive software engineering...).

Would you be willing to try the following:

a. Instead of returning the map, parse_queue_args returns a ParsedArguments class, and that class encapsulates the map (as a private member) and also exposes public accessors for get_int_arg and get_float_arg.

b. The call signature for get_int_arg and get_float_arg has an optional variable for the default value. If the argument was not provided, and no default value was provided at the call site, the method throws an exception for a missing required argument. If the argument was not provided, and a default value WAS provided, the method provides the default value but also logs a warning to stderr ("Note: Using default value of xxx for parameter yyy").

  1. Once those are both done, please squash and make 1-3 semantic commits without fixup commits in the mix.

Other than that I think it's good to go. Thanks again for all your work on this.

Loading

@keithw
Copy link
Collaborator

@keithw keithw commented Oct 11, 2018

Wanted to follow up on this -- we're still really interested in merging RED.

Loading

@squidarth
Copy link
Author

@squidarth squidarth commented Oct 11, 2018

Hey @keithw, sorry about the radio silence (i've had a really busy past couple months). I'd love to finish this up--I'll take care of it this weekend!

Loading

@squidarth squidarth force-pushed the ss-add-red-queuing branch from 575ddd6 to a100967 Nov 17, 2018
squidarth added 2 commits Nov 17, 2018
Enforce & require the Red packet queue to have packet_limit.
Fix indentation.

Remove spurious change.

Parse args properly.
@squidarth squidarth force-pushed the ss-add-red-queuing branch from a100967 to fee2dc3 Nov 17, 2018
int get_int_arg(const string & argName)
{

if (argMap_.count(argName) > 0) {
Copy link
Author

@squidarth squidarth Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know what you think of using map.count here

Loading

@squidarth
Copy link
Author

@squidarth squidarth commented Nov 17, 2018

Hey @keithw,

Sorry again for being so late to get back on this--I just started a new job which has been crazy. I resolved the changes that you mentioned--the main file to look at in the diff is "parsed_arguments.hh". I put it in the frontend folder, which makes the import in the packet classes somewhat awkward, do you think it makes sense to put it in util? That didn't seem totally right either.

Besides that, I think this should be good to go. Sorry again for the long delay.

Loading

@keithw
Copy link
Collaborator

@keithw keithw commented Nov 17, 2018

Yeah, let's put it in util so you don't have to do the weird import.

Loading

@squidarth squidarth force-pushed the ss-add-red-queuing branch from fee2dc3 to 56ab626 Nov 17, 2018
@squidarth squidarth force-pushed the ss-add-red-queuing branch from 56ab626 to e1bfc89 Nov 17, 2018
@squidarth
Copy link
Author

@squidarth squidarth commented Nov 17, 2018

Updated!

Loading

@squidarth
Copy link
Author

@squidarth squidarth commented Dec 8, 2018

Hey @keithw, just wanted to check-in again to see if there's anything else you wanted to see here!

Loading

@squidarth
Copy link
Author

@squidarth squidarth commented Jan 23, 2019

hey--i know this has been sitting here for a while but just wanted to double check if there was anything else you needed here!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants