Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 22 Jan 2019 00:50:24 +0000 (UTC)
From:      Patrick Kelsey <pkelsey@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r343287 - head/sbin/pfctl
Message-ID:  <201901220050.x0M0oOB1012753@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: pkelsey
Date: Tue Jan 22 00:50:24 2019
New Revision: 343287
URL: https://svnweb.freebsd.org/changeset/base/343287

Log:
  Reduce pf.conf parsing cost for configs that define N queues from O(N^2) to O(N)
  
  The number of syscalls made during parsing of any config that
  defines tables is also reduced, and incorrect warnings that HFSC
  parent queue bandwidths were smaller than the sum of their child
  bandwidths have been fixed.
  
  Reviewed by:	kp
  MFC after:	1 week
  Sponsored by:	RG Nets
  Differential Revision:	https://reviews.freebsd.org/D18759

Modified:
  head/sbin/pfctl/pfctl.h
  head/sbin/pfctl/pfctl_altq.c
  head/sbin/pfctl/pfctl_parser.c
  head/sbin/pfctl/pfctl_parser.h

Modified: head/sbin/pfctl/pfctl.h
==============================================================================
--- head/sbin/pfctl/pfctl.h	Mon Jan 21 23:54:33 2019	(r343286)
+++ head/sbin/pfctl/pfctl.h	Tue Jan 22 00:50:24 2019	(r343287)
@@ -114,7 +114,6 @@ extern	int loadopt;
 
 int		 check_commit_altq(int, int);
 void		 pfaltq_store(struct pf_altq *);
-struct pf_altq	*pfaltq_lookup(const char *);
 char		*rate2str(double);
 
 void	 print_addr(struct pf_addr_wrap *, sa_family_t, int);

Modified: head/sbin/pfctl/pfctl_altq.c
==============================================================================
--- head/sbin/pfctl/pfctl_altq.c	Mon Jan 21 23:54:33 2019	(r343286)
+++ head/sbin/pfctl/pfctl_altq.c	Tue Jan 22 00:50:24 2019	(r343287)
@@ -24,6 +24,7 @@ __FBSDID("$FreeBSD$");
 #define PFIOC_USE_LATEST
 
 #include <sys/types.h>
+#include <sys/bitset.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>
 
@@ -36,6 +37,7 @@ __FBSDID("$FreeBSD$");
 #include <inttypes.h>
 #include <limits.h>
 #include <math.h>
+#include <search.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -53,33 +55,40 @@ __FBSDID("$FreeBSD$");
 
 #define is_sc_null(sc)	(((sc) == NULL) || ((sc)->m1 == 0 && (sc)->m2 == 0))
 
-static TAILQ_HEAD(altqs, pf_altq) altqs = TAILQ_HEAD_INITIALIZER(altqs);
-static LIST_HEAD(gen_sc, segment) rtsc, lssc;
+static STAILQ_HEAD(interfaces, pfctl_altq) interfaces = STAILQ_HEAD_INITIALIZER(interfaces);
+static struct hsearch_data queue_map;
+static struct hsearch_data if_map;
+static struct hsearch_data qid_map;
 
-struct pf_altq	*qname_to_pfaltq(const char *, const char *);
-u_int32_t	 qname_to_qid(const char *);
+static struct pfctl_altq *pfaltq_lookup(char *ifname);
+static struct pfctl_altq *qname_to_pfaltq(const char *, const char *);
+static u_int32_t	 qname_to_qid(char *);
 
-static int	eval_pfqueue_cbq(struct pfctl *, struct pf_altq *);
+static int	eval_pfqueue_cbq(struct pfctl *, struct pf_altq *,
+		    struct pfctl_altq *);
 static int	cbq_compute_idletime(struct pfctl *, struct pf_altq *);
-static int	check_commit_cbq(int, int, struct pf_altq *);
+static int	check_commit_cbq(int, int, struct pfctl_altq *);
 static int	print_cbq_opts(const struct pf_altq *);
 
 static int	print_codel_opts(const struct pf_altq *,
 		    const struct node_queue_opt *);
 
-static int	eval_pfqueue_priq(struct pfctl *, struct pf_altq *);
-static int	check_commit_priq(int, int, struct pf_altq *);
+static int	eval_pfqueue_priq(struct pfctl *, struct pf_altq *,
+		    struct pfctl_altq *);
+static int	check_commit_priq(int, int, struct pfctl_altq *);
 static int	print_priq_opts(const struct pf_altq *);
 
-static int	eval_pfqueue_hfsc(struct pfctl *, struct pf_altq *);
-static int	check_commit_hfsc(int, int, struct pf_altq *);
+static int	eval_pfqueue_hfsc(struct pfctl *, struct pf_altq *,
+		    struct pfctl_altq *, struct pfctl_altq *);
+static int	check_commit_hfsc(int, int, struct pfctl_altq *);
 static int	print_hfsc_opts(const struct pf_altq *,
 		    const struct node_queue_opt *);
 
-static int	eval_pfqueue_fairq(struct pfctl *, struct pf_altq *);
+static int	eval_pfqueue_fairq(struct pfctl *, struct pf_altq *,
+		    struct pfctl_altq *, struct pfctl_altq *);
 static int	print_fairq_opts(const struct pf_altq *,
 		    const struct node_queue_opt *);
