Add dummynet AQM and scheduler configuration support to pfSense Limit… #3941

mattund · 2018-05-05T17:28:34Z

Add dummynet AQM and scheduler configuration support to pfSense Limiters through the GUI. Only shaper.inc was changed.

Screenshot: https://i.imgur.com/N36gpXF.png
Related discussion: https://forum.pfsense.org/index.php?topic=126637

Highlights

FQ_CODEL instead of altq method (FairQ + single CoDel queue)
Shape Multi-WAN + Multi-LAN configurations in one queue per direction per WAN
Shape ixgbe, LAGG, among others not supported via altq (in my experience, anyway)
Load default params from sysctl, or set your own for PIE, FQ_PIE, RED, GRED, CoDel, and FQ_CoDel (target ms, flows, etc.)
Set human-readable names for parameters for ease of use ("tudpate" becomes "Interval (ms)")
Descriptive help for schedulers+AQMs to make sense of options
Future development: add/change AQM/Scheduler arrays/models to drop in/change out features, parameters, etc.

Usage

Create a Limiter and configure basic BW/schedule options
Assign a scheduler and/or AQM to the limiter
Create a queue on the limiter
Assign an AQM, if applicable, to the queue
Make a floating rule, dir out, on the iface desired to shape and assign pipes' queues as usual

Concept & Changes

I have added a new section ("Queue") to both Limiters and their child queues. Limiters have applicable several options:

AQM Algorithm
AQM Parameters (if algorithm has any)
Scheduler
Scheduler Algorithm (if scheduler has any)
Queue Length (moved from Advanced Options)
ECN

NOTE: Limiters by default have an in-configurable (?) FIFO scheduler attached to the underlying pipe. So, you should not attach traffic to a limiter if you expect to schedule it with a different scheduler than FIFO. For this, you have to attach it to a child queue. See the related forum thread for details on this.

Limiter children have all of the same options, except do not define a scheduler. This is inherited from their configured parent's scheduler, and shared among the respective limiter's queues:

AQM Algorithm
AQM Parameters (if algorithm has any)
Queue Length (moved from Advanced Options)
ECN

Generated ipfw commands (rules.limiter):

pipe # config [bw...] {pipe->aqm->parameter_format}
sched # config pipe # {pipe->scheduler->parameter_format}
queue n config sched # {queue->aqm->parameter_format}
queue n+1 config sched # {queue->aqm->parameter_format}

Redmine Issue: https://redmine.pfsense.org/issues/6620
Ready for review

…ers through the GUI. Only shaper.inc was changed. Presently, the traffic shaper is versatile however outbound shaping can be tricky. This patch aims to solve that, allowing not only outgoing shaping through dummynet pipes but also enabling users to attach configurable shaping to virtually any interface. Right now, altq does not support LAGG ifaces, ixgbe, among others. Additionally, this patch adds an extraordinary amount of configuration to PIE, RED, and CoDel. Saving a limiter will show a table with each configuration parameter. This patch also offers descriptive help for intermediate users like myself. For developers, this patch makes adding new scheds/aqms very easy; just update the huge array at the top of the file and everything will fall in place. Also, ipfw command formats are defined dynamically with %variables.

sbeaver-netgate

The construct $q =& new class() is not valid in PHP 7.

sbeaver-netgate · 2018-05-05T19:33:26Z

All PHP code needs to be PHP 7 compliant since support for 5.6 ends this year.

mattund · 2018-05-05T19:53:38Z

@sbeaver-netgate,

Thanks -- good catch. I based my shaper.inc on an earlier point in the branch. I've revised and made the requested changes.

mattund · 2018-05-07T02:51:12Z

Just noticed a problem with this patch where if you select a scheduler or AQM which doesn't support ECN it still writes out noecn, causing the limiter not to actually apply, so I'm going to write a quick fix for that

EDIT:

Fixed

…AQM to work: noecn was appended in rules.limiter. - Made a change to an array reference that was breaking my test

mattund · 2018-05-07T12:52:43Z

Also,

