Hi bgpd and esp. bgpd-spamd users,
Currently the pftable code does not keep track how often a prefix was
added to a pftable. Because of this using the same pftable for multiple
neighbor tables does not work well. If one neighbor withdraws a route the
pftable entry is removed from the table no matter if the prefix is still
available from the other neighbor.
This diff changes this behaviour and introduces proper reference counting.
A pftable entry will now remain in the table until the last neighbor
withdraws the prefix. This makes much more sense and should not break
working setups. It will fix configs where more than one neighbor feeds
into the bgpd pftable.
As a side-effect bgpd will no commit pftable updates on a more regular
basis and not wait until some operations (full table walk) finished. This
should result in better responsiveness of updates.
Please test :)
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.506
diff -u -p -r1.506 rde.c
--- rde.c 5 Nov 2020 14:44:59 -0000 1.506
+++ rde.c 9 Nov 2020 10:06:51 -0000
@@ -69,6 +69,7 @@ void rde_dump_ctx_terminate(pid_t);
void rde_dump_mrt_new(struct mrt *, pid_t, int);
int rde_l3vpn_import(struct rde_community *, struct l3vpn *);
+static void rde_commit_pftable(void);
void rde_reload_done(void);
static void rde_softreconfig_in_done(void *, u_int8_t);
static void rde_softreconfig_out_done(void *, u_int8_t);
@@ -296,6 +297,8 @@ rde_main(int debug, int verbose)
for (aid = AID_INET6; aid < AID_MAX; aid++)
rde_update6_queue_runner(aid);
}
+ /* commit pftable once per poll loop */
+ rde_commit_pftable();
}
/* do not clean up on shutdown on production, it takes ages. */
@@ -497,8 +500,6 @@ badnetdel:
RDE_RUNNER_ROUNDS, peerself, network_flush_upcall,
NULL, NULL) == -1)
log_warn("rde_dispatch: IMSG_NETWORK_FLUSH");
- /* Deletions were performed in network_flush_upcall */
- rde_send_pftable_commit();
break;
case IMSG_FILTER_SET:
if (imsg.hdr.len - IMSG_HEADER_SIZE !=
@@ -1389,7 +1390,6 @@ rde_update_dispatch(struct rde_peer *pee
done:
rde_filterstate_clean(&state);
- rde_send_pftable_commit();
}
int
@@ -2950,10 +2950,31 @@ rde_update6_queue_runner(u_int8_t aid)
/*
* pf table specific functions
*/
+struct rde_pftable_node {
+ RB_ENTRY(rde_pftable_node) entry;
+ struct pt_entry *prefix;
+ int refcnt;
+ u_int16_t id;
+};
+RB_HEAD(rde_pftable_tree, rde_pftable_node);
+
+static inline int
+rde_pftable_cmp(struct rde_pftable_node *a, struct rde_pftable_node *b)
+{
+ if (a->prefix > b->prefix)
+ return 1;
+ if (a->prefix < b->prefix)
+ return -1;
+ return (a->id - b->id);
+}
+
+RB_GENERATE_STATIC(rde_pftable_tree, rde_pftable_node, entry, rde_pftable_cmp);
+
+struct rde_pftable_tree pftable_tree = RB_INITIALIZER(&pftable_tree);
int need_commit;
-void
-rde_send_pftable(u_int16_t id, struct bgpd_addr *addr,
- u_int8_t len, int del)
+
+static void
+rde_pftable_send(u_int16_t id, struct pt_entry *pt, int del)
{
struct pftable_msg pfm;
@@ -2966,8 +2987,8 @@ rde_send_pftable(u_int16_t id, struct bg
bzero(&pfm, sizeof(pfm));
strlcpy(pfm.pftable, pftable_id2name(id), sizeof(pfm.pftable));
- memcpy(&pfm.addr, addr, sizeof(pfm.addr));
- pfm.len = len;
+ pt_getaddr(pt, &pfm.addr);
+ pfm.len = pt->prefixlen;
if (imsg_compose(ibuf_main,
del ? IMSG_PFTABLE_REMOVE : IMSG_PFTABLE_ADD,
@@ -2978,7 +2999,55 @@ rde_send_pftable(u_int16_t id, struct bg
}
void
-rde_send_pftable_commit(void)
+rde_pftable_add(u_int16_t id, struct prefix *p)
+{
+ struct rde_pftable_node *pfn, node;
+
+ memset(&node, 0, sizeof(node));
+ node.prefix = p->pt;
+ node.id = id;
+
+ pfn = RB_FIND(rde_pftable_tree, &pftable_tree, &node);
+ if (pfn == NULL) {
+ if ((pfn = calloc(1, sizeof(*pfn))) == NULL)
+ fatal("%s", __func__);
+ pfn->prefix = pt_ref(p->pt);
+ pfn->id = id;
+
+ if (RB_INSERT(rde_pftable_tree, &pftable_tree, pfn) != NULL)
+ fatalx("%s: tree corrupt", __func__);
+
+ rde_pftable_send(id, p->pt, 0);
+ }
+ pfn->refcnt++;
+}
+
+void
+rde_pftable_del(u_int16_t id, struct prefix *p)
+{
+ struct rde_pftable_node *pfn, node;
+
+ memset(&node, 0, sizeof(node));
+ node.prefix = p->pt;
+ node.id = id;
+
+ pfn = RB_FIND(rde_pftable_tree, &pftable_tree, &node);
+ if (pfn == NULL)
+ return;
+
+ if (--pfn->refcnt <= 0) {
+ rde_pftable_send(id, p->pt, 1);
+
+ if (RB_REMOVE(rde_pftable_tree, &pftable_tree, pfn) == NULL)
+ fatalx("%s: tree corrupt", __func__);
+
+ pt_unref(pfn->prefix);
+ free(pfn);
+ }
+}
+
+void
+rde_commit_pftable(void)
{
/* do not run while cleaning up */
if (rde_quit)
@@ -3165,9 +3234,6 @@ rde_reload_done(void)
free_rde_prefixsets(&originsets_old);
as_sets_free(&as_sets_old);
- /* Deletions may have been performed in rib_free() */
- rde_send_pftable_commit();
-
log_info("RDE reconfigured");
if (reload > 0) {
@@ -3196,8 +3262,6 @@ rde_softreconfig_in_done(void *arg, u_in
log_info("softreconfig in done");
}
- /* Changes may have been performed during softreconfig_in run */
- rde_send_pftable_commit();
/* now do the Adj-RIB-Out sync and a possible FIB sync */
softreconfig = 0;
@@ -3621,7 +3685,6 @@ network_add(struct network_config *nc, s
nc->prefixlen, vstate);
}
filterset_free(&nc->attrset);
- rde_send_pftable_commit();
}
void
@@ -3688,8 +3751,6 @@ network_delete(struct network_config *nc
if (prefix_withdraw(rib_byid(RIB_ADJ_IN), peerself, &nc->prefix,
nc->prefixlen))
peerself->prefix_cnt--;
-
- rde_send_pftable_commit();
}
static void
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.234
diff -u -p -r1.234 rde.h
--- rde.h 5 Jun 2020 19:50:59 -0000 1.234
+++ rde.h 26 Oct 2020 14:48:24 -0000
@@ -368,9 +368,8 @@ void rde_update_log(const char *, u_in
void rde_send_kroute_flush(struct rib *);
void rde_send_kroute(struct rib *, struct prefix *, struct prefix *);
void rde_send_nexthop(struct bgpd_addr *, int);
-void rde_send_pftable(u_int16_t, struct bgpd_addr *,
- u_int8_t, int);
-void rde_send_pftable_commit(void);
+void rde_pftable_add(u_int16_t, struct prefix *);
+void rde_pftable_del(u_int16_t, struct prefix *);
void rde_generate_updates(struct rib *, struct prefix *,
struct prefix *);
Index: rde_peer.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
retrieving revision 1.5
diff -u -p -r1.5 rde_peer.c
--- rde_peer.c 12 Feb 2020 10:33:56 -0000 1.5
+++ rde_peer.c 9 Nov 2020 10:05:46 -0000
@@ -236,9 +236,8 @@ peer_flush_upcall(struct rib_entry *re,
rp = prefix_get(rib, peer, &addr, prefixlen);
if (rp) {
asp = prefix_aspath(rp);
- if (asp->pftableid)
- rde_send_pftable(asp->pftableid, &addr,
- prefixlen, 1);
+ if (asp && asp->pftableid)
+ rde_pftable_del(asp->pftableid, rp);
prefix_destroy(rp);
rde_update_log("flush", i, peer, NULL,
@@ -384,9 +383,6 @@ peer_flush(struct rde_peer *peer, u_int8
if (rib_dump_new(RIB_ADJ_IN, aid, 0, &pf, peer_flush_upcall,
NULL, NULL) == -1)
fatal("%s: rib_dump_new", __func__);
-
- /* Deletions may have been performed in peer_flush_upcall */
- rde_send_pftable_commit();
/* every route is gone so reset staletime */
if (aid == AID_UNSPEC) {
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.217
diff -u -p -r1.217 rde_rib.c
--- rde_rib.c 5 Nov 2020 11:51:13 -0000 1.217
+++ rde_rib.c 9 Nov 2020 09:50:40 -0000
@@ -252,14 +252,8 @@ rib_free(struct rib *rib)
*/
while ((p = LIST_FIRST(&re->prefix_h))) {
struct rde_aspath *asp = prefix_aspath(p);
- if (asp && asp->pftableid) {
- struct bgpd_addr addr;
-
- pt_getaddr(p->pt, &addr);
- /* Commit is done in rde_reload_done() */
- rde_send_pftable(asp->pftableid, &addr,
- p->pt->prefixlen, 1);
- }
+ if (asp && asp->pftableid)
+ rde_pftable_del(asp->pftableid, p);
prefix_destroy(p);
}
}
@@ -967,9 +961,6 @@ prefix_update(struct rib *rib, struct rd
struct rde_community *comm, *ncomm = &state->communities;
struct prefix *p;
- if (nasp->pftableid)
- rde_send_pftable(nasp->pftableid, prefix, prefixlen, 0);
-
/*
* First try to find a prefix in the specified RIB.
*/
@@ -1056,6 +1047,10 @@ prefix_move(struct prefix *p, struct rde
nexthop_link(np);
np->lastchange = getmonotime();
+ /* add possible pftable reference from new aspath */
+ if (asp && asp->pftableid)
+ rde_pftable_add(asp->pftableid, np);
+
/*
* no need to update the peer prefix count because we are only moving
* the prefix without changing the peer.
@@ -1075,6 +1070,10 @@ prefix_move(struct prefix *p, struct rde
/* remove old prefix node */
/* as before peer count needs no update because of move */
+ /* remove possible pftable reference first */
+ if (p->aspath && p->aspath->pftableid)
+ rde_pftable_del(p->aspath->pftableid, p);
+
/* destroy all references to other objects and free the old prefix */
nexthop_unlink(p);
nexthop_unref(p->nexthop);
@@ -1109,7 +1108,7 @@ prefix_withdraw(struct rib *rib, struct
asp = prefix_aspath(p);
if (asp && asp->pftableid)
/* only prefixes in the local RIB were pushed into pf */
- rde_send_pftable(asp->pftableid, prefix, prefixlen, 1);
+ rde_pftable_del(asp->pftableid, p);
prefix_destroy(p);
@@ -1596,6 +1595,10 @@ prefix_link(struct prefix *p, struct rib
p->nexthop = nexthop_ref(nexthop);
nexthop_link(p);
p->lastchange = getmonotime();
+
+ /* add possible pftable reference from aspath */
+ if (asp && asp->pftableid)
+ rde_pftable_add(asp->pftableid, p);
/* make route decision */
prefix_evaluate(p, re);
