Page MenuHomeFreeBSD

Add ALTQ(9) CoDel algorithm support
ClosedPublic

Authored by loos on Aug 2 2015, 12:59 AM.

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
Lint
Lint WarningsExcuse: Not a typo.
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 Unit Test Coverage

Event Timeline

loos updated this revision to Diff 7578.Aug 2 2015, 12:59 AM
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 accepted this revision.Aug 2 2015, 10:40 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.

eri edited edge metadata.Aug 3 2015, 6:51 AM

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

loos updated this revision to Diff 7738.Aug 6 2015, 9:23 PM
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.
loos added a comment.Aug 6 2015, 9:32 PM
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).

loos added inline comments.Aug 6 2015, 9:41 PM
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).

hiren added a subscriber: hiren.Aug 6 2015, 9:44 PM

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 accepted this revision.Aug 6 2015, 11:05 PM
rpaulo edited edge metadata.
glebius added inline comments.Aug 7 2015, 11:49 AM
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?

eri added a comment.Aug 7 2015, 5:51 PM

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.

eri added a comment.Aug 7 2015, 5:56 PM

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?

loos added inline comments.Aug 21 2015, 1:07 AM
sys/net/altq/altq_codel.c
105

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

loos added a comment.Aug 21 2015, 1:20 AM
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 accepted this revision.Aug 25 2015, 7:10 AM
gnn edited edge metadata.
bcr accepted this revision.Aug 5 2017, 9:38 PM
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 accepted this revision.Aug 8 2017, 10:34 PM
adrian added a subscriber: adrian.

yes please!

kristof closed this revision.Aug 9 2017, 8:01 AM
kristof added a subscriber: kristof.

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.