Stuart Henderson([email protected]) on 2012.07.01 13:39:48 +0100:
> 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 ;)

Hi,

i am running this on 3 machines now and it fixes the memory problem i was
seeing.

/Benno

> 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.



> ? .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;
> 

-- 

Reply via email to