ok!
Claudio Jeker([email protected]) on 2019.06.17 23:08:37 +0200:
> On Mon, Jun 17, 2019 at 10:20:57PM +0200, Sebastian Benoit wrote:
> > Claudio Jeker([email protected]) on 2019.06.17 21:34:30 +0200:
> > > Hi,
> > >
> > > Now that the community rewrite is in here another diff to make bgpd more
> > > awesome. There was one loop left that did not run asynchronous compared to
> > > the main event loop. This loop was in nexthop_update() now a route server
> > > or a big route reflector would be hit hard by this loop since it will send
> > > out many updates when a nexthop becomes valid. I have seen times where the
> > > RDE was busy for 15min in nexthop_update().
> > >
> > > This diff changes nexthop_update() to do this work asynchronous and
> > > therefor returns to the main event loop much more often.
> > > Nexthops now have a prefix pointer next_prefix which is used for looping
> > > through all prefixes. The for loop is broken up in RDE_RUNNER_ROUNDS steps
> > > which should hopefully return quickly.
> > >
> > > Additionally prefixes belonging to a RIB that is not evaluated (no
> > > decision process running) are no longer linked into this nexthop list
> > > which should also shorten the loop a bit (at least Adj-RIB-In and
> > > Adj-RIB-Out are now skipped). To make this work correctly the linking and
> > > unlinking of prefixes has been adjusted to always work in the right way
> > > (nexhop last on link and first on unlink).
> >
> > reads ok, one comment.
>
> Fixed.
>
> > > When testing please watch out for this message:
> > > prefix_updateall: prefix with F_RIB_NOEVALUATE hit
> > > This should never happen.
> > >
> > > I'm running this on a few routers with good success. Please check if you
> > > see
> > > a) worse convergence time (updates propagating much slower)
> > > b) better responsiveness (to e.g. bgpctl commands)
> >
> > i see things much faster, actually too fast, i cant meaningdul test bgpctl
> > responsiveness.
>
> You just need around 500 peers which are affected by a big nexthop update :)
>
> > > There is some potential for tuning but I would prefer to do this at a
> > > later stage.
> >
> > sure. one question about that: is the TAILQ_INSERT_TAIL/TAILQ_INSERT_HEAD
> > optimal?
>
> The idea is to make this like a ring. Start with the latest arrival
> first and then process one after the other. This makes sure that every
> entry gets about the same amount of process time. The TAILQ_INSERT_HEAD
> in nexthop_update() will reduce initial latency for nexthops with only
> few prefixes since they will be done in one nexthop_runner() call.
>
> There is also additional interactions with the other runners
> (imsg processing, rib_dump_runner and rde_update_queue_runner) which I
> need to profile to understand them better. It looks like
> rde_dispatch_imsg_session() is running ahead and produces a lot of work
> that can't be absorbed that quickly by the other runners. At least that is
> the case in the big route server setup that I'm working with.
>
> > > --
> > > :wq Claudio
> > >
> > > Index: rde.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> > > retrieving revision 1.470
> > > diff -u -p -r1.470 rde.c
> > > --- rde.c 17 Jun 2019 13:35:43 -0000 1.470
> > > +++ rde.c 17 Jun 2019 13:56:16 -0000
> > > @@ -256,9 +256,6 @@ rde_main(int debug, int verbose)
> > > set_pollfd(&pfd[PFD_PIPE_SESSION], ibuf_se);
> > > set_pollfd(&pfd[PFD_PIPE_SESSION_CTL], ibuf_se_ctl);
> > >
> > > - if (rib_dump_pending() || rde_update_queue_pending())
> > > - timeout = 0;
> > > -
> > > i = PFD_PIPE_COUNT;
> > > for (mctx = LIST_FIRST(&rde_mrts); mctx != 0; mctx = xmctx) {
> > > xmctx = LIST_NEXT(mctx, entry);
> > > @@ -275,6 +272,10 @@ rde_main(int debug, int verbose)
> > > }
> > > }
> > >
> > > + if (rib_dump_pending() || rde_update_queue_pending() ||
> > > + nexthop_pending())
> > > + timeout = 0;
> > > +
> > > if (poll(pfd, i, timeout) == -1) {
> > > if (errno != EINTR)
> > > fatal("poll error");
> > > @@ -311,12 +312,13 @@ rde_main(int debug, int verbose)
> > > mctx = LIST_NEXT(mctx, entry);
> > > }
> > >
> > > + rib_dump_runner();
> > > + nexthop_runner();
> > > if (ibuf_se && ibuf_se->w.queued < SESS_MSG_HIGH_MARK) {
> > > rde_update_queue_runner();
> > > for (aid = AID_INET6; aid < AID_MAX; aid++)
> > > rde_update6_queue_runner(aid);
> > > }
> > > - rib_dump_runner();
> > > }
> > >
> > > /* do not clean up on shutdown on production, it takes ages. */
> > > Index: rde.h
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> > > retrieving revision 1.214
> > > diff -u -p -r1.214 rde.h
> > > --- rde.h 17 Jun 2019 13:35:43 -0000 1.214
> > > +++ rde.h 17 Jun 2019 13:56:16 -0000
> > > @@ -228,12 +228,15 @@ struct rde_aspath {
> > > enum nexthop_state {
> > > NEXTHOP_LOOKUP,
> > > NEXTHOP_UNREACH,
> > > - NEXTHOP_REACH
> > > + NEXTHOP_REACH,
> > > + NEXTHOP_FLAPPED
> > > };
> > >
> > > struct nexthop {
> > > LIST_ENTRY(nexthop) nexthop_l;
> > > + TAILQ_ENTRY(nexthop) runner_l;
> > > struct prefix_list prefix_h;
> > > + struct prefix *next_prefix;
> > > struct bgpd_addr exit_nexthop;
> > > struct bgpd_addr true_nexthop;
> > > struct bgpd_addr nexthop_net;
> > > @@ -246,6 +249,7 @@ struct nexthop {
> > > #endif
> > > int refcnt;
> > > enum nexthop_state state;
> > > + enum nexthop_state oldstate;
> > > u_int8_t nexthop_netlen;
> > > u_int8_t flags;
> > > #define NEXTHOP_CONNECTED 0x01
> > > @@ -593,6 +597,8 @@ prefix_vstate(struct prefix *p)
> > >
> > > void nexthop_init(u_int32_t);
> > > void nexthop_shutdown(void);
> > > +int nexthop_pending(void);
> > > +void nexthop_runner(void);
> > > void nexthop_modify(struct nexthop *, enum action_types,
> > > u_int8_t,
> > > struct nexthop **, u_int8_t *);
> > > void nexthop_link(struct prefix *);
> > > Index: rde_rib.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> > > retrieving revision 1.191
> > > diff -u -p -r1.191 rde_rib.c
> > > --- rde_rib.c 17 Jun 2019 11:02:20 -0000 1.191
> > > +++ rde_rib.c 17 Jun 2019 19:10:07 -0000
> > > @@ -517,13 +517,15 @@ path_empty(struct rde_aspath *asp)
> > > return (asp == NULL || asp->refcnt <= 0);
> > > }
> > >
> > > -static inline void
> > > +static inline struct rde_aspath *
> > > path_ref(struct rde_aspath *asp)
> > > {
> > > if ((asp->flags & F_ATTR_LINKED) == 0)
> > > fatalx("%s: unlinked object", __func__);
> > > asp->refcnt++;
> > > rdemem.path_refs++;
> > > +
> > > + return asp;
> > > }
> > >
> > > static inline void
> > > @@ -955,19 +957,16 @@ prefix_move(struct prefix *p, struct rde
> > > fatalx("prefix_move: cross peer move");
> > >
> > > np = prefix_alloc();
> > > - np->aspath = asp;
> > > - np->communities = comm;
> > > - np->nexthop = nexthop_ref(nexthop);
> > > - nexthop_link(np);
> > > + /* add reference to new AS path and communities */
> > > + np->aspath = path_ref(asp);
> > > + np->communities = communities_get(comm);
> > > np->peer = peer;
> > > np->re = p->re;
> > > - np->lastchange = time(NULL);
> > > - np->nhflags = nhflags;
> > > np->validation_state = vstate;
> > > -
> > > - /* add reference to new AS path and communities */
> > > - path_ref(asp);
> > > - communities_get(comm);
> > > + np->nhflags = nhflags;
> > > + np->nexthop = nexthop_ref(nexthop);
> > > + nexthop_link(np);
> > > + np->lastchange = time(NULL);
> > >
> > > /*
> > > * no need to update the peer prefix count because we are only moving
> > > @@ -988,18 +987,18 @@ prefix_move(struct prefix *p, struct rde
> > > /* remove old prefix node */
> > > /* as before peer count needs no update because of move */
> > > oasp = p->aspath;
> > > - if (oasp)
> > > - path_unref(oasp);
> > >
> > > /* destroy all references to other objects and free the old prefix */
> > > - p->aspath = NULL;
> > > + nexthop_unlink(p);
> > > + nexthop_put(p->nexthop);
> > > communities_put(p->communities);
> > > + if (oasp)
> > > + path_unref(oasp);
> > > p->communities = NULL;
> > > + p->nexthop = NULL;
> > > + p->aspath = NULL;
> > > p->peer = NULL;
> > > p->re = NULL;
> > > - nexthop_unlink(p);
> > > - nexthop_put(p->nexthop);
> > > - p->nexthop = NULL;
> > > prefix_free(p);
> > >
> > > /* destroy old path if empty */
> > > @@ -1252,19 +1251,23 @@ prefix_updateall(struct prefix *p, enum
> > > enum nexthop_state oldstate)
> > > {
> > > /* Skip non local-RIBs or RIBs that are flagged as noeval. */
> > > - if (re_rib(p->re)->flags & F_RIB_NOEVALUATE)
> > > + if (re_rib(p->re)->flags & F_RIB_NOEVALUATE) {
> > > + log_warnx("%s: prefix with F_RIB_NOEVALUATE hit", __func__);
> > > return;
> > > + }
> > >
> > > - if (oldstate == state && state == NEXTHOP_REACH) {
> > > + if (oldstate == state) {
> > > /*
> > > * The state of the nexthop did not change. The only
> > > * thing that may have changed is the true_nexthop
> > > * or other internal infos. This will not change
> > > * the routing decision so shortcut here.
> > > */
> > > - if ((re_rib(p->re)->flags & F_RIB_NOFIB) == 0 &&
> > > - p == p->re->active)
> > > - rde_send_kroute(re_rib(p->re), p, NULL);
> > > + 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);
> > > + }
> > > return;
> > > }
> > >
> > > @@ -1305,17 +1308,15 @@ 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)
> > > {
> > > - path_ref(asp);
> > > -
> > > - p->aspath = asp;
> > > - p->peer = peer;
> > > + p->aspath = path_ref(asp);
> > > p->communities = communities_get(comm);
> > > + p->peer = peer;
> > > + p->re = re;
> > > + p->validation_state = vstate;
> > > + p->nhflags = nhflags;
> > > p->nexthop = nexthop_ref(nexthop);
> > > nexthop_link(p);
> > > - p->re = re;
> > > p->lastchange = time(NULL);
> > > - p->nhflags = nhflags;
> > > - p->validation_state = vstate;
> > >
> > > /* make route decision */
> > > prefix_evaluate(p, re);
> > > @@ -1336,22 +1337,21 @@ prefix_unlink(struct prefix *p)
> > > LIST_REMOVE(p, rib_l);
> > > prefix_evaluate(NULL, re);
> > >
> > > - if (p->aspath)
> > > - path_unref(p->aspath);
> > > -
> > > - if (rib_empty(re))
> > > - rib_remove(re);
> > > -
> > > /* destroy all references to other objects */
> > > - communities_put(p->communities);
> > > nexthop_unlink(p);
> > > nexthop_put(p->nexthop);
> > > + communities_put(p->communities);
> > > + if (p->aspath)
> > > + path_unref(p->aspath);
> > > p->communities = NULL;
> > > p->nexthop = NULL;
> > > p->aspath = NULL;
> > > p->peer = NULL;
> > > p->re = NULL;
> > >
> > > + if (rib_empty(re))
> > > + rib_remove(re);
> > > +
> > > /*
> > > * It's the caller's duty to do accounting and remove empty aspath
> > > * structures. Also freeing the unlinked prefix is the caller's duty.
> > > @@ -1401,6 +1401,8 @@ struct nexthop_table {
> > >
> > > SIPHASH_KEY nexthoptablekey;
> > >
> > > +TAILQ_HEAD(nexthop_queue, nexthop) nexthop_runners;
> > > +
> > > void
> > > nexthop_init(u_int32_t hashsize)
> > > {
> > > @@ -1412,6 +1414,7 @@ nexthop_init(u_int32_t hashsize)
> > > if (nexthoptable.nexthop_hashtbl == NULL)
> > > fatal("nextop_init");
> > >
> > > + TAILQ_INIT(&nexthop_runners);
> > > for (i = 0; i < hs; i++)
> > > LIST_INIT(&nexthoptable.nexthop_hashtbl[i]);
> > > arc4random_buf(&nexthoptablekey, sizeof(nexthoptablekey));
> > > @@ -1443,12 +1446,45 @@ nexthop_shutdown(void)
> > > free(nexthoptable.nexthop_hashtbl);
> > > }
> > >
> > > +int
> > > +nexthop_pending(void)
> > > +{
> > > + return !TAILQ_EMPTY(&nexthop_runners);
> > > +}
> > > +
> > > +void
> > > +nexthop_runner(void)
> > > +{
> > > + struct nexthop *nh;
> > > + struct prefix *p;
> > > + u_int32_t j;
> > > +
> > > + nh = TAILQ_FIRST(&nexthop_runners);
> > > + if (nh == NULL)
> > > + return;
> > > +
> > > + /* remove from runnner queue */
> > > + TAILQ_REMOVE(&nexthop_runners, nh, runner_l);
> > > +
> > > + p = nh->next_prefix;
> > > + for (j = 0; p != NULL && j < RDE_RUNNER_ROUNDS; j++) {
> > > + prefix_updateall(p, nh->state, nh->oldstate);
> > > + p = LIST_NEXT(p, nexthop_l);
> > > + }
> > > +
> > > + /* prep for next run, if not finished readd to tail of queue */
> > > + nh->next_prefix = p;
> > > + if (p != NULL)
> > > + TAILQ_INSERT_TAIL(&nexthop_runners, nh, runner_l);
> > > + else
> > > + log_debug("nexthop %s update finished",
> > > + log_addr(&nh->exit_nexthop));
> > > +}
> > > +
> > > void
> > > nexthop_update(struct kroute_nexthop *msg)
> > > {
> > > struct nexthop *nh;
> > > - struct prefix *p;
> > > - enum nexthop_state oldstate;
> > >
> > > nh = nexthop_lookup(&msg->nexthop);
> > > if (nh == NULL) {
> > > @@ -1457,7 +1493,20 @@ nexthop_update(struct kroute_nexthop *ms
> > > return;
> > > }
> > >
> > > - oldstate = nh->state;
> > > + nh->oldstate = nh->state;
> > > + if (nh->oldstate == NEXTHOP_LOOKUP)
> > > + /* drop reference which was hold during the lookup */
> > > + if (nexthop_put(nh))
> > > + return; /* nh lost last ref, no work left */
> >
> > trailing whitespace
> >
> > > +
> > > + if (nh->next_prefix) {
> > > + /*
> > > + * If nexthop_runner() is not finished with this nexthop
> > > + * then ensure that all prefixes are updated by setting
> > > + * the oldstate to NEXTHOP_FLAPPED.
> > > + */
> > > + nh->oldstate = NEXTHOP_FLAPPED;
> > > + }
> > >
> > > if (msg->valid)
> > > nh->state = NEXTHOP_REACH;
> > > @@ -1476,21 +1525,16 @@ nexthop_update(struct kroute_nexthop *ms
> > > sizeof(nh->nexthop_net));
> > > nh->nexthop_netlen = msg->netlen;
> > >
> > > - if (oldstate == NEXTHOP_LOOKUP)
> > > - /* drop reference which was hold during the lookup */
> > > - if (nexthop_put(nh))
> > > - return;
> > > -
> > > if (rde_noevaluate())
> > > /*
> > > * if the decision process is turned off there is no need
> > > - * for the aspath list walk.
> > > + * for the prefix list walk.
> > > */
> > > return;
> > >
> > > - LIST_FOREACH(p, &nh->prefix_h, nexthop_l) {
> > > - prefix_updateall(p, nh->state, oldstate);
> > > - }
> > > + nh->next_prefix = LIST_FIRST(&nh->prefix_h);
> > > + TAILQ_INSERT_HEAD(&nexthop_runners, nh, runner_l);
> > > + log_debug("nexthop %s update starting", log_addr(&nh->exit_nexthop));
> > > }
> > >
> > > void
> > > @@ -1532,6 +1576,10 @@ nexthop_link(struct prefix *p)
> > > if (p->nexthop == NULL)
> > > return;
> > >
> > > + /* no need to link prefixes in RIBs that have no decision process */
> > > + if (re_rib(p->re)->flags & F_RIB_NOEVALUATE)
> > > + return;
> > > +
> > > LIST_INSERT_HEAD(&p->nexthop->prefix_h, p, nexthop_l);
> > > }
> > >
> > > @@ -1541,6 +1589,12 @@ nexthop_unlink(struct prefix *p)
> > > if (p->nexthop == NULL)
> > > return;
> > >
> > > + if (re_rib(p->re)->flags & F_RIB_NOEVALUATE)
> > > + return;
> > > +
> > > + if (p == p->nexthop->next_prefix)
> > > + p->nexthop->next_prefix = LIST_NEXT(p, nexthop_l);
> > > +
> > > LIST_REMOVE(p, nexthop_l);
> > > }
> > >
> > > @@ -1588,7 +1642,11 @@ nexthop_put(struct nexthop *nh)
> > >
> > > /* sanity check */
> > > if (!LIST_EMPTY(&nh->prefix_h) || nh->state == NEXTHOP_LOOKUP)
> > > - fatalx("nexthop_put: refcnt error");
> > > + fatalx("%s: refcnt error", __func__);
> > > +
> > > + /* is nexthop update running? impossible, that is a refcnt error */
> > > + if (nh->next_prefix)
> > > + fatalx("%s: next_prefix not NULL", __func__);
> > >
> > > LIST_REMOVE(nh, nexthop_l);
> > > rde_send_nexthop(&nh->exit_nexthop, 0);
> > >
> >
>
> --
> :wq Claudio
>
