Re: bgpd: refactor update generation a bit
On Wed, Jul 06, 2022 at 06:56:28PM +0200, Claudio Jeker wrote:
> On Wed, Jul 06, 2022 at 06:15:45PM +0200, Theo Buehler wrote:
> > On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> > > This diff changes various loops which call into up_generate_update() so
> > > that all these loops call the same function peer_generate_update() which
> > > then calls up_generate_update(). This is a step to add an alternative path
> > > to generate updates for add-path send support without altering many
> > > code-paths.
> > >
> > > If I did not fool myself this should not alter current behaviour.
> > > rde_up_dump_upcall() gets a few more checks but all those checks should be
> > > false in this case (e.g. peer_dump checks the export_type already).
> >
> > This looks good and I agree that the behavior is mostly preserved. I
> > noticed are two changes of behavior and I have a small question for
> > rde_up_dump_upcall():
> >
> > > @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> > > struct rde_peer *peer = ptr;
> > > struct prefix *p;
> > >
> > > - if (peer->state != PEER_UP)
> > > - return;
> > > - if (re->rib_id != peer->loc_rib_id)
> > > - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> > > - peer->loc_rib_id);
> >
> > Failure of this check will now be silent.
>
> This is fine, that code path was acting as an assert().
> rde_up_dump_upcall() is called by a RIB table walk for the
> peer->loc_rib_id RIB (see peer_dump()) so there should be no way that
> if (re->rib_id != peer->loc_rib_id) is true.
Indeed.
>
> > > - if (peer->capa.mp[re->prefix->aid] == 0)
> > > - fatalx("%s: Unexpected %s prefix", __func__,
> > > - aid2str(re->prefix->aid));
> >
> > Same here.
>
> Similar reason. The rib walks each address family independent:
> for (i = 0; i < AID_MAX; i++) {
> if (peer->capa.mp[i])
> peer_dump(peer, i);
> }
>
> Now this check is missing in the IMSG_REFRESH case in rde.c but other than
> that there should be no way to reach this code with a wrong aid.
> Skipping routes with non-negotiated address families should not matter
> here but it will make the code more robust.
>
Makes sense.
>
> > I got lost when checking that re->prefix->aid == p->pt->aid. I assume
> > that follows from the definition of the rib_entry's prefix queue?
>
> If the prefix p is on the list of prefixes in re (part of re->prefix_h)
> then re->prefix == p->pt and so the aid check also needs to match.
> We cache the struct pt_entry * in a few places to simplify access.
> Since the code uses prefix_best(re) to get the p we know that p is part of
> the re list of prefixes.
All these comments made things a lot clearer. Thanks
>
> > > -
> > > /* no eligible prefix, not even for 'evaluate all' */
> > > if ((p = prefix_best(re)) == NULL)
> > > return;
> > > -
> > > - up_generate_updates(out_rules, peer, p, NULL);
> > > + peer_generate_update(peer, re->rib_id, p, NULL, 0);
> > > }
> > >
> > > static void
>
> Updated diff below that adds the extra aid check in the IMSG_REFRESH case.
ok tb
>
> --
> :wq Claudio
>
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.547
> diff -u -p -r1.547 rde.c
> --- rde.c 27 Jun 2022 13:26:51 - 1.547
> +++ rde.c 6 Jul 2022 16:43:22 -
> @@ -1123,12 +1123,19 @@ rde_dispatch_imsg_peer(struct rde_peer *
> break;
> case IMSG_REFRESH:
> if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(rr)) {
> - log_warnx("%s: wrong imsg len", __func__);
> + log_warnx("route refresh: wrong imsg len");
> break;
> }
> memcpy(, imsg.data, sizeof(rr));
> if (rr.aid >= AID_MAX) {
> - log_warnx("%s: bad AID", __func__);
> + log_peer_warnx(>conf,
> + "route refresh: bad AID %d", rr.aid);
> + break;
> + }
> + if (peer->capa.mp[rr.aid]) {
> + log_peer_warnx(>conf,
> + "route refresh: AID %s not negotiated",
> + aid2str(rr.aid));
> break;
> }
> switch (rr.subtype) {
> @@ -1156,7 +1163,8 @@ rde_dispatch_imsg_peer(struct rde_peer *
> aid2str(rr.aid));
> break;
> default:
> - log_warnx("%s: bad subtype %d", __func__, rr.subtype);
> + log_peer_warnx(>conf,
> + "route refresh: bad subtype %d", rr.subtype);
> break;
> }
> break;
> @@ -3037,59 +3045,6 @@ rde_evaluate_all(void)
> return
Re: bgpd: refactor update generation a bit
On Wed, Jul 06, 2022 at 06:15:45PM +0200, Theo Buehler wrote:
> On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> > This diff changes various loops which call into up_generate_update() so
> > that all these loops call the same function peer_generate_update() which
> > then calls up_generate_update(). This is a step to add an alternative path
> > to generate updates for add-path send support without altering many
> > code-paths.
> >
> > If I did not fool myself this should not alter current behaviour.
> > rde_up_dump_upcall() gets a few more checks but all those checks should be
> > false in this case (e.g. peer_dump checks the export_type already).
>
> This looks good and I agree that the behavior is mostly preserved. I
> noticed are two changes of behavior and I have a small question for
> rde_up_dump_upcall():
>
> > @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> > struct rde_peer *peer = ptr;
> > struct prefix *p;
> >
> > - if (peer->state != PEER_UP)
> > - return;
> > - if (re->rib_id != peer->loc_rib_id)
> > - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> > - peer->loc_rib_id);
>
> Failure of this check will now be silent.
This is fine, that code path was acting as an assert().
rde_up_dump_upcall() is called by a RIB table walk for the
peer->loc_rib_id RIB (see peer_dump()) so there should be no way that
if (re->rib_id != peer->loc_rib_id) is true.
> > - if (peer->capa.mp[re->prefix->aid] == 0)
> > - fatalx("%s: Unexpected %s prefix", __func__,
> > - aid2str(re->prefix->aid));
>
> Same here.
Similar reason. The rib walks each address family independent:
for (i = 0; i < AID_MAX; i++) {
if (peer->capa.mp[i])
peer_dump(peer, i);
}
Now this check is missing in the IMSG_REFRESH case in rde.c but other than
that there should be no way to reach this code with a wrong aid.
Skipping routes with non-negotiated address families should not matter
here but it will make the code more robust.
> I got lost when checking that re->prefix->aid == p->pt->aid. I assume
> that follows from the definition of the rib_entry's prefix queue?
If the prefix p is on the list of prefixes in re (part of re->prefix_h)
then re->prefix == p->pt and so the aid check also needs to match.
We cache the struct pt_entry * in a few places to simplify access.
Since the code uses prefix_best(re) to get the p we know that p is part of
the re list of prefixes.
> > -
> > /* no eligible prefix, not even for 'evaluate all' */
> > if ((p = prefix_best(re)) == NULL)
> > return;
> > -
> > - up_generate_updates(out_rules, peer, p, NULL);
> > + peer_generate_update(peer, re->rib_id, p, NULL, 0);
> > }
> >
> > static void
Updated diff below that adds the extra aid check in the IMSG_REFRESH case.
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.547
diff -u -p -r1.547 rde.c
--- rde.c 27 Jun 2022 13:26:51 - 1.547
+++ rde.c 6 Jul 2022 16:43:22 -
@@ -1123,12 +1123,19 @@ rde_dispatch_imsg_peer(struct rde_peer *
break;
case IMSG_REFRESH:
if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(rr)) {
- log_warnx("%s: wrong imsg len", __func__);
+ log_warnx("route refresh: wrong imsg len");
break;
}
memcpy(, imsg.data, sizeof(rr));
if (rr.aid >= AID_MAX) {
- log_warnx("%s: bad AID", __func__);
+ log_peer_warnx(>conf,
+ "route refresh: bad AID %d", rr.aid);
+ break;
+ }
+ if (peer->capa.mp[rr.aid]) {
+ log_peer_warnx(>conf,
+ "route refresh: AID %s not negotiated",
+ aid2str(rr.aid));
break;
}
switch (rr.subtype) {
@@ -1156,7 +1163,8 @@ rde_dispatch_imsg_peer(struct rde_peer *
aid2str(rr.aid));
break;
default:
- log_warnx("%s: bad subtype %d", __func__, rr.subtype);
+ log_peer_warnx(>conf,
+ "route refresh: bad subtype %d", rr.subtype);
break;
}
break;
@@ -3037,59 +3045,6 @@ rde_evaluate_all(void)
return rde_eval_all;
}
-static int
-rde_skip_peer(struct rde_peer *peer, uint16_t rib_id, uint8_t aid)
-{
- /* skip ourself */
- if (peer == peerself)
- return 1;
- if (peer->state != PEER_UP)
- return 1;
- /* skip peers using a different rib */
-
Re: bgpd: refactor update generation a bit
On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> This diff changes various loops which call into up_generate_update() so
> that all these loops call the same function peer_generate_update() which
> then calls up_generate_update(). This is a step to add an alternative path
> to generate updates for add-path send support without altering many
> code-paths.
>
> If I did not fool myself this should not alter current behaviour.
> rde_up_dump_upcall() gets a few more checks but all those checks should be
> false in this case (e.g. peer_dump checks the export_type already).
This looks good and I agree that the behavior is mostly preserved. I
noticed are two changes of behavior and I have a small question for
rde_up_dump_upcall():
> @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> struct rde_peer *peer = ptr;
> struct prefix *p;
>
> - if (peer->state != PEER_UP)
> - return;
> - if (re->rib_id != peer->loc_rib_id)
> - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> - peer->loc_rib_id);
Failure of this check will now be silent.
> - if (peer->capa.mp[re->prefix->aid] == 0)
> - fatalx("%s: Unexpected %s prefix", __func__,
> - aid2str(re->prefix->aid));
Same here.
I got lost when checking that re->prefix->aid == p->pt->aid. I assume
that follows from the definition of the rib_entry's prefix queue?
> -
> /* no eligible prefix, not even for 'evaluate all' */
> if ((p = prefix_best(re)) == NULL)
> return;
> -
> - up_generate_updates(out_rules, peer, p, NULL);
> + peer_generate_update(peer, re->rib_id, p, NULL, 0);
> }
>
> static void
bgpd: refactor update generation a bit
This diff changes various loops which call into up_generate_update() so
that all these loops call the same function peer_generate_update() which
then calls up_generate_update(). This is a step to add an alternative path
to generate updates for add-path send support without altering many
code-paths.
If I did not fool myself this should not alter current behaviour.
rde_up_dump_upcall() gets a few more checks but all those checks should be
false in this case (e.g. peer_dump checks the export_type already).
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.547
diff -u -p -r1.547 rde.c
--- rde.c 27 Jun 2022 13:26:51 - 1.547
+++ rde.c 28 Jun 2022 19:38:42 -
@@ -3037,59 +3037,6 @@ rde_evaluate_all(void)
return rde_eval_all;
}
-static int
-rde_skip_peer(struct rde_peer *peer, uint16_t rib_id, uint8_t aid)
-{
- /* skip ourself */
- if (peer == peerself)
- return 1;
- if (peer->state != PEER_UP)
- return 1;
- /* skip peers using a different rib */
- if (peer->loc_rib_id != rib_id)
- return 1;
- /* check if peer actually supports the address family */
- if (peer->capa.mp[aid] == 0)
- return 1;
- /* skip peers with special export types */
- if (peer->export_type == EXPORT_NONE ||
- peer->export_type == EXPORT_DEFAULT_ROUTE)
- return 1;
-
- return 0;
-}
-
-void
-rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old,
-int eval_all)
-{
- struct rde_peer *peer;
- uint8_t aid;
-
- /*
-* If old is != NULL we know it was active and should be removed.
-* If new is != NULL we know it is reachable and then we should
-* generate an update.
-*/
- if (old == NULL && new == NULL)
- return;
-
- if (new)
- aid = new->pt->aid;
- else
- aid = old->pt->aid;
-
- LIST_FOREACH(peer, , peer_l) {
- if (rde_skip_peer(peer, rib->id, aid))
- continue;
- /* skip regular peers if the best path didn't change */
- if ((peer->flags & PEERFLAG_EVALUATE_ALL) == 0 && eval_all)
- continue;
-
- up_generate_updates(out_rules, peer, new, old);
- }
-}
-
/* flush Adj-RIB-Out by withdrawing all prefixes */
static void
rde_up_flush_upcall(struct prefix *p, void *ptr)
@@ -3760,26 +3707,16 @@ rde_softreconfig_in(struct rib_entry *re
}
static void
-rde_softreconfig_out(struct rib_entry *re, void *bula)
+rde_softreconfig_out(struct rib_entry *re, void *arg)
{
- struct prefix *p;
- struct rde_peer *peer;
- uint8_t aid = re->prefix->aid;
+ struct rib *rib = arg;
+ struct prefix *p;
if ((p = prefix_best(re)) == NULL)
/* no valid path for prefix */
return;
- LIST_FOREACH(peer, , peer_l) {
- if (rde_skip_peer(peer, re->rib_id, aid))
- continue;
- /* skip peers which don't need to reconfigure */
- if (peer->reconf_out == 0)
- continue;
-
- /* Regenerate all updates. */
- up_generate_updates(out_rules, peer, p, p);
- }
+ rde_generate_updates(rib, p, NULL, EVAL_RECONF);
}
static void
Index: rde.h
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.254
diff -u -p -r1.254 rde.h
--- rde.h 27 Jun 2022 13:26:51 - 1.254
+++ rde.h 6 Jul 2022 14:53:36 -
@@ -362,6 +362,12 @@ struct filterstate {
uint8_t nhflags;
};
+enum eval_mode {
+ EVAL_DEFAULT,
+ EVAL_ALL,
+ EVAL_RECONF,
+};
+
extern struct rde_memstats rdemem;
/* prototypes */
@@ -384,7 +390,7 @@ voidrde_pftable_del(uint16_t, struct p
intrde_evaluate_all(void);
void rde_generate_updates(struct rib *, struct prefix *,
- struct prefix *, int);
+ struct prefix *, enum eval_mode);
uint32_t rde_local_as(void);
intrde_decisionflags(void);
void rde_peer_send_rrefresh(struct rde_peer *, uint8_t, uint8_t);
Index: rde_decide.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
retrieving revision 1.91
diff -u -p -r1.91 rde_decide.c
--- rde_decide.c22 Mar 2022 10:53:08 - 1.91
+++ rde_decide.c6 Jul 2022 14:54:58 -
@@ -493,7 +493,7 @@ prefix_evaluate(struct rib_entry *re, st
* but remember that xp may be NULL aka ineligible.
* Additional decision may be made by
