Removing the rte_modify API

For BGP LLGR purposes, there was an API allowing a protocol to directly
modify their stale routes in table before flushing them. This API was
called by the table prune routine which violates the future locking
requirements.

Instead of this, BGP now requests a special route export and reimports
these routes into the table, allowing for asynchronous execution without
locking the table on export.

lib/route.h

@@ -33,7 +33,6 @@ typedef struct rte {
 } rte;
 
 #define REF_FILTERED	2		/* Route is rejected by import filter */
-#define REF_MODIFY	16		/* Route is scheduled for modify */
 #define REF_PENDING	32		/* Route has not propagated completely yet */
 
 /* Route is valid for propagation (may depend on other flags in the future), accepts NULL */

nest/proto.c

@@ -438,7 +438,6 @@ channel_start_import(struct channel *c)
     .dump_req = channel_dump_import_req,
     .log_state_change = channel_import_log_state_change,
     .preimport = channel_preimport,
-    .rte_modify = c->proto->rte_modify,
   };
 
   ASSERT(c->channel_state == CS_UP);

nest/protocol.h

@@ -189,7 +189,6 @@ struct proto {
   int (*rte_recalculate)(struct rtable *, struct network *, struct rte *, struct rte *, struct rte *);
   int (*rte_better)(struct rte *, struct rte *);
   int (*rte_mergable)(struct rte *, struct rte *);
-  struct rte *(*rte_modify)(struct rte *, struct linpool *);
   void (*rte_insert)(struct network *, struct rte *);
   void (*rte_remove)(struct network *, struct rte *);
   u32 (*rte_igp_metric)(const struct rte *);

nest/rt-table.c

@@ -1322,8 +1322,6 @@ rte_recalculate(struct rt_import_hook *c, net *net, rte *new, struct rte_src *sr
 	  if (new && rte_same(old, &new_stored->rte))
 	    {
 	      /* No changes, ignore the new route and refresh the old one */
-
-	      old->flags &= ~REF_MODIFY;
 	      old->stale_cycle = new->stale_cycle;
 
 	      if (!rte_is_filtered(new))
@@ -1673,24 +1671,6 @@ rte_discard(net *net, rte *old)	/* Non-filtered route deletion, used during garb
   rte_update_unlock();
 }
 
-/* Modify existing route by protocol hook, used for long-lived graceful restart */
-static inline void
-rte_modify(net *net, rte *old)
-{
-  rte_update_lock();
-
-  rte *new = old->sender->req->rte_modify(old, rte_update_pool);
-  if (new != old)
-  {
-    if (new)
-      new->flags = old->flags & ~REF_MODIFY;
-
-    rte_recalculate(old->sender, net, new, old->src);
-  }
-
-  rte_update_unlock();
-}
-
 /* Check rtable for best route to given net whether it would be exported do p */
 int
 rt_examine(rtable *t, net_addr *a, struct channel *c, const struct filter *filter)
@@ -1977,29 +1957,6 @@ rt_refresh_end(struct rt_import_request *req)
     log(L_TRACE "%s: route refresh end [%u]", req->name, hook->stale_valid);
 }
 
-void
-rt_modify_stale(rtable *t, struct rt_import_request *req)
-{
-  int prune = 0;
-  struct rt_import_hook *s = req->hook;
-
-  FIB_WALK(&t->fib, net, n)
-    {
-      for (struct rte_storage *e = n->routes; e; e = e->next)
-	if ((e->rte.sender == s) &&
-	    !(e->rte.flags & REF_FILTERED) &&
-	    (e->rte.stale_cycle + 1 == s->stale_set))
-	  {
-	    e->rte.flags |= REF_MODIFY;
-	    prune = 1;
-	  }
-    }
-  FIB_WALK_END;
-
-  if (prune)
-    rt_schedule_prune(t);
-}
-
 /**
  * rte_dump - dump a route
  * @e: &rte to be dumped
@@ -2499,14 +2456,6 @@ again:
 	    rte_discard(n, &e->rte);
 	    limit--;
 
-	    goto rescan;
-	  }
-
-	if (e->rte.flags & REF_MODIFY)
-	  {
-	    rte_modify(n, &e->rte);
-	    limit--;
-
 	    goto rescan;
 	  }
       }

nest/rt.h

@@ -181,7 +181,6 @@ struct rt_import_request {
   /* Preimport is called when the @new route is just-to-be inserted, replacing @old.
    * Return a route (may be different or modified in-place) to continue or NULL to withdraw. */
   int (*preimport)(struct rt_import_request *req, struct rte *new, struct rte *old);
-  struct rte *(*rte_modify)(struct rte *, struct linpool *);
 };
 
 struct rt_import_hook {

proto/bgp/attrs.c

@@ -2546,27 +2546,59 @@ bgp_rte_recalculate(rtable *table, net *net, rte *new, rte *old, rte *old_best)
     return !old_suppressed;
 }
 
-rte *
-bgp_rte_modify_stale(struct rte *r, struct linpool *pool)
+void
+bgp_rte_modify_stale(struct rt_export_request *req, const net_addr *n, struct rt_pending_export *rpe UNUSED, rte **feed, uint count)
 {
-  eattr *ea = ea_find(r->attrs, BGP_EA_ID(BA_COMMUNITY));
-  const struct adata *ad = ea ? ea->u.ptr : NULL;
-  uint flags = ea ? ea->flags : BAF_PARTIAL;
+  struct bgp_channel *c = SKIP_BACK(struct bgp_channel, stale_feed, req);
+  struct rt_import_hook *irh = c->c.in_req.hook;
 
-  if (ad && int_set_contains(ad, BGP_COMM_NO_LLGR))
-    return NULL;
+  /* Find our routes among others */
+  for (uint i=0; i<count; i++)
+  {
+    rte *r = feed[i];
 
-  if (ad && int_set_contains(ad, BGP_COMM_LLGR_STALE))
-    return r;
+    /* Not our route */
+    if (r->sender != irh)
+      continue;
 
-  _Thread_local static rte e0;
-  e0 = *r;
+    /* A new route, do not mark as stale */
+    if (r->stale_cycle == irh->stale_set)
+      continue;
 
-  bgp_set_attr_ptr(&e0.attrs, BA_COMMUNITY, flags,
-		   int_set_add(pool, ad, BGP_COMM_LLGR_STALE));
-  e0.pflags |= BGP_REF_STALE;
+    eattr *ea = ea_find(r->attrs, BGP_EA_ID(BA_COMMUNITY));
+    const struct adata *ad = ea ? ea->u.ptr : NULL;
+    uint flags = ea ? ea->flags : BAF_PARTIAL;
 
-  return &e0;
+    /* LLGR not allowed, withdraw the route */
+    if (ad && int_set_contains(ad, BGP_COMM_NO_LLGR))
+    {
+      rte_import(&c->c.in_req, n, NULL, r->src);
+      continue;
+    }
+
+    /* Route already marked as LLGR, do nothing */
+    if (ad && int_set_contains(ad, BGP_COMM_LLGR_STALE))
+      continue;
+
+    /* Store the tmp_linpool state to aggresively save memory */
+    struct lp_state tmpp;
+    lp_save(tmp_linpool, &tmpp);
+
+    /* Mark the route as LLGR */
+    rte e0 = *r;
+    bgp_set_attr_ptr(&e0.attrs, BA_COMMUNITY, flags, int_set_add(tmp_linpool, ad, BGP_COMM_LLGR_STALE));
+    e0.pflags &= ~BGP_REF_NOT_STALE;
+    e0.pflags |= BGP_REF_STALE;
+
+    /* We need to update the route but keep it stale. */
+    ASSERT_DIE(irh->stale_set == irh->stale_valid + 1);
+    irh->stale_set--;
+    rte_import(&c->c.in_req, n, &e0, r->src);
+    irh->stale_set++;
+
+    /* Restore the memory state */
+    lp_restore(tmp_linpool, &tmpp);
+  }
 }
 
 

proto/bgp/bgp.c

@@ -139,6 +139,9 @@ static void bgp_update_bfd(struct bgp_proto *p, const struct bfd_options *bfd);
 static int bgp_incoming_connection(sock *sk, uint dummy UNUSED);
 static void bgp_listen_sock_err(sock *sk UNUSED, int err);
 
+static void bgp_graceful_restart_feed(struct bgp_channel *c);
+
+
 /**
  * bgp_open - open a BGP instance
  * @p: BGP instance
@@ -770,7 +773,7 @@ bgp_handle_graceful_restart(struct bgp_proto *p)
 
       case BGP_GRS_LLGR:
 	rt_refresh_begin(&c->c.in_req);
-	rt_modify_stale(c->c.table, &c->c.in_req);
+	bgp_graceful_restart_feed(c);
 	break;
       }
     }
@@ -796,6 +799,52 @@ bgp_handle_graceful_restart(struct bgp_proto *p)
   tm_start(p->gr_timer, p->conn->remote_caps->gr_time S);
 }
 
+static void
+bgp_graceful_restart_feed_done(struct rt_export_request *req)
+{
+  req->hook = NULL;
+}
+
+static void
+bgp_graceful_restart_feed_dump_req(struct rt_export_request *req)
+{
+  struct bgp_channel *c = SKIP_BACK(struct bgp_channel, stale_feed, req);
+  debug("  BGP-GR %s.%s export request %p\n", c->c.proto->name, c->c.name, req);
+}
+
+static void
+bgp_graceful_restart_feed_log_state_change(struct rt_export_request *req, u8 state)
+{
+  struct bgp_channel *c = SKIP_BACK(struct bgp_channel, stale_feed, req);
+  struct bgp_proto *p = (void *) c->c.proto;
+  BGP_TRACE(D_EVENTS, "Long-lived graceful restart export state changed to %s", rt_export_state_name(state));
+
+  if (state == TES_READY)
+    rt_stop_export(req, bgp_graceful_restart_feed_done);
+}
+
+static void
+bgp_graceful_restart_drop_export(struct rt_export_request *req UNUSED, const net_addr *n UNUSED, struct rt_pending_export *rpe UNUSED)
+{ /* Nothing to do */ }
+
+static void
+bgp_graceful_restart_feed(struct bgp_channel *c)
+{
+  c->stale_feed = (struct rt_export_request) {
+    .name = "BGP-GR",
+    .trace_routes = c->c.debug | c->c.proto->debug,
+    .dump_req = bgp_graceful_restart_feed_dump_req,
+    .log_state_change = bgp_graceful_restart_feed_log_state_change,
+    .export_bulk = bgp_rte_modify_stale,
+    .export_one = bgp_graceful_restart_drop_export,
+  };
+
+  rt_request_export(&c->c.table->exporter, &c->stale_feed);
+}
+
+
+
+
 /**
  * bgp_graceful_restart_done - finish active BGP graceful restart
  * @c: BGP channel
@@ -861,7 +910,7 @@ bgp_graceful_restart_timeout(timer *t)
       /* Channel is in GR, and supports LLGR -> start LLGR */
       c->gr_active = BGP_GRS_LLGR;
       tm_start(c->stale_timer, c->stale_time S);
-      rt_modify_stale(c->c.table, &c->c.in_req);
+      bgp_graceful_restart_feed(c);
     }
   }
   else
@@ -1672,7 +1721,6 @@ bgp_init(struct proto_config *CF)
   P->rte_better = bgp_rte_better;
   P->rte_mergable = bgp_rte_mergable;
   P->rte_recalculate = cf->deterministic_med ? bgp_rte_recalculate : NULL;
-  P->rte_modify = bgp_rte_modify_stale;
   P->rte_igp_metric = bgp_rte_igp_metric;
 
   p->cf = cf;

proto/bgp/bgp.h

@@ -371,6 +371,7 @@ struct bgp_channel {
 
   timer *stale_timer;			/* Long-lived stale timer for LLGR */
   u32 stale_time;			/* Stored LLGR stale time from last session */
+  struct rt_export_request stale_feed;	/* Feeder request for stale route modification */
 
   u8 add_path_rx;			/* Session expects receive of ADD-PATH extended NLRI */
   u8 add_path_tx;			/* Session expects transmit of ADD-PATH extended NLRI */
@@ -576,7 +577,7 @@ void bgp_done_prefix(struct bgp_channel *c, struct bgp_prefix *px, struct bgp_bu
 int bgp_rte_better(struct rte *, struct rte *);
 int bgp_rte_mergable(rte *pri, rte *sec);
 int bgp_rte_recalculate(rtable *table, net *net, rte *new, rte *old, rte *old_best);
-struct rte *bgp_rte_modify_stale(struct rte *r, struct linpool *pool);
+void bgp_rte_modify_stale(struct rt_export_request *req, const net_addr *n, struct rt_pending_export *rpe UNUSED, rte **feed, uint count);
 u32 bgp_rte_igp_metric(const rte *);
 void bgp_rt_notify(struct proto *P, struct channel *C, const net_addr *n, rte *new, const rte *old);
 int bgp_preexport(struct channel *, struct rte *);