Claudio Jeker([email protected]) on 2018.10.31 16:24:49 +0100:
> This diff introduces a real Adj-RIB-Out. It is the minimal change to
> introduce the new RIB. This removes the update_rib introduced before 6.4
> lock because that is now replaced with a real RIB.
> The code used by bgpctl show rib is now a fair amount easier since now one
> RIB runner can be used for all cases.
> path_update() is now returning if a prefix was not modified which is
> needed in the update path to suppress unneeded updates.
> The softreconfig out case becomes simpler because there is no more the
> need to run with both filters and check results.
> Last the shutdown code of the RDE had to be adjusted a bit to still work
> with the Adj-RIB-Out.
>
> This may cause memory consumption to go up but my testing has shown that
> the result is actually not significant. Needs the commits from earlier
> today to apply.
> --
> :wq Claudio
i dont see a problem with the diff other than the memory usage (and denis
remarks).
so if we decide that the memory is worth it, ok benno@
not sure if double the usage is acceptable though.
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.444
> diff -u -p -r1.444 rde.c
> --- rde.c 31 Oct 2018 14:50:07 -0000 1.444
> +++ rde.c 31 Oct 2018 15:09:39 -0000
> @@ -1395,7 +1395,7 @@ rde_update_update(struct rde_peer *peer,
>
> /* add original path to the Adj-RIB-In */
> if (path_update(&ribs[RIB_ADJ_IN].rib, peer, in, prefix, prefixlen,
> - vstate))
> + vstate) == 1)
> peer->prefix_cnt++;
>
> /* max prefix checker */
> @@ -2124,16 +2124,17 @@ rde_reflector(struct rde_peer *peer, str
> * control specific functions
> */
> static void
> -rde_dump_rib_as(struct prefix *p, struct rde_aspath *asp,
> - struct nexthop *nexthop, pid_t pid, int flags)
> +rde_dump_rib_as(struct prefix *p, struct rde_aspath *asp, pid_t pid, int
> flags)
> {
> struct ctl_show_rib rib;
> struct ibuf *wbuf;
> struct attr *a;
> + struct nexthop *nexthop;
> void *bp;
> time_t staletime;
> u_int8_t l;
>
> + nexthop = prefix_nexthop(p);
> bzero(&rib, sizeof(rib));
> rib.lastchange = p->lastchange;
> rib.local_pref = asp->lpref;
> @@ -2208,70 +2209,37 @@ rde_dump_rib_as(struct prefix *p, struct
> }
>
> static void
> -rde_dump_filterout(struct rde_peer *peer, struct prefix *p,
> - struct ctl_show_rib_request *req)
> -{
> - struct filterstate state;
> - enum filter_actions a;
> -
> - if (up_test_update(peer, p) != 1)
> - return;
> -
> - rde_filterstate_prep(&state, prefix_aspath(p), prefix_nexthop(p),
> - prefix_nhflags(p));
> - a = rde_filter(out_rules, peer, p, &state);
> -
> - if (a == ACTION_ALLOW)
> - rde_dump_rib_as(p, &state.aspath, state.nexthop, req->pid,
> - req->flags);
> -
> - rde_filterstate_clean(&state);
> -}
> -
> -static void
> rde_dump_filter(struct prefix *p, struct ctl_show_rib_request *req)
> {
> - struct rde_peer *peer;
> struct rde_aspath *asp;
>
> - if (req->flags & F_CTL_ADJ_OUT) {
> - if (p->re->active != p)
> - /* only consider active prefix */
> - return;
> - if (req->peerid) {
> - if ((peer = peer_get(req->peerid)) != NULL)
> - rde_dump_filterout(peer, p, req);
> - return;
> - }
> - } else {
> - asp = prefix_aspath(p);
> - if (req->peerid && req->peerid != prefix_peer(p)->conf.id)
> - return;
> - if ((req->flags & F_CTL_ACTIVE) && p->re->active != p)
> - return;
> - if ((req->flags & F_CTL_INVALID) &&
> - (asp->flags & F_ATTR_PARSE_ERR) == 0)
> - return;
> - if (req->type == IMSG_CTL_SHOW_RIB_AS &&
> - !aspath_match(asp->aspath->data, asp->aspath->len,
> - &req->as, 0))
> - return;
> - if (req->type == IMSG_CTL_SHOW_RIB_COMMUNITY &&
> - !community_match(asp, req->community.as,
> - req->community.type))
> - return;
> - if (req->type == IMSG_CTL_SHOW_RIB_EXTCOMMUNITY &&
> - !community_ext_match(asp, &req->extcommunity, 0))
> - return;
> - if (req->type == IMSG_CTL_SHOW_RIB_LARGECOMMUNITY &&
> - !community_large_match(asp, req->large_community.as,
> - req->large_community.ld1, req->large_community.ld2))
> - return;
> - if (!ovs_match(p, req->flags))
> - return;
> - rde_dump_rib_as(p, asp, prefix_nexthop(p), req->pid,
> - req->flags);
> - }
> + if (req->peerid && req->peerid != prefix_peer(p)->conf.id)
> + return;
> +
> + asp = prefix_aspath(p);
> + if ((req->flags & F_CTL_ACTIVE) && p->re->active != p)
> + return;
> + if ((req->flags & F_CTL_INVALID) &&
> + (asp->flags & F_ATTR_PARSE_ERR) == 0)
> + return;
> + if (req->type == IMSG_CTL_SHOW_RIB_AS &&
> + !aspath_match(asp->aspath->data, asp->aspath->len,
> + &req->as, 0))
> + return;
> + if (req->type == IMSG_CTL_SHOW_RIB_COMMUNITY &&
> + !community_match(asp, req->community.as,
> + req->community.type))
> + return;
> + if (req->type == IMSG_CTL_SHOW_RIB_EXTCOMMUNITY &&
> + !community_ext_match(asp, &req->extcommunity, 0))
> + return;
> + if (req->type == IMSG_CTL_SHOW_RIB_LARGECOMMUNITY &&
> + !community_large_match(asp, req->large_community.as,
> + req->large_community.ld1, req->large_community.ld2))
> + return;
> + if (!ovs_match(p, req->flags))
> + return;
> + rde_dump_rib_as(p, asp, req->pid, req->flags);
> }
>
> static void
> @@ -2342,6 +2310,8 @@ rde_dump_ctx_new(struct ctl_show_rib_req
> }
> if (req->flags & (F_CTL_ADJ_IN | F_CTL_INVALID)) {
> rid = RIB_ADJ_IN;
> + } else if (req->flags & F_CTL_ADJ_OUT) {
> + rid = RIB_ADJ_OUT;
> } else if ((rid = rib_find(req->rib)) == RIB_NOTFOUND) {
> log_warnx("rde_dump_ctx_new: no such rib %s", req->rib);
> error = CTL_RES_NOSUCHPEER;
> @@ -2591,6 +2561,19 @@ rde_up_dump_upcall(struct rib_entry *re,
> }
>
> static void
> +rde_up_flush_upcall(struct rib_entry *re, void *ptr)
> +{
> + struct rde_peer *peer = ptr;
> + struct prefix *p, *np;
> +
> + LIST_FOREACH_SAFE(p, &re->prefix_h, rib_l, np) {
> + if (peer != prefix_peer(p))
> + continue;
> + up_generate_updates(out_rules, peer, NULL, p);
> + }
> +}
> +
> +static void
> rde_up_dump_done(void *ptr, u_int8_t aid)
> {
> struct rde_peer *peer = ptr;
> @@ -2805,6 +2788,8 @@ rde_reload_done(void)
> u_int16_t rid;
> int reload = 0;
>
> + softreconfig = 0;
> +
> /* first merge the main config */
> if ((conf->flags & BGPD_FLAG_NO_EVALUATE) &&
> (nconf->flags & BGPD_FLAG_NO_EVALUATE) == 0) {
> @@ -2887,11 +2872,16 @@ rde_reload_done(void)
> char *p = log_fmt_peer(&peer->conf);
> log_debug("rib change: reloading peer %s", p);
> free(p);
> - up_withdraw_all(peer);
> peer->loc_rib_id = rib_find(peer->conf.rib);
> if (peer->loc_rib_id == RIB_NOTFOUND)
> fatalx("King Bula's peer met an unknown RIB");
> peer->reconf_rib = 1;
> + softreconfig++;
> + if (rib_dump_new(RIB_ADJ_OUT, AID_UNSPEC,
> + RDE_RUNNER_ROUNDS, peer, rde_up_flush_upcall,
> + rde_softreconfig_in_done, NULL) == -1)
> + fatal("%s: rib_dump_new", __func__);
> + log_peer_info(&peer->conf, "flushing Adj-RIB-Out");
> continue;
> }
> if (!rde_filter_equal(out_rules, out_rules_tmp, peer)) {
> @@ -2941,14 +2931,13 @@ rde_reload_done(void)
> }
> log_info("RDE reconfigured");
>
> - softreconfig = 0;
> if (reload > 0) {
> - log_info("running softreconfig in");
> softreconfig++;
> if (rib_dump_new(RIB_ADJ_IN, AID_UNSPEC,
> RDE_RUNNER_ROUNDS, &ribs[RIB_ADJ_IN], rde_softreconfig_in,
> rde_softreconfig_in_done, NULL) == -1)
> fatal("%s: rib_dump_new", __func__);
> + log_info("running softreconfig in");
> } else {
> rde_softreconfig_in_done(NULL, AID_UNSPEC);
> }
> @@ -3113,61 +3102,18 @@ rde_softreconfig_in(struct rib_entry *re
> }
>
> static void
> -rde_softreconfig_out_peer(struct rib_entry *re, struct rde_peer *peer)
> -{
> - struct filterstate ostate, nstate;
> - struct bgpd_addr addr;
> - struct prefix *p = re->active;
> - struct pt_entry *pt;
> - enum filter_actions oa, na;
> -
> - pt = re->prefix;
> - pt_getaddr(pt, &addr);
> -
> - if (up_test_update(peer, p) != 1)
> - return;
> -
> - rde_filterstate_prep(&ostate, prefix_aspath(p), prefix_nexthop(p),
> - prefix_nhflags(p));
> - rde_filterstate_prep(&nstate, prefix_aspath(p), prefix_nexthop(p),
> - prefix_nhflags(p));
> - oa = rde_filter(out_rules_tmp, peer, p, &ostate);
> - na = rde_filter(out_rules, peer, p, &nstate);
> -
> - /* go through all 4 possible combinations */
> - /* if (oa == ACTION_DENY && na == ACTION_DENY) */
> - /* nothing todo */
> - if (oa == ACTION_DENY && na == ACTION_ALLOW) {
> - /* send update */
> - up_rib_add(peer, re);
> - up_generate(peer, &nstate, &addr, pt->prefixlen);
> - } else if (oa == ACTION_ALLOW && na == ACTION_DENY) {
> - /* send withdraw */
> - up_rib_remove(peer, re);
> - up_generate(peer, NULL, &addr, pt->prefixlen);
> - } else if (oa == ACTION_ALLOW && na == ACTION_ALLOW) {
> - /* send update if anything changed */
> - if (nstate.nhflags != ostate.nhflags ||
> - nstate.nexthop != ostate.nexthop ||
> - path_compare(&nstate.aspath, &ostate.aspath) != 0)
> - up_generate(peer, &nstate, &addr, pt->prefixlen);
> - }
> -
> - rde_filterstate_clean(&ostate);
> - rde_filterstate_clean(&nstate);
> -}
> -
> -static void
> rde_softreconfig_out(struct rib_entry *re, void *bula)
> {
> + struct prefix *new = re->active;
> struct rde_peer *peer;
>
> - if (re->active == NULL)
> + if (new == NULL)
> return;
>
> LIST_FOREACH(peer, &peerlist, peer_l) {
> if (peer->loc_rib_id == re->rib_id && peer->reconf_out)
> - rde_softreconfig_out_peer(re, peer);
> + /* Regenerate all updates. */
> + up_generate_updates(out_rules, peer, new, new);
> }
> }
>
> @@ -3687,7 +3633,7 @@ network_add(struct network_config *nc, i
> vstate = rde_roa_validity(&conf->rde_roa, &nc->prefix,
> nc->prefixlen, asp->source_as);
> if (path_update(&ribs[RIB_ADJ_IN].rib, peerself, &state, &nc->prefix,
> - nc->prefixlen, vstate))
> + nc->prefixlen, vstate) == 1)
> peerself->prefix_cnt++;
> for (i = RIB_LOC_START; i < rib_size; i++) {
> if (!rib_valid(i))
> @@ -3835,21 +3781,20 @@ rde_shutdown(void)
> * rde_shutdown depends on this.
> */
>
> - /*
> - * All peers go down
> - */
> + /* Frist all peers go down */
> for (i = 0; i <= peertable.peer_hashmask; i++)
> while ((p = LIST_FIRST(&peertable.peer_hashtbl[i])) != NULL)
> peer_down(p->conf.id);
>
> + /* then since decision process is off, kill RIB_ADJ_OUT */
> + rib_free(rib_byid(RIB_ADJ_OUT));
> +
> /* free filters */
> filterlist_free(out_rules);
> - for (i = 0; i < rib_size; i++) {
> - if (!rib_valid(i))
> - continue;
> - filterlist_free(ribs[i].in_rules);
> - }
> + filterlist_free(out_rules_tmp);
>
> + /* now check everything */
> + rib_shutdown();
> nexthop_shutdown();
> path_shutdown();
> aspath_shutdown();
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.201
> diff -u -p -r1.201 rde.h
> --- rde.h 31 Oct 2018 14:50:07 -0000 1.201
> +++ rde.h 31 Oct 2018 15:09:39 -0000
> @@ -79,7 +79,6 @@ LIST_HEAD(attr_list, attr);
> LIST_HEAD(aspath_head, rde_aspath);
> RB_HEAD(uptree_prefix, update_prefix);
> RB_HEAD(uptree_attr, update_attr);
> -RB_HEAD(uptree_rib, update_rib);
>
> TAILQ_HEAD(uplist_prefix, update_prefix);
> TAILQ_HEAD(uplist_attr, update_attr);
> @@ -91,7 +90,6 @@ struct rde_peer {
> struct bgpd_addr remote_addr;
> struct bgpd_addr local_v4_addr;
> struct bgpd_addr local_v6_addr;
> - struct uptree_rib up_rib;
> struct uptree_prefix up_prefix;
> struct uptree_attr up_attrs;
> struct uplist_attr updates[AID_MAX];
> @@ -440,6 +438,7 @@ struct rib *rib_byid(u_int16_t);
> u_int16_t rib_find(char *);
> struct rib_desc *rib_desc(struct rib *);
> void rib_free(struct rib *);
> +void rib_shutdown(void);
> struct rib_entry *rib_get(struct rib *, struct bgpd_addr *, int);
> struct rib_entry *rib_lookup(struct rib *, struct bgpd_addr *);
> int rib_dump_pending(void);
> Index: rde_rib.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> retrieving revision 1.184
> diff -u -p -r1.184 rde_rib.c
> --- rde_rib.c 31 Oct 2018 14:50:07 -0000 1.184
> +++ rde_rib.c 31 Oct 2018 15:09:39 -0000
> @@ -216,6 +216,21 @@ rib_free(struct rib *rib)
> bzero(rd, sizeof(struct rib_desc));
> }
>
> +void
> +rib_shutdown(void)
> +{
> + u_int16_t id;
> +
> + for (id = 0; id < rib_size; id++) {
> + if (!rib_valid(id))
> + continue;
> + if (!RB_EMPTY(rib_tree(&ribs[id].rib)))
> + log_warnx("%s: rib %s is not empty", __func__,
> + ribs[id].name);
> + rib_free(&ribs[id].rib);
> + }
> +}
> +
> struct rib_entry *
> rib_get(struct rib *rib, struct bgpd_addr *prefix, int prefixlen)
> {
> @@ -553,6 +568,11 @@ path_hash_stats(struct rde_hashstats *hs
> }
> }
>
> +/*
> + * Update a prefix belonging to a possible new aspath.
> + * Return 1 if prefix was newly added, 0 if it was just changed or 2 if no
> + * change happened at all.
> + */
> int
> path_update(struct rib *rib, struct rde_peer *peer, struct filterstate
> *state,
> struct bgpd_addr *prefix, int prefixlen, u_int8_t vstate)
> @@ -575,7 +595,7 @@ path_update(struct rib *rib, struct rde_
> /* no change, update last change */
> p->lastchange = time(NULL);
> p->validation_state = vstate;
> - return (0);
> + return (2);
> }
> }
>
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 rde_update.c
> --- rde_update.c 24 Oct 2018 08:26:37 -0000 1.102
> +++ rde_update.c 31 Oct 2018 15:09:39 -0000
> @@ -53,15 +53,9 @@ struct update_attr {
> u_int16_t mpattr_len;
> };
>
> -struct update_rib {
> - RB_ENTRY(update_rib) entry;
> - struct rib_entry *re;
> -};
> -
> void up_clear(struct uplist_attr *, struct uplist_prefix *);
> int up_prefix_cmp(struct update_prefix *, struct update_prefix *);
> int up_attr_cmp(struct update_attr *, struct update_attr *);
> -int up_rib_cmp(struct update_rib *, struct update_rib *);
> int up_add(struct rde_peer *, struct update_prefix *, struct update_attr *);
>
> RB_PROTOTYPE(uptree_prefix, update_prefix, entry, up_prefix_cmp)
> @@ -70,9 +64,6 @@ RB_GENERATE(uptree_prefix, update_prefix
> RB_PROTOTYPE(uptree_attr, update_attr, entry, up_attr_cmp)
> RB_GENERATE(uptree_attr, update_attr, entry, up_attr_cmp)
>
> -RB_PROTOTYPE(uptree_rib, update_rib, entry, up_rib_cmp)
> -RB_GENERATE(uptree_rib, update_rib, entry, up_rib_cmp)
> -
> SIPHASH_KEY uptree_key;
>
> void
> @@ -86,7 +77,6 @@ up_init(struct rde_peer *peer)
> }
> RB_INIT(&peer->up_prefix);
> RB_INIT(&peer->up_attrs);
> - RB_INIT(&peer->up_rib);
> peer->up_pcnt = 0;
> peer->up_acnt = 0;
> peer->up_nlricnt = 0;
> @@ -120,7 +110,6 @@ up_clear(struct uplist_attr *updates, st
> void
> up_down(struct rde_peer *peer)
> {
> - struct update_rib *ur, *nur;
> u_int8_t i;
>
> for (i = 0; i < AID_MAX; i++)
> @@ -128,10 +117,6 @@ up_down(struct rde_peer *peer)
>
> RB_INIT(&peer->up_prefix);
> RB_INIT(&peer->up_attrs);
> - RB_FOREACH_SAFE(ur, uptree_rib, &peer->up_rib, nur) {
> - RB_REMOVE(uptree_rib, &peer->up_rib, ur);
> - free(ur);
> - }
>
> peer->up_pcnt = 0;
> peer->up_acnt = 0;
> @@ -210,58 +195,6 @@ up_attr_cmp(struct update_attr *a, struc
> }
>
> int
> -up_rib_cmp(struct update_rib *a, struct update_rib *b)
> -{
> - if (a->re != b->re)
> - return (a->re > b->re ? 1 : -1);
> - return 0;
> -}
> -
> -int
> -up_rib_remove(struct rde_peer *peer, struct rib_entry *re)
> -{
> - struct update_rib *ur, u;
> - u.re = re;
> -
> - ur = RB_FIND(uptree_rib, &peer->up_rib, &u);
> - if (ur != NULL) {
> - RB_REMOVE(uptree_rib, &peer->up_rib, ur);
> - free(ur);
> - return 1;
> - } else
> - return 0;
> -}
> -
> -void
> -up_rib_add(struct rde_peer *peer, struct rib_entry *re)
> -{
> - struct update_rib *ur;
> -
> - if ((ur = calloc(1, sizeof(*ur))) == NULL)
> - fatal("%s", __func__);
> - ur->re = re;
> -
> - if (RB_INSERT(uptree_rib, &peer->up_rib, ur) != NULL)
> - free(ur);
> -}
> -
> -void
> -up_withdraw_all(struct rde_peer *peer)
> -{
> - struct bgpd_addr addr;
> - struct update_rib *ur, *nur;
> -
> - RB_FOREACH_SAFE(ur, uptree_rib, &peer->up_rib, nur) {
> - RB_REMOVE(uptree_rib, &peer->up_rib, ur);
> -
> - /* withdraw prefix */
> - pt_getaddr(ur->re->prefix, &addr);
> - up_generate(peer, NULL, &addr, ur->re->prefix->prefixlen);
> - free(ur);
> - }
> -}
> -
> -int
> up_add(struct rde_peer *peer, struct update_prefix *p, struct update_attr *a)
> {
> struct update_attr *na = NULL;
> @@ -473,14 +406,16 @@ up_generate_updates(struct filter_head *
>
> if (new == NULL) {
> withdraw:
> - if (up_test_update(peer, old) != 1)
> - return;
> -
> - if (!up_rib_remove(peer, old->re))
> + if (old == NULL)
> return;
>
> /* withdraw prefix */
> pt_getaddr(old->re->prefix, &addr);
> + if (prefix_remove(&ribs[RIB_ADJ_OUT].rib, peer, &addr,
> + old->re->prefix->prefixlen) == 0) {
> + /* not in table, no need to send withdraw */
> + return;
> + }
> up_generate(peer, NULL, &addr, old->re->prefix->prefixlen);
> } else {
> switch (up_test_update(peer, new)) {
> @@ -500,8 +435,12 @@ withdraw:
> }
>
> pt_getaddr(new->re->prefix, &addr);
> - up_generate(peer, &state, &addr, new->re->prefix->prefixlen);
> - up_rib_add(peer, new->re);
> + if (path_update(&ribs[RIB_ADJ_OUT].rib, peer, &state, &addr,
> + new->re->prefix->prefixlen, prefix_vstate(new)) != 2) {
> + /* only send update if path changed */
> + up_generate(peer, &state, &addr,
> + new->re->prefix->prefixlen);
> + }
>
> rde_filterstate_clean(&state);
> }
>
