FAIRQ discipline import from DragonFLY
ClosedPublic
Actions

Authored by eri on Jun 17 2015, 2:19 PM.

Details

Reviewers

wblock
glebius

Group Reviewers

network
manpages

Commits

rS284814: Correct r284777 to use proper includes and remove dead code to unbreak kernel…
rS284777: ALTQ FAIRQ discipline import from DragonFLY

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 updated this revision to Diff 6265.Jun 17 2015, 2:19 PM

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.

Herald added a subscriber: imp. · View Herald TranscriptJun 17 2015, 2:19 PM

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.

eri added a subscriber: glebius.Jun 17 2015, 2:28 PM

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

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?

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.

eri added inline comments.Jun 18 2015, 8:28 PM

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.