-static int	check_commit_fairq(int, int, struct pf_altq *);
+static int	check_commit_fairq(int, int, struct pfctl_altq *);
 
 static void		 gsc_add_sc(struct gen_sc *, struct service_curve *);
 static int		 is_gsc_under_sc(struct gen_sc *,
@@ -104,59 +113,101 @@ void		 print_hfsc_sc(const char *, u_int, u_int, u_int
 void		 print_fairq_sc(const char *, u_int, u_int, u_int,
 		     const struct node_fairq_sc *);
 
+static __attribute__((constructor)) void
+pfctl_altq_init(void)
+{
+	/*
+	 * As hdestroy() will never be called on these tables, it will be
+	 * safe to use references into the stored data as keys.
+	 */
+	if (hcreate_r(0, &queue_map) == 0)
+		err(1, "Failed to create altq queue map");
+	if (hcreate_r(0, &if_map) == 0)
+		err(1, "Failed to create altq interface map");
+	if (hcreate_r(0, &qid_map) == 0)
+		err(1, "Failed to create altq queue id map");
+}
+
 void
 pfaltq_store(struct pf_altq *a)
 {
-	struct pf_altq	*altq;
-
+	struct pfctl_altq	*altq;
+	ENTRY 			 item;
+	ENTRY			*ret_item;
+	size_t			 key_size;
+	
 	if ((altq = malloc(sizeof(*altq))) == NULL)
-		err(1, "malloc");
-	memcpy(altq, a, sizeof(struct pf_altq));
-	TAILQ_INSERT_TAIL(&altqs, altq, entries);
+		err(1, "queue malloc");
+	memcpy(&altq->pa, a, sizeof(struct pf_altq));
+	memset(&altq->meta, 0, sizeof(altq->meta));
+
+	if (a->qname[0] == 0) {
+		item.key = altq->pa.ifname;
+		item.data = altq;
+		if (hsearch_r(item, ENTER, &ret_item, &if_map) == 0)
+			err(1, "interface map insert");
+		STAILQ_INSERT_TAIL(&interfaces, altq, meta.link);
+	} else {
+		key_size = sizeof(a->ifname) + sizeof(a->qname);
+		if ((item.key = malloc(key_size)) == NULL)
+			err(1, "queue map key malloc");
+		snprintf(item.key, key_size, "%s:%s", a->ifname, a->qname);
+		item.data = altq;
+		if (hsearch_r(item, ENTER, &ret_item, &queue_map) == 0)
+			err(1, "queue map insert");
+
+		item.key = altq->pa.qname;
+		item.data = &altq->pa.qid;
+		if (hsearch_r(item, ENTER, &ret_item, &qid_map) == 0)
+			err(1, "qid map insert");
+	}
 }
 
-struct pf_altq *
-pfaltq_lookup(const char *ifname)
+static struct pfctl_altq *
+pfaltq_lookup(char *ifname)
 {
-	struct pf_altq	*altq;
+	ENTRY	 item;
+	ENTRY	*ret_item;
 
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(ifname, altq->ifname, IFNAMSIZ) == 0 &&
-		    altq->qname[0] == 0)
-			return (altq);
-	}
-	return (NULL);
+	item.key = ifname;
+	if (hsearch_r(item, FIND, &ret_item, &if_map) == 0)
+		return (NULL);
+
+	return (ret_item->data);
 }
 
-struct pf_altq *
+static struct pfctl_altq *
 qname_to_pfaltq(const char *qname, const char *ifname)
 {
-	struct pf_altq	*altq;
+	ENTRY	 item;
+	ENTRY	*ret_item;
+	char	 key[IFNAMSIZ + PF_QNAME_SIZE];
 
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(ifname, altq->ifname, IFNAMSIZ) == 0 &&
-		    strncmp(qname, altq->qname, PF_QNAME_SIZE) == 0)
-			return (altq);
-	}
-	return (NULL);
+	item.key = key;
+	snprintf(item.key, sizeof(key), "%s:%s", ifname, qname);
+	if (hsearch_r(item, FIND, &ret_item, &queue_map) == 0)
+		return (NULL);
+
+	return (ret_item->data);
 }
 
-u_int32_t
-qname_to_qid(const char *qname)
+static u_int32_t
+qname_to_qid(char *qname)
 {
-	struct pf_altq	*altq;
-
+	ENTRY	 item;
+	ENTRY	*ret_item;
+	uint32_t qid;
+	
 	/*
 	 * We guarantee that same named queues on different interfaces
-	 * have the same qid, so we do NOT need to limit matching on
-	 * one interface!
+	 * have the same qid.
 	 */
+	item.key = qname;
+	if (hsearch_r(item, FIND, &ret_item, &qid_map) == 0)
+		return (0);
 
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(qname, altq->qname, PF_QNAME_SIZE) == 0)
-			return (altq->qid);
-	}
-	return (0);
+	qid = *(uint32_t *)ret_item->data;
+	return (qid);
 }
 
 void
