Npfreview - ALTQ integration in NPF #41

Emmankoko · 2024-12-25T17:12:44Z

This is a PR opened solely for review purpose. after all checks are cleared, it will be merged into the CVS main source.
@zoulasc @MartinHusemann

use right jobs altq interface to be detached from and set your error variable

this follows a style.....: altq on $interface $your_queue_options $your_queue_managements {child queues here} then.... queue child_queue and its defintions example for cbq: altq on wm0 cbq qlimit 60 tbrsize .. bandwidth 10Mb queue {deflt, audio, video} queue deflt bandwidth 1Mb priority 1 cbq(default, red) queue audio bandwidth 2Mb priority 2 cbq(borrow, ecn) queue video bandwidth 3Mb priority 3 cbq(red) example for hfsc: altq on wm0 hfsc qlimit 60 tbrsize .. bandwidth 10Mb queue {deflt, audio, video} queue deflt bandwidth 1Mb priority 1 hfsc(default, red) queue audio bandwidth 2Mb priority 2 hfsc( ecn) queue video bandwidth 3Mb priority 3 hfsc(red) for CBQ and HFSC, CBQ borrows unlike HFSC. example for priq: altq on wm0 qlimit 60 tbrsize .. bandwidth 10Mb queue { audio, video } queue audio priority 1 priq(default) queue video priority 2 priq(ecn) NB in PRIQ: you cannot set bandwidth for child queues in PRIQ as child queues depend on priority NB in ALL: all parents must have default queueus on a rule.... can be appended on a rule as... pass in on wm0 from $mac to $bsd apply "log" queue "audio".

when altq is being attached to a network interface through a packet filter(PF), it uses pf_altq which is a structure containing all pf's elements necessary for use in adding queues, removing queues, similar structure is implemented for npf (struct npf_altq) and it's being used to substitute pf structure. this automatically kills pf in altq. this hits out backs to move npf to where we want to take it to. if we want pf still supported in altq, maybe a functions with (pf_altq) same functions with (npf_altq) going to make code messy and unattractive. but I'm ready to listen to your suggestions.

three IOCLS: IOC_NPF_BEGIN_ALTQ: acts as a safe guard to only initialize and allocate memory when adding queue for the first time or adding after a flush. if altq is already loaded, it doesn't initlise or allocate, just clears already existing altq and ensures availablity of ifq structure to safely take on new queues. IOC_NPF_ADD_ALTQ: adds queues to the altq framework(using altq_add) which is called directly into ALTQ. queues are represented with npf_altq structure linked together in a TAILQ.(categorized into active and inactive). IOC_NPF_GET_ALTQ: checks if altq is supported to allow the queues to be sent to the kernel.

main functions are : expand_altq and expand_queue. expand_altq acts on: altq on $interface $scheduler $queue_options $child_queues expand_queue acts on: defined child queues.. queue blabla bandwidth x priotiy y .... the rest for calcualting bandwidth, tbrsizes, qlenght etc. most were re-implemeted from PF and has been well tested. safety guards: before a succesful addition into the kernel, it's checked if only on discipline is being loaded as it doesn't allow the defintion of two or more disciplines.

in the userland, it's main job is to enure the queue is defined and also the altq config conforms with the best practices. i.e: a default rule must be present and cbq and hfcs must ahve root queues afterwords, sets nvlist name of "queue" and value of "the queue name" and appends it to the rule nvlist" userland npf_altq.c has been renamed to npfctl_altq.c

basically, when a queue is found, on a rule, the queueing discipline of that queue is attached to the network interface using the npf_altq interface. done in the npf_altq_commit function. this sets the enqueue, dequeue, request, disc type etc. on the network interface. and then altq is enabled on the network interface to allow for queueing. when a packet is passed by the filter, it is then tagged with an ALTQ tag with the qid of the queue being set on the rule and then enqueued in the kernel

this is the flush implementation which involes; npfctl flush: which flush both altq and filtering if altq isn't loaded, flushes only filtering or npfctl flush -q: to flush only altq NB: if (npf_altq_loaded) { pool_destroy(&npf_altq_pl); npf_altq_loaded = 0; } serves as safe guard to avoid calling pool_destroy twice or more if a user runs: npfctl flush npfctl flush. in a first flush, npf_altq_loaded is set to 0 so a second nffctl flush skips the block

there's no special command required to start ALTQ. when you npfctl start it checks if any altq configuration is added in the config and then starts altq when npf is starting if no queue config is present, it ignores altq start

stopping queueing also requires no special commands. when a user npfctl stop: it checks if queuing is active, if it is, it stops it. if altq isn't running, it ignores it and detach the filtering hooks

"npfctl stats" have been modified into npfctl stats -f | -q nfctl stats -q prints queueing statistics and npfctl stats -f prints filtering statistics. with npfctl stats -q, it calls into the kernel npf_altq_get_qstats which then calls altq_getstats on the particular queue. then drops, xmit counts, queued counts etc are printed out.

showing reloaded queues is triggered passing a -q flag on npfctl show. npfctl show displays loaded filtering rules and also displays the state of altq in npf(either enabled or disabled).

zoulasc · 2024-12-26T16:12:05Z

sys/altq/altq_jobs.c

