Re: bgpd refactor route decision process
On Wed, Jan 13, 2021 at 11:24:32AM +0100, Denis Fondras wrote:
> Le Tue, Jan 12, 2021 at 05:39:02PM +0100, Claudio Jeker a écrit :
> > This diff changes two things:
> > - First, it move the kroute update into rde_generate_updates() simplifying
> > prefix_evaluate a little bit.
> >
> > - Second, it changes prefix_evaluate to take an additional argument for the
> > old prefix (to be removed). Instead of doing this outside of
> > prefix_evaluate() with some drawbacks in case the same prefix is removed
> > and readded, the code is now in prefix_evaluate() and does all the magic
> > itself.
> >
> > Index: rde_decide.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
> > retrieving revision 1.78
> > diff -u -p -r1.78 rde_decide.c
> > --- rde_decide.c9 Aug 2019 13:44:27 - 1.78
> > +++ rde_decide.c12 Jan 2021 16:24:36 -
> > @@ -238,14 +238,16 @@ prefix_cmp(struct prefix *p1, struct pre
> > * The to evaluate prefix must not be in the prefix list.
> > */
> > void
> > -prefix_evaluate(struct prefix *p, struct rib_entry *re)
> > +prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix
> > *old)
> > {
> > struct prefix *xp;
> >
> > if (re_rib(re)->flags & F_RIB_NOEVALUATE) {
> > /* decision process is turned off */
> > - if (p != NULL)
> > - LIST_INSERT_HEAD(>prefix_h, p, entry.list.rib);
> > + if (old != NULL)
> > + LIST_REMOVE(old, entry.list.rib);
> > + if (new != NULL)
> > + LIST_INSERT_HEAD(>prefix_h, new, entry.list.rib);
>
> Would it be beneficial to have a p == new test ?
You mean old == new? Not sure if it is worth the trouble.
Currently this has the benefit that that most recent update is at the head
of the list. Now one could argue that this code is supposed to be as fast
as possible and so skipping the remove & insert could be benefitial.
I'm currently after a bigger issue so I'm happy to leave this for others
:)
>
> Otherwise OK denis@
>
--
:wq Claudio
Re: bgpd refactor route decision process
Le Tue, Jan 12, 2021 at 05:39:02PM +0100, Claudio Jeker a écrit :
> This diff changes two things:
> - First, it move the kroute update into rde_generate_updates() simplifying
> prefix_evaluate a little bit.
>
> - Second, it changes prefix_evaluate to take an additional argument for the
> old prefix (to be removed). Instead of doing this outside of
> prefix_evaluate() with some drawbacks in case the same prefix is removed
> and readded, the code is now in prefix_evaluate() and does all the magic
> itself.
>
> Index: rde_decide.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 rde_decide.c
> --- rde_decide.c 9 Aug 2019 13:44:27 - 1.78
> +++ rde_decide.c 12 Jan 2021 16:24:36 -
> @@ -238,14 +238,16 @@ prefix_cmp(struct prefix *p1, struct pre
> * The to evaluate prefix must not be in the prefix list.
> */
> void
> -prefix_evaluate(struct prefix *p, struct rib_entry *re)
> +prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix *old)
> {
> struct prefix *xp;
>
> if (re_rib(re)->flags & F_RIB_NOEVALUATE) {
> /* decision process is turned off */
> - if (p != NULL)
> - LIST_INSERT_HEAD(>prefix_h, p, entry.list.rib);
> + if (old != NULL)
> + LIST_REMOVE(old, entry.list.rib);
> + if (new != NULL)
> + LIST_INSERT_HEAD(>prefix_h, new, entry.list.rib);
Would it be beneficial to have a p == new test ?
Otherwise OK denis@
bgpd refactor route decision process
This diff changes two things:
- First, it move the kroute update into rde_generate_updates() simplifying
prefix_evaluate a little bit.
- Second, it changes prefix_evaluate to take an additional argument for the
old prefix (to be removed). Instead of doing this outside of
prefix_evaluate() with some drawbacks in case the same prefix is removed
and readded, the code is now in prefix_evaluate() and does all the magic
itself.
This is a necessary step to finally fix MED sorting.
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.511
diff -u -p -r1.511 rde.c
--- rde.c 9 Jan 2021 16:49:41 - 1.511
+++ rde.c 12 Jan 2021 16:17:31 -
@@ -2825,6 +2825,9 @@ rde_generate_updates(struct rib *rib, st
if (old == NULL && new == NULL)
return;
+ if ((rib->flags & F_RIB_NOFIB) == 0)
+ rde_send_kroute(rib, new, old);
+
if (new)
aid = new->pt->aid;
else
@@ -3533,8 +3536,7 @@ rde_softreconfig_sync_reeval(struct rib_
/* need to re-link the nexthop if not already linked */
if ((p->flags & PREFIX_NEXTHOP_LINKED) == 0)
nexthop_link(p);
- LIST_REMOVE(p, entry.list.rib);
- prefix_evaluate(p, re);
+ prefix_evaluate(re, p, p);
}
}
Index: rde.h
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.235
diff -u -p -r1.235 rde.h
--- rde.h 4 Dec 2020 11:57:13 - 1.235
+++ rde.h 12 Jan 2021 16:17:31 -
@@ -483,10 +483,10 @@ communities_unref(struct rde_community *
communities_unlink(comm);
}
-int community_to_rd(struct community *, u_int64_t *);
+intcommunity_to_rd(struct community *, u_int64_t *);
/* rde_decide.c */
-voidprefix_evaluate(struct prefix *, struct rib_entry *);
+void prefix_evaluate(struct rib_entry *, struct prefix *, struct prefix *);
/* rde_filter.c */
void rde_apply_set(struct filter_set_head *, struct rde_peer *,
Index: rde_decide.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
retrieving revision 1.78
diff -u -p -r1.78 rde_decide.c
--- rde_decide.c9 Aug 2019 13:44:27 - 1.78
+++ rde_decide.c12 Jan 2021 16:24:36 -
@@ -238,14 +238,16 @@ prefix_cmp(struct prefix *p1, struct pre
* The to evaluate prefix must not be in the prefix list.
*/
void
-prefix_evaluate(struct prefix *p, struct rib_entry *re)
+prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix *old)
{
struct prefix *xp;
if (re_rib(re)->flags & F_RIB_NOEVALUATE) {
/* decision process is turned off */
- if (p != NULL)
- LIST_INSERT_HEAD(>prefix_h, p, entry.list.rib);
+ if (old != NULL)
+ LIST_REMOVE(old, entry.list.rib);
+ if (new != NULL)
+ LIST_INSERT_HEAD(>prefix_h, new, entry.list.rib);
if (re->active) {
/*
* During reloads it is possible that the decision
@@ -259,19 +261,22 @@ prefix_evaluate(struct prefix *p, struct
return;
}
- if (p != NULL) {
+ if (old != NULL)
+ LIST_REMOVE(old, entry.list.rib);
+
+ if (new != NULL) {
if (LIST_EMPTY(>prefix_h))
- LIST_INSERT_HEAD(>prefix_h, p, entry.list.rib);
+ LIST_INSERT_HEAD(>prefix_h, new, entry.list.rib);
else {
LIST_FOREACH(xp, >prefix_h, entry.list.rib) {
- if (prefix_cmp(p, xp) > 0) {
- LIST_INSERT_BEFORE(xp, p,
+ if (prefix_cmp(new, xp) > 0) {
+ LIST_INSERT_BEFORE(xp, new,
entry.list.rib);
break;
} else if (LIST_NEXT(xp, entry.list.rib) ==
NULL) {
/* if xp last element ... */
- LIST_INSERT_AFTER(xp, p,
+ LIST_INSERT_AFTER(xp, new,
entry.list.rib);
break;
}
@@ -290,18 +295,17 @@ prefix_evaluate(struct prefix *p, struct
xp = NULL;
}
- if (re->active != xp) {
- /* need to generate an update */
-
+ /*
+* If the active prefix changed or the active prefix was removed
+* and
