Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.07.14 09:33:19 +0200: > On Tue, Jun 29, 2021 at 12:00:24PM +0200, Claudio Jeker wrote: > > This diff moves the rib_entry pointer re into the union to safe some > > space. For add-path I need to add a few more u_int32_t and that would > > blow the size of struct prefix from 128 to 132 bytes. malloc would round > > that up to 256bytes and that is bad for the struct that is allocted in > > millions in bgpd. > > > > To make this somewhat save introduce PREFIX_FLAG_ADJOUT to mark prefixes > > that live in the adj-rib-out. Those prefixes can not access the re pointer > > also use a wrapper prefix_re() which returns the re pointer or NULL. > > Also add some assertions to make sure that prefixes don't end up in the > > wrong tree. > > > > This change shrinks the struct back to 120bytes and gives me the space > > needed for add-path. > > > > Please test >
reads ok, and works for me. I don't like all those fatals. We are bound to overlook something and hit. But of course those are hard to work around. > -- > :wq Claudio > > Index: rde.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v > retrieving revision 1.530 > diff -u -p -r1.530 rde.c > --- rde.c 25 Jun 2021 09:25:48 -0000 1.530 > +++ rde.c 29 Jun 2021 08:28:33 -0000 > @@ -2298,6 +2298,7 @@ rde_dump_rib_as(struct prefix *p, struct > struct ibuf *wbuf; > struct attr *a; > struct nexthop *nexthop; > + struct rib_entry *re; > void *bp; > time_t staletime; > size_t aslen; > @@ -2330,7 +2331,8 @@ rde_dump_rib_as(struct prefix *p, struct > rib.origin = asp->origin; > rib.validation_state = p->validation_state; > rib.flags = 0; > - if (p->re != NULL && p->re->active == p) > + re = prefix_re(p); > + if (re != NULL && re->active == p) > rib.flags |= F_PREF_ACTIVE; > if (!prefix_peer(p)->conf.ebgp) > rib.flags |= F_PREF_INTERNAL; > @@ -2412,14 +2414,16 @@ static void > rde_dump_filter(struct prefix *p, struct ctl_show_rib_request *req) > { > struct rde_aspath *asp; > + struct rib_entry *re; > > if (!rde_match_peer(prefix_peer(p), &req->neighbor)) > return; > > asp = prefix_aspath(p); > + re = prefix_re(p); > if (asp == NULL) /* skip pending withdraw in Adj-RIB-Out */ > return; > - if ((req->flags & F_CTL_ACTIVE) && p->re->active != p) > + if ((req->flags & F_CTL_ACTIVE) && re != NULL && re->active != p) > return; > if ((req->flags & F_CTL_INVALID) && > (asp->flags & F_ATTR_PARSE_ERR) == 0) > Index: rde.h > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v > retrieving revision 1.240 > diff -u -p -r1.240 rde.h > --- rde.h 17 Jun 2021 16:05:26 -0000 1.240 > +++ rde.h 29 Jun 2021 08:33:18 -0000v > @@ -56,10 +56,10 @@ struct rib { > struct filter_head *in_rules_tmp; > u_int rtableid; > u_int rtableid_tmp; > + enum reconf_action state, fibstate; > + u_int16_t id; > u_int16_t flags; > u_int16_t flags_tmp; > - u_int16_t id; > - enum reconf_action state, fibstate; > }; > > #define RIB_ADJ_IN 0 > @@ -317,13 +317,13 @@ struct prefix { > union { > struct { > LIST_ENTRY(prefix) rib, nexthop; > + struct rib_entry *re; > } list; > struct { > RB_ENTRY(prefix) index, update; > } tree; > } entry; > struct pt_entry *pt; > - struct rib_entry *re; > struct rde_aspath *aspath; > struct rde_community *communities; > struct rde_peer *peer; > @@ -338,6 +338,7 @@ struct prefix { > #define PREFIX_FLAG_DEAD 0x04 /* locked but removed */ > #define PREFIX_FLAG_STALE 0x08 /* stale entry (graceful > reload) */ > #define PREFIX_FLAG_MASK 0x0f /* mask for the prefix types */ > +#define PREFIX_FLAG_ADJOUT 0x10 /* prefix is in the adj-out rib > */ > #define PREFIX_NEXTHOP_LINKED 0x40 /* prefix is linked onto > nexthop list */ > #define PREFIX_FLAG_LOCKED 0x80 /* locked by rib walker */ > }; > @@ -637,6 +638,14 @@ static inline u_int8_t > prefix_vstate(struct prefix *p) > { > return (p->validation_state & ROA_MASK); > +} > + > +static inline struct rib_entry * > +prefix_re(struct prefix *p) > +{ > + if (p->flags & PREFIX_FLAG_ADJOUT) > + return NULL; > + return (p->entry.list.re); > } > > void nexthop_init(u_int32_t); > Index: rde_rib.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v > retrieving revision 1.222 > diff -u -p -r1.222 rde_rib.c > --- rde_rib.c 17 Jun 2021 08:16:04 -0000 1.222 > +++ rde_rib.c 29 Jun 2021 09:41:21 -0000 > @@ -1031,6 +1031,9 @@ prefix_move(struct prefix *p, struct rde > { > struct prefix *np; > > + if (p->flags & PREFIX_FLAG_ADJOUT) > + fatalx("%s: prefix with PREFIX_FLAG_ADJOUT hit", __func__); > + > if (peer != prefix_peer(p)) > fatalx("prefix_move: cross peer move"); > > @@ -1040,7 +1043,7 @@ prefix_move(struct prefix *p, struct rde > np->aspath = path_ref(asp); > np->communities = communities_ref(comm); > np->peer = peer; > - np->re = p->re; > + np->entry.list.re = prefix_re(p); > np->pt = p->pt; /* skip refcnt update since ref is moved */ > np->validation_state = vstate; > np->nhflags = nhflags; > @@ -1056,7 +1059,7 @@ prefix_move(struct prefix *p, struct rde > * no need to update the peer prefix count because we are only moving > * the prefix without changing the peer. > */ > - prefix_evaluate(np->re, np, p); > + prefix_evaluate(prefix_re(np), np, p); > > /* remove possible pftable reference from old path first */ > if (p->aspath && p->aspath->pftableid) > @@ -1071,8 +1074,8 @@ prefix_move(struct prefix *p, struct rde > p->nexthop = NULL; > p->aspath = NULL; > p->peer = NULL; > - p->re = NULL; > p->pt = NULL; > + p->entry.list.re = NULL; > prefix_free(p); > > return (0); > @@ -1093,6 +1096,8 @@ prefix_withdraw(struct rib *rib, struct > if (p == NULL) /* Got a dummy withdrawn request. */ > return (0); > > + if (p->flags & PREFIX_FLAG_ADJOUT) > + fatalx("%s: prefix with PREFIX_FLAG_ADJOUT hit", __func__); > asp = prefix_aspath(p); > if (asp && asp->pftableid) > /* only prefixes in the local RIB were pushed into pf */ > @@ -1112,8 +1117,8 @@ prefix_add_eor(struct rde_peer *peer, u_ > struct prefix *p; > > p = prefix_alloc(); > + p->flags = PREFIX_FLAG_ADJOUT | PREFIX_FLAG_UPDATE; > p->eor = 1; > - p->flags = PREFIX_FLAG_UPDATE; > if (RB_INSERT(prefix_tree, &peer->updates[aid], p) != NULL) > /* no need to add if EoR marker already present */ > prefix_free(p); > @@ -1134,6 +1139,9 @@ prefix_adjout_update(struct rde_peer *pe > int created = 0; > > if ((p = prefix_lookup(peer, prefix, prefixlen)) != NULL) { > + if ((p->flags & PREFIX_FLAG_ADJOUT) == 0) > + fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", > + __func__); > /* prefix is already in the Adj-RIB-Out */ > if (p->flags & PREFIX_FLAG_WITHDRAW) { > created = 1; /* consider this a new entry */ > @@ -1171,6 +1179,7 @@ prefix_adjout_update(struct rde_peer *pe > /* peer and pt remain */ > } else { > p = prefix_alloc(); > + p->flags |= PREFIX_FLAG_ADJOUT; > created = 1; > > p->pt = pt_get(prefix, prefixlen); > @@ -1225,6 +1234,9 @@ prefix_adjout_withdraw(struct rde_peer * > if (p == NULL) /* Got a dummy withdrawn request. */ > return (0); > > + if ((p->flags & PREFIX_FLAG_ADJOUT) == 0) > + fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__); > + > /* already a withdraw, shortcut */ > if (p->flags & PREFIX_FLAG_WITHDRAW) { > p->lastchange = getmonotime(); > @@ -1285,6 +1297,9 @@ prefix_adjout_destroy(struct prefix *p) > { > struct rde_peer *peer = prefix_peer(p); > > + if ((p->flags & PREFIX_FLAG_ADJOUT) == 0) > + fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__); > + > if (p->eor) { > /* EOR marker is not linked in the index */ > prefix_free(p); > @@ -1486,8 +1501,10 @@ static void > prefix_evaluate_all(struct prefix *p, enum nexthop_state state, > enum nexthop_state oldstate) > { > + struct rib_entry *re = prefix_re(p); > + > /* Skip non local-RIBs or RIBs that are flagged as noeval. */ > - if (re_rib(p->re)->flags & F_RIB_NOEVALUATE) { > + if (re_rib(re)->flags & F_RIB_NOEVALUATE) { > log_warnx("%s: prefix with F_RIB_NOEVALUATE hit", __func__); > return; > } > @@ -1500,15 +1517,15 @@ prefix_evaluate_all(struct prefix *p, en > * the routing decision so shortcut here. > */ > if (state == NEXTHOP_REACH) { > - if ((re_rib(p->re)->flags & F_RIB_NOFIB) == 0 && > - p == p->re->active) > - rde_send_kroute(re_rib(p->re), p, NULL); > + if ((re_rib(re)->flags & F_RIB_NOFIB) == 0 && > + p == re->active) > + rde_send_kroute(re_rib(re), p, NULL); > } > return; > } > > /* redo the route decision */ > - prefix_evaluate(p->re, p, p); > + prefix_evaluate(prefix_re(p), p, p); > } > > /* kill a prefix. */ > @@ -1516,7 +1533,7 @@ void > prefix_destroy(struct prefix *p) > { > /* make route decision */ > - prefix_evaluate(p->re, NULL, p); > + prefix_evaluate(prefix_re(p), NULL, p); > > prefix_unlink(p); > prefix_free(p); > @@ -1530,11 +1547,14 @@ prefix_link(struct prefix *p, struct rib > struct rde_aspath *asp, struct rde_community *comm, > struct nexthop *nexthop, u_int8_t nhflags, u_int8_t vstate) > { > + if (p->flags & PREFIX_FLAG_ADJOUT) > + fatalx("%s: prefix with PREFIX_FLAG_ADJOUT hit", __func__); > + > + p->entry.list.re = re; > p->aspath = path_ref(asp); > p->communities = communities_ref(comm); > p->peer = peer; > p->pt = pt_ref(re->prefix); > - p->re = re; > p->validation_state = vstate; > p->nhflags = nhflags; > p->nexthop = nexthop_ref(nexthop); > @@ -1555,7 +1575,7 @@ prefix_link(struct prefix *p, struct rib > static void > prefix_unlink(struct prefix *p) > { > - struct rib_entry *re = p->re; > + struct rib_entry *re = prefix_re(p); > > /* destroy all references to other objects */ > nexthop_unlink(p); > @@ -1567,7 +1587,6 @@ prefix_unlink(struct prefix *p) > p->nexthop = NULL; > p->aspath = NULL; > p->peer = NULL; > - p->re = NULL; > p->pt = NULL; > > if (re && rib_empty(re)) > @@ -1795,7 +1814,7 @@ nexthop_link(struct prefix *p) > return; > > /* no need to link prefixes in RIBs that have no decision process */ > - if (re_rib(p->re)->flags & F_RIB_NOEVALUATE) > + if (re_rib(prefix_re(p))->flags & F_RIB_NOEVALUATE) > return; > > p->flags |= PREFIX_NEXTHOP_LINKED; >