Index | Thread | Search

From:
Theo Buehler <tb@theobuehler.org>
Subject:
Re: bgpd: rework the force_update logic in adjout
To:
tech@openbsd.org
Date:
Wed, 15 Jul 2026 13:25:49 +0200

Download raw body.

Thread
On Wed, Jul 15, 2026 at 12:02:39PM +0200, Claudio Jeker wrote:
> On initial connect the system needs to sync the adj-rib-out of a peer with
> the Loc-RIB. This happens via peer_dump() and in this case the system
> needs to push out all updates (but can drop any withdraw).
> 
> The other user of peer_dump() is during config reloads where the RIB of a
> peer is altered. In that case the adj-rib-out is first flushed
> (withdrawing all paths) and then refilled via peer_dump(). Again in that
> case all updates should be pushed out and there should be no withdraw at
> all since the table was just flushed.
> 
> To do this a force_update flag was introduced but actually it is better to
> use the mode and introduce EVAL_SYNC for this case.
> If mode == EVAL_SYNC adjout_prefix_update needs to force the update and
> adjout_prefix_withdraw can drop the withdraw.

I like this.

> While there also remove the NOTYET bits in peer_generate_update() and
> replace them with a simple hack inside up_generate_addpath_all()
> to just call up_generate_addpath().
> 
> This is another stepping stone to bring back the add-path send all
> optimisation.

ok tb

> -- 
> :wq Claudio
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> diff -u -p -r1.708 rde.c
> --- rde.c	1 Jul 2026 09:53:47 -0000	1.708
> +++ rde.c	15 Jul 2026 09:43:03 -0000
> @@ -3671,7 +3671,7 @@ rde_up_flush_upcall(struct pt_entry *pte
>  {
>  	struct rde_peer *peer = ptr;
>  
> -	adjout_prefix_withdraw(peer, pte, p);
> +	adjout_prefix_withdraw(peer, pte, p, 0);
>  }
>  
>  int
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> diff -u -p -r1.357 rde.h
> --- rde.h	13 Jul 2026 12:27:34 -0000	1.357
> +++ rde.h	15 Jul 2026 09:32:29 -0000
> @@ -359,6 +359,7 @@ struct filterstate {
>  
>  enum eval_mode {
>  	EVAL_NONE,
> +	EVAL_SYNC,
>  	EVAL_REEVAL,
>  	EVAL_DEFAULT,
>  	EVAL_ALL,
> @@ -777,7 +778,7 @@ struct adjout_prefix	*adjout_prefix_next
>  void		 adjout_prefix_update(struct adjout_prefix *, struct rde_peer *,
>  		    struct filterstate *, struct pt_entry *, uint32_t, int);
>  void		 adjout_prefix_withdraw(struct rde_peer *, struct pt_entry *,
> -		    struct adjout_prefix *);
> +		    struct adjout_prefix *, int);
>  void		 adjout_prefix_reaper(struct rde_peer *);
>  void		 adjout_prefix_dump_cleanup(struct rib_context *);
>  void		 adjout_prefix_dump_r(struct rib_context *);
> @@ -808,9 +809,12 @@ void		 pend_prefix_stats(struct ch_stats
>  void		 adjout_attr_stats(struct ch_stats *);
>  
>  /* rde_update.c */
> -void	 up_generate_updates(struct rde_peer *, struct rib_entry *, int);
> -void	 up_generate_addpath(struct rde_peer *, struct rib_entry *, int);
> -void	 up_generate_addpath_all(struct rde_peer *, struct rib_entry *, int);
> +void	 up_generate_updates(struct rde_peer *, struct rib_entry *,
> +	    enum eval_mode);
> +void	 up_generate_addpath(struct rde_peer *, struct rib_entry *,
> +	    enum eval_mode);
> +void	 up_generate_addpath_all(struct rde_peer *, struct rib_entry *,
> +	    enum eval_mode);
>  void	 up_generate_default(struct rde_peer *, uint8_t);
>  int	 up_is_eor(struct rde_peer *, uint8_t);
>  void	 up_dump_withdraws(struct imsgbuf *, struct rde_peer *, uint8_t);
> Index: rde_adjout.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_adjout.c,v
> diff -u -p -r1.21 rde_adjout.c
> --- rde_adjout.c	13 Jul 2026 12:27:34 -0000	1.21
> +++ rde_adjout.c	15 Jul 2026 09:32:29 -0000
> @@ -516,11 +516,12 @@ adjout_prefix_next(struct pt_entry *pte,
>  
>  /*
>   * Put a prefix from the Adj-RIB-Out onto the update queue.
> + * If force_send is set, ensure an UPDATE is sent in any case to the peer.
>   */
>  void
>  adjout_prefix_update(struct adjout_prefix *p, struct rde_peer *peer,
>      struct filterstate *state, struct pt_entry *pte, uint32_t path_id_tx,
> -    int force_update)
> +    int force_send)
>  {
>  	struct adjout_attr *attrs;
>  
> @@ -542,7 +543,7 @@ adjout_prefix_update(struct adjout_prefi
>  		    attrs->communities) &&
>  		    path_equal(&state->aspath, attrs->aspath)) {
>  			/* nothing changed */
> -			if (force_update && peer_is_up(peer))
> +			if (force_send && peer_is_up(peer))
>  				pend_prefix_add(peer, attrs, pte, path_id_tx);
>  			return;
>  		}
> @@ -563,15 +564,16 @@ adjout_prefix_update(struct adjout_prefi
>  /*
>   * Withdraw a prefix from the Adj-RIB-Out, this unlinks the aspath but leaves
>   * the prefix in the RIB linked to the peer withdraw list.
> + * If skip_send is set there is no need to send a withdraw to the peer.
>   */
>  void
>  adjout_prefix_withdraw(struct rde_peer *peer, struct pt_entry *pte,
> -    struct adjout_prefix *p)
> +    struct adjout_prefix *p, int skip_send)
>  {
>  	if (bitmap_test(&p->peermap, peer->adjout_bid) == 0)
>  		fatalx("%s: king bula is unhappy", __func__);
>  
> -	if (peer_is_up(peer))
> +	if (peer_is_up(peer) && !skip_send)
>  		pend_prefix_add(peer, NULL, pte, p->path_id_tx);
>  
>  	adjout_prefix_unlink(p, pte, peer);
> Index: rde_peer.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
> diff -u -p -r1.79 rde_peer.c
> --- rde_peer.c	13 Jul 2026 12:27:34 -0000	1.79
> +++ rde_peer.c	15 Jul 2026 09:36:22 -0000
> @@ -244,7 +244,7 @@ RB_GENERATE(peer_tree, rde_peer, entry, 
>  
>  static void
>  peer_generate_update(struct rde_peer *peer, struct rib_entry *re,
> -    enum eval_mode mode, int force_update)
> +    enum eval_mode mode)
>  {
>  	uint8_t		 aid;
>  
> @@ -269,26 +269,17 @@ peer_generate_update(struct rde_peer *pe
>  
>  	/* handle peers with add-path */
>  	if (peer_has_add_path(peer, aid, CAPA_AP_SEND)) {
> -#if NOTYET
> -		/* XXX skip addpath all optimisation for now until the queue
> -		 * is extended to allow this mode to work again.
> -		 * This mode is not very common and so taking the slow path
> -		 * here like for all other addpath send modes is not that
> -		 * big of an issue right now.
> -		 */
> -		if (peer->eval.mode == ADDPATH_EVAL_ALL) {
> -			up_generate_addpath_all(peer, re, force_update);
> -			return;
> -		}
> -#endif
> -		up_generate_addpath(peer, re, force_update);
> +		if (peer->eval.mode == ADDPATH_EVAL_ALL)
> +			up_generate_addpath_all(peer, re, mode);
> +		else
> +			up_generate_addpath(peer, re, mode);
>  		return;
>  	}
>  
>  	/* skip regular peers if the best path didn't change */
>  	if (mode == EVAL_ALL && (peer->flags & PEERFLAG_EVALUATE_ALL) == 0)
>  		return;
> -	up_generate_updates(peer, re, force_update);
> +	up_generate_updates(peer, re, mode);
>  }
>  
>  /*
> @@ -298,8 +289,8 @@ peer_generate_update(struct rde_peer *pe
>   *	EVAL_ALL is sent for any other update (needed for peers with
>   *	addpath or evaluate all set).
>   *	EVAL_REEVAL is used by config reloads (a full RIB refresh is needed)
> - *	and for single peer RIB dumps but those call peer_generate_update()
> - *	directly.
> + *	EVAL_SYNC is used for single peer RIB dumps but those call
> + *	peer_generate_update() directly.
>   * peer, newpath and old_pathid_tx are ignored if mode is EVAL_REEVAL.
>   * In the other cases either newpath or old_pathid_tx are valid but not
>   * both at the same time.
> @@ -317,13 +308,14 @@ rde_enqueue_updates(struct rib_entry *re
>  		RB_FOREACH(p, peer_tree, &peertable) {
>  			if (p->reconf_out == 0)
>  				continue;
> -			peer_generate_update(p, re, EVAL_REEVAL, 0);
> +			peer_generate_update(p, re, mode);
>  		}
>  		adjout_prefix_collect(re->prefix);
>  		return;
>  	case EVAL_DEFAULT:
>  	case EVAL_ALL:
>  		break;
> +	case EVAL_SYNC:
>  	case EVAL_NONE:
>  		fatalx("bad eval mode in %s", __func__);
>  	}
> @@ -354,7 +346,7 @@ peer_process_updates(struct rde_peer *pe
>  	peer->stats.rib_entry_count--;
>  
>  	RB_FOREACH(p, peer_tree, &peertable)
> -		peer_generate_update(p, re, re->pq_mode, 0);
> +		peer_generate_update(p, re, re->pq_mode);
>  
>  	adjout_prefix_collect(re->prefix);
>  	rib_pq_dequeue(re);
> @@ -505,7 +497,7 @@ peer_reaper_upcall(struct pt_entry *pte,
>  {
>  	struct rde_peer		*peer = ptr;
>  
> -	adjout_prefix_withdraw(peer, pte, p);
> +	adjout_prefix_withdraw(peer, pte, p, 1);
>  }
>  
>  /*
> @@ -656,7 +648,7 @@ peer_dump_upcall(struct rib_entry *re, v
>  		/* no eligible prefix, not even for 'evaluate all' */
>  		return;
>  
> -	peer_generate_update(peer, re, EVAL_REEVAL, 1);
> +	peer_generate_update(peer, re, EVAL_SYNC);
>  	adjout_prefix_collect(re->prefix);
>  }
>  
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> diff -u -p -r1.200 rde_update.c
> --- rde_update.c	13 Jul 2026 12:27:34 -0000	1.200
> +++ rde_update.c	15 Jul 2026 10:00:42 -0000
> @@ -159,7 +159,7 @@ up_enforce_open_policy(struct rde_peer *
>   */
>  static enum up_state
>  up_process_prefix(struct rde_peer *peer, struct prefix *new,
> -    struct adjout_prefix *p, int force_update)
> +    struct adjout_prefix *p, enum eval_mode mode)
>  {
>  	struct filterstate state;
>  	struct bgpd_addr addr;
> @@ -202,7 +202,7 @@ up_process_prefix(struct rde_peer *peer,
>  
>  	up_prep_adjout(peer, &state, new->pt->aid);
>  	adjout_prefix_update(p, peer, &state, new->pt, new_path_id_tx,
> -	    force_update);
> +	    mode == EVAL_SYNC);
>  	rde_filterstate_clean(&state);
>  
>  	/* max prefix checker outbound */
> @@ -221,7 +221,7 @@ up_process_prefix(struct rde_peer *peer,
>  
>  void
>  up_generate_updates(struct rde_peer *peer, struct rib_entry *re,
> -    int force_update)
> +    enum eval_mode mode)
>  {
>  	struct prefix		*new;
>  	struct adjout_prefix	*p;
> @@ -230,7 +230,7 @@ up_generate_updates(struct rde_peer *pee
>  
>  	new = prefix_best(re);
>  	while (new != NULL) {
> -		switch (up_process_prefix(peer, new, p, force_update)) {
> +		switch (up_process_prefix(peer, new, p, mode)) {
>  		case UP_OK:
>  		case UP_ERR_LIMIT:
>  			return;
> @@ -249,7 +249,7 @@ up_generate_updates(struct rde_peer *pee
>  done:
>  	/* withdraw prefix */
>  	if (p != NULL)
> -		adjout_prefix_withdraw(peer, re->prefix, p);
> +		adjout_prefix_withdraw(peer, re->prefix, p, mode == EVAL_SYNC);
>  }
>  
>  /*
> @@ -261,7 +261,7 @@ done:
>   */
>  void
>  up_generate_addpath(struct rde_peer *peer, struct rib_entry *re,
> -    int force_update)
> +    enum eval_mode mode)
>  {
>  	struct prefix		*new;
>  	struct adjout_prefix	*head, *p;
> @@ -322,8 +322,7 @@ up_generate_addpath(struct rde_peer *pee
>  		if (extra != 0 && extrapaths >= peer->eval.extrapaths)
>  			break;
>  
> -		switch (up_process_prefix(peer, new, (void *)-1,
> -		    force_update)) {
> +		switch (up_process_prefix(peer, new, (void *)-1, mode)) {
>  		case UP_OK:
>  			maxpaths++;
>  			extrapaths += extra;
> @@ -354,7 +353,8 @@ up_generate_addpath(struct rde_peer *pee
>  			p = adjout_prefix_get(peer, addpath_prefix_list[i],
>  			    re->prefix);
>  			if (p != NULL)
> -				adjout_prefix_withdraw(peer, re->prefix, p);
> +				adjout_prefix_withdraw(peer, re->prefix, p,
> +				    mode == EVAL_SYNC);
>  		}
>  	}
>  }
> @@ -365,7 +365,7 @@ up_generate_addpath(struct rde_peer *pee
>   */
>  void
>  up_generate_addpath_all(struct rde_peer *peer, struct rib_entry *re,
> -    int force_update)
> +    enum eval_mode mode)
>  {
>  #if 0
>  	/* XXX needs to be rewritten */
> @@ -376,7 +376,7 @@ up_generate_addpath_all(struct rde_peer 
>  	 * use up_generate_addpath() for that.
>  	 */
>  	if (old_pathid_tx == 0 && new == NULL) {
> -		up_generate_addpath(peer, re, force_update);
> +		up_generate_addpath(peer, re, mode);
>  		return;
>  	}
>  
> @@ -387,8 +387,7 @@ up_generate_addpath_all(struct rde_peer 
>  
>  	if (new != NULL) {
>  		/* add new path */
> -		switch (up_process_prefix(peer, new, (void *)-1,
> -		    force_update)) {
> +		switch (up_process_prefix(peer, new, (void *)-1, mode)) {
>  		case UP_OK:
>  			/* don't remove old if an existing prefix was updated */
>  			if (old_pathid_tx == new->path_id_tx)
> @@ -407,8 +406,11 @@ up_generate_addpath_all(struct rde_peer 
>  		/* withdraw old path */
>  		p = adjout_prefix_get(peer, old_pathid_tx, re->prefix);
>  		if (p != NULL)
> -			adjout_prefix_withdraw(peer, re->prefix, p);
> +			adjout_prefix_withdraw(peer, re->prefix, p,
> +			    mode == EVAL_SYNC);
>  	}
> +#else
> +	up_generate_addpath(peer, re, mode);
>  #endif
>  }
>  
>