From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: more churn, split PREFIX flags
To: tech@openbsd.org
Date: Thu, 20 Nov 2025 14:35:32 +0100

On Thu, Nov 20, 2025 at 01:57:28PM +0100, Claudio Jeker wrote:
> Split up the PREFIX flags, split the name space and use PREFIX_ADJOUT
> for those that only affect that struct.
> In the process retire PREFIX_FLAG_ADJOUT.

Reads fine. One question below which you can fix (or not) before commit.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> diff -u -p -r1.667 rde.c
> --- rde.c	20 Nov 2025 10:47:36 -0000	1.667
> +++ rde.c	20 Nov 2025 12:55:49 -0000
> @@ -3142,9 +3142,7 @@ rde_dump_adjout_upcall(struct prefix_adj
>  {
>  	struct rde_dump_ctx	*ctx = ptr;
>  
> -	if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
> -		fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
> -	if (p->flags & PREFIX_FLAG_WITHDRAW)
> +	if (p->flags & PREFIX_ADJOUT_FLAG_WITHDRAW)
>  		return;
>  	rde_dump_adjout_filter(p, &ctx->req);
>  }
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> diff -u -p -r1.322 rde.h
> --- rde.h	20 Nov 2025 10:47:36 -0000	1.322
> +++ rde.h	20 Nov 2025 12:55:49 -0000
> @@ -283,16 +283,9 @@ struct prefix {
>  	uint8_t			 nhflags;
>  	int8_t			 dmetric;	/* decision metric */
>  };
> -#define	PREFIX_FLAG_WITHDRAW	0x0001	/* enqueued on withdraw queue */
> -#define	PREFIX_FLAG_UPDATE	0x0002	/* enqueued on update queue */
> -#define	PREFIX_FLAG_DEAD	0x0004	/* locked but removed */
> -#define	PREFIX_FLAG_STALE	0x0008	/* stale entry (for addpath) */
> -#define	PREFIX_FLAG_MASK	0x000f	/* mask for the prefix types */
> -#define	PREFIX_FLAG_ADJOUT	0x0010	/* prefix is in the adj-out rib */
> -#define	PREFIX_FLAG_EOR		0x0020	/* prefix is EoR */
> -#define	PREFIX_NEXTHOP_LINKED	0x0040	/* prefix is linked onto nexthop list */
> -#define	PREFIX_FLAG_LOCKED	0x0080	/* locked by rib walker */
> -#define	PREFIX_FLAG_FILTERED	0x0100	/* prefix is filtered (ineligible) */
> +
> +#define	PREFIX_NEXTHOP_LINKED	0x01	/* prefix is linked onto nexthop list */
> +#define	PREFIX_FLAG_FILTERED	0x04	/* prefix is filtered (ineligible) */
>  
>  #define	PREFIX_DMETRIC_NONE	0
>  #define	PREFIX_DMETRIC_INVALID	1
> @@ -319,11 +312,18 @@ struct prefix_adjout {
>  	monotime_t			 lastchange;
>  	uint32_t			 path_id;
>  	uint32_t			 path_id_tx;
> -	uint16_t			 flags;
> +	uint8_t			 	 flags;
>  	uint8_t				 validation_state;
>  	uint8_t				 nhflags;
>  	int8_t				 dmetric;	/* decision metric */
>  };
> +#define	PREFIX_ADJOUT_FLAG_WITHDRAW	0x01	/* enqueued on withdraw queue */
> +#define	PREFIX_ADJOUT_FLAG_UPDATE	0x02	/* enqueued on update queue */
> +#define	PREFIX_ADJOUT_FLAG_DEAD		0x04	/* locked but removed */
> +#define	PREFIX_ADJOUT_FLAG_STALE	0x08	/* stale entry (for addpath) */
> +#define	PREFIX_ADJOUT_FLAG_MASK		0x0f	/* mask for the prefix types */
> +#define	PREFIX_ADJOUT_FLAG_EOR		0x10	/* prefix is EoR */
> +#define	PREFIX_ADJOUT_FLAG_LOCKED	0x20	/* locked by rib walker */
>  
>  struct filterstate {
>  	struct rde_aspath	 aspath;
> Index: rde_adjout.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_adjout.c,v
> diff -u -p -r1.4 rde_adjout.c
> --- rde_adjout.c	20 Nov 2025 10:47:36 -0000	1.4
> +++ rde_adjout.c	20 Nov 2025 12:55:49 -0000
> @@ -33,31 +33,31 @@
>  static inline struct prefix_adjout *
>  prefix_adjout_lock(struct prefix_adjout *p)
>  {
> -	if (p->flags & PREFIX_FLAG_LOCKED)
> +	if (p->flags & PREFIX_ADJOUT_FLAG_LOCKED)
>  		fatalx("%s: locking locked prefix", __func__);
> -	p->flags |= PREFIX_FLAG_LOCKED;
> +	p->flags |= PREFIX_ADJOUT_FLAG_LOCKED;
>  	return p;
>  }
>  
>  static inline struct prefix_adjout *
>  prefix_adjout_unlock(struct prefix_adjout *p)
>  {
> -	if ((p->flags & PREFIX_FLAG_LOCKED) == 0)
> +	if ((p->flags & PREFIX_ADJOUT_FLAG_LOCKED) == 0)
>  		fatalx("%s: unlocking unlocked prefix", __func__);
> -	p->flags &= ~PREFIX_FLAG_LOCKED;
> +	p->flags &= ~PREFIX_ADJOUT_FLAG_LOCKED;
>  	return p;
>  }
>  
>  static inline int
>  prefix_is_locked(struct prefix_adjout *p)
>  {
> -	return (p->flags & PREFIX_FLAG_LOCKED) != 0;
> +	return (p->flags & PREFIX_ADJOUT_FLAG_LOCKED) != 0;
>  }
>  
>  static inline int
>  prefix_is_dead(struct prefix_adjout *p)
>  {
> -	return (p->flags & PREFIX_FLAG_DEAD) != 0;
> +	return (p->flags & PREFIX_ADJOUT_FLAG_DEAD) != 0;
>  }
>  
>  static void	 prefix_adjout_link(struct prefix_adjout *, struct pt_entry *,
> @@ -88,10 +88,11 @@ prefix_index_cmp(struct prefix_adjout *a
>  static inline int
>  prefix_cmp(struct prefix_adjout *a, struct prefix_adjout *b)
>  {
> -	if ((a->flags & PREFIX_FLAG_EOR) != (b->flags & PREFIX_FLAG_EOR))
> -		return (a->flags & PREFIX_FLAG_EOR) ? 1 : -1;
> +	if ((a->flags & PREFIX_ADJOUT_FLAG_EOR) !=
> +	    (b->flags & PREFIX_ADJOUT_FLAG_EOR))
> +		return (a->flags & PREFIX_ADJOUT_FLAG_EOR) ? 1 : -1;
>  	/* if EOR marker no need to check the rest */
> -	if (a->flags & PREFIX_FLAG_EOR)
> +	if (a->flags & PREFIX_ADJOUT_FLAG_EOR)
>  		return 0;
>  
>  	if (a->aspath != b->aspath)
> @@ -209,7 +210,7 @@ prefix_add_eor(struct rde_peer *peer, ui
>  	struct prefix_adjout *p;
>  
>  	p = prefix_adjout_alloc();
> -	p->flags = PREFIX_FLAG_ADJOUT | PREFIX_FLAG_UPDATE | PREFIX_FLAG_EOR;
> +	p->flags = PREFIX_ADJOUT_FLAG_UPDATE | PREFIX_ADJOUT_FLAG_EOR;
>  	if (RB_INSERT(prefix_tree, &peer->updates[aid], p) != NULL)
>  		/* no need to add if EoR marker already present */
>  		prefix_adjout_free(p);
> @@ -229,7 +230,7 @@ prefix_adjout_update(struct prefix_adjou
>  	if (p == NULL) {
>  		p = prefix_adjout_alloc();
>  		/* initially mark DEAD so code below is skipped */
> -		p->flags |= PREFIX_FLAG_ADJOUT | PREFIX_FLAG_DEAD;
> +		p->flags |= PREFIX_ADJOUT_FLAG_DEAD;
>  
>  		p->pt = pt_ref(pte);
>  		p->peer = peer;
> @@ -239,9 +240,8 @@ prefix_adjout_update(struct prefix_adjou
>  			fatalx("%s: RB index invariant violated", __func__);
>  	}
>  
> -	if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
> -		fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
> -	if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0) {
> +	if ((p->flags & (PREFIX_ADJOUT_FLAG_WITHDRAW |
> +	    PREFIX_ADJOUT_FLAG_DEAD)) == 0) {
>  		/*
>  		 * XXX for now treat a different path_id_tx like different
>  		 * attributes and force out an update. It is unclear how
> @@ -257,12 +257,11 @@ prefix_adjout_update(struct prefix_adjou
>  			/* nothing changed */
>  			p->validation_state = state->vstate;
>  			p->lastchange = getmonotime();
> -			p->flags &= ~PREFIX_FLAG_STALE;

Maybe I'm missing something, but should this not do

			p->flags &= ~PREFIX_ADJOUT_FLAG_STALE;


>  			return;
>  		}
>  
>  		/* if pending update unhook it before it is unlinked */
> -		if (p->flags & PREFIX_FLAG_UPDATE) {
> +		if (p->flags & PREFIX_ADJOUT_FLAG_UPDATE) {
>  			RB_REMOVE(prefix_tree, &peer->updates[pte->aid], p);
>  			peer->stats.pending_update--;
>  		}
> @@ -271,13 +270,13 @@ prefix_adjout_update(struct prefix_adjou
>  		prefix_adjout_unlink(p);
>  		peer->stats.prefix_out_cnt--;
>  	}
> -	if (p->flags & PREFIX_FLAG_WITHDRAW) {
> +	if (p->flags & PREFIX_ADJOUT_FLAG_WITHDRAW) {
>  		RB_REMOVE(prefix_tree, &peer->withdraws[pte->aid], p);
>  		peer->stats.pending_withdraw--;
>  	}
>  
> -	/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
> -	p->flags &= ~PREFIX_FLAG_MASK;
> +	/* nothing needs to be done for PREFIX_ADJOUT_FLAG_DEAD and STALE */
> +	p->flags &= ~PREFIX_ADJOUT_FLAG_MASK;
>  
>  	/* update path_id_tx now that the prefix is unlinked */
>  	if (p->path_id_tx != path_id_tx) {
> @@ -299,10 +298,10 @@ prefix_adjout_update(struct prefix_adjou
>  	    state->nexthop, state->nhflags, state->vstate);
>  	peer->stats.prefix_out_cnt++;
>  
> -	if (p->flags & PREFIX_FLAG_MASK)
> +	if (p->flags & PREFIX_ADJOUT_FLAG_MASK)
>  		fatalx("%s: bad flags %x", __func__, p->flags);
>  	if (peer_is_up(peer)) {
> -		p->flags |= PREFIX_FLAG_UPDATE;
> +		p->flags |= PREFIX_ADJOUT_FLAG_UPDATE;
>  		if (RB_INSERT(prefix_tree, &peer->updates[pte->aid], p) != NULL)
>  			fatalx("%s: RB tree invariant violated", __func__);
>  		peer->stats.pending_update++;
> @@ -318,39 +317,37 @@ prefix_adjout_withdraw(struct prefix_adj
>  {
>  	struct rde_peer *peer = prefix_adjout_peer(p);
>  
> -	if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
> -		fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
> -
>  	/* already a withdraw, shortcut */
> -	if (p->flags & PREFIX_FLAG_WITHDRAW) {
> +	if (p->flags & PREFIX_ADJOUT_FLAG_WITHDRAW) {
>  		p->lastchange = getmonotime();
> -		p->flags &= ~PREFIX_FLAG_STALE;
> +		p->flags &= ~PREFIX_ADJOUT_FLAG_STALE;
>  		return;
>  	}
>  	/* pending update just got withdrawn */
> -	if (p->flags & PREFIX_FLAG_UPDATE) {
> +	if (p->flags & PREFIX_ADJOUT_FLAG_UPDATE) {
>  		RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
>  		peer->stats.pending_update--;
>  	}
>  	/* unlink prefix if it was linked (not a withdraw or dead) */
> -	if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0) {
> +	if ((p->flags & (PREFIX_ADJOUT_FLAG_WITHDRAW |
> +	    PREFIX_ADJOUT_FLAG_DEAD)) == 0) {
>  		prefix_adjout_unlink(p);
>  		peer->stats.prefix_out_cnt--;
>  	}
>  
> -	/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
> -	p->flags &= ~PREFIX_FLAG_MASK;
> +	/* nothing needs to be done for PREFIX_ADJOUT_FLAG_DEAD and STALE */
> +	p->flags &= ~PREFIX_ADJOUT_FLAG_MASK;
>  	p->lastchange = getmonotime();
>  
>  	if (peer_is_up(peer)) {
> -		p->flags |= PREFIX_FLAG_WITHDRAW;
> +		p->flags |= PREFIX_ADJOUT_FLAG_WITHDRAW;
>  		if (RB_INSERT(prefix_tree, &peer->withdraws[p->pt->aid],
>  		    p) != NULL)
>  			fatalx("%s: RB tree invariant violated", __func__);
>  		peer->stats.pending_withdraw++;
>  	} else {
>  		/* mark prefix dead to skip unlink on destroy */
> -		p->flags |= PREFIX_FLAG_DEAD;
> +		p->flags |= PREFIX_ADJOUT_FLAG_DEAD;
>  		prefix_adjout_destroy(p);
>  	}
>  }
> @@ -360,35 +357,33 @@ prefix_adjout_destroy(struct prefix_adjo
>  {
>  	struct rde_peer *peer = prefix_adjout_peer(p);
>  
> -	if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
> -		fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
> -
> -	if (p->flags & PREFIX_FLAG_EOR) {
> +	if (p->flags & PREFIX_ADJOUT_FLAG_EOR) {
>  		/* EOR marker is not linked in the index */
>  		prefix_adjout_free(p);
>  		return;
>  	}
>  
> -	if (p->flags & PREFIX_FLAG_WITHDRAW) {
> +	if (p->flags & PREFIX_ADJOUT_FLAG_WITHDRAW) {
>  		RB_REMOVE(prefix_tree, &peer->withdraws[p->pt->aid], p);
>  		peer->stats.pending_withdraw--;
>  	}
> -	if (p->flags & PREFIX_FLAG_UPDATE) {
> +	if (p->flags & PREFIX_ADJOUT_FLAG_UPDATE) {
>  		RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
>  		peer->stats.pending_update--;
>  	}
>  	/* unlink prefix if it was linked (not a withdraw or dead) */
> -	if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0) {
> +	if ((p->flags & (PREFIX_ADJOUT_FLAG_WITHDRAW |
> +	    PREFIX_ADJOUT_FLAG_DEAD)) == 0) {
>  		prefix_adjout_unlink(p);
>  		peer->stats.prefix_out_cnt--;
>  	}
>  
> -	/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
> -	p->flags &= ~PREFIX_FLAG_MASK;
> +	/* nothing needs to be done for PREFIX_ADJOUT_FLAG_DEAD and STALE */
> +	p->flags &= ~PREFIX_ADJOUT_FLAG_MASK;
>  
>  	if (prefix_is_locked(p)) {
>  		/* mark prefix dead but leave it for prefix_restart */
> -		p->flags |= PREFIX_FLAG_DEAD;
> +		p->flags |= PREFIX_ADJOUT_FLAG_DEAD;
>  	} else {
>  		RB_REMOVE(prefix_index, &peer->adj_rib_out, p);
>  		/* remove the last prefix reference before free */
> @@ -408,9 +403,9 @@ prefix_adjout_flush_pending(struct rde_p
>  			prefix_adjout_destroy(p);
>  		}
>  		RB_FOREACH_SAFE(p, prefix_tree, &peer->updates[aid], np) {
> -			p->flags &= ~PREFIX_FLAG_UPDATE;
> +			p->flags &= ~PREFIX_ADJOUT_FLAG_UPDATE;
>  			RB_REMOVE(prefix_tree, &peer->updates[aid], p);
> -			if (p->flags & PREFIX_FLAG_EOR) {
> +			if (p->flags & PREFIX_ADJOUT_FLAG_EOR) {
>  				prefix_adjout_destroy(p);
>  			} else {
>  				peer->stats.pending_update--;
> Index: rde_peer.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
> diff -u -p -r1.55 rde_peer.c
> --- rde_peer.c	20 Nov 2025 10:47:36 -0000	1.55
> +++ rde_peer.c	20 Nov 2025 12:55:49 -0000
> @@ -520,10 +520,10 @@ peer_blast_upcall(struct prefix_adjout *
>  {
>  	struct rde_peer		*peer;
>  
> -	if ((p->flags & PREFIX_FLAG_MASK) == 0) {
> +	if ((p->flags & PREFIX_ADJOUT_FLAG_MASK) == 0) {
>  		peer = prefix_adjout_peer(p);
>  		/* put entries on the update queue if not already on a queue */
> -		p->flags |= PREFIX_FLAG_UPDATE;
> +		p->flags |= PREFIX_ADJOUT_FLAG_UPDATE;
>  		if (RB_INSERT(prefix_tree, &peer->updates[p->pt->aid],
>  		    p) != NULL)
>  			fatalx("%s: RB tree invariant violated", __func__);
> Index: rde_rib.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> diff -u -p -r1.280 rde_rib.c
> --- rde_rib.c	20 Nov 2025 10:47:36 -0000	1.280
> +++ rde_rib.c	20 Nov 2025 12:55:49 -0000
> @@ -913,9 +913,6 @@ prefix_move(struct prefix *p, struct rde
>  {
>  	struct prefix		*np;
>  
> -	if (p->flags & PREFIX_FLAG_ADJOUT)
> -		fatalx("%s: prefix with PREFIX_FLAG_ADJOUT hit", __func__);
> -
>  	if (peer != prefix_peer(p))
>  		fatalx("prefix_move: cross peer move");
>  
> @@ -963,8 +960,6 @@ prefix_withdraw(struct rib *rib, struct 
>  	if (p == NULL)		/* Got a dummy withdrawn request. */
>  		return (0);
>  
> -	if (p->flags & PREFIX_FLAG_ADJOUT)
> -		fatalx("%s: prefix with PREFIX_FLAG_ADJOUT hit", __func__);
>  	asp = prefix_aspath(p);
>  	if (asp && asp->pftableid)
>  		/* only prefixes in the local RIB were pushed into pf */
> @@ -1319,12 +1314,6 @@ void
>  nexthop_link(struct prefix *p)
>  {
>  	p->nhflags &= ~NEXTHOP_VALID;
> -
> -	if (p->flags & PREFIX_FLAG_ADJOUT) {
> -		/* All nexthops are valid in Adj-RIB-Out */
> -		p->nhflags |= NEXTHOP_VALID;
> -		return;
> -	}
>  
>  	/* no need to link prefixes in RIBs that have no decision process */
>  	if (re_rib(prefix_re(p))->flags & F_RIB_NOEVALUATE)
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> diff -u -p -r1.181 rde_update.c
> --- rde_update.c	20 Nov 2025 10:47:36 -0000	1.181
> +++ rde_update.c	20 Nov 2025 12:55:49 -0000
> @@ -266,7 +266,7 @@ up_generate_addpath(struct rde_peer *pee
>  
>  	/* mark all paths as stale */
>  	for (p = head; p != NULL; p = prefix_adjout_next(peer, p))
> -		p->flags |= PREFIX_FLAG_STALE;
> +		p->flags |= PREFIX_ADJOUT_FLAG_STALE;
>  
>  	/* update paths */
>  	new = prefix_best(re);
> @@ -333,7 +333,7 @@ up_generate_addpath(struct rde_peer *pee
>  
>  	/* withdraw stale paths */
>  	for (p = head; p != NULL; p = prefix_adjout_next(peer, p)) {
> -		if (p->flags & PREFIX_FLAG_STALE)
> +		if (p->flags & PREFIX_ADJOUT_FLAG_STALE)
>  			prefix_adjout_withdraw(p);
>  	}
>  }
> @@ -797,13 +797,13 @@ up_is_eor(struct rde_peer *peer, uint8_t
>  	struct prefix_adjout *p;
>  
>  	p = RB_MIN(prefix_tree, &peer->updates[aid]);
> -	if (p != NULL && (p->flags & PREFIX_FLAG_EOR)) {
> +	if (p != NULL && (p->flags & PREFIX_ADJOUT_FLAG_EOR)) {
>  		/*
>  		 * Need to remove eor from update tree because
>  		 * prefix_adjout_destroy() can't handle that.
>  		 */
>  		RB_REMOVE(prefix_tree, &peer->updates[aid], p);
> -		p->flags &= ~PREFIX_FLAG_UPDATE;
> +		p->flags &= ~PREFIX_ADJOUT_FLAG_UPDATE;
>  		prefix_adjout_destroy(p);
>  		return 1;
>  	}
> @@ -824,7 +824,7 @@ up_prefix_free(struct prefix_tree *prefi
>  	} else {
>  		/* prefix still in Adj-RIB-Out, keep it */
>  		RB_REMOVE(prefix_tree, prefix_head, p);
> -		p->flags &= ~PREFIX_FLAG_UPDATE;
> +		p->flags &= ~PREFIX_ADJOUT_FLAG_UPDATE;
>  		peer->stats.pending_update--;
>  		peer->stats.prefix_sent_update++;
>  	}
> @@ -856,7 +856,7 @@ up_dump_prefix(struct ibuf *buf, struct 
>  		    np->communities != p->communities ||
>  		    np->nexthop != p->nexthop ||
>  		    np->nhflags != p->nhflags ||
> -		    (np->flags & PREFIX_FLAG_EOR))
> +		    (np->flags & PREFIX_ADJOUT_FLAG_EOR))
>  			done = 1;
>  
>  		rv = 0;
>