@@ -315,28 +366,26 @@ eval_pfaltq(struct pfctl *pf, struct pf_altq *pa, stru
 int
 check_commit_altq(int dev, int opts)
 {
-	struct pf_altq	*altq;
-	int		 error = 0;
+	struct pfctl_altq	*if_ppa;
+	int			 error = 0;
 
 	/* call the discipline check for each interface. */
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (altq->qname[0] == 0) {
-			switch (altq->scheduler) {
-			case ALTQT_CBQ:
-				error = check_commit_cbq(dev, opts, altq);
-				break;
-			case ALTQT_PRIQ:
-				error = check_commit_priq(dev, opts, altq);
-				break;
-			case ALTQT_HFSC:
-				error = check_commit_hfsc(dev, opts, altq);
-				break;
-			case ALTQT_FAIRQ:
-				error = check_commit_fairq(dev, opts, altq);
-				break;
-			default:
-				break;
-			}
+	STAILQ_FOREACH(if_ppa, &interfaces, meta.link) {
+		switch (if_ppa->pa.scheduler) {
+		case ALTQT_CBQ:
+			error = check_commit_cbq(dev, opts, if_ppa);
+			break;
+		case ALTQT_PRIQ:
+			error = check_commit_priq(dev, opts, if_ppa);
+			break;
+		case ALTQT_HFSC:
+			error = check_commit_hfsc(dev, opts, if_ppa);
+			break;
+		case ALTQT_FAIRQ:
+			error = check_commit_fairq(dev, opts, if_ppa);
+			break;
+		default:
+			break;
 		}
 	}
 	return (error);
@@ -350,17 +399,16 @@ eval_pfqueue(struct pfctl *pf, struct pf_altq *pa, str
     struct node_queue_opt *opts)
 {
 	/* should be merged with expand_queue */
-	struct pf_altq	*if_pa, *parent, *altq;
-	u_int64_t	 bwsum;
-	int		 error = 0;
+	struct pfctl_altq	*if_ppa, *parent;
+	int		 	 error = 0;
 
 	/* find the corresponding interface and copy fields used by queues */
-	if ((if_pa = pfaltq_lookup(pa->ifname)) == NULL) {
+	if ((if_ppa = pfaltq_lookup(pa->ifname)) == NULL) {
 		fprintf(stderr, "altq not defined on %s\n", pa->ifname);
 		return (1);
 	}
-	pa->scheduler = if_pa->scheduler;
-	pa->ifbandwidth = if_pa->ifbandwidth;
+	pa->scheduler = if_ppa->pa.scheduler;
+	pa->ifbandwidth = if_ppa->pa.ifbandwidth;
 
 	if (qname_to_pfaltq(pa->qname, pa->ifname) != NULL) {
 		fprintf(stderr, "queue %s already exists on interface %s\n",
@@ -377,16 +425,32 @@ eval_pfqueue(struct pfctl *pf, struct pf_altq *pa, str
 			    pa->parent, pa->qname);
 			return (1);
 		}
-		pa->parent_qid = parent->qid;
+		pa->parent_qid = parent->pa.qid;
 	}
 	if (pa->qlimit == 0)
 		pa->qlimit = DEFAULT_QLIMIT;
 
+	if (eval_queue_opts(pa, opts,
+		parent == NULL ? pa->ifbandwidth : parent->pa.bandwidth))
+		return (1);
+
 	if (pa->scheduler == ALTQT_CBQ || pa->scheduler == ALTQT_HFSC ||
 		pa->scheduler == ALTQT_FAIRQ) {
 		pa->bandwidth = eval_bwspec(bw,
-		    parent == NULL ? pa->ifbandwidth : parent->bandwidth);
+		    parent == NULL ? pa->ifbandwidth : parent->pa.bandwidth);
 
+		/*
+		 * For HFSC, if the linkshare service curve m2 parameter is
+		 * set, it overrides the provided queue bandwidth parameter,
+		 * so adjust the queue bandwidth parameter accordingly here
+		 * to avoid false positives in the total child bandwidth
+		 * check below.
+		 */
+		if ((pa->scheduler == ALTQT_HFSC) &&
+		    (pa->pq_u.hfsc_opts.lssc_m2 != 0)) {
+			pa->bandwidth = pa->pq_u.hfsc_opts.lssc_m2;
+		}
+
 		if (pa->bandwidth > pa->ifbandwidth) {
 			fprintf(stderr, "bandwidth for %s higher than "
 			    "interface\n", pa->qname);
@@ -394,44 +458,36 @@ eval_pfqueue(struct pfctl *pf, struct pf_altq *pa, str
 		}
 		/* check the sum of the child bandwidth is under parent's */
 		if (parent != NULL) {
-			if (pa->bandwidth > parent->bandwidth) {
+			if (pa->bandwidth > parent->pa.bandwidth) {
 				warnx("bandwidth for %s higher than parent",
 				    pa->qname);
 				return (1);
 			}
-			bwsum = 0;
-			TAILQ_FOREACH(altq, &altqs, entries) {
-				if (strncmp(altq->ifname, pa->ifname,
-				    IFNAMSIZ) == 0 &&
-				    altq->qname[0] != 0 &&
-				    strncmp(altq->parent, pa->parent,
-				    PF_QNAME_SIZE) == 0)
-					bwsum += altq->bandwidth;
+			parent->meta.bwsum += pa->bandwidth;
+			if (parent->meta.bwsum > parent->pa.bandwidth) {
+				warnx("the sum of the child bandwidth (%" PRIu64
+				    ") higher than parent \"%s\" (%" PRIu64 ")",
+				    parent->meta.bwsum, parent->pa.qname,
+				    parent->pa.bandwidth);
 			}
-			bwsum += pa->bandwidth;
-			if (bwsum > parent->bandwidth) {
-				warnx("the sum of the child bandwidth higher"
-				    " than parent \"%s\"", parent->qname);
-			}
 		}
 	}
 
-	if (eval_queue_opts(pa, opts,
-		parent == NULL ? pa->ifbandwidth : parent->bandwidth))
-		return (1);
-
+	if (parent != NULL)
+		parent->meta.children++;
+	
 	switch (pa->scheduler) {
 	case ALTQT_CBQ:
-		error = eval_pfqueue_cbq(pf, pa);
+		error = eval_pfqueue_cbq(pf, pa, if_ppa);
 		break;
 	case ALTQT_PRIQ:
-		error = eval_pfqueue_priq(pf, pa);
+		error = eval_pfqueue_priq(pf, pa, if_ppa);
 		break;
 	case ALTQT_HFSC:
-		error = eval_pfqueue_hfsc(pf, pa);
+		error = eval_pfqueue_hfsc(pf, pa, if_ppa, parent);
 		break;
 	case ALTQT_FAIRQ:
-		error = eval_pfqueue_fairq(pf, pa);
+		error = eval_pfqueue_fairq(pf, pa, if_ppa, parent);
 		break;
 	default:
 		break;
@@ -446,7 +502,7 @@ eval_pfqueue(struct pfctl *pf, struct pf_altq *pa, str
 #define	RM_NS_PER_SEC	(1000000000)
 
 static int
-eval_pfqueue_cbq(struct pfctl *pf, struct pf_altq *pa)
+eval_pfqueue_cbq(struct pfctl *pf, struct pf_altq *pa, struct pfctl_altq *if_ppa)
 {
 	struct cbq_opts	*opts;
 	u_int		 ifmtu;
@@ -476,6 +532,11 @@ eval_pfqueue_cbq(struct pfctl *pf, struct pf_altq *pa)
 	if (pa->parent[0] == 0)
 		opts->flags |= (CBQCLF_ROOTCLASS | CBQCLF_WRR);
 
+	if (pa->pq_u.cbq_opts.flags & CBQCLF_ROOTCLASS)
+		if_ppa->meta.root_classes++;
+	if (pa->pq_u.cbq_opts.flags & CBQCLF_DEFCLASS)
+		if_ppa->meta.default_classes++;
+	
 	cbq_compute_idletime(pf, pa);
 	return (0);
 }
@@ -568,33 +629,20 @@ cbq_compute_idletime(struct pfctl *pf, struct pf_altq 
 }
 
 static int
-check_commit_cbq(int dev, int opts, struct pf_altq *pa)
+check_commit_cbq(int dev, int opts, struct pfctl_altq *if_ppa)
 {
-	struct pf_altq	*altq;
-	int		 root_class, default_class;
-	int		 error = 0;
+	int	error = 0;
 
 	/*
 	 * check if cbq has one root queue and one default queue
 	 * for this interface
 	 */
-	root_class = default_class = 0;
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-		if (altq->pq_u.cbq_opts.flags & CBQCLF_ROOTCLASS)
-			root_class++;
-		if (altq->pq_u.cbq_opts.flags & CBQCLF_DEFCLASS)
-			default_class++;
-	}
-	if (root_class != 1) {
-		warnx("should have one root queue on %s", pa->ifname);
+	if (if_ppa->meta.root_classes != 1) {
+		warnx("should have one root queue on %s", if_ppa->pa.ifname);
 		error++;
 	}
-	if (default_class != 1) {
-		warnx("should have one default queue on %s", pa->ifname);
+	if (if_ppa->meta.default_classes != 1) {
+		warnx("should have one default queue on %s", if_ppa->pa.ifname);
 		error++;
 	}
 	return (error);
@@ -641,51 +689,37 @@ print_cbq_opts(const struct pf_altq *a)
  * PRIQ support functions
  */
 static int
-eval_pfqueue_priq(struct pfctl *pf, struct pf_altq *pa)
+eval_pfqueue_priq(struct pfctl *pf, struct pf_altq *pa, struct pfctl_altq *if_ppa)
 {
-	struct pf_altq	*altq;
 
 	if (pa->priority >= PRIQ_MAXPRI) {
 		warnx("priority out of range: max %d", PRIQ_MAXPRI - 1);
 		return (-1);
 	}
-	/* the priority should be unique for the interface */
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) == 0 &&
-		    altq->qname[0] != 0 && altq->priority == pa->priority) {
-			warnx("%s and %s have the same priority",
-			    altq->qname, pa->qname);
-			return (-1);
-		}
-	}
+	if (BIT_ISSET(QPRI_BITSET_SIZE, pa->priority, &if_ppa->meta.qpris)) {
+		warnx("%s does not have a unique priority on interface %s",
+		    pa->qname, pa->ifname);
+		return (-1);
+	} else
+		BIT_SET(QPRI_BITSET_SIZE, pa->priority, &if_ppa->meta.qpris);
 
+	if (pa->pq_u.priq_opts.flags & PRCF_DEFAULTCLASS)
+		if_ppa->meta.default_classes++;
 	return (0);
 }
 
 static int
-check_commit_priq(int dev, int opts, struct pf_altq *pa)
+check_commit_priq(int dev, int opts, struct pfctl_altq *if_ppa)
 {
-	struct pf_altq	*altq;
-	int		 default_class;
-	int		 error = 0;
 
 	/*
 	 * check if priq has one default class for this interface
 	 */
-	default_class = 0;
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-		if (altq->pq_u.priq_opts.flags & PRCF_DEFAULTCLASS)
-			default_class++;
+	if (if_ppa->meta.default_classes != 1) {
+		warnx("should have one default queue on %s", if_ppa->pa.ifname);
+		return (1);
 	}
-	if (default_class != 1) {
-		warnx("should have one default queue on %s", pa->ifname);
-		error++;
-	}
-	return (error);
+	return (0);
 }
 
 static int
@@ -720,15 +754,15 @@ print_priq_opts(const struct pf_altq *a)
  * HFSC support functions
  */
 static int
-eval_pfqueue_hfsc(struct pfctl *pf, struct pf_altq *pa)
+eval_pfqueue_hfsc(struct pfctl *pf, struct pf_altq *pa, struct pfctl_altq *if_ppa,
+    struct pfctl_altq *parent)
 {
-	struct pf_altq		*altq, *parent;
 	struct hfsc_opts_v1	*opts;
 	struct service_curve	 sc;
 
 	opts = &pa->pq_u.hfsc_opts;
 
-	if (pa->parent[0] == 0) {
+	if (parent == NULL) {
 		/* root queue */
 		opts->lssc_m1 = pa->ifbandwidth;
 		opts->lssc_m2 = pa->ifbandwidth;
@@ -736,9 +770,21 @@ eval_pfqueue_hfsc(struct pfctl *pf, struct pf_altq *pa
 		return (0);
 	}
 
-	LIST_INIT(&rtsc);
-	LIST_INIT(&lssc);
+	/* First child initializes the parent's service curve accumulators. */
+	if (parent->meta.children == 1) {
+		LIST_INIT(&parent->meta.rtsc);
+		LIST_INIT(&parent->meta.lssc);
+	}
 
+	if (parent->pa.pq_u.hfsc_opts.flags & HFCF_DEFAULTCLASS) {
+		warnx("adding %s would make default queue %s not a leaf",
+		    pa->qname, pa->parent);
+		return (-1);
+	}
+
+	if (pa->pq_u.hfsc_opts.flags & HFCF_DEFAULTCLASS)
+		if_ppa->meta.default_classes++;
+	
 	/* if link_share is not specified, use bandwidth */
 	if (opts->lssc_m2 == 0)
 		opts->lssc_m2 = pa->bandwidth;
@@ -768,51 +814,22 @@ eval_pfqueue_hfsc(struct pfctl *pf, struct pf_altq *pa
 	 * be smaller than the interface bandwidth, and the upper-limit should
 	 * be larger than the real-time service curve when both are defined.
 	 */
-	parent = qname_to_pfaltq(pa->parent, pa->ifname);
-	if (parent == NULL)
-		errx(1, "parent %s not found for %s", pa->parent, pa->qname);
-
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-
-		/* if the class has a real-time service curve, add it. */
-		if (opts->rtsc_m2 != 0 && altq->pq_u.hfsc_opts.rtsc_m2 != 0) {
-			sc.m1 = altq->pq_u.hfsc_opts.rtsc_m1;
-			sc.d = altq->pq_u.hfsc_opts.rtsc_d;
-			sc.m2 = altq->pq_u.hfsc_opts.rtsc_m2;
-			gsc_add_sc(&rtsc, &sc);
-		}
-
-		if (strncmp(altq->parent, pa->parent, PF_QNAME_SIZE) != 0)
-			continue;
-
-		/* if the class has a linkshare service curve, add it. */
-		if (opts->lssc_m2 != 0 && altq->pq_u.hfsc_opts.lssc_m2 != 0) {
-			sc.m1 = altq->pq_u.hfsc_opts.lssc_m1;
-			sc.d = altq->pq_u.hfsc_opts.lssc_d;
-			sc.m2 = altq->pq_u.hfsc_opts.lssc_m2;
-			gsc_add_sc(&lssc, &sc);
-		}
-	}
-
+	
 	/* check the real-time service curve.  reserve 20% of interface bw */
 	if (opts->rtsc_m2 != 0) {
 		/* add this queue to the sum */
 		sc.m1 = opts->rtsc_m1;
 		sc.d = opts->rtsc_d;
 		sc.m2 = opts->rtsc_m2;
-		gsc_add_sc(&rtsc, &sc);
+		gsc_add_sc(&parent->meta.rtsc, &sc);
 		/* compare the sum with 80% of the interface */
 		sc.m1 = 0;
 		sc.d = 0;
 		sc.m2 = pa->ifbandwidth / 100 * 80;
-		if (!is_gsc_under_sc(&rtsc, &sc)) {
+		if (!is_gsc_under_sc(&parent->meta.rtsc, &sc)) {
 			warnx("real-time sc exceeds 80%% of the interface "
 			    "bandwidth (%s)", rate2str((double)sc.m2));
-			goto err_ret;
+			return (-1);
 		}
 	}
 
@@ -822,14 +839,14 @@ eval_pfqueue_hfsc(struct pfctl *pf, struct pf_altq *pa
 		sc.m1 = opts->lssc_m1;
 		sc.d = opts->lssc_d;
 		sc.m2 = opts->lssc_m2;
-		gsc_add_sc(&lssc, &sc);
+		gsc_add_sc(&parent->meta.lssc, &sc);
 		/* compare the sum of the children with parent's sc */
-		sc.m1 = parent->pq_u.hfsc_opts.lssc_m1;
-		sc.d = parent->pq_u.hfsc_opts.lssc_d;
-		sc.m2 = parent->pq_u.hfsc_opts.lssc_m2;
-		if (!is_gsc_under_sc(&lssc, &sc)) {
+		sc.m1 = parent->pa.pq_u.hfsc_opts.lssc_m1;
+		sc.d = parent->pa.pq_u.hfsc_opts.lssc_d;
+		sc.m2 = parent->pa.pq_u.hfsc_opts.lssc_m2;
+		if (!is_gsc_under_sc(&parent->meta.lssc, &sc)) {
 			warnx("linkshare sc exceeds parent's sc");
-			goto err_ret;
+			return (-1);
 		}
 	}
 
@@ -838,38 +855,30 @@ eval_pfqueue_hfsc(struct pfctl *pf, struct pf_altq *pa
 		if (opts->ulsc_m1 > pa->ifbandwidth ||
 		    opts->ulsc_m2 > pa->ifbandwidth) {
 			warnx("upper-limit larger than interface bandwidth");
-			goto err_ret;
+			return (-1);
 		}
 		if (opts->rtsc_m2 != 0 && opts->rtsc_m2 > opts->ulsc_m2) {
 			warnx("upper-limit sc smaller than real-time sc");
-			goto err_ret;
+			return (-1);
 		}
 	}
 
-	gsc_destroy(&rtsc);
-	gsc_destroy(&lssc);
-
 	return (0);
-
-err_ret:
-	gsc_destroy(&rtsc);
-	gsc_destroy(&lssc);
-	return (-1);
 }
 
 /*
  * FAIRQ support functions
  */
 static int
-eval_pfqueue_fairq(struct pfctl *pf __unused, struct pf_altq *pa)
+eval_pfqueue_fairq(struct pfctl *pf __unused, struct pf_altq *pa,
+    struct pfctl_altq *if_ppa, struct pfctl_altq *parent)
 {
-	struct pf_altq		*altq, *parent;
 	struct fairq_opts	*opts;
 	struct service_curve	 sc;
 
 	opts = &pa->pq_u.fairq_opts;
 
-	if (pa->parent[0] == 0) {
+	if (pa->parent == NULL) {
 		/* root queue */
 		opts->lssc_m1 = pa->ifbandwidth;
 		opts->lssc_m2 = pa->ifbandwidth;
@@ -877,8 +886,19 @@ eval_pfqueue_fairq(struct pfctl *pf __unused, struct p
 		return (0);
 	}
 
-	LIST_INIT(&lssc);
+	/* First child initializes the parent's service curve accumulator. */
+	if (parent->meta.children == 1)
+		LIST_INIT(&parent->meta.lssc);
 
+	if (parent->pa.pq_u.fairq_opts.flags & FARF_DEFAULTCLASS) {
+		warnx("adding %s would make default queue %s not a leaf",
+		    pa->qname, pa->parent);
+		return (-1);
+	}
+
+	if (pa->pq_u.fairq_opts.flags & FARF_DEFAULTCLASS)
+		if_ppa->meta.default_classes++;
+
 	/* if link_share is not specified, use bandwidth */
 	if (opts->lssc_m2 == 0)
 		opts->lssc_m2 = pa->bandwidth;
@@ -894,122 +914,49 @@ eval_pfqueue_fairq(struct pfctl *pf __unused, struct p
 	 * be smaller than the interface bandwidth, and the upper-limit should
 	 * be larger than the real-time service curve when both are defined.
 	 */
-	parent = qname_to_pfaltq(pa->parent, pa->ifname);
-	if (parent == NULL)
-		errx(1, "parent %s not found for %s", pa->parent, pa->qname);
 
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-
-		if (strncmp(altq->parent, pa->parent, PF_QNAME_SIZE) != 0)
-			continue;
-
-		/* if the class has a link-sharing service curve, add it. */
-		if (opts->lssc_m2 != 0 && altq->pq_u.fairq_opts.lssc_m2 != 0) {
-			sc.m1 = altq->pq_u.fairq_opts.lssc_m1;
-			sc.d = altq->pq_u.fairq_opts.lssc_d;
-			sc.m2 = altq->pq_u.fairq_opts.lssc_m2;
-			gsc_add_sc(&lssc, &sc);
-		}
-	}
-
-	/* check the link-sharing service curve. */
+	/* check the linkshare service curve. */
 	if (opts->lssc_m2 != 0) {
-		sc.m1 = parent->pq_u.fairq_opts.lssc_m1;
-		sc.d = parent->pq_u.fairq_opts.lssc_d;
-		sc.m2 = parent->pq_u.fairq_opts.lssc_m2;
-		if (!is_gsc_under_sc(&lssc, &sc)) {
+		/* add this queue to the child sum */
+		sc.m1 = opts->lssc_m1;
+		sc.d = opts->lssc_d;
+		sc.m2 = opts->lssc_m2;
+		gsc_add_sc(&parent->meta.lssc, &sc);
+		/* compare the sum of the children with parent's sc */
+		sc.m1 = parent->pa.pq_u.fairq_opts.lssc_m1;
+		sc.d = parent->pa.pq_u.fairq_opts.lssc_d;
+		sc.m2 = parent->pa.pq_u.fairq_opts.lssc_m2;
+		if (!is_gsc_under_sc(&parent->meta.lssc, &sc)) {
 			warnx("link-sharing sc exceeds parent's sc");
-			goto err_ret;
+			return (-1);
 		}
 	}
 
-	gsc_destroy(&lssc);
-
 	return (0);
-
-err_ret:
-	gsc_destroy(&lssc);
-	return (-1);
 }
 
 static int
-check_commit_hfsc(int dev, int opts, struct pf_altq *pa)
+check_commit_hfsc(int dev, int opts, struct pfctl_altq *if_ppa)
 {
-	struct pf_altq	*altq, *def = NULL;
-	int		 default_class;
-	int		 error = 0;
 
 	/* check if hfsc has one default queue for this interface */
-	default_class = 0;
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-		if (altq->parent[0] == 0)  /* dummy root */
-			continue;
-		if (altq->pq_u.hfsc_opts.flags & HFCF_DEFAULTCLASS) {
-			default_class++;
-			def = altq;
-		}
-	}
-	if (default_class != 1) {
-		warnx("should have one default queue on %s", pa->ifname);
+	if (if_ppa->meta.default_classes != 1) {
+		warnx("should have one default queue on %s", if_ppa->pa.ifname);
 		return (1);
 	}
-	/* make sure the default queue is a leaf */
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-		if (strncmp(altq->parent, def->qname, PF_QNAME_SIZE) == 0) {
-			warnx("default queue is not a leaf");
-			error++;
-		}
-	}
-	return (error);
+	return (0);
 }
 
 static int
-check_commit_fairq(int dev __unused, int opts __unused, struct pf_altq *pa)
+check_commit_fairq(int dev __unused, int opts __unused, struct pfctl_altq *if_ppa)
 {
-	struct pf_altq	*altq, *def = NULL;
-	int		 default_class;
-	int		 error = 0;
 
 	/* check if fairq has one default queue for this interface */
-	default_class = 0;
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-		if (altq->pq_u.fairq_opts.flags & FARF_DEFAULTCLASS) {
-			default_class++;
-			def = altq;
-		}
-	}
-	if (default_class != 1) {
-		warnx("should have one default queue on %s", pa->ifname);
+	if (if_ppa->meta.default_classes != 1) {
+		warnx("should have one default queue on %s", if_ppa->pa.ifname);
 		return (1);
 	}
-	/* make sure the default queue is a leaf */
-	TAILQ_FOREACH(altq, &altqs, entries) {
-		if (strncmp(altq->ifname, pa->ifname, IFNAMSIZ) != 0)
-			continue;
-		if (altq->qname[0] == 0)  /* this is for interface */
-			continue;
-		if (strncmp(altq->parent, def->qname, PF_QNAME_SIZE) == 0) {
-			warnx("default queue is not a leaf");
-			error++;
-		}
-	}
-	return (error);
+	return (0);
 }
 
 static int
@@ -1351,8 +1298,7 @@ getifspeed(char *ifname)
 	struct ifreq	ifr;
 	struct if_data	ifrdat;
 
-	if ((s = socket(get_socket_domain(), SOCK_DGRAM, 0)) < 0)
-		err(1, "socket");
+	s = get_query_socket();
 	bzero(&ifr, sizeof(ifr));
 	if (strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) >=
 	    sizeof(ifr.ifr_name))
@@ -1360,8 +1306,6 @@ getifspeed(char *ifname)
 	ifr.ifr_data = (caddr_t)&ifrdat;
 	if (ioctl(s, SIOCGIFDATA, (caddr_t)&ifr) == -1)
 		err(1, "SIOCGIFDATA");
-	if (close(s))
-		err(1, "close");
 	return ((u_int32_t)ifrdat.ifi_baudrate);
 }
 #endif
@@ -1372,8 +1316,7 @@ getifmtu(char *ifname)
 	int		s;
 	struct ifreq	ifr;
 
-	if ((s = socket(get_socket_domain(), SOCK_DGRAM, 0)) < 0)
-		err(1, "socket");
+	s = get_query_socket();
 	bzero(&ifr, sizeof(ifr));
 	if (strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) >=
 	    sizeof(ifr.ifr_name))
@@ -1384,8 +1327,6 @@ getifmtu(char *ifname)
 #else
 		err(1, "SIOCGIFMTU");
 #endif
-	if (close(s))
-		err(1, "close");
 	if (ifr.ifr_mtu > 0)
 		return (ifr.ifr_mtu);
 	else {

Modified: head/sbin/pfctl/pfctl_parser.c
==============================================================================
--- head/sbin/pfctl/pfctl_parser.c	Mon Jan 21 23:54:33 2019	(r343286)
+++ head/sbin/pfctl/pfctl_parser.c	Tue Jan 22 00:50:24 2019	(r343287)
@@ -50,6 +50,7 @@ __FBSDID("$FreeBSD$");
 #include <net/pfvar.h>
 #include <arpa/inet.h>
 
+#include <search.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -72,7 +73,6 @@ void		 print_fromto(struct pf_rule_addr *, pf_osfp_t,
 		    struct pf_rule_addr *, u_int8_t, u_int8_t, int, int);
 int		 ifa_skip_if(const char *filter, struct node_host *p);
 
-struct node_host	*ifa_grouplookup(const char *, int);
 struct node_host	*host_if(const char *, int);
 struct node_host	*host_v4(const char *, int);
 struct node_host	*host_v6(const char *, int);
@@ -209,6 +209,19 @@ const struct pf_timeout pf_timeouts[] = {
 	{ NULL,			0 }
 };
 
+static struct hsearch_data isgroup_map;
+
+static __attribute__((constructor)) void
+pfctl_parser_init(void)
+{
+	/*
+	 * As hdestroy() will never be called on these tables, it will be
+	 * safe to use references into the stored data as keys.
+	 */
+	if (hcreate_r(0, &isgroup_map) == 0)
+		err(1, "Failed to create interface group query response map");
+}
+
 const struct icmptypeent *
 geticmptypebynumber(u_int8_t type, sa_family_t af)
 {
@@ -1153,6 +1166,71 @@ check_netmask(struct node_host *h, sa_family_t af)
 
 static struct node_host	*iftab;
 
+/*
+ * Retrieve the list of groups this interface is a member of and make sure
+ * each group is in the group map.
+ */
+static void
+ifa_add_groups_to_map(char *ifa_name)
+{
+	int			 s, len;
+	struct ifgroupreq	 ifgr;
+	struct ifg_req		*ifg;
+
+	s = get_query_socket();
+
+	/* Get size of group list for this interface */
+	memset(&ifgr, 0, sizeof(ifgr));
+	strlcpy(ifgr.ifgr_name, ifa_name, IFNAMSIZ);
+	if (ioctl(s, SIOCGIFGROUP, (caddr_t)&ifgr) == -1)
+		err(1, "SIOCGIFGROUP");
+
+	/* Retrieve group list for this interface */
+	len = ifgr.ifgr_len;
+	ifgr.ifgr_groups =
+	    (struct ifg_req *)calloc(len / sizeof(struct ifg_req),
+		sizeof(struct ifg_req));
+	if (ifgr.ifgr_groups == NULL)
+		err(1, "calloc");
+	if (ioctl(s, SIOCGIFGROUP, (caddr_t)&ifgr) == -1)
+		err(1, "SIOCGIFGROUP");
+
+	ifg = ifgr.ifgr_groups;
+	for (; ifg && len >= sizeof(struct ifg_req); ifg++) {
+		len -= sizeof(struct ifg_req);
+		if (strcmp(ifg->ifgrq_group, "all")) {
+			ENTRY	 		 item;
+			ENTRY			*ret_item;
+			int			*answer;
+	
+			item.key = ifg->ifgrq_group;
+			if (hsearch_r(item, FIND, &ret_item, &isgroup_map) == 0) {
+				struct ifgroupreq	 ifgr2;
+
+				/* Don't know the answer yet */
+				if ((answer = malloc(sizeof(int))) == NULL)
+					err(1, "malloc");
+
+				bzero(&ifgr2, sizeof(ifgr2));
+				strlcpy(ifgr2.ifgr_name, ifg->ifgrq_group,
+				    sizeof(ifgr2.ifgr_name));
+				if (ioctl(s, SIOCGIFGMEMB, (caddr_t)&ifgr2) == 0)
+					*answer = ifgr2.ifgr_len;
+				else
+					*answer = 0;
+
+				item.key = strdup(ifg->ifgrq_group);
+				item.data = answer;
+				if (hsearch_r(item, ENTER, &ret_item,
+					&isgroup_map) == 0)
+					err(1, "interface group query response"
+					    " map insert");
+			}
+		}
+	}
+	free(ifgr.ifgr_groups);
+}
+
 void
 ifa_load(void)
 {
@@ -1220,6 +1298,8 @@ ifa_load(void)
 				    sizeof(struct in6_addr));
 			n->ifindex = ((struct sockaddr_in6 *)
 			    ifa->ifa_addr)->sin6_scope_id;
+		} else if (n->af == AF_LINK) {
+			ifa_add_groups_to_map(ifa->ifa_name);
 		}
 		if ((n->ifname = strdup(ifa->ifa_name)) == NULL)
 			err(1, "ifa_load: strdup");
@@ -1237,7 +1317,7 @@ ifa_load(void)
 	freeifaddrs(ifap);
 }
 
-int
+static int
 get_socket_domain(void)
 {

*** DIFF OUTPUT TRUNCATED AT 1000 LINES ***



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201901220050.x0M0oOB1012753>