@@ -1831,14 +1831,14 @@ int
 jobsclose(dev_t dev, int flag, int fmt,
    struct lwp *l)
 {
+	int error = 0;


Revert this, and return 0 at the end.

zoulasc · 2024-12-26T16:12:35Z

usr.sbin/npf/npfctl/npf_parse.y

@@ -181,8 +203,9 @@ yyerror(const char *fmt, ...)
 %token	<str>		PARAM
 %token	<str>		TABLE_ID
 %token	<str>		VAR_ID
+%token  <str>       BW_SPEC


zoulasc · 2024-12-26T16:12:48Z

usr.sbin/npf/npfctl/npf_parse.y

@@ -199,6 +222,15 @@ yyerror(const char *fmt, ...)
 %type	<filtopts>	filt_opts all_or_filt_opts
 %type	<optproto>	rawproto
 %type	<rulegroup>	group_opts
+%type	<queue_opts> queue_opts queue_opt queue_opts_l


zoulasc · 2024-12-26T16:12:58Z

usr.sbin/npf/npfctl/npf_parse.y

+%type	<queue_options>	scheduler
+%type	<num>		cbqflags_list cbqflags_item
+%type	<num>		priqflags_list priqflags_item
+%type	<hfsc_opts>		hfscopts_list hfscopts_item hfsc_opts


zoulasc · 2024-12-26T16:13:07Z

usr.sbin/npf/npfctl/npf_parse.y

+%type	<num>		priqflags_list priqflags_item
+%type	<hfsc_opts>		hfscopts_list hfscopts_item hfsc_opts
+%type	<queue_bwspec>	bandwidth
+%type   <str>			queue_flags


zoulasc · 2024-12-27T14:58:29Z

sys/net/npf/npf_altq.c

 	/* allocate and fill new struct npf_tagname */
-	tag = (struct npf_tagname *)malloc(sizeof(struct npf_tagname),
+	tag = (struct npf_tagname *)malloc(sizeof(*tag),


no cast in malloc

zoulasc · 2024-12-27T15:00:13Z

usr.sbin/npf/npfctl/npfctl_qstats.c

-		err(1, "npfctl_insert_altq_node: calloc");
-	memcpy(&node->altq, &altq, sizeof(struct npf_altq));
+		err(EXIT_FAILURE, "npfctl_insert_altq_node: calloc");
+	memcpy(&node->altq, &altq, sizeof(altq));


it is safer to make it sizeof(dst) instead of sizeof(src)

right right right. but if there's a check, then we check if dst >= src (for overflow) right?

zoulasc · 2024-12-28T11:29:10Z

sys/altq/altq_priq.c

@@ -39,6 +39,11 @@ __KERNEL_RCSID(0, "$NetBSD: altq_priq.c,v 1.28 2021/09/21 14:30:15 christos Exp
 #include "pf.h"
 #endif

+#ifndef NPF
+#define NPF 1
+#endif


why is this needed?

zoulasc · 2024-12-28T11:29:25Z

sys/altq/altq_subr.c

@@ -36,6 +36,11 @@ __KERNEL_RCSID(0, "$NetBSD: altq_subr.c,v 1.33 2017/03/14 09:03:08 ozaki-r Exp $
 #include "pf.h"
 #endif

+#ifndef NPF


Why is this needed?

zoulasc · 2024-12-28T11:30:48Z

sys/net/npf/npf.h

@@ -309,6 +311,13 @@ typedef struct npf_ioctl_table {
 	} nct_data;
 } npf_ioctl_table_t;

+struct npfioc_altq {


This reverts commit d9c7834.

Emmankoko added 12 commits December 25, 2024 17:08

Compile-time error fix in altq

d9c7834

use right jobs altq interface to be detached from and set your error variable

starting ALTQ in npf

5cc7846

there's no special command required to start ALTQ. when you npfctl start it checks if any altq configuration is added in the config and then starts altq when npf is starting if no queue config is present, it ignores altq start

stop queueing

6cbbed7

stopping queueing also requires no special commands. when a user npfctl stop: it checks if queuing is active, if it is, it stops it. if altq isn't running, it ignores it and detach the filtering hooks

show altq using npfctl show -q

9599712

showing reloaded queues is triggered passing a -q flag on npfctl show. npfctl show displays loaded filtering rules and also displays the state of altq in npf(either enabled or disabled).

Emmankoko changed the title ~~Npfreview - ALTQ integration in ALTQ~~ Npfreview - ALTQ integration in NPF Dec 25, 2024

Emmankoko added 4 commits December 27, 2024 11:56

fix err, return parens, sizeof args, and copyright issues

d9ba839

proper spacing between functions

7f07037

fix whitespaces

cbfff41

fix copyright

e7216e7

zoulasc requested changes Dec 28, 2024

View reviewed changes

Revert "Compile-time error fix in altq" and return 0

86cd137

This reverts commit d9c7834.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Npfreview - ALTQ integration in NPF #41

Npfreview - ALTQ integration in NPF #41

Emmankoko commented Dec 25, 2024

zoulasc Dec 26, 2024

zoulasc Dec 26, 2024

zoulasc Dec 26, 2024

zoulasc Dec 26, 2024

zoulasc Dec 26, 2024

zoulasc Dec 27, 2024

zoulasc Dec 27, 2024

Emmankoko Dec 28, 2024 •

edited

Loading

zoulasc Dec 28, 2024

zoulasc Dec 28, 2024

zoulasc Dec 28, 2024

Npfreview - ALTQ integration in NPF #41

Are you sure you want to change the base?

Npfreview - ALTQ integration in NPF #41

Conversation

Emmankoko commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emmankoko Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emmankoko Dec 28, 2024 •

edited

Loading