5.2.1.  Interval

   The _interval_ parameter has the same semantics as CoDel and is used
   to ensure that the minimum sojourn time of packets in a queue used as
   an estimator by the CoDel control algorithm is a relatively up-to-
   date value.  That is, CoDel only reacts to delay experienced in the
   last epoch of length interval.  It SHOULD be set to be on the order
   of the worst-case RTT through the bottleneck to give end-points
   sufficient time to react.

   The default interval value is 100 ms.

My sysctl calls assume a unit in milliseconds. I need to factor for this. Will make another change. This would affect a few other params as well

EDIT:

Fixed. I also changed the way my parameters are formatted as commands, as well as made some slight changes to the sched config (assign to pipe explicitly by #)

…ed in ms. they appear to be microseconds instead (thanks Harvy66) - Fix a problem where I was recursively assigning parameters in FormatParameters(), but that was not an ideal method (using vsprintf instead) - Fix scheduler configuration to better mirror pfSense forum's command syntax (configuring scheduler w/ pipe now with: 'sched 1 config pipe 1' instead of 'sched 1 config')

… request that)

mattund · 2018-05-14T15:09:54Z

Hey all,

Changes made as requested last week. Been using this patch on my system for the past week and observed no issues save those already addressed. Any chance this could get reviewed soon? Anything needed from me?

sbeaver-netgate · 2018-05-17T16:12:39Z

Nothing further needed. Just requires testing

grandrivers · 2018-06-08T13:07:42Z

what is status on this is there an easy way to help test this?

jim-p

I have a few questions and notes for things you may need/want to change. Nothing too major. It will still need testing afterward.

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/config.inc

@@ -176,4 +176,4 @@ if ($config_parsed == true) {
 	}
 }

-?>
+?>


Why is this file changed? It doesn't appear related

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+	return array_column(array_map($f, array_keys($a), $a), 1, 0);
+}
+function build_queue_params($array, $selected, $params, $id) {
+	$form .= '<div id="params_' . gettext($id) . '">';


Why does this $id need run through gettext()? From the other uses of this function, that's always hardcoded to an internal-looking value.

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+			$form .= "<td class=\"col-xs-4\">" . gettext($value["name"]) . "</td>";
+			$form .= "<td class=\"col-xs-8\"><input class=\"form-control\" type=" . gettext($value["type"]) 
+					. " name=\"param_" . gettext($selected) . "_" . gettext($key) . "\" placeholder=\"" .
+					gettext($value["default"]) . "\" value=\"" . gettext($currentValue) . "\"/></td>";


Since value will contain user input, it needs encoding. Change this to htmlspecialchars(gettext($currentValue)).

The other variables appear to be hardcoded values so they should be OK.

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+
+	$form .= '</div>';
+	$form .= '<script type="text/javascript">';
+	$form .= 'events.push(function() {$("#' . gettext($id) . '").change(function() {$("#params_' .


Another instance of $id run through gettext() when it probably isn't necessary.

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+		/* Limiter patch */
+		$selectedScheduler = getSchedulers()[$data['sched']];
+		if (!$selectedScheduler)
+			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');


If they submitted a scheduler that isn't valid, they would have to have bypassed a drop-down and odds are it's not worth printing the resulting value. It would get encoded by the error display function later but it's better to leave it off anyhow.

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');
+		$selectedAqm = getAQMs()[$data['aqm']];
+		if (!$selectedAqm)
+			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');


See above about printing this value, and should this not reference AQM and not Scheduler?

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+		if ($data['ecn'] && $data['ecn'] == 'on' && !$selectedScheduler["ecn"] && !$selectedAqm["ecn"]) {
+			$input_errors[] = gettext("Explicit Congestion Notification is selected, but neither " . $selectedAqm["name"] . " nor " . $selectedScheduler["name"] . " support it.");
+		}
+		/* End limiter patch */


Ideally you should also have input validation to ensure values are in the correct format (e.g. integers, etc) and are within supported ranges. That is not necessarily a blocker, though.

jim-p · 2018-06-08T18:05:23Z

src/etc/inc/shaper.inc

+		/* Limiter patch */
+		$selectedAqm = getAQMs()[$data['aqm']];
+		if (!$selectedAqm)
+			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');


See previous comment about printing this value, and also the message says scheduler but it looks like it should be AQM

jim-p · 2018-06-08T18:06:33Z

@grandrivers You can test it by using the System Patches package, add .diff to the end of the PR URL and feed that to the package, set a path strip value of 2. So you'd use https://github.com/pfsense/pfsense/pull/3941.diff for this PR.

…me funny EOL stuff that happened. 2. Unwrapped gettext() 3. Agreed. Sanitized. 4. Unwrapped gettext() 5. Took out input_errors item 6. Took out input_errors item 7. I like the idea of this. I would love to add heavier validation; unless I just affix for datatype I would need to know the constraints on which the parameters live for the AQM/scheds. Maybe that's documented. I can revisit this. 8. Took out input_errors item

mattund · 2018-06-14T12:40:50Z

@jim-p ,

I rewound src/etc/inc/config.inc back to you guys' base. It was some funny EOL stuff that happened.
Unwrapped gettext()
Agreed. Sanitized.
Unwrapped gettext()
Took out input_errors item
Took out input_errors item
I like the idea of this. I would love to add heavier validation; unless I just affix for datatype I would need to know the constraints on which the parameters live for the AQM/scheds. Maybe that's documented (surely its in the dummynet source). I can revisit this.
Took out input_errors item

Thanks for taking a look, Jim! I appreciate the feedback.

ghost · 2018-06-15T04:25:46Z

working great over vpn interface. One issue I had when I clicked save was the extra params showed up with bunch of php nonsense and it went away when I clicked apply.

Edit: just noticed the params don't show up properly in ui. All I see is values where label for param is suppose to be.

edit2: here are the error messages:

ghost · 2018-06-15T22:15:54Z

on 2.4.4 devel applied using url: https://github.com/pfsense/pfsense/pull/3941.diff

This is what I see:

mattund · 2018-06-16T19:36:56Z

@blackyellowred

This was my fault; it should be fixed now :)

