Skip to content

bgpd: carry reception time at microsecond precision in BMP adj-in - #22990

Open
tobez wants to merge 2 commits into
FRRouting:masterfrom
tobez:bmp-adjin-usec
Open

bgpd: carry reception time at microsecond precision in BMP adj-in#22990
tobez wants to merge 2 commits into
FRRouting:masterfrom
tobez:bmp-adjin-usec

Conversation

@tobez

@tobez tobez commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

BMP Route Monitoring per-peer header timestamps are reconstituted from a
seconds-only monotonic time_t, so monotime_to_realtime() fills the
microsecond field with the sub-second offset between CLOCK_REALTIME and
CLOCK_MONOTONIC -- a boot-constant value, identical in every RM message
until the host reboots.

For the pre-policy (Adj-RIB-In) feed full precision is free: the
reception time lives in struct bgp_adj_in, written in one place and read
only by BMP. Store the microseconds in the struct's padding hole (no
size growth on LP64), stamp both fields from one clock read in
bgp_adj_in_set(), and pass bmp_monitor() a monotonic struct timeval,
NULL meaning "time unavailable" in place of the (time_t)(-1L) sentinel.

Post-policy, Loc-RIB and peer-state timestamps intentionally stay
second-precision: their sources are shared time_t fields used across
bgpd.

Includes a topotest asserting that the microsecond components of
pre-policy updates received seconds apart actually differ.

tobez added 2 commits August 8, 2026 11:33
Every BMP Route Monitoring per-peer header timestamp is reconstituted
from a seconds-only monotonic time_t, so monotime_to_realtime() fills
the microsecond field with the sub-second offset between CLOCK_REALTIME
and CLOCK_MONOTONIC -- a boot-constant value dressed up as precision,
identical in every message until the host reboots.

For the pre-policy (Adj-RIB-In) feed full precision is free: the
reception time lives in struct bgp_adj_in, is written in exactly one
place and read only by the BMP code.  Store the microseconds in the
struct's padding hole (no size growth on LP64), stamp both fields from
a single clock read in bgp_adj_in_set(), and pass bmp_monitor() a
monotonic struct timeval, with NULL meaning "time unavailable" in place
of the (time_t)(-1L) sentinel (which was also run through
monotime_to_realtime() only to throw the result away).

Post-policy, Loc-RIB and peer-state timestamps intentionally stay
second-precision: their sources are shared time_t fields (bpi->uptime,
bgp_rib_uptime, peer->uptime) used across bgpd, and truncating to the
second keeps correct floor semantics (reported <= true < reported + 1s)
that zeroed microseconds would make strictly worse.

Signed-off-by: Anton Berezin <tobez@tobez.org>
Drive several re-announcements of a watched prefix, each spaced by
the 5s route-map delay timer, and assert that the microsecond
components of their pre-policy timestamps actually differ (maximum
pairwise circular distance above 100us).  With microseconds fabricated
from a seconds-only monotonic timestamp every sample sits within a few
microseconds of the boot-constant CLOCK_REALTIME-CLOCK_MONOTONIC
offset and the check fails; with genuine reception times the
components of updates seconds apart are effectively uniform and the
false-failure probability is about 1e-8 per run.  No sleeps.

Signed-off-by: Anton Berezin <tobez@tobez.org>
@frrbot frrbot Bot added bgp tests Topotests, make check, etc labels Aug 8, 2026
@greptile-apps

greptile-apps Bot commented Aug 8, 2026

Copy link
Copy Markdown

Greptile Summary

This PR preserves microsecond precision for pre-policy BMP Route Monitoring timestamps by storing the complete monotonic Adj-RIB-In reception time and passing nullable timeval values through BMP timestamp conversion.

  • Adds a microsecond field to struct bgp_adj_in and stamps seconds and microseconds from one monotonic clock read.
  • Changes bmp_monitor() from a time_t sentinel interface to a nullable struct timeval interface.
  • Preserves existing second-resolution behavior for post-policy and Loc-RIB timestamps.
  • Adds a topotest that distinguishes genuine reception microseconds from the prior boot-constant conversion artifact.

Confidence Score: 5/5

The PR appears safe to merge, with the intended pre-policy timestamp precision preserved through every changed storage and BMP emission path.

Adj-RIB-In timestamps are captured atomically, all consumers initialize the complete timeval, unavailable timestamps retain their previous zero encoding, and no blocking behavioral or contract failure remains.

Important Files Changed

Filename Overview
bgpd/bgp_advertise.c Captures one monotonic timeval and consistently stores both components for new and updated Adj-RIB-In entries.
bgpd/bgp_advertise.h Adds microsecond storage without introducing a serialization or binary-layout dependency.
bgpd/bgp_bmp.c Converts bmp_monitor() to a nullable timeval contract while preserving previous missing-time and seconds-only behavior at all callers.
tests/topotests/bgp_bmp/test_bgp_bmp_timestamps.py Adds coverage proving that pre-policy timestamps carry varying reception-time microseconds across re-announcements.

Sequence Diagram

sequenceDiagram
    participant BGP as BGP update processing
    participant Adj as Adj-RIB-In
    participant BMP as BMP monitor
    participant C as BMP collector
    BGP->>BGP: "monotime(&now)"
    BGP->>Adj: store now.tv_sec and now.tv_usec
    Adj->>BMP: pass monotonic timeval
    BMP->>BMP: monotime_to_realtime()
    BMP->>C: Route Monitoring header with sec/usec
Loading

Reviews (1): Last reviewed commit: "tests: check BMP pre-policy timestamps c..." | Re-trigger Greptile

@tobez

tobez commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

ci:rerun

2 similar comments
@tobez

tobez commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

ci:rerun

@tobez

tobez commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

ci:rerun

@ton31337 ton31337 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we change time_t to struct timeval adj->uptime at all? This way we could avoid having adj->uptime_usec?

@tobez

tobez commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

What if we change time_t to struct timeval adj->uptime at all? This way we could avoid having adj->uptime_usec?

It would be cleaner but we'll pay for that in memory per entry. The 4-byte uptime_usec in the PR fits into a padding hole, so the struct size stays at 56 bytes (which incidentally is at the limit of 64-bit glibc usable chunk size. Replacing it all with timeval (2 x 8 bytes) would mean addpath_rx_id will still need padding, so the struct becomes 64 bytes, and the next usable chunk is 72 bytes. So we would be paying 16 byte penalty for each entry. (Clearly, all this is applicable for LP64 architecture with glibc, but that's probably the vast majority of all installs).

@riw777 riw777 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@ton31337

Copy link
Copy Markdown
Member

TL;DR: IMO, I prefer maintainability here (using struct timeval) versus adding an additional uptime_usec for struct bgp_adj_in.

That's eventually 8-bytes more, but still it's under the same 64 bytes "bucket", 56 (by filling padding gap) versus 64 with the timeval struct. And since it's very small in general (we are not going e.g. from 1 to 2 cache lines by adding timeval, I prefer maintainability. (It's just my opinion, might be wrong completely, sorry, and I'd like to ask @donaldsharp and @eqvinox opinions also).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bgp master size/L tests Topotests, make check, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants