Re: bgpd refactor route decision process

2021-01-13 Thread Claudio Jeker
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

2021-01-13 Thread Denis Fondras
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@