ghost · 2018-06-17T18:36:05Z

Thanks its fixed now

jsbsmd · 2018-06-28T16:02:02Z

Updated shaper.inc and rebooted. Comes up with php warning warning error.

Crash report begins. Anonymous machine information:

amd64
11.2-RELEASE
FreeBSD 11.2-RELEASE #18 7ea1caafea9(RELENG_2_4_4): Tue Jun 26 19:56:12 EDT 2018 root@buildbot3:/builder/ce-master/tmp/obj/builder/ce-master/tmp/FreeBSD-src/sys/pfSense

Crash report details:

PHP Errors:
[28-Jun-2018 12:00:00 America/Toronto] PHP Warning: Invalid argument supplied for foreach() in /etc/inc/shaper.inc on line 3937
[28-Jun-2018 12:00:00 America/Toronto] PHP Warning: Invalid argument supplied for foreach() in /etc/inc/shaper.inc on line 3937
[28-Jun-2018 12:00:02 America/Toronto] PHP Warning: Invalid argument supplied for foreach() in /etc/inc/shaper.inc on line 3937
[28-Jun-2018 12:00:02 America/Toronto] PHP Warning: Invalid argument supplied for foreach() in /etc/inc/shaper.inc on line 3937

No FreeBSD crash data found.

ie:

foreach ($selectedParameters as $key => $value) {
$config_key = 'param_' . $this->GetScheduler() . '_' . $key;
if (isset($q[$config_key]) && $q[$config_key] <> "") {
$this->SetSchedulerParameter($key, $q[$config_key]);
} else {
$this->SetSchedulerParameter($key, $value["default"]);
}
}

mattund · 2018-06-29T13:39:13Z

Hey @jsbsmd it's because I set the default scheduler to "FIFO" when it's defined as "fifo" in code, but PHP is case sensitive. I'm making a patch right now for everyone.

EDIT: Should be fixed now.

jsbsmd · 2018-06-29T14:28:45Z

