Task deferrer: kinda more dumb-resistant macro

Originally, this mechanism required to check whether there's enough time to work
and then to send an event. This macro combines all the logic and goes more straightforwardly
to the _end_ of the export processing loop.

One should note that there were two cases where the export processing loop
was deferred at the _beginning_, which led to ignoring some routes on
reimports. This wasn't easily noticeable in the tests until the one-task
limit got a ceiling on 300 ms to keep reasonable latency.

diff --git a/lib/io-loop.h b/lib/io-loop.h
index 4b93919e38fd2b3b713cd8afec03b0dcb47d1a1b..6c9465314f82e692857b89d3e06864a3c5a7b132 100644 (file)
--- a/lib/io-loop.h
+++ b/lib/io-loop.h
@@ -22,6 +22,13 @@ extern _Thread_local struct birdloop *this_birdloop;
 /* Check that the task has enough time to do a bit more */
 _Bool task_still_in_limit(void);
 
+#define MAYBE_DEFER_TASK(target, event, fmt, args...) do { \
+  if (!task_still_in_limit()) { \
+    if (config && config->latency_debug) \
+      log(L_TRACE "Deferring " fmt, ##args); \
+    return ev_send(target, event); \
+  } } while (0)
+
 /* Start a new birdloop owned by given pool and domain */
 struct birdloop *birdloop_new(pool *p, uint order, btime max_latency, const char *fmt, ...);
 

diff --git a/nest/proto.c b/nest/proto.c
index 512cca3d32c9cd58ab41e40e53189ca9114fab05..fb3f6a81ffb85ba2b3b3b8f9eb4424a1e06eca18 100644 (file)
--- a/nest/proto.c
+++ b/nest/proto.c
@@ -763,36 +763,33 @@ channel_do_reload(void *_c)
   struct channel *c = _c;
 
   RT_FEED_WALK(&c->reimporter, f)
-    if (task_still_in_limit())
+  {
+    for (uint i = 0; i < f->count_routes; i++)
     {
-      for (uint i = 0; i < f->count_routes; i++)
-      {
-       rte *r = &f->block[i];
+      rte *r = &f->block[i];
 
-       if (r->flags & REF_OBSOLETE)
-         break;
+      if (r->flags & REF_OBSOLETE)
+       break;
 
-       if (r->sender == c->in_req.hook)
-       {
-         /* Strip the table-specific information */
-         rte new = rte_init_from(r);
+      if (r->sender == c->in_req.hook)
+      {
+       /* Strip the table-specific information */
+       rte new = rte_init_from(r);
 
-         /* Strip the later attribute layers */
-         new.attrs = ea_strip_to(new.attrs, BIT32_ALL(EALS_PREIMPORT));
+       /* Strip the later attribute layers */
+       new.attrs = ea_strip_to(new.attrs, BIT32_ALL(EALS_PREIMPORT));
 
-         /* And reload the route */
-         rte_update(c, r->net, &new, new.src);
-       }
+       /* And reload the route */
+       rte_update(c, r->net, &new, new.src);
       }
-
-      /* Local data needed no more */
-      tmp_flush();
-    }
-    else
-    {
-      ev_send(proto_work_list(c->proto), &c->reimport_event);
-      return;
     }
+
+    /* Local data needed no more */
+    tmp_flush();
+
+    MAYBE_DEFER_TASK(proto_work_list(c->proto), &c->reimport_event,
+       "%s.%s reimport", c->proto->name, c->name);
+  }
 }
 
 /* Called by protocol to activate in_table */

diff --git a/nest/rt-table.c b/nest/rt-table.c
index 1afa26a93aef4cee9026544de1b07a53cd1b0717..d9a0255e5452116c95a8717dda0238464df6b064 100644 (file)
--- a/nest/rt-table.c
+++ b/nest/rt-table.c
@@ -921,8 +921,8 @@ channel_notify_accepted(void *_channel)
        }
     }
 
-    if (!task_still_in_limit())
-      return ev_send(c->out_req.r.target, c->out_req.r.event);
+    MAYBE_DEFER_TASK(c->out_req.r.target, c->out_req.r.event,
+       "export to %s.%s (secondary)", c->proto->name, c->name);
   }
 }
 
@@ -1054,8 +1054,8 @@ channel_notify_merged(void *_channel)
        }
     }
 
-    if (!task_still_in_limit())
-      return ev_send(c->out_req.r.target, c->out_req.r.event);
+    MAYBE_DEFER_TASK(c->out_req.r.target, c->out_req.r.event,
+       "export to %s.%s (merged)", c->proto->name, c->name);
   }
 }
 
