Page MenuHomeFreeBSD

Add ALTQ(9) CoDel algorithm support
ClosedPublic

Authored by loos on Aug 2 2015, 12:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 23 2017, 5:39 PM
Unknown Object (File)
Apr 22 2017, 1:07 AM
Unknown Object (File)
Apr 15 2017, 9:51 PM
Unknown Object (File)
Apr 6 2017, 2:59 PM
Unknown Object (File)
Mar 31 2017, 12:26 AM
Unknown Object (File)
Mar 16 2017, 5:12 AM
Unknown Object (File)
Mar 1 2017, 6:22 AM
Unknown Object (File)
Feb 3 2017, 9:42 PM

Details

Summary

Reworked patch to add ALTQ(9) support for CoDel algorithm.

CoDel is a parameterless queue discipline that handles variable bandwidth and RTT.

It can be used as the single queue discipline on an interface or as a sub discipline of existing queue disciplines.

First submitted by eri here:
https://lists.freebsd.org/pipermail/freebsd-net/2013-June/035731.html

This patch address all the issues raised in this thread.

Obtained from: pfsense

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsys/net/altq/altq_codel.c:7SPELL1Possible Spelling Mistake
Warningsys/net/altq/altq_codel.c:7SPELL1Possible Spelling Mistake
Warningsys/net/altq/altq_codel.h:7SPELL1Possible Spelling Mistake
Warningsys/net/altq/altq_codel.h:7SPELL1Possible Spelling Mistake
Unit
No Test Coverage

Event Timeline

loos retitled this revision from to Add ALTQ(9) CoDel algorithm support.
loos updated this object.
loos edited the test plan for this revision. (Show Details)
loos added reviewers: network, glebius, eri, gnn.
gnn requested changes to this revision.Aug 2 2015, 10:37 PM
gnn edited edge metadata.

Please fix up the spl issue. Other than that this looks good to commit.

sys/net/altq/altq_codel.c
87

Big lock? We don't have spl routines anymore. These need proper locks or just remove the spl code if no new lock is necessary.

This revision now requires changes to proceed.Aug 2 2015, 10:37 PM
rpaulo added a reviewer: rpaulo.
rpaulo added a subscriber: rpaulo.

Probably should be reviewed by one more person...

sbin/pfctl/parse.y
303

node_codel_opts?

1898–1904

Wrong indentation?

sys/net/altq/altq_codel.c
87

These are no-ops and perhaps should be replaced by proper locking.

359

Can you quote a book instead of wikipedia...? Newton's method can be found in any numerical analysis book.

Can you please put my copyright in the codel files since it was forgotten addition when i did the port?

loos edited edge metadata.

Changes in this version:

  • Removed the spl() locking, it was a noop and it doesn't seems to require additional locking (besides the existing locking in altq).
  • Removed the quote to wikipedia, the Newton method is well known and doesn't need a wikipedia reference.
  • Added the Ermal's copyright.
  • Added a 'target' keyword to avoid abusing from 'qlimit' in CoDel setup.
In D3272#66304, @eri wrote:

Can you please put my copyright in the codel files since it was forgotten addition when i did the port?

Sure Ermal, already done in this version.

Let me know if I need to change something (I have no idea about what year(s) I should add there).

sbin/pfctl/parse.y
303

We don't have or use node_code_opts here. In this case, this is correct.

1898–1904

Yeah, it's weird but this also follows the indentation used in the rest of file (i.e. matches the other cases).

Very nice work! Thanks a lot.

Can you share any testing details if possible? How was this tested or how can one help test this?

rpaulo edited edge metadata.
sys/net/altq/altq_codel.c
105

As usually in ALTQ they use M_NOWAIT in ioctl() context. Why? Why don't make the configuration codepath non faulty?

This can go in.
I am working on restructuring all this to be a bit more modern and not so...hackish.

The testing done on this code has been through many installations of pfSense and no issues have been reported so far that i am aware of.

One thing not mentioned in this review is that the codel queue type can be used on any other scheduler like PRIQ, CBQ, HFSC, FAIRQ.
It is quite flexible at that.

It surely needs a man page but even the other schedulers do not have it probably can take the others from NetBSD?

sys/net/altq/altq_codel.c
105

In this case altq_add() is called with PF_RULES_WLOCK() held. Sleeping here is not safe.

In D3272#67425, @hiren wrote:

Very nice work! Thanks a lot.

Can you share any testing details if possible? How was this tested or how can one help test this?

I'm also looking for a controlled way to validate this change. I'll try to push some different loads on my test environment and check if I can simulate a good test case.

Thanks!

gnn edited edge metadata.
bcr added a subscriber: bcr.

OK from manpages, but don't forget to bump the document date (.Dd).

This revision is now accepted and ready to land.Aug 5 2017, 9:38 PM
adrian added a subscriber: adrian.

yes please!

kp added a subscriber: kp.

yes please!

Note that this was committed back in 2015:
https://svnweb.freebsd.org/base?view=revision&revision=287009

I'm not sure why phabricator didn't pick it up.