From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: kill some adj-rib-out flags
To: tech@openbsd.org
Date: Thu, 11 Dec 2025 18:11:16 +0100

On Thu, Dec 11, 2025 at 05:29:20PM +0100, Claudio Jeker wrote:
> PREFIX_ADJOUT_FLAG_DEAD is no longer needed and can be replaced with
> a check that the attrs pointer is NULL. Refactor the code now a bit
> since the logic got a bit simpler.

ok tb

> This leaves PREFIX_ADJOUT_FLAG_STALE and PREFIX_ADJOUT_FLAG_LOCKED.
> PREFIX_ADJOUT_FLAG_STALE will die sooner than later and in a subsequent
> step PREFIX_ADJOUT_FLAG_LOCKED will follow the dodo.

great :)

> 
> -- 
> :wq Claudio
> 
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> diff -u -p -r1.329 rde.h
> --- rde.h	10 Dec 2025 12:36:51 -0000	1.329
> +++ rde.h	11 Dec 2025 16:24:28 -0000
> @@ -324,9 +324,7 @@ struct adjout_prefix {
>  	uint32_t			 path_id_tx;
>  	uint8_t			 	 flags;
>  };
> -#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_STALE	0x01	/* stale entry (for addpath) */
>  #define	PREFIX_ADJOUT_FLAG_LOCKED	0x20	/* locked by rib walker */
>  
>  struct pend_attr {
> Index: rde_adjout.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_adjout.c,v
> diff -u -p -r1.9 rde_adjout.c
> --- rde_adjout.c	10 Dec 2025 12:36:51 -0000	1.9
> +++ rde_adjout.c	11 Dec 2025 16:24:28 -0000
> @@ -412,7 +412,7 @@ prefix_is_locked(struct adjout_prefix *p
>  static inline int
>  prefix_is_dead(struct adjout_prefix *p)
>  {
> -	return (p->flags & PREFIX_ADJOUT_FLAG_DEAD) != 0;
> +	return p->attrs == NULL;
>  }
>  
>  static void	 adjout_prefix_link(struct adjout_prefix *, struct rde_peer *,
> @@ -545,16 +545,13 @@ adjout_prefix_update(struct adjout_prefi
>  	if (p == NULL) {
>  		p = adjout_prefix_alloc();
>  		/* initially mark DEAD so code below is skipped */
> -		p->flags |= PREFIX_ADJOUT_FLAG_DEAD;
>  
>  		p->pt = pt_ref(pte);
>  		p->path_id_tx = path_id_tx;
>  
>  		if (RB_INSERT(prefix_index, &peer->adj_rib_out, p) != NULL)
>  			fatalx("%s: RB index invariant violated", __func__);
> -	}
> -
> -	if ((p->flags & PREFIX_ADJOUT_FLAG_DEAD) == 0) {
> +	} else {
>  		/*
>  		 * XXX for now treat a different path_id_tx like different
>  		 * attributes and force out an update. It is unclear how
> @@ -577,8 +574,8 @@ adjout_prefix_update(struct adjout_prefi
>  		peer->stats.prefix_out_cnt--;
>  	}
>  
> -	/* nothing needs to be done for PREFIX_ADJOUT_FLAG_DEAD and STALE */
> -	p->flags &= ~PREFIX_ADJOUT_FLAG_MASK;
> +	/* clear PREFIX_ADJOUT_FLAG_STALE for up_generate_addpath() */
> +	p->flags &= ~PREFIX_ADJOUT_FLAG_STALE;
>  
>  	/* update path_id_tx now that the prefix is unlinked */
>  	if (p->path_id_tx != path_id_tx) {
> @@ -594,11 +591,8 @@ adjout_prefix_update(struct adjout_prefi
>  	adjout_prefix_link(p, peer, attrs, p->pt, p->path_id_tx);
>  	peer->stats.prefix_out_cnt++;
>  
> -	if (p->flags & PREFIX_ADJOUT_FLAG_MASK)
> -		fatalx("%s: bad flags %x", __func__, p->flags);
> -	if (peer_is_up(peer)) {
> +	if (peer_is_up(peer))
>  		pend_prefix_add(peer, p->attrs, p->pt, p->path_id_tx);
> -	}
>  }
>  
>  /*
> @@ -618,18 +612,15 @@ void
>  adjout_prefix_destroy(struct rde_peer *peer, struct adjout_prefix *p)
>  {
>  	/* unlink prefix if it was linked (not dead) */
> -	if ((p->flags & PREFIX_ADJOUT_FLAG_DEAD) == 0) {
> +	if (!prefix_is_dead(p)) {
>  		adjout_prefix_unlink(p, peer);
>  		peer->stats.prefix_out_cnt--;
>  	}
>  
> -	/* nothing needs to be done for PREFIX_ADJOUT_FLAG_DEAD and STALE */
> -	p->flags &= ~PREFIX_ADJOUT_FLAG_MASK;
> +	/* clear PREFIX_ADJOUT_FLAG_STALE just in case */
> +	p->flags &= ~PREFIX_ADJOUT_FLAG_STALE;
>  
> -	if (prefix_is_locked(p)) {
> -		/* mark prefix dead but leave it for prefix_restart */
> -		p->flags |= PREFIX_ADJOUT_FLAG_DEAD;
> -	} else {
> +	if (!prefix_is_locked(p)) {
>  		RB_REMOVE(prefix_index, &peer->adj_rib_out, p);
>  		/* remove the last prefix reference before free */
>  		pt_unref(p->pt);
>