On 2012/07/01 10:54, Florian Obser wrote:
> Hi,
> after a very brief reading of ospfd sources I think I understand
> why it touches the route on the external interface. (This might be
> entirely wrong, but I think it does its spf dance and then puts the
> best routes into the fib wether they changed or not.)
>
> In any case since I understand ospfd a lot less then bgpd so I decided
> to change bgpd by figuring out if the nexthop actually changed or not.
>
> There is already this comment in prefix_updateall:
> /*
> * 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.
> */
>
> The following patch compares true_nexthop, flags, etc with the old
> values and skips prefix_updateall if noting has changed.
>
> This is either obviously correct or horribly wrong - but it fixes the
> memory consumption in rde :)
I don't know enough about the internal design to know whether it's
right but it makes enough sense that I'm going to try running one
machine with it anyway ;)
It does seem to me that ospfd should probably not update the fib
in this situation however whatever happens there, this is a fairly
cheap check which saves bgpd doing extra work at a time when it's
already heavily loaded, so if it's not *wrong* then it seems useful
to have. At the time when bgpd is handling bulk route messages it's
fairly likely to suffer an overflow on the route socket so cutting
processing time for these is good.
> Btw. I'm not sure about the style for the sizeof(oldtrue_nexthop) in
> the memcmp call, should this be indented by an additional tab?
Not an additional tab, just the four spaces is right. I don't think
style(9) covers it explicitly but I think it looks a bit odd with
the operators at the start of the line so maybe something like this
(I also renamed the variables, s/nexthop/nh, to get things to fit
on fewer lines). (Lots of context in my diff to make it easier to
see where it fits in without going back to the original file).
? .todo
? kroute.c-desync
? leakfinder.shar
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.133
diff -u -p -u -1 -3 -r1.133 rde_rib.c
--- rde_rib.c 1 Jul 2012 11:55:13 -0000 1.133
+++ rde_rib.c 1 Jul 2012 12:21:12 -0000
@@ -1046,63 +1046,75 @@ nexthop_shutdown(void)
(void)nexthop_delete(nh);
}
if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i]))
log_warnx("nexthop_shutdown: non-free table");
}
free(nexthoptable.nexthop_hashtbl);
}
void
nexthop_update(struct kroute_nexthop *msg)
{
struct nexthop *nh;
+ struct bgpd_addr oldtrue_nh, oldnh_net;
struct rde_aspath *asp;
enum nexthop_state oldstate;
+ u_int8_t oldflags, oldnh_netlen;
nh = nexthop_lookup(&msg->nexthop);
if (nh == NULL) {
log_warnx("nexthop_update: non-existent nexthop %s",
log_addr(&msg->nexthop));
return;
}
oldstate = nh->state;
+ oldflags = nh->flags;
+ oldnh_netlen = nh->nexthop_netlen;
+ memcpy(&oldtrue_nh, &nh->true_nexthop, sizeof(oldtrue_nh));
+ memcpy(&oldnh_net, &nh->nexthop_net, sizeof(oldnh_net));
if (msg->valid)
nh->state = NEXTHOP_REACH;
else
nh->state = NEXTHOP_UNREACH;
if (msg->connected) {
nh->flags |= NEXTHOP_CONNECTED;
memcpy(&nh->true_nexthop, &nh->exit_nexthop,
sizeof(nh->true_nexthop));
} else
memcpy(&nh->true_nexthop, &msg->gateway,
sizeof(nh->true_nexthop));
memcpy(&nh->nexthop_net, &msg->net,
sizeof(nh->nexthop_net));
nh->nexthop_netlen = msg->netlen;
if (nexthop_delete(nh))
/* nexthop no longer used */
return;
if (rde_noevaluate())
/*
* if the decision process is turned off there is no need
* for the aspath list walk.
*/
+ return;
+
+ if (oldstate == nh->state && oldflags == nh->flags &&
+ memcmp(&oldtrue_nh, &nh->true_nexthop, sizeof(oldtrue_nh)) == 0 &&
+ memcmp(&oldnh_net, &nh->nexthop_net, sizeof(oldnh_net)) == 0)
+ /* nexthop didn't change */
return;
LIST_FOREACH(asp, &nh->path_h, nexthop_l) {
prefix_updateall(asp, nh->state, oldstate);
}
}
void
nexthop_modify(struct rde_aspath *asp, struct bgpd_addr *nexthop,
enum action_types type, u_int8_t aid)
{
struct nexthop *nh;
