I've been using this for a week or so now and it's been very boring, which is an improvement in my experience.
I has my ok if that has any value. dlg > On 9 Nov 2020, at 8:16 pm, Claudio Jeker <[email protected]> wrote: > > 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); >
