Page MenuHomeFreeBSD

FAIRQ discipline import from DragonFLY
ClosedPublic

Authored by eri on Jun 17 2015, 2:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 27, 3:54 PM
Unknown Object (File)
Apr 12 2017, 10:26 PM
Unknown Object (File)
Apr 11 2017, 7:07 AM
Unknown Object (File)
Apr 10 2017, 1:40 AM
Unknown Object (File)
Apr 9 2017, 3:51 AM
Unknown Object (File)
Apr 8 2017, 11:47 PM
Unknown Object (File)
Apr 5 2017, 3:55 PM
Unknown Object (File)
Feb 18 2017, 8:14 PM

Details

Summary

It provides a good way to distribute traffic on queues and can help reduce buffer bloat.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

eri retitled this revision from to FAIRQ discipline import from DragonFLY.
eri updated this object.
eri edited the test plan for this revision. (Show Details)
eri added a reviewer: network.
eri set the repository for this revision to rS FreeBSD src repository - subversion.
eri changed the edit policy from "All Users" to "network (Project)".
eri added a subscriber: gnn.

This is in part of the work for code reduction and patches import from pfSense.
Next will come CodelQ scheduler implementation.

It will allow to integrate some improvements to ALTQ to current stack gluing and auto loading as module.

glebius requested changes to this revision.Jun 17 2015, 2:46 PM
glebius added a reviewer: glebius.

I don't see any locking in the code, but I see spl(9). This tells me that code isn't mpsafe. Do I mistake here?

Also, I've got some minor comments that are inlined.

sys/net/altq/altq_fairq.c
158 ↗(On Diff #6265)

Please change the MALLOC/FREE macros to malloc()/free(). The macros are obsoleted for > 10 years and shouldn''t be introduced in new code.

160–161 ↗(On Diff #6265)

You did M_WAITOK, you don't need this check.

349 ↗(On Diff #6265)

Same here.

358 ↗(On Diff #6265)

here

479–481 ↗(On Diff #6265)

You don't need this "sanity" that just pollutes code. INVARIANTS in malloc(9) itself do the job much better.

This revision now requires changes to proceed.Jun 17 2015, 2:46 PM
eri edited edge metadata.

Remove spl(9) and MALLOC/FREE

In D2847#54917, @eri wrote:

This is in part of the work for code reduction and patches import from pfSense.
Next will come CodelQ scheduler implementation.

Is that also going to come from DragonFlyBSD?

It will allow to integrate some improvements to ALTQ to current stack gluing and auto loading as module.

Thank you for your work.

In D2847#55014, @hiren wrote:
In D2847#54917, @eri wrote:

This is in part of the work for code reduction and patches import from pfSense.
Next will come CodelQ scheduler implementation.

Is that also going to come from DragonFlyBSD?

Its up to DragonFly to catch up with that, no?

glebius requested changes to this revision.Jun 18 2015, 2:05 PM
glebius edited edge metadata.

Awfully sorry, I didn't mentioned couple more comments on malloc(9) usage on previous review. There shouldn't be cast and M_ZERO is preferred over bzero(). Places in code are marked.

sys/net/altq/altq_fairq.c
158 ↗(On Diff #6268)

Cast before malloc() isn't needed in general. It actually can hide errors at certain circumstances. Let's not introduce such new code.

160 ↗(On Diff #6268)

Better use M_ZERO right at malloc(9) instead of bzero().

348 ↗(On Diff #6268)

Please remove cast and use M_ZERO here as well.

This revision now requires changes to proceed.Jun 18 2015, 2:05 PM

A couple of nits to pick but this is starting to look OK.

sys/net/altq/altq_fairq.c
66 ↗(On Diff #6268)

I'd remove the comment below into a manual page. It's not really necessary here.

762 ↗(On Diff #6268)

Put this under a real DEBUG macro.

sys/net/altq/altq_fairq.c
66 ↗(On Diff #6268)

Yeah i know on it but most ALTQ man pages are dropped and only altq general one exists in FreeBSD.

And also i am not good at man page writing.
Surely i will update altq man page to refer to this new scheduler.

eri edited edge metadata.

Update to take into account the comments on malloc casting and removing bzero().
Added the changes to altq(4) manual page.

wblock added a reviewer: wblock.
wblock added a subscriber: wblock.

No complaints on the man page changes, just a note to remember to update Dd.

glebius edited edge metadata.
This revision is now accepted and ready to land.Jun 22 2015, 4:12 AM
This revision was automatically updated to reflect the committed changes.