Re: bgpd optimize filter rules

2018-12-03 Thread Job Snijders
On Mon, Dec 03, 2018 at 12:14:13PM +0100, Claudio Jeker wrote:
> There is a trivial optimization that bgpd can do when loading the filter
> ruleset. If the rule is the same as the previous rule than the filterset
> can be merged.  e.g.
> 
> match from ebgp set community delete $myAS:*
> match from ebgp set community $myAS:15
> match from ebgp set med 100
> 
> Will be optimized into:
> 
> match from ebgp set { metric 100 community delete $myAS:* community 
> $myAS:15 }
> 
> The following diff is doing this and saves around 5% of the rules in
> arouteserver configs and probably similar amount in other peoples config.

OK job@



bgpd optimize filter rules

2018-12-03 Thread Claudio Jeker
There is a trivial optimization that bgpd can do when loading the filter
ruleset. If the rule is the same as the previous rule than the filterset
can be merged.  e.g.

match from ebgp set community delete $myAS:*
match from ebgp set community $myAS:15
match from ebgp set med 100

Will be optimized into:

match from ebgp set { metric 100 community delete $myAS:* community 
$myAS:15 }

The following diff is doing this and saves around 5% of the rules in
arouteserver configs and probably similar amount in other peoples config.
-- 
:wq Claudio

Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.355
diff -u -p -r1.355 bgpd.h
--- bgpd.h  28 Nov 2018 08:32:26 -  1.355
+++ bgpd.h  28 Nov 2018 10:07:50 -
@@ -966,10 +966,10 @@ enum action_types {
ACTION_SET_NEXTHOP_BLACKHOLE,
ACTION_SET_NEXTHOP_NOMODIFY,
ACTION_SET_NEXTHOP_SELF,
-   ACTION_SET_COMMUNITY,
ACTION_DEL_COMMUNITY,
-   ACTION_SET_EXT_COMMUNITY,
+   ACTION_SET_COMMUNITY,
ACTION_DEL_EXT_COMMUNITY,
+   ACTION_SET_EXT_COMMUNITY,
ACTION_PFTABLE,
ACTION_PFTABLE_ID,
ACTION_RTLABEL,
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.364
diff -u -p -r1.364 parse.y
--- parse.y 28 Nov 2018 08:32:27 -  1.364
+++ parse.y 28 Nov 2018 10:09:34 -
@@ -150,7 +150,7 @@ int  expand_rule(struct filter_rule *, 
 int str2key(char *, char *, size_t);
 int neighbor_consistent(struct peer *);
 int merge_filterset(struct filter_set_head *, struct filter_set *);
-voidmerge_filter_lists(struct filter_head *, struct filter_head *);
+voidoptimize_filters(struct filter_head *);
 struct filter_rule *get_rule(enum action_types);
 
 int parsecommunity(struct filter_community *, int, char *);
@@ -3345,13 +3345,15 @@ parse_config(char *filename, struct bgpd
free_config(conf);
} else {
/*
-* Move filter list and static group and peer filtersets
+* Concatenate filter list and static group and peer filtersets
 * together. Static group sets come first then peer sets
 * last normal filter rules.
 */
-   merge_filter_lists(conf->filters, groupfilter_l);
-   merge_filter_lists(conf->filters, peerfilter_l);
-   merge_filter_lists(conf->filters, filter_l);
+   TAILQ_CONCAT(conf->filters, groupfilter_l, entry);
+   TAILQ_CONCAT(conf->filters, peerfilter_l, entry);
+   TAILQ_CONCAT(conf->filters, filter_l, entry);
+
+   optimize_filters(conf->filters);
 
errors += mrt_mergeconfig(xconf->mrt, conf->mrt);
errors += merge_config(xconf, conf, peer_l);
@@ -4180,39 +4182,17 @@ neighbor_consistent(struct peer *p)
return (0);
 }
 
-int
-merge_filterset(struct filter_set_head *sh, struct filter_set *s)
+static void
+filterset_add(struct filter_set_head *sh, struct filter_set *s)
 {
struct filter_set   *t;
 
TAILQ_FOREACH(t, sh, entry) {
-   /*
-* need to cycle across the full list because even
-* if types are not equal filterset_cmp() may return 0.
-*/
-   if (filterset_cmp(s, t) == 0) {
-   if (s->type == ACTION_SET_COMMUNITY)
-   yyerror("community is already set");
-   else if (s->type == ACTION_DEL_COMMUNITY)
-   yyerror("community will already be deleted");
-   else if (s->type == ACTION_SET_EXT_COMMUNITY)
-   yyerror("ext-community is already set");
-   else if (s->type == ACTION_DEL_EXT_COMMUNITY)
-   yyerror(
-   "ext-community will already be deleted");
-   else
-   yyerror("redefining set parameter %s",
-   filterset_name(s->type));
-   return (-1);
-   }
-   }
-
-   TAILQ_FOREACH(t, sh, entry) {
if (s->type < t->type) {
TAILQ_INSERT_BEFORE(t, s, entry);
-   return (0);
+   return;
}
-   if (s->type == t->type)
+   if (s->type == t->type) {
switch (s->type) {
case ACTION_SET_COMMUNITY:
case ACTION_DEL_COMMUNITY:
@@ -4220,42 +4200,145 @@ merge_filterset(struct filter_set_head *
>action.community,