@@ -1145,8 +1145,8 @@ channel_notify_basic(void *_channel)
        }
     }
 
-    if (!task_still_in_limit())
-      return ev_send(c->out_req.r.target, c->out_req.r.event);
+    MAYBE_DEFER_TASK(c->out_req.r.target, c->out_req.r.event,
+       "export to %s.%s (regular)", c->proto->name, c->name);
   }
 }
 
@@ -2569,8 +2569,8 @@ rt_flowspec_export(void *_link)
       rt_schedule_nhu(dst);
     }
 
-    if (!task_still_in_limit())
-      return ev_send_loop(dst_pub->loop, &ln->event);
+    MAYBE_DEFER_TASK(birdloop_event_list(dst_pub->loop), &ln->event,
+       "flowspec ctl export from %s to %s", ln->src->name, dst_pub->name);
   }
 }
 
@@ -4294,8 +4294,8 @@ hc_notify_export(void *_hc)
       }
     }
 
-    if (!task_still_in_limit())
-      return ev_send(hc->req.r.target, hc->req.r.event);
+    MAYBE_DEFER_TASK(hc->req.r.target, hc->req.r.event,
+       "hostcache updater in %s", tab->name);
   }
 }
 

diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
index f92f61d3144275905a4aa79a7df28bdd81fae095..1d3bd793dd424ea65e69186788b7ebd74c402fa9 100644 (file)
--- a/proto/bgp/attrs.c
+++ b/proto/bgp/attrs.c
@@ -2793,49 +2793,46 @@ bgp_rte_modify_stale(void *_bc)
   struct rt_import_hook *irh = c->c.in_req.hook;
 
   RT_FEED_WALK(&c->stale_feed, f) TMP_SAVED
-    if (task_still_in_limit())
+  {
+    for (uint i = 0; i < f->count_routes; i++)
     {
-      for (uint i = 0; i < f->count_routes; i++)
+      rte *r = &f->block[i];
+      if ((r->sender != irh) ||                /* Not our route */
+         (r->stale_cycle == irh->stale_set))   /* A new route, do not mark as stale */
+       continue;
+
+      /* Don't reinstate obsolete routes */
+      if (r->flags & REF_OBSOLETE)
+       break;
+
+      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;
+
+      /* LLGR not allowed, withdraw the route */
+      if (ad && int_set_contains(ad, BGP_COMM_NO_LLGR))
       {
-       rte *r = &f->block[i];
-       if ((r->sender != irh) ||               /* Not our route */
-           (r->stale_cycle == irh->stale_set)) /* A new route, do not mark as stale */
-         continue;
-
-       /* Don't reinstate obsolete routes */
-       if (r->flags & REF_OBSOLETE)
-         break;
-
-       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;
-
-       /* LLGR not allowed, withdraw the route */
-       if (ad && int_set_contains(ad, BGP_COMM_NO_LLGR))
-       {
-         rte_import(&c->c.in_req, r->net, NULL, r->src);
-         continue;
-       }
-
-       /* Route already marked as LLGR, do nothing */
-       if (ad && int_set_contains(ad, BGP_COMM_LLGR_STALE))
-         continue;
-
-       /* Mark the route as LLGR */
-       bgp_set_attr_ptr(&r->attrs, BA_COMMUNITY, flags, int_set_add(tmp_linpool, ad, BGP_COMM_LLGR_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, r->net, r, r->src);
-       irh->stale_set++;
+       rte_import(&c->c.in_req, r->net, NULL, r->src);
+       continue;
       }
+
+      /* Route already marked as LLGR, do nothing */
+      if (ad && int_set_contains(ad, BGP_COMM_LLGR_STALE))
+       continue;
+
+      /* Mark the route as LLGR */
+      bgp_set_attr_ptr(&r->attrs, BA_COMMUNITY, flags, int_set_add(tmp_linpool, ad, BGP_COMM_LLGR_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, r->net, r, r->src);
+      irh->stale_set++;
     }
-    else
-    {
-      proto_send_event(c->c.proto, &c->stale_event);
-      return;
-    }
+
+    MAYBE_DEFER_TASK(proto_event_list(c->c.proto), &c->stale_event,
+       "BGP %s.%s LLGR updater", c->c.proto->name, c->c.name);
+  }
 
   rt_feeder_unsubscribe(&c->stale_feed);
 }