works perfect now. thanks.

jim-p · 2018-07-02T17:40:47Z

src/etc/inc/shaper.inc

@@ -3824,11 +3824,7 @@ class dnpipe_class extends dummynet_class {

 		/* Limiter patch */
 		$selectedScheduler = getSchedulers()[$data['sched']];
-		if (!$selectedScheduler)
-			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');


This should still be tested -- Keep the test but do not put the variable in the error message.

jim-p · 2018-07-02T17:40:47Z

src/etc/inc/shaper.inc

 		$selectedAqm = getAQMs()[$data['aqm']];
-		if (!$selectedAqm)
-			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');


This should still be tested -- Keep the test but do not put the variable in the error message.

jim-p · 2018-07-02T17:40:47Z

src/etc/inc/shaper.inc

@@ -4511,8 +4507,6 @@ class dnqueue_class extends dummynet_class {

 		/* Limiter patch */
 		$selectedAqm = getAQMs()[$data['aqm']];
-		if (!$selectedAqm)
-			$input_errors[] = gettext("Selected scheduler not recognized: " . $data['sched'] . '.');


This should still be tested -- Keep the test but do not put the variable in the error message.

mattund · 2018-07-02T17:54:41Z

@jim-p, thanks again for taking a look -- I've made the requested changes.

dtaht · 2018-08-15T05:17:48Z

I just wanted to applaud y'all for getting fq_codel in there. Got any benchmarks? (rrul test from flent.org is good)

dtaht · 2018-08-16T23:47:13Z

That's quite an improvement. Welcome to the debloated internet club. :) I'm so glad pfsense got this code running and y'all can have way better internet.

We have flent servers all over the world, if you would like to try a closer one (my guess is you are on the east coast?) - try flent-newark.bufferbloat.net.

It would be interesting to get a docsis-pie result also (just shape the download), but at first eyeball I don't think pie is on. (one way to tell might be: flent -H flent-newark.bufferbloat.net tcp_upload) - pie ping latency on the upload would peak at ~20ms)

"This is, of course, my home connection. I'm testing from a Debian VM hosted on a 2x X5650 server using an I350-T as the adapter in 2xGigE active load-balance LACP with the Modem. The pfSense VM is virtualized via Hyper-V on the same server, and I'm not using SR-IOV or vf's with the NIC. The Debian VM communicates with pfSense over a vSwitch on that host."

As for this I can't help but laugh at how geeky we all are. Can you fix your parent's internet with something less extreme? Your neighbors? :) It's kind of weirdly wonderful that with all those layers you can still have reasonably accurate clock resolution.

dtaht · 2018-08-17T15:47:40Z

I'm also curious as to what happens with w/wo fq_codel, applied without a shaper, to the ethernet interface, over a 1gige or 100mbit local, native link. Linux regulates the local ring buffer with BQL (see https://lwn.net/Articles/454390/ ) which is necessary as they went hog-wild with offloads.

victorhooi · 2018-09-02T22:35:36Z

@Manevolent Curious - how did you create those charts in #3941 (comment) above?

dtaht · 2018-09-03T01:20:39Z

@victorhooi that's a standard flent.org rrul plot.

victorhooi · 2018-09-03T05:46:19Z

Oh - Flent looks very awesome!

Just trying to find articles about it now:

https://blog.tohojo.dk/2017/04/the-story-of-flent-the-flexible-network-tester.html

iperf/netperf is for testing within the LAN however - so I'm curious what people are using as the remote endpoint to test for bufferbloat?

Or do you setup your own netperf/iperf system on a cloud server somewhere nearby and try that?

dtaht · 2018-09-03T08:06:33Z

we have 6-7 public flent servers in the cloud at any given time.

flent-fremont.bufferbloat.net (west coast)
flent-newark.bufferbloat.net (east coast)
flent-london.bufferbloat.net (british coast)

And the rest are in europe. We generally hope that folk will setup netserver and irtt ( https://github.com/heistp/irtt ) on their own clouds/servers... these are not good much past 5-600Mbit.

if anyone wants to donate a server, that's nice too. :)

sbeaver-netgate requested changes May 5, 2018

View changes

pyvirus added 2 commits May 5, 2018

Revising my PR as it was not PHP7 compliant.

5fefcda

Wrote to wrong file.

eace74f

sbeaver-netgate added the Pending review/testing label May 5, 2018

sbeaver-netgate requested a review from jim-p May 5, 2018

pyvirus added 2 commits May 7, 2018

- Fixed a bug where you could not get an ECN-incapable scheduler or …

e9685c4

…AQM to work: noecn was appended in rules.limiter. - Made a change to an array reference that was breaking my test

Change back to PHP7 compliance

d237e64

pyvirus added 6 commits May 7, 2018

Fix newlines back

8ac763a

CRLF

254581a

Fix command syntax in format_parameters

1de72f6

Setters and getters for $aqm/sched params (anticipating a review will…

50d4c4f

… request that)

Also delete scheduler on cleanup

bcb08ce

sbeaver-netgate approved these changes May 17, 2018

View changes

jim-p requested changes Jun 8, 2018

View changes

sbeaver-netgate added the Changes requested label Jun 8, 2018

Fixing GitHub reported issues

5ee16aa

jim-p requested changes Jul 2, 2018

View changes

Fixing @jim-p's change requests

202411c

jim-p approved these changes Jul 2, 2018

View changes

jim-p added the Needs to be tested label Jul 2, 2018

netgate-git-updates merged commit 202411c into pfsense:master Jul 2, 2018

netgate-git-updates pushed a commit that referenced this pull request Jul 2, 2018

Merge pull request #3941 from mattund/master

88b10e5

@@ @@ -176,4 +176,4 @@ if ($config_parsed == true) { @@
         	}
         }
-        ?>
+        ?>

pfsense / pfsense

Add dummynet AQM and scheduler configuration support to pfSense Limit… #3941

Add dummynet AQM and scheduler configuration support to pfSense Limit… #3941

Conversation

mattund commented May 5, 2018 • edited

sbeaver-netgate left a comment

sbeaver-netgate commented May 5, 2018

mattund commented May 5, 2018

mattund commented May 7, 2018 • edited

mattund commented May 7, 2018 • edited

mattund commented May 14, 2018 • edited

sbeaver-netgate commented May 17, 2018

grandrivers commented Jun 8, 2018

jim-p left a comment

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

This comment has been minimized.

jim-p Jun 8, 2018 Contributor

jim-p commented Jun 8, 2018

mattund commented Jun 14, 2018 • edited

ghost commented Jun 15, 2018 • edited by ghost

ghost commented Jun 15, 2018

mattund commented Jun 16, 2018 • edited

ghost commented Jun 17, 2018

jsbsmd commented Jun 28, 2018 • edited

mattund commented Jun 29, 2018 • edited

jsbsmd commented Jun 29, 2018

This comment has been minimized.

jim-p Jul 2, 2018 Contributor

This comment has been minimized.

jim-p Jul 2, 2018 Contributor

This comment has been minimized.

jim-p Jul 2, 2018 Contributor

mattund commented Jul 2, 2018

dtaht commented Aug 15, 2018

dtaht commented Aug 16, 2018

dtaht commented Aug 17, 2018

victorhooi commented Sep 2, 2018

dtaht commented Sep 3, 2018

victorhooi commented Sep 3, 2018

dtaht commented Sep 3, 2018

mattund commented May 5, 2018 •

edited

mattund commented May 7, 2018 •

edited

mattund commented May 7, 2018 •

edited

mattund commented May 14, 2018 •

edited

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

jim-p Jun 8, 2018
Contributor

mattund commented Jun 14, 2018 •

edited

ghost commented Jun 15, 2018 •

edited by ghost

mattund commented Jun 16, 2018 •

edited

jsbsmd commented Jun 28, 2018 •

edited

mattund commented Jun 29, 2018 •

edited

jim-p Jul 2, 2018
Contributor

jim-p Jul 2, 2018
Contributor

jim-p Jul 2, 2018
Contributor