This diff moves the rde_send_pftable_commit() out of the prefix update and
withdraw functions and up into the functions calling them.
This will result in bigger batches being committed to pf(4) but the main
reason is that this is needed to allow pipelining of RIB tables.
I move the early returns from rde_update_dispatch() up in the function so
that everything later will get to the end of the function where the commit
happens. The move should not change current behaviour though.
Also network_flush_upcall() was changed to call prefix_withdraw() to
respect the pftable setting on announced prefixes. Apart from that the
diff should be straight forward.
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.487
diff -u -p -r1.487 rde.c
--- rde.c 14 Aug 2019 11:57:21 -0000 1.487
+++ rde.c 26 Sep 2019 11:59:52 -0000
@@ -568,6 +568,8 @@ 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 !=
@@ -682,7 +684,7 @@ rde_dispatch_imsg_parent(struct imsgbuf
static struct l3vpn *vpn;
struct imsg imsg;
struct mrt xmrt;
- struct rde_rib rn;
+ struct rde_rib rr;
struct filterstate state;
struct imsgbuf *i;
struct filter_head *nr;
@@ -787,18 +789,18 @@ rde_dispatch_imsg_parent(struct imsgbuf
if (imsg.hdr.len - IMSG_HEADER_SIZE !=
sizeof(struct rde_rib))
fatalx("IMSG_RECONF_RIB bad len");
- memcpy(&rn, imsg.data, sizeof(rn));
- rib = rib_byid(rib_find(rn.name));
+ memcpy(&rr, imsg.data, sizeof(rr));
+ rib = rib_byid(rib_find(rr.name));
if (rib == NULL) {
- rib = rib_new(rn.name, rn.rtableid, rn.flags);
- } else if (rib->flags == rn.flags &&
- rib->rtableid == rn.rtableid) {
+ rib = rib_new(rr.name, rr.rtableid, rr.flags);
+ } else if (rib->flags == rr.flags &&
+ rib->rtableid == rr.rtableid) {
/* no change to rib apart from filters */
rib->state = RECONF_KEEP;
} else {
/* reload rib because somehing changed */
- rib->flags_tmp = rn.flags;
- rib->rtableid_tmp = rn.rtableid;
+ rib->flags_tmp = rr.flags;
+ rib->rtableid_tmp = rr.rtableid;
rib->state = RECONF_RELOAD;
}
break;
@@ -1073,8 +1075,23 @@ rde_update_dispatch(struct imsg *imsg)
nlri_len =
imsg->hdr.len - IMSG_HEADER_SIZE - 4 - withdrawn_len - attrpath_len;
- bzero(&mpa, sizeof(mpa));
+ if (attrpath_len == 0) {
+ /* 0 = no NLRI information in this message */
+ if (nlri_len != 0) {
+ /* crap at end of update which should not be there */
+ rde_update_err(peer, ERR_UPDATE,
+ ERR_UPD_ATTRLIST, NULL, 0);
+ return (-1);
+ }
+ if (withdrawn_len == 0) {
+ /* EoR marker */
+ peer_recv_eor(peer, AID_INET);
+ return (0);
+ }
+ }
+
+ bzero(&mpa, sizeof(mpa));
rde_filterstate_prep(&state, NULL, NULL, NULL, 0);
if (attrpath_len != 0) { /* 0 = no NLRI information in this message */
/* parse path attributes */
@@ -1110,12 +1127,20 @@ rde_update_dispatch(struct imsg *imsg)
}
}
+ /* aspath needs to be loop free. This is not a hard error. */
+ if (state.aspath.flags & F_ATTR_ASPATH &&
+ peer->conf.ebgp &&
+ peer->conf.enforce_local_as == ENFORCE_AS_ON &&
+ !aspath_loopfree(state.aspath.aspath, peer->conf.local_as))
+ state.aspath.flags |= F_ATTR_LOOP;
+
rde_reflector(peer, &state.aspath);
}
p = imsg->data;
len = withdrawn_len;
p += 2;
+
/* withdraw prefix */
while (len > 0) {
if ((pos = nlri_get_prefix(p, len, &prefix,
@@ -1143,21 +1168,6 @@ rde_update_dispatch(struct imsg *imsg)
rde_update_withdraw(peer, &prefix, prefixlen);
}
- if (attrpath_len == 0) {
- /* 0 = no NLRI information in this message */
- if (nlri_len != 0) {
- /* crap at end of update which should not be there */
- rde_update_err(peer, ERR_UPDATE,
- ERR_UPD_ATTRLIST, NULL, 0);
- return (-1);
- }
- if (withdrawn_len == 0) {
- /* EoR marker */
- peer_recv_eor(peer, AID_INET);
- }
- return (0);
- }
-
/* withdraw MP_UNREACH_NLRI if available */
if (mpa.unreach_len != 0) {
mpp = mpa.unreach;
@@ -1257,12 +1267,6 @@ rde_update_dispatch(struct imsg *imsg)
/* shift to NLRI information */
p += 2 + attrpath_len;
- /* aspath needs to be loop free nota bene this is not a hard error */
- if (peer->conf.ebgp &&
- peer->conf.enforce_local_as == ENFORCE_AS_ON &&
- !aspath_loopfree(state.aspath.aspath, peer->conf.local_as))
- state.aspath.flags |= F_ATTR_LOOP;
-
/* parse nlri prefix */
while (nlri_len > 0) {
if ((pos = nlri_get_prefix(p, nlri_len, &prefix,
@@ -1393,6 +1397,7 @@ rde_update_dispatch(struct imsg *imsg)
done:
rde_filterstate_clean(&state);
+ rde_send_pftable_commit();
return (error);
}
@@ -2957,6 +2962,7 @@ rde_update6_queue_runner(u_int8_t aid)
/*
* pf table specific functions
*/
+int need_commit;
void
rde_send_pftable(u_int16_t id, struct bgpd_addr *addr,
u_int8_t len, int del)
@@ -2979,6 +2985,8 @@ rde_send_pftable(u_int16_t id, struct bg
del ? IMSG_PFTABLE_REMOVE : IMSG_PFTABLE_ADD,
0, 0, -1, &pfm, sizeof(pfm)) == -1)
fatal("%s %d imsg_compose error", __func__, __LINE__);
+
+ need_commit = 1;
}
void
@@ -2988,9 +2996,14 @@ rde_send_pftable_commit(void)
if (rde_quit)
return;
+ if (!need_commit)
+ return;
+
if (imsg_compose(ibuf_main, IMSG_PFTABLE_COMMIT, 0, 0, -1, NULL, 0) ==
-1)
fatal("%s %d imsg_compose error", __func__, __LINE__);
+
+ need_commit = 0;
}
/*
@@ -3164,6 +3177,9 @@ 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) {
@@ -3192,6 +3208,8 @@ 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;
@@ -3766,7 +3784,7 @@ peer_flush(struct rde_peer *peer, u_int8
NULL, NULL) == -1)
fatal("%s: rib_dump_new", __func__);
- /* Deletions are performed in path_remove() */
+ /* Deletions may have been performed in peer_flush_upcall */
rde_send_pftable_commit();
/* flushed no need to keep staletime */
@@ -3984,6 +4002,7 @@ network_add(struct network_config *nc, s
nc->prefixlen, vstate);
}
filterset_free(&nc->attrset);
+ rde_send_pftable_commit();
}
void
@@ -4050,6 +4069,8 @@ 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
@@ -4090,36 +4111,32 @@ static void
network_flush_upcall(struct rib_entry *re, void *ptr)
{
struct rde_peer *peer = ptr;
- struct rde_aspath *asp;
struct bgpd_addr addr;
- struct prefix *p, *np, *rp;
+ struct prefix *p;
u_int32_t i;
u_int8_t prefixlen;
+ p = prefix_bypeer(re, peer);
+ if (p == NULL)
+ return;
+ if ((prefix_aspath(p)->flags & F_ANN_DYNAMIC) != F_ANN_DYNAMIC)
+ return;
+
pt_getaddr(re->prefix, &addr);
prefixlen = re->prefix->prefixlen;
- LIST_FOREACH_SAFE(p, &re->prefix_h, entry.list.rib, np) {
- if (prefix_peer(p) != peer)
- continue;
- asp = prefix_aspath(p);
- if ((asp->flags & F_ANN_DYNAMIC) != F_ANN_DYNAMIC)
- continue;
- for (i = RIB_LOC_START; i < rib_size; i++) {
- struct rib *rib = rib_byid(i);
- if (rib == NULL)
- continue;
- rp = prefix_get(rib, peer, &addr, prefixlen);
- if (rp) {
- prefix_destroy(rp);
- rde_update_log("flush announce", i, peer,
- NULL, &addr, prefixlen);
- }
- }
+ for (i = RIB_LOC_START; i < rib_size; i++) {
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
+ continue;
+ if (prefix_withdraw(rib, peer, &addr, prefixlen) == 1)
+ rde_update_log("flush announce", i, peer,
+ NULL, &addr, prefixlen);
+ }
- prefix_destroy(p);
+ if (prefix_withdraw(rib_byid(RIB_ADJ_IN), peer, &addr,
+ prefixlen) == 1)
peer->prefix_cnt--;
- }
}
/* clean up */
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.206
diff -u -p -r1.206 rde_rib.c
--- rde_rib.c 14 Aug 2019 11:57:21 -0000 1.206
+++ rde_rib.c 26 Sep 2019 11:56:17 -0000
@@ -256,7 +256,7 @@ rib_free(struct rib *rib)
struct bgpd_addr addr;
pt_getaddr(p->pt, &addr);
- /* Commit is done in peer_down() */
+ /* Commit is done in rde_reload_done() */
rde_send_pftable(asp->pftableid, &addr,
p->pt->prefixlen, 1);
}
@@ -971,10 +971,8 @@ prefix_update(struct rib *rib, struct rd
struct rde_community *comm, *ncomm = &state->communities;
struct prefix *p;
- if (nasp->pftableid) {
+ if (nasp->pftableid)
rde_send_pftable(nasp->pftableid, prefix, prefixlen, 0);
- rde_send_pftable_commit();
- }
/*
* First try to find a prefix in the specified RIB.
@@ -1113,11 +1111,9 @@ prefix_withdraw(struct rib *rib, struct
return (0);
asp = prefix_aspath(p);
- if (asp && asp->pftableid) {
+ if (asp && asp->pftableid)
/* only prefixes in the local RIB were pushed into pf */
rde_send_pftable(asp->pftableid, prefix, prefixlen, 1);
- rde_send_pftable_commit();
- }
prefix_destroy(p);
