bgpd change the way ribs are allocated

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

bgpd change the way ribs are allocated

Claudio Jeker
Currently all RIBs in the RDE are stored in an array which is resized on
demand. This does not work well when you plan to store pointers to RIB
elements in the RDE. Since I want this I changed the RDE to use an array
of pointers to individual RIB elements instead. This way pointers to the
elements remain stable even when the array is enlarged.
Nice side-effect this removes the exported ribs pointer and kills
rib_valid() since now all RIB loops use rib_byid() instead.

OK?
--
:wq Claudio

Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.486
diff -u -p -r1.486 rde.c
--- rde.c 14 Aug 2019 07:39:04 -0000 1.486
+++ rde.c 14 Aug 2019 08:32:38 -0000
@@ -777,10 +777,10 @@ rde_dispatch_imsg_parent(struct imsgbuf
  copy_config(nconf, imsg.data);
 
  for (rid = 0; rid < rib_size; rid++) {
- if (!rib_valid(rid))
+ if ((rib = rib_byid(rid)) == NULL)
  continue;
- ribs[rid].state = RECONF_DELETE;
- ribs[rid].fibstate = RECONF_NONE;
+ rib->state = RECONF_DELETE;
+ rib->fibstate = RECONF_NONE;
  }
  break;
  case IMSG_RECONF_RIB:
@@ -1412,7 +1412,7 @@ rde_update_update(struct rde_peer *peer,
     aspath_origin(in->aspath.aspath));
 
  /* add original path to the Adj-RIB-In */
- if (prefix_update(&ribs[RIB_ADJ_IN], peer, in, prefix, prefixlen,
+ if (prefix_update(rib_byid(RIB_ADJ_IN), peer, in, prefix, prefixlen,
     vstate) == 1)
  peer->prefix_cnt++;
 
@@ -1428,21 +1428,22 @@ rde_update_update(struct rde_peer *peer,
  wmsg = "path invalid, withdraw";
 
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
  rde_filterstate_prep(&state, &in->aspath, &in->communities,
     in->nexthop, in->nhflags);
  /* input filter */
- action = rde_filter(ribs[i].in_rules, peer, peer, prefix,
+ action = rde_filter(rib->in_rules, peer, peer, prefix,
     prefixlen, vstate, &state);
 
  if (action == ACTION_ALLOW) {
  rde_update_log("update", i, peer,
     &state.nexthop->exit_nexthop, prefix,
     prefixlen);
- prefix_update(&ribs[i], peer, &state, prefix,
+ prefix_update(rib, peer, &state, prefix,
     prefixlen, vstate);
- } else if (prefix_withdraw(&ribs[i], peer, prefix,
+ } else if (prefix_withdraw(rib, peer, prefix,
     prefixlen)) {
  rde_update_log(wmsg, i, peer,
     NULL, prefix, prefixlen);
@@ -1461,15 +1462,16 @@ rde_update_withdraw(struct rde_peer *pee
  u_int16_t i;
 
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
- if (prefix_withdraw(&ribs[i], peer, prefix, prefixlen))
+ if (prefix_withdraw(rib, peer, prefix, prefixlen))
  rde_update_log("withdraw", i, peer, NULL, prefix,
     prefixlen);
  }
 
  /* remove original path form the Adj-RIB-In */
- if (prefix_withdraw(&ribs[RIB_ADJ_IN], peer, prefix, prefixlen))
+ if (prefix_withdraw(rib_byid(RIB_ADJ_IN), peer, prefix, prefixlen))
  peer->prefix_cnt--;
 
  peer->prefix_rcvd_withdraw++;
@@ -2040,7 +2042,7 @@ rde_update_log(const char *message, u_in
  if (asprintf(&p, "%s/%u", log_addr(prefix), prefixlen) == -1)
  p = NULL;
  l = log_fmt_peer(&peer->conf);
- log_info("Rib %s: %s AS%s: %s %s%s", ribs[rid].name,
+ log_info("Rib %s: %s AS%s: %s %s%s", rib_byid(rid)->name,
     l, log_as(peer->conf.remote_as), message,
     p ? p : "out of memory", n ? n : "");
 
@@ -3115,43 +3117,44 @@ rde_reload_done(void)
  }
  /* bring ribs in sync */
  for (rid = 0; rid < rib_size; rid++) {
- if (!rib_valid(rid))
+ struct rib *rib = rib_byid(rid);
+ if (rib == NULL)
  continue;
- rde_filter_calc_skip_steps(ribs[rid].in_rules_tmp);
+ rde_filter_calc_skip_steps(rib->in_rules_tmp);
 
  /* flip rules, make new active */
- fh = ribs[rid].in_rules;
- ribs[rid].in_rules = ribs[rid].in_rules_tmp;
- ribs[rid].in_rules_tmp = fh;
+ fh = rib->in_rules;
+ rib->in_rules = rib->in_rules_tmp;
+ rib->in_rules_tmp = fh;
 
- switch (ribs[rid].state) {
+ switch (rib->state) {
  case RECONF_DELETE:
- rib_free(&ribs[rid]);
+ rib_free(rib);
  break;
  case RECONF_RELOAD:
- rib_update(&ribs[rid]);
- ribs[rid].state = RECONF_KEEP;
+ rib_update(rib);
+ rib->state = RECONF_KEEP;
  /* FALLTHROUGH */
  case RECONF_KEEP:
- if (rde_filter_equal(ribs[rid].in_rules,
-    ribs[rid].in_rules_tmp, NULL))
+ if (rde_filter_equal(rib->in_rules,
+    rib->in_rules_tmp, NULL))
  /* rib is in sync */
  break;
  log_debug("in filter change: reloading RIB %s",
-    ribs[rid].name);
- ribs[rid].state = RECONF_RELOAD;
+    rib->name);
+ rib->state = RECONF_RELOAD;
  reload++;
  break;
  case RECONF_REINIT:
  /* new rib */
- ribs[rid].state = RECONF_RELOAD;
+ rib->state = RECONF_RELOAD;
  reload++;
  break;
  case RECONF_NONE:
  break;
  }
- filterlist_free(ribs[rid].in_rules_tmp);
- ribs[rid].in_rules_tmp = NULL;
+ filterlist_free(rib->in_rules_tmp);
+ rib->in_rules_tmp = NULL;
  }
 
  filterlist_free(out_rules_tmp);
@@ -3165,8 +3168,8 @@ rde_reload_done(void)
 
  if (reload > 0) {
  softreconfig++;
- if (rib_dump_new(RIB_ADJ_IN, AID_UNSPEC,
-    RDE_RUNNER_ROUNDS, &ribs[RIB_ADJ_IN], rde_softreconfig_in,
+ if (rib_dump_new(RIB_ADJ_IN, AID_UNSPEC, RDE_RUNNER_ROUNDS,
+    rib_byid(RIB_ADJ_IN), rde_softreconfig_in,
     rde_softreconfig_in_done, NULL) == -1)
  fatal("%s: rib_dump_new", __func__);
  log_info("running softreconfig in");
@@ -3176,10 +3179,10 @@ rde_reload_done(void)
 }
 
 static void
-rde_softreconfig_in_done(void *arg, u_int8_t aid)
+rde_softreconfig_in_done(void *arg, u_int8_t dummy)
 {
  struct rde_peer *peer;
- u_int16_t rid;
+ u_int16_t i;
 
  if (arg != NULL) {
  softreconfig--;
@@ -3192,54 +3195,56 @@ rde_softreconfig_in_done(void *arg, u_in
 
  /* now do the Adj-RIB-Out sync and a possible FIB sync */
  softreconfig = 0;
- for (rid = 0; rid < rib_size; rid++) {
- if (!rib_valid(rid))
- continue;
- ribs[rid].state = RECONF_NONE;
- if (ribs[rid].fibstate == RECONF_RELOAD) {
- if (rib_dump_new(rid, AID_UNSPEC, RDE_RUNNER_ROUNDS,
-    &ribs[rid], rde_softreconfig_sync_fib,
+ for (i = 0; i < rib_size; i++) {
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
+ continue;
+ rib->state = RECONF_NONE;
+ if (rib->fibstate == RECONF_RELOAD) {
+ if (rib_dump_new(i, AID_UNSPEC, RDE_RUNNER_ROUNDS,
+    rib, rde_softreconfig_sync_fib,
     rde_softreconfig_sync_done, NULL) == -1)
  fatal("%s: rib_dump_new", __func__);
  softreconfig++;
  log_info("starting fib sync for rib %s",
-    ribs[rid].name);
- } else if (ribs[rid].fibstate == RECONF_REINIT) {
- if (rib_dump_new(rid, AID_UNSPEC, RDE_RUNNER_ROUNDS,
-    &ribs[rid], rde_softreconfig_sync_reeval,
+    rib->name);
+ } else if (rib->fibstate == RECONF_REINIT) {
+ if (rib_dump_new(i, AID_UNSPEC, RDE_RUNNER_ROUNDS,
+    rib, rde_softreconfig_sync_reeval,
     rde_softreconfig_sync_done, NULL) == -1)
  fatal("%s: rib_dump_new", __func__);
  softreconfig++;
  log_info("starting re-evaluation of rib %s",
-    ribs[rid].name);
+    rib->name);
  }
  }
 
  LIST_FOREACH(peer, &peerlist, peer_l) {
  if (peer->reconf_out)
- ribs[peer->loc_rib_id].state = RECONF_RELOAD;
+ rib_byid(peer->loc_rib_id)->state = RECONF_RELOAD;
  else if (peer->reconf_rib) {
- u_int8_t i;
+ u_int8_t aid;
 
  /* dump the full table to neighbors that changed rib */
- for (i = 0; i < AID_MAX; i++) {
- if (peer->capa.mp[i])
- peer_dump(peer->conf.id, i);
+ for (aid = 0; aid < AID_MAX; aid++) {
+ if (peer->capa.mp[aid])
+ peer_dump(peer->conf.id, aid);
  }
  }
  }
 
- for (rid = 0; rid < rib_size; rid++) {
- if (!rib_valid(rid))
+ for (i = 0; i < rib_size; i++) {
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
- if (ribs[rid].state == RECONF_RELOAD) {
- if (rib_dump_new(rid, AID_UNSPEC, RDE_RUNNER_ROUNDS,
-    &ribs[rid], rde_softreconfig_out,
+ if (rib->state == RECONF_RELOAD) {
+ if (rib_dump_new(i, AID_UNSPEC, RDE_RUNNER_ROUNDS,
+    rib, rde_softreconfig_out,
     rde_softreconfig_out_done, NULL) == -1)
  fatal("%s: rib_dump_new", __func__);
  softreconfig++;
  log_info("starting softreconfig out for rib %s",
-    ribs[rid].name);
+    rib->name);
  }
  }
 
@@ -3264,12 +3269,13 @@ rde_softreconfig_out_done(void *arg, u_i
 static void
 rde_softreconfig_done(void)
 {
- u_int16_t rid;
+ u_int16_t i;
 
- for (rid = 0; rid < rib_size; rid++) {
- if (!rib_valid(rid))
+ for (i = 0; i < rib_size; i++) {
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
- ribs[rid].state = RECONF_NONE;
+ rib->state = RECONF_NONE;
  }
 
  log_info("RDE soft reconfiguration done");
@@ -3314,10 +3320,10 @@ rde_softreconfig_in(struct rib_entry *re
  continue;
 
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ rib = rib_byid(i);
+ if (rib == NULL)
  continue;
 
- rib = &ribs[i];
  if (rib->state != RECONF_RELOAD && !force_eval)
  continue;
 
@@ -3724,9 +3730,10 @@ peer_flush_upcall(struct rib_entry *re,
  continue;
 
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
- rp = prefix_get(&ribs[i], peer, &addr, prefixlen);
+ rp = prefix_get(rib, peer, &addr, prefixlen);
  if (rp) {
  asp = prefix_aspath(rp);
  if (asp->pftableid)
@@ -3963,16 +3970,17 @@ network_add(struct network_config *nc, s
 
  vstate = rde_roa_validity(&conf->rde_roa, &nc->prefix,
     nc->prefixlen, aspath_origin(state->aspath.aspath));
- if (prefix_update(&ribs[RIB_ADJ_IN], peerself, state, &nc->prefix,
+ if (prefix_update(rib_byid(RIB_ADJ_IN), peerself, state, &nc->prefix,
     nc->prefixlen, vstate) == 1)
  peerself->prefix_cnt++;
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
  rde_update_log("announce", i, peerself,
     state->nexthop ? &state->nexthop->exit_nexthop : NULL,
     &nc->prefix, nc->prefixlen);
- prefix_update(&ribs[i], peerself, state, &nc->prefix,
+ prefix_update(rib, peerself, state, &nc->prefix,
     nc->prefixlen, vstate);
  }
  filterset_free(&nc->attrset);
@@ -4031,14 +4039,15 @@ network_delete(struct network_config *nc
  }
 
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
- if (prefix_withdraw(&ribs[i], peerself, &nc->prefix,
+ if (prefix_withdraw(rib, peerself, &nc->prefix,
     nc->prefixlen))
  rde_update_log("withdraw announce", i, peerself,
     NULL, &nc->prefix, nc->prefixlen);
  }
- if (prefix_withdraw(&ribs[RIB_ADJ_IN], peerself, &nc->prefix,
+ if (prefix_withdraw(rib_byid(RIB_ADJ_IN), peerself, &nc->prefix,
     nc->prefixlen))
  peerself->prefix_cnt--;
 }
@@ -4097,9 +4106,10 @@ network_flush_upcall(struct rib_entry *r
  continue;
 
  for (i = RIB_LOC_START; i < rib_size; i++) {
- if (!rib_valid(i))
+ struct rib *rib = rib_byid(i);
+ if (rib == NULL)
  continue;
- rp = prefix_get(&ribs[i], peer, &addr, prefixlen);
+ rp = prefix_get(rib, peer, &addr, prefixlen);
  if (rp) {
  prefix_destroy(rp);
  rde_update_log("flush announce", i, peer,
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.225
diff -u -p -r1.225 rde.h
--- rde.h 14 Aug 2019 07:39:04 -0000 1.225
+++ rde.h 14 Aug 2019 08:36:00 -0000
@@ -502,8 +502,7 @@ pt_unref(struct pt_entry *pt)
 }
 
 /* rde_rib.c */
-extern u_int16_t rib_size;
-extern struct rib *ribs;
+extern u_int16_t rib_size;
 
 struct rib *rib_new(char *, u_int, u_int16_t);
 void rib_update(struct rib *);
@@ -525,14 +524,6 @@ static inline struct rib *
 re_rib(struct rib_entry *re)
 {
  return rib_byid(re->rib_id);
-}
-
-static inline int
-rib_valid(u_int16_t rid)
-{
- if (rid == RIB_NOTFOUND || rid >= rib_size || *ribs[rid].name == '\0')
- return 0;
- return 1;
 }
 
 void path_init(u_int32_t);
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.205
diff -u -p -r1.205 rde_rib.c
--- rde_rib.c 14 Aug 2019 07:39:04 -0000 1.205
+++ rde_rib.c 14 Aug 2019 08:43:43 -0000
@@ -37,7 +37,7 @@
  * This is achieved by heavily linking the different parts together.
  */
 u_int16_t rib_size;
-struct rib *ribs;
+struct rib **ribs;
 
 struct rib_entry *rib_add(struct rib *, struct bgpd_addr *, int);
 static inline int rib_compare(const struct rib_entry *,
@@ -136,39 +136,40 @@ rib_compare(const struct rib_entry *a, c
 struct rib *
 rib_new(char *name, u_int rtableid, u_int16_t flags)
 {
- struct rib *xribs;
+ struct rib *new;
  u_int16_t id;
 
  for (id = 0; id < rib_size; id++) {
- if (!rib_valid(id))
+ if (ribs[id] == NULL)
  break;
  }
 
  if (id >= rib_size) {
- if ((xribs = reallocarray(ribs, id + 1,
-    sizeof(struct rib))) == NULL) {
- /* XXX this is not clever */
+ if ((ribs = recallocarray(ribs, id, id + 8,
+    sizeof(struct rib))) == NULL)
  fatal(NULL);
- }
- ribs = xribs;
- rib_size = id + 1;
+ rib_size = id + 8;
  }
 
- memset(&ribs[id], 0, sizeof(struct rib));
- strlcpy(ribs[id].name, name, sizeof(ribs[id].name));
- RB_INIT(rib_tree(&ribs[id]));
- ribs[id].state = RECONF_REINIT;
- ribs[id].id = id;
- ribs[id].flags = flags;
- ribs[id].rtableid = rtableid;
+ if ((new = calloc(1, sizeof(*new))) == NULL)
+ fatal(NULL);
+
+ strlcpy(new->name, name, sizeof(new->name));
+ RB_INIT(rib_tree(new));
+ new->state = RECONF_REINIT;
+ new->id = id;
+ new->flags = flags;
+ new->rtableid = rtableid;
 
- ribs[id].in_rules = calloc(1, sizeof(struct filter_head));
- if (ribs[id].in_rules == NULL)
+ new->in_rules = calloc(1, sizeof(struct filter_head));
+ if (new->in_rules == NULL)
  fatal(NULL);
- TAILQ_INIT(ribs[id].in_rules);
+ TAILQ_INIT(new->in_rules);
+
+ ribs[id] = new;
 
  log_debug("%s: %s -> %u", __func__, name, id);
- return (&ribs[id]);
+ return (new);
 }
 
 /*
@@ -200,11 +201,11 @@ rib_update(struct rib *rib)
 }
 
 struct rib *
-rib_byid(u_int16_t rid)
+rib_byid(u_int16_t id)
 {
- if (rib_valid(rid))
- return &ribs[rid];
- return NULL;
+ if (id == RIB_NOTFOUND || id >= rib_size || ribs[id] == NULL)
+ return NULL;
+ return ribs[id];
 }
 
 u_int16_t
@@ -217,7 +218,7 @@ rib_find(char *name)
  return RIB_LOC_START;
 
  for (id = 0; id < rib_size; id++) {
- if (!strcmp(ribs[id].name, name))
+ if (ribs[id] != NULL && !strcmp(ribs[id]->name, name))
  return id;
  }
 
@@ -266,27 +267,34 @@ rib_free(struct rib *rib)
  return; /* never remove the default ribs */
  filterlist_free(rib->in_rules_tmp);
  filterlist_free(rib->in_rules);
- memset(rib, 0, sizeof(struct rib));
+ ribs[rib->id] = NULL;
+ free(rib);
 }
 
 void
 rib_shutdown(void)
 {
+ struct rib *rib;
  u_int16_t id;
 
  for (id = 0; id < rib_size; id++) {
- if (!rib_valid(id))
+ rib = rib_byid(id);
+ if (rib == NULL)
  continue;
- if (!RB_EMPTY(rib_tree(&ribs[id]))) {
+ if (!RB_EMPTY(rib_tree(ribs[id]))) {
  log_warnx("%s: rib %s is not empty", __func__,
-    ribs[id].name);
+    ribs[id]->name);
  }
- rib_free(&ribs[id]);
+ rib_free(ribs[id]);
  }
  for (id = 0; id <= RIB_LOC_START; id++) {
- filterlist_free(ribs[id].in_rules_tmp);
- filterlist_free(ribs[id].in_rules);
- memset(&ribs[id], 0, sizeof(struct rib));
+ rib = rib_byid(id);
+ if (rib == NULL)
+ continue;
+ filterlist_free(rib->in_rules_tmp);
+ filterlist_free(rib->in_rules);
+ ribs[id] = NULL;
+ free(rib);
  }
  free(ribs);
 }