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;
