bgpd: loc-rib uptime moved to bgp_path_info_extra and set in header

moved loc-rib uptime field "bgp_rib_uptime" to struct bgp_path_info_extra for memory concerns
moved logic into bgp_route_update's callback bmp_route_update
written timestamp in per peer header

Signed-off-by: Maxence Younsi <mx.yns@outlook.fr>

6 files changed, 158 insertions, 108 deletions

diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c
index 68057bcba..a6cd915ab 100644
--- a/bgpd/bgp_bmp.c
+++ b/bgpd/bgp_bmp.c
@@ -250,24 +250,40 @@ static inline uint64_t bmp_get_peer_distinguisher(struct bmp *bmp, afi_t afi,
 {
 
 	/* remove this check when the other peer types get correct peer dist.
-	// (RFC7854) impl. */
+	 *(RFC7854) impl.
+	 */
 	if (peer_type != BMP_PEER_TYPE_LOC_RIB_INSTANCE)
 		return 0;
 
 	/* sending vrf_id or rd could be turned into an option at some point */
 	struct bgp *bgp = bmp->targets->bgp;
-	struct prefix_rd *prd = &bgp->vpn_policy[afi].tovpn_rd;
-	/*
-	 * default vrf => can't have RD => 0
+
+	/*  default vrf => can't have RD => 0
 	 * vrf => has RD?
 	 *		if yes => use RD value
-	 *		else => use vrf_id and convert it so that
-	 * peer_distinguisher is 0::vrf_id
+	 *		else => use vrf_id
+	 *			vrf_id == VRF_UNKNOWN ?
+	 *				if yes => 0
+	 *				else => convert it so that
+	 *				peer_distinguisher is 0::vrf_id
 	 */
-	return bgp->inst_type == VRF_DEFAULT
-		       ? 0
-		       : prd ? *(uint64_t *)prd->val
-			     : (((uint64_t)htonl(bgp->vrf_id)) << 32);
+	if (bgp->inst_type == VRF_DEFAULT)
+		return 0;
+
+	struct prefix_rd *prd = &bgp->vpn_policy[afi].tovpn_rd;
+
+	if (CHECK_FLAG(bgp->vpn_policy[afi].flags,
+		       BGP_VPN_POLICY_TOVPN_RD_SET)) {
+		uint64_t peerd = 0;
+
+		memcpy(&peerd, prd->val, sizeof(prd->val));
+		return peerd;
+	}
+
+	if (bgp->vrf_id == VRF_UNKNOWN)
+		return 0;
+
+	return ((uint64_t)htonl(bgp->vrf_id)) << 32;
 }
 
 static void bmp_common_hdr(struct stream *s, uint8_t ver, uint8_t type)
@@ -304,7 +320,8 @@ static void bmp_per_peer_hdr(struct stream *s, struct bgp *bgp,
 
 	/* Peer Address */
 	/* Set to 0 if it's a LOC-RIB INSTANCE (RFC 9069) or if it's not an
-	 * IPv4/6 address */
+	 * IPv4/6 address
+	 */
 	if (is_locrib || (peer->connection->su.sa.sa_family != AF_INET6 &&
 			  peer->connection->su.sa.sa_family != AF_INET)) {
 		stream_putl(s, 0);
@@ -322,20 +339,23 @@ static void bmp_per_peer_hdr(struct stream *s, struct bgp *bgp,
 
 	/* Peer AS */
 	/* set peer ASN but for LOC-RIB INSTANCE (RFC 9069) put the local bgp
-	 * ASN if available or 0 */
+	 * ASN if available or 0
+	 */
 	as_t asn = !is_locrib ? peer->as : bgp ? bgp->as : 0L;
+
 	stream_putl(s, asn);
 
 	/* Peer BGP ID */
 	/* set router-id but for LOC-RIB INSTANCE (RFC 9069) put the instance
-	 * router-id if available or 0 */
+	 * router-id if available or 0
+	 */
 	struct in_addr *bgp_id =
 		!is_locrib ? &peer->remote_id : bgp ? &bgp->router_id : NULL;
+
 	stream_put_in_addr(s, bgp_id);
 
 	/* Timestamp */
-	/* set to 0 for LOC-RIB INSTANCE as install uptime is not saved atm */
-	if (!is_locrib && tv) {
+	if (tv) {
 		stream_putl(s, tv->tv_sec);
 		stream_putl(s, tv->tv_usec);
 	} else {
@@ -361,6 +381,7 @@ bmp_put_vrftablename_info_tlv(struct stream *s, struct bmp *bmp)
 	const char *vrftablename = "global";
 	if (bmp->targets->bgp->inst_type != BGP_INSTANCE_TYPE_DEFAULT) {
 		struct vrf *vrf = vrf_lookup_by_id(bmp->targets->bgp->vrf_id);
+
 		vrftablename = vrf ? vrf->name : NULL;
 	}
 	if (vrftablename != NULL)
@@ -715,8 +736,7 @@ static bool bmp_wrmirror(struct bmp *bmp, struct pullwr *pullwr)
 
 	bmp_common_hdr(s, BMP_VERSION_3, BMP_TYPE_ROUTE_MIRRORING);
 	bmp_per_peer_hdr(s, bmp->targets->bgp, peer, 0,
-			 BMP_PEER_TYPE_GLOBAL_INSTANCE, 0,
-			 &bmq->tv);
+			 BMP_PEER_TYPE_GLOBAL_INSTANCE, 0, &bmq->tv);
 
 	/* BMP Mirror TLV. */
 	stream_putw(s, BMP_MIRROR_TLV_TYPE_BGP_MESSAGE);
@@ -995,7 +1015,7 @@ static void bmp_monitor(struct bmp *bmp, struct peer *peer, uint8_t flags,
 	bmp_common_hdr(hdr, BMP_VERSION_3, BMP_TYPE_ROUTE_MONITORING);
 	bmp_per_peer_hdr(hdr, bmp->targets->bgp, peer, flags, peer_type_flag,
 			 bmp_get_peer_distinguisher(bmp, afi, peer_type_flag),
-			 &uptime_real);
+			 uptime == (time_t)(-1L) ? NULL : &uptime_real);
 
 	stream_putl_at(hdr, BMP_LENGTH_POS,
 			stream_get_endp(hdr) + stream_get_endp(msg));
@@ -1180,16 +1200,17 @@ afibreak:
 		prd = (struct prefix_rd *)bgp_dest_get_prefix(bmp->syncrdpos);
 
 	if (bpi && CHECK_FLAG(bpi->flags, BGP_PATH_SELECTED) &&
-	     CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_LOC_RIB)) {
+	    CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_LOC_RIB)) {
 		bmp_monitor(bmp, bpi->peer, 0, BMP_PEER_TYPE_LOC_RIB_INSTANCE,
-			    bn_p, prd, bpi->attr, afi, safi, bpi->rib_uptime);
+			    bn_p, prd, bpi->attr, afi, safi,
+			    bpi && bpi->extra ? bpi->extra->bgp_rib_uptime
+					      : (time_t)(-1L));
 	}
 
 	if (bpi && CHECK_FLAG(bpi->flags, BGP_PATH_VALID) &&
 	    CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_POSTPOLICY))
 		bmp_monitor(bmp, bpi->peer, BMP_PEER_FLAG_L,
-			    BMP_PEER_TYPE_GLOBAL_INSTANCE, bn_p, prd,
-			    bpi->attr,
+			    BMP_PEER_TYPE_GLOBAL_INSTANCE, bn_p, prd, bpi->attr,
 			    afi, safi, bpi->uptime);
 
 	if (adjin)
@@ -1253,9 +1274,8 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
 	afi_t afi = bqe->afi;
 	safi_t safi = bqe->safi;
 
-	if (!CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_LOC_RIB)) {
+	if (!CHECK_FLAG(bmp->targets->afimon[afi][safi], BMP_MON_LOC_RIB))
 		goto out;
-	}
 
 	switch (bmp->afistate[afi][safi]) {
 	case BMP_AFI_INACTIVE:
@@ -1280,11 +1300,13 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
 
 	peer = QOBJ_GET_TYPESAFE(bqe->peerid, peer);
 	if (!peer) {
-		// skipping queued item for deleted peer
+		/* skipping queued item for deleted peer
+		 */
 		goto out;
 	}
 	if (peer != bmp->targets->bgp->peer_self && !peer_established(peer->connection)) {
-		// peer is neither self, nor established
+		/* peer is neither self, nor established
+		 */
 		goto out;
 	}
 
@@ -1292,10 +1314,12 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
 		      (bqe->safi == SAFI_MPLS_VPN);
 
 	struct prefix_rd *prd = is_vpn ? &bqe->rd : NULL;
+
 	bn = bgp_safi_node_lookup(bmp->targets->bgp->rib[afi][safi], safi,
-				 &bqe->p, prd);
+				  &bqe->p, prd);
 
 	struct bgp_path_info *bpi;
+
 	for (bpi = bn ? bgp_dest_get_bgp_path_info(bn) : NULL; bpi;
 	     bpi = bpi->next) {
 		if (!CHECK_FLAG(bpi->flags, BGP_PATH_SELECTED))
@@ -1306,7 +1330,8 @@ static bool bmp_wrqueue_locrib(struct bmp *bmp, struct pullwr *pullwr)
 
 	bmp_monitor(bmp, peer, 0, BMP_PEER_TYPE_LOC_RIB_INSTANCE, &bqe->p, prd,
 		    bpi ? bpi->attr : NULL, afi, safi,
-		    bpi ? bpi->rib_uptime : monotime(NULL));
+		    bpi && bpi->extra ? bpi->extra->bgp_rib_uptime
+				      : (time_t)(-1L));
 	written = true;
 
 out:
@@ -1358,7 +1383,6 @@ static bool bmp_wrqueue(struct bmp *bmp, struct pullwr *pullwr)
 	if (!peer_established(peer->connection))
 		goto out;
 
-
 	bool is_vpn = (bqe->afi == AFI_L2VPN && bqe->safi == SAFI_EVPN) ||
 		      (bqe->safi == SAFI_MPLS_VPN);
 
@@ -1484,7 +1508,8 @@ bmp_process_one(struct bmp_targets *bt, struct bmp_qhash_head *updhash,
 	return bqe;
 
 	/* need to update correct queue pos for all sessions of the target after
-	 * a call to this function */
+	 * a call to this function
+	 */
 }
 
 static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
@@ -1507,7 +1532,8 @@ static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
 
 	frr_each(bmp_targets, &bmpbgp->targets, bt) {
 		/* check if any monitoring is enabled (ignoring loc-rib since it
-		 * uses another hook & queue */
+		 * uses another hook & queue
+		 */
 		if (!(bt->afimon[afi][safi] & ~BMP_MON_LOC_RIB))
 			continue;
 
@@ -1515,8 +1541,9 @@ static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
 			bmp_process_one(bt, &bt->updhash, &bt->updlist, bgp,
 					afi, safi, bn, peer);
 
-		if (!last_item) // if bmp_process_one returns NULL we don't have
-				// anything to do next
+		// if bmp_process_one returns NULL
+		// we don't have anything to do next
+		if (!last_item)
 			continue;
 
 		frr_each(bmp_session, &bt->sessions, bmp) {
@@ -1562,8 +1589,7 @@ static void bmp_stats(struct event *thread)
 		s = stream_new(BGP_MAX_PACKET_SIZE);
 		bmp_common_hdr(s, BMP_VERSION_3, BMP_TYPE_STATISTICS_REPORT);
 		bmp_per_peer_hdr(s, bt->bgp, peer, 0,
-				 BMP_PEER_TYPE_GLOBAL_INSTANCE, 0,
-				 &tv);
+				 BMP_PEER_TYPE_GLOBAL_INSTANCE, 0, &tv);
 
 		count_pos = stream_get_endp(s);
 		stream_putl(s, 0);
@@ -2436,9 +2462,9 @@ DEFPY(bmp_monitor_cfg, bmp_monitor_cmd,
 	argv_find_and_parse_afi(argv, argc, &index, &afi);
 	argv_find_and_parse_safi(argv, argc, &index, &safi);
 
-	if (policy[0] == 'l') {
+	if (policy[0] == 'l')
 		flag = BMP_MON_LOC_RIB;
-	} else if (policy[1] == 'r')
+	else if (policy[1] == 'r')
 		flag = BMP_MON_PREPOLICY;
 	else
 		flag = BMP_MON_POSTPOLICY;
@@ -2772,23 +2798,49 @@ static int bgp_bmp_init(struct event_loop *tm)
 /* TODO remove "bn", redundant with updated_route->net ? */
 static int bmp_route_update(struct bgp *bgp, afi_t afi, safi_t safi,
 			    struct bgp_dest *bn,
-			    struct bgp_path_info *updated_route, bool withdraw)
+			    struct bgp_path_info *old_route,
+			    struct bgp_path_info *new_route)
 {
-
+	bool is_locribmon_enabled = false;
+	bool is_withdraw = old_route && !new_route;
+	struct bgp_path_info *updated_route =
+		is_withdraw ? old_route : new_route;
 	struct bmp_bgp *bmpbgp = bmp_bgp_get(bgp);
 	struct peer *peer = updated_route->peer;
 	struct bmp_targets *bt;
 	struct bmp *bmp;
 
 	frr_each (bmp_targets, &bmpbgp->targets, bt) {
+		if ((is_locribmon_enabled |=
+		     (bt->afimon[afi][safi] & BMP_MON_LOC_RIB)))
+			break;
+	}
+
+	if (!is_locribmon_enabled)
+		return 0;
+
+	/* route is not installed in locrib anymore and rib uptime was saved */
+	if (old_route && old_route->extra)
+		bgp_path_info_extra_get(old_route)->bgp_rib_uptime =
+			(time_t)(-1L);
+
+	/* route is installed in locrib from now on so
+	 * save rib uptime in bgp_path_info_extra
+	 */
+	if (new_route)
+		bgp_path_info_extra_get(new_route)->bgp_rib_uptime =
+			monotime(NULL);
+
+	frr_each (bmp_targets, &bmpbgp->targets, bt) {
 		if (bt->afimon[afi][safi] & BMP_MON_LOC_RIB) {
 
 			struct bmp_queue_entry *last_item = bmp_process_one(
 				bt, &bt->locupdhash, &bt->locupdlist, bgp, afi,
 				safi, bn, peer);
 
-			if (!last_item) // if bmp_process_one returns NULL we
-					// don't have anything to do next
+			// if bmp_process_one returns NULL
+			// we don't have anything to do next
+			if (!last_item)
 				continue;
 
 			frr_each (bmp_session, &bt->sessions, bmp) {
diff --git a/bgpd/bgp_bmp.h b/bgpd/bgp_bmp.h
index 152e7eb0e..dadd99eb6 100644
--- a/bgpd/bgp_bmp.h
+++ b/bgpd/bgp_bmp.h
@@ -216,15 +216,14 @@ struct bmp_targets {
 	int stat_msec;
 
 	/* only supporting:
-	 * - IPv4 / unicast & multicast
-	 * - IPv6 / unicast & multicast
+	 * - IPv4 / unicast & multicast & VPN
+	 * - IPv6 / unicast & multicast & VPN
 	 * - L2VPN / EVPN
 	 */
 #define BMP_MON_PREPOLICY	(1 << 0)
 #define BMP_MON_POSTPOLICY	(1 << 1)
-
-/* TODO define BMP_MON_LOC_RIB flag */
 #define BMP_MON_LOC_RIB (1 << 2)
+
 	uint8_t afimon[AFI_MAX][SAFI_MAX];
 	bool mirror;
 
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index d8a87c1fa..560da893f 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -87,8 +87,8 @@ DEFINE_HOOK(bgp_rpki_prefix_status,
 
 DEFINE_HOOK(bgp_route_update,
 	    (struct bgp *bgp, afi_t afi, safi_t safi, struct bgp_dest *bn,
-	     struct bgp_path_info *updated_route, bool withdraw),
-	    (bgp, afi, safi, bn, updated_route, withdraw));
+	     struct bgp_path_info *old_route, struct bgp_path_info *new_route),
+	    (bgp, afi, safi, bn, old_route, new_route));
 
 /* Extern from bgp_dump.c */
 extern const char *bgp_origin_str[];
@@ -3454,17 +3454,11 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest,
 	}
 
 	/* call bmp hook for loc-rib route update / withdraw after flags were
-	 * set */
+	 * set
+	 */
 	if (old_select || new_select) {
-
-		if (old_select) /* route is not installed in locrib anymore */
-			old_select->rib_uptime = (time_t)(-1L);
-		if (new_select) /* route is installed in locrib from now on */
-			new_select->rib_uptime = bgp_clock();
-		bool is_withdraw = old_select && !new_select;
-
-		hook_call(bgp_route_update, bgp, afi, safi, dest,
-			  is_withdraw ? old_select : new_select, is_withdraw);
+		hook_call(bgp_route_update, bgp, afi, safi, dest, old_select,
+			  new_select);
 	}
 
 
@@ -3993,7 +3987,6 @@ struct bgp_path_info *info_make(int type, int sub_type, unsigned short instance,
 	new->peer = peer;
 	new->attr = attr;
 	new->uptime = monotime(NULL);
-	new->rib_uptime = (time_t)(-1L);
 	new->net = dest;
 	return new;
 }
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index 232d68018..3057a4259 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -211,6 +211,9 @@ struct bgp_path_info_extra {
 	mpls_label_t label[BGP_MAX_LABELS];
 	uint32_t num_labels;
 
+	/* timestamp of the rib installation */
+	time_t bgp_rib_uptime;
+
 	/*For EVPN*/
 	struct bgp_path_info_extra_evpn *evpn;
 
@@ -295,7 +298,6 @@ struct bgp_path_info {
 
 	/* Uptime.  */
 	time_t uptime;
-	time_t rib_uptime;
 
 	/* reference count */
 	int lock;
@@ -678,8 +680,8 @@ DECLARE_HOOK(bgp_process,
 /* called when a route is updated in the rib */
 DECLARE_HOOK(bgp_route_update,
 	     (struct bgp *bgp, afi_t afi, safi_t safi, struct bgp_dest *bn,
-	      struct bgp_path_info *updated_route, bool withdraw),
-	     (bgp, afi, safi, bn, updated_route, withdraw));
+	      struct bgp_path_info *old_route, struct bgp_path_info *new_route),
+	     (bgp, afi, safi, bn, old_route, new_route));
 
 /* BGP show options */
 #define BGP_SHOW_OPT_JSON (1 << 0)
diff --git a/doc/developer/bmp.rst b/doc/developer/bmp.rst
new file mode 100644
index 000000000..1c0e4b045
--- /dev/null
+++ b/doc/developer/bmp.rst
@@ -0,0 +1,49 @@
+.. _bmp:
+
+***
+BMP
+***
+
+RFC 7854
+========
+Missing features (non exhaustive):
+  - Per-Peer Header
+
+    - Peer Type Flag
+    - Peer Distingsher
+
+  - Peer Up
+
+    - Reason codes (according to TODO comments in code)
+
+Peer Type Flag and Peer Distinguisher can be implemented easily using RFC 9069's base code.
+
+RFC 9069
+========
+Everything that isn't listed here is implemented and should be working.
+Missing features (should be exhaustive):
+
+- Per-Peer Header
+
+  - Timestamp
+
+    - set to 0
+    - value is now saved `struct bgp_path_info -> locrib_uptime`
+    - needs testing
+
+- Peer Up/Down
+
+  - VRF/Table Name TLV
+
+    - code for TLV exists
+    - need better RFC understanding
+
+- Peer Down Only
+
+  - Reason code (bc not supported in RFC 7854 either)
+
+- Statistics Report
+
+  - Stat Type = 8: (64-bit Gauge) Number of routes in Loc-RIB.
+  - Stat Type = 10: Number of routes in per-AFI/SAFI Loc-RIB. The value is
+    structured as: 2-byte AFI, 1-byte SAFI, followed by a 64-bit Gauge.
diff --git a/doc/user/bmp.rst b/doc/user/bmp.rst
index e02c24c68..6cd7e19ca 100644
--- a/doc/user/bmp.rst
+++ b/doc/user/bmp.rst
@@ -36,7 +36,7 @@ The `BMP` implementation in FRR has the following properties:
   successfully.  OPEN messages for failed sessions cannot currently be
   mirrored.
 
-- **route monitoring** is available for IPv4 and IPv6 AFIs, unicast, multicast 
+- **route monitoring** is available for IPv4 and IPv6 AFIs, unicast, multicast
   EVPN and VPN SAFIs. Other SAFIs (VPN, Labeled-Unicast, Flowspec, etc.) are not
   currently supported.
 
@@ -57,51 +57,6 @@ The `BMP` implementation in FRR has the following properties:
 
 - monitoring peers with :rfc:`5549` extended next-hops has not been tested.
 
-RFC 7854
-========
-Unsupported features (non exhaustive):
-  - Per-Peer Header
-    
-    - Peer Type Flag
-    - Peer Distingsher
-  
-  - Peer Up
-  
-    - Reason codes (according to TODO comments in code)
-
-Peer Type Flag and Peer Distinguisher can be implemented easily using RFC 9069's base code.
-    
-RFC 9069
-========
-Everything that isn't listed here is implemented and should be working.
-Unsupported features (should be exhaustive):
-
-- Per-Peer Header
-
-  - Timestamp
-
-    - set to 0
-    - value is now saved `struct bgp_path_info -> locrib_uptime`
-    - needs testing
-
-- Peer Up/Down
-
-  - VRF/Table Name TLV
-  
-    - code for TLV exists
-    - need better RFC understanding
-
-- Peer Down Only
-
-  - Reason code (bc not supported in RFC 7854 either)
-
-- Statistics Report
-
-  - Stat Type = 8: (64-bit Gauge) Number of routes in Loc-RIB.
-  - Stat Type = 10: Number of routes in per-AFI/SAFI Loc-RIB. The value is 
-    structured as: 2-byte AFI, 1-byte SAFI, followed by a 64-bit Gauge.
- 
-   
 Starting BMP
 ============
 
@@ -194,7 +149,7 @@ associated with a particular ``bmp targets``:
 .. clicmd:: bmp monitor AFI SAFI <pre-policy|post-policy|loc-rib>
 
    Perform Route Monitoring for the specified AFI and SAFI.  Only IPv4 and
-   IPv6 are currently valid for AFI. SAFI valid values are currently 
+   IPv6 are currently valid for AFI. SAFI valid values are currently
    unicast, multicast, evpn and vpn.
    Other AFI/SAFI combinations may be added in the future.