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
Ping -- :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 -0000 @@ -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;