[PATCH 1/2] pcapng: revert use of reciprocal divide for timestamps

[PATCH 1/2] pcapng: revert use of reciprocal divide for timestamps

[Stephen Hemminger]

The use of recprocal divide caused in calculating timestamps
caused overflow wraparound. This bug was introduced by confusion
about bits, shift, and the test was too short to catch the
problem.

Go back to just doing the divide which ends up faster on many
than having to do 128 bit math on many CPU types.

This is not a pure git revert because there were other
good things in that commit like handling earlier packets,
and catching if TSC hz was zero.

Fixes: 4fc65615b274 ("pcapng: improve performance of timestamping")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/pcapng/rte_pcapng.c | 108 ++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 70 deletions(-)

diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c
index b5d1026891..df38e4819c 100644
--- a/lib/pcapng/rte_pcapng.c
+++ b/lib/pcapng/rte_pcapng.c
@@ -26,7 +26,6 @@
 #include <rte_mbuf.h>
 #include <rte_os_shim.h>
 #include <rte_pcapng.h>
-#include <rte_reciprocal.h>
 #include <rte_time.h>
 
 #include "pcapng_proto.h"
@@ -37,23 +36,12 @@
 /* upper bound for strings in pcapng option data */
 #define PCAPNG_STR_MAX	UINT16_MAX
 
-/*
- * Converter from TSC values to nanoseconds since Unix epoch.
- * Uses reciprocal multiply to avoid runtime division.
- */
-struct tsc_clock {
-	uint64_t tsc_base;          /* TSC value at initialization. */
-	uint64_t ns_base;           /* Nanoseconds since epoch at init. */
-	struct rte_reciprocal_u64 tsc_hz_inv; /* Reciprocal of TSC frequency. */
-	uint32_t shift;             /* Pre-shift to avoid overflow. */
-};
-
 /* Format of the capture file handle */
 struct rte_pcapng {
 	int  outfd;		/* output file */
 	unsigned int ports;	/* number of interfaces added */
-
-	struct tsc_clock clock;
+	uint64_t offset_ns;	/* ns since 1/1/1970 when initialized */
+	uint64_t tsc_base;	/* TSC when started */
 
 	/* DPDK port id to interface index in file */
 	uint32_t port_index[RTE_MAX_ETHPORTS];
@@ -110,62 +98,36 @@ static ssize_t writev(int fd, const struct iovec *iov, int iovcnt)
 #endif
 
 /*
- * Initialize TSC-to-epoch-ns converter.
+ * Convert a count of cycles to nanoseconds.
  *
- * Captures current TSC and system clock as a reference point.
+ * Compute the whole seconds first, so that the remainder is always
+ * less than the frequency and scaling it by NS_PER_S cannot wrap.
  */
-static int
-tsc_clock_init(struct tsc_clock *clk)
+static uint64_t
+pcapng_cycles_to_ns(uint64_t delta)
 {
-	struct timespec ts;
-	uint64_t cycles, tsc_hz, divisor;
-	uint32_t shift;
-
-	memset(clk, 0, sizeof(*clk));
-
-	/* If Hz is zero, something is seriously broken. */
-	tsc_hz = rte_get_tsc_hz();
-	if (tsc_hz == 0)
-		return -1;
-
-	/*
-	 * Choose shift so (delta >> shift) * NSEC_PER_SEC fits in uint64_t.
-	 * For typical GHz-range TSC and ~1s deltas this is 0.
-	 */
-	shift = 0;
-	divisor = tsc_hz;
-	while (divisor > UINT64_MAX / NSEC_PER_SEC) {
-		divisor >>= 1;
-		shift++;
-	}
-
-	clk->shift = shift;
-	clk->tsc_hz_inv = rte_reciprocal_value_u64(divisor);
+	const uint64_t hz = rte_get_tsc_hz();
+	uint64_t secs = delta / hz;
+	uint64_t rem = delta % hz;
 
-	/* Sample TSC and system clock as close together as possible. */
-	cycles = rte_get_tsc_cycles();
-	clock_gettime(CLOCK_REALTIME, &ts);
-	clk->tsc_base = (cycles + rte_get_tsc_cycles()) / 2;
-	clk->ns_base = (uint64_t)ts.tv_sec * NSEC_PER_SEC + ts.tv_nsec;
-
-	return 0;
+	return secs * NS_PER_S + (rem * NS_PER_S) / hz;
 }
 
-/* Convert a TSC value to nanoseconds since Unix epoch. */
-static inline uint64_t
-tsc_to_ns_epoch(const struct tsc_clock *clk, uint64_t tsc)
+/* Convert from TSC (CPU cycles) to nanoseconds */
+static uint64_t
+pcapng_timestamp(const rte_pcapng_t *self, uint64_t cycles)
 {
-	uint64_t delta, ns;
-
-	if (unlikely(tsc < clk->tsc_base)) {
-		delta = clk->tsc_base - tsc;
-		ns = (delta >> clk->shift) * NSEC_PER_SEC;
-		return clk->ns_base - rte_reciprocal_divide_u64(ns, &clk->tsc_hz_inv);
-	}
+	/*
+	 * A packet may be copied before the file was opened, so the TSC
+	 * can be behind the reference point.  Handle both directions on
+	 * an unsigned magnitude.
+	 */
+	if (unlikely(cycles < self->tsc_base))
+		return self->offset_ns -
+			pcapng_cycles_to_ns(self->tsc_base - cycles);
 
-	delta = tsc - clk->tsc_base;
-	ns = (delta >> clk->shift) * NSEC_PER_SEC;
-	return clk->ns_base + rte_reciprocal_divide_u64(ns, &clk->tsc_hz_inv);
+	return self->offset_ns +
+		pcapng_cycles_to_ns(cycles - self->tsc_base);
 }
 
 /* length of option including padding */
@@ -399,7 +361,7 @@ rte_pcapng_write_stats(rte_pcapng_t *self, uint16_t port_id,
 {
 	struct pcapng_statistics *hdr;
 	struct pcapng_option *opt;
-	uint64_t start_time = self->clock.ns_base;
+	uint64_t start_time = self->offset_ns;
 	uint64_t sample_time;
 	uint32_t optlen, len;
 	uint32_t *buf;
@@ -452,7 +414,7 @@ rte_pcapng_write_stats(rte_pcapng_t *self, uint16_t port_id,
 	hdr->block_length = len;
 	hdr->interface_id = self->port_index[port_id];
 
-	sample_time = tsc_to_ns_epoch(&self->clock, rte_get_tsc_cycles());
+	sample_time = pcapng_timestamp(self, rte_get_tsc_cycles());
 	hdr->timestamp_hi = sample_time >> 32;
 	hdr->timestamp_lo = (uint32_t)sample_time;
 
@@ -737,13 +699,10 @@ rte_pcapng_write_packets(rte_pcapng_t *self,
 			return -1;
 		}
 
-		/*
-		 * When data is captured by pcapng_copy the current TSC is stored.
-		 * Adjust the value recorded in file to PCAP epoch units.
-		 */
+		/* adjust timestamp recorded in packet */
 		cycles = (uint64_t)epb->timestamp_hi << 32;
 		cycles += epb->timestamp_lo;
-		timestamp = tsc_to_ns_epoch(&self->clock, cycles);
+		timestamp = pcapng_timestamp(self, cycles);
 		epb->timestamp_hi = timestamp >> 32;
 		epb->timestamp_lo = (uint32_t)timestamp;
 
@@ -789,6 +748,8 @@ rte_pcapng_fdopen(int fd,
 {
 	unsigned int i;
 	rte_pcapng_t *self;
+	struct timespec ts;
+	uint64_t cycles;
 	int ret;
 
 	if ((osname && strlen(osname) > PCAPNG_STR_MAX) ||
@@ -808,11 +769,18 @@ rte_pcapng_fdopen(int fd,
 	self->outfd = fd;
 	self->ports = 0;
 
-	if (tsc_clock_init(&self->clock) < 0) {
+	/* If Hz is zero, something is seriously broken. */
+	if (rte_get_tsc_hz() == 0) {
 		rte_errno = ENODEV;
 		goto fail;
 	}
 
+	/* record start time in ns since 1/1/1970 */
+	cycles = rte_get_tsc_cycles();
+	clock_gettime(CLOCK_REALTIME, &ts);
+	self->tsc_base = (cycles + rte_get_tsc_cycles()) / 2;
+	self->offset_ns = rte_timespec_to_ns(&ts);
+
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
 		self->port_index[i] = UINT32_MAX;
 
-- 
2.53.0

[PATCH 2/2] test/pcapng: check timestamps over a long capture

[Stephen Hemminger]

The cycles to nanoseconds conversion can overflow after only a few
seconds of capture, but the test ran for less than that so it never
saw it.  Forge the cycle count in the packet header to get timestamps
up to ten days out without waiting.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_pcapng.c | 140 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 1 deletion(-)

diff --git a/app/test/test_pcapng.c b/app/test/test_pcapng.c
index d14ea84f0d..688b58794e 100644
--- a/app/test/test_pcapng.c
+++ b/app/test/test_pcapng.c
@@ -2,6 +2,7 @@
  * Copyright (c) 2021 Microsoft Corporation
  */
 
+#include <inttypes.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -466,6 +467,47 @@ valid_pcapng_file(const char *file_name, uint64_t started, unsigned int expected
 	return ret;
 }
 
+/* Record the timestamp of the first packet in the file. */
+static void
+first_timestamp_cb(u_char *user, const struct pcap_pkthdr *h,
+		   const u_char *bytes __rte_unused)
+{
+	uint64_t *ts_ns = (uint64_t *)user;
+
+	/* File has nanosecond precision, so tv_usec holds ns. */
+	if (*ts_ns == 0)
+		*ts_ns = (uint64_t)h->ts.tv_sec * NS_PER_S + h->ts.tv_usec;
+}
+
+static int
+read_one_timestamp(const char *file_name, uint64_t *ts_ns)
+{
+	char errbuf[PCAP_ERRBUF_SIZE];
+	pcap_t *pcap;
+	int ret;
+
+	*ts_ns = 0;
+	pcap = pcap_open_offline_with_tstamp_precision(file_name,
+						       PCAP_TSTAMP_PRECISION_NANO,
+						       errbuf);
+	if (pcap == NULL) {
+		printf("pcap_open_offline('%s') failed: %s\n",
+			file_name, errbuf);
+		return -1;
+	}
+
+	ret = pcap_loop(pcap, 0, first_timestamp_cb, (u_char *)ts_ns);
+	if (ret != 0)
+		printf("pcap_loop: failed: %s\n", pcap_geterr(pcap));
+	pcap_close(pcap);
+
+	if (ret == 0 && *ts_ns == 0) {
+		printf("no packet found in %s\n", file_name);
+		return -1;
+	}
+	return ret;
+}
+
 static int
 test_add_interface(void)
 {
@@ -607,7 +649,7 @@ test_write_before_open(void)
 	mbuf1_resize(&mbfs, rte_rand_max(MAX_DATA_SIZE));
 
 	/* Copy packets BEFORE opening the pcapng file.
-	 * This exercises the negative TSC delta path in tsc_to_ns_epoch().
+	 * This exercises the negative TSC delta path.
 	 */
 	for (i = 0; i < (int)count; i++) {
 		clones[i] = rte_pcapng_copy(port_id, 0, &mbfs.mb[0], mp,
@@ -679,6 +721,101 @@ test_cleanup(void)
 	rte_vdev_uninit(null_dev);
 }
 
+/*
+ * Converting a cycle count to nanoseconds overflows past
+ * UINT64_MAX / NS_PER_S cycles, which is only a few seconds of real
+ * capture.  Forge the timestamp in the block header to reach large
+ * deltas without waiting.
+ */
+static int
+test_long_timestamp(void)
+{
+	/* seconds into the future */
+	static const unsigned int offsets[] = { 1, 8, 3600, 10 * 86400 };
+	struct pcapng_test_hdr {
+		uint32_t block_type;
+		uint32_t block_length;
+		uint32_t interface_id;
+		uint32_t timestamp_hi;
+		uint32_t timestamp_lo;
+	} *epb;
+	struct dummy_mbuf mbfs;
+	uint64_t hz = rte_get_tsc_hz();
+	unsigned int i;
+
+	TEST_ASSERT(hz != 0, "TSC frequency is zero");
+
+	mbuf1_prepare(&mbfs);
+	mbuf1_resize(&mbfs, 512);
+
+	for (i = 0; i < RTE_DIM(offsets); i++) {
+		char file_name[PATH_MAX] = "/tmp/pcapng_test_XXXXXX.pcapng";
+		uint64_t cycles, base_ns, want_ns, got_ns, diff;
+		struct rte_mbuf *mc;
+		rte_pcapng_t *pcapng;
+		int ret, tmp_fd;
+		ssize_t len;
+
+		mc = rte_pcapng_copy(port_id, 0, &mbfs.mb[0], mp,
+				     rte_pktmbuf_pkt_len(&mbfs.mb[0]),
+				     RTE_PCAPNG_DIRECTION_IN, NULL);
+		TEST_ASSERT(mc != NULL, "rte_pcapng_copy failed");
+
+		tmp_fd = mkstemps(file_name, strlen(".pcapng"));
+		if (tmp_fd == -1) {
+			rte_pktmbuf_free(mc);
+			TEST_ASSERT(false, "mkstemps() failed");
+		}
+
+		base_ns = current_timestamp();
+		pcapng = rte_pcapng_fdopen(tmp_fd, NULL, NULL, "longts", NULL);
+		if (pcapng == NULL) {
+			close(tmp_fd);
+			rte_pktmbuf_free(mc);
+			TEST_ASSERT(false, "rte_pcapng_fdopen failed");
+		}
+
+		ret = rte_pcapng_add_interface(pcapng, port_id, DLT_EN10MB,
+					       NULL, NULL, NULL);
+		if (ret < 0) {
+			rte_pcapng_close(pcapng);
+			rte_pktmbuf_free(mc);
+			TEST_ASSERT(false, "can not add port %u", port_id);
+		}
+
+		/* Conversion happens on write, so move the capture time. */
+		epb = rte_pktmbuf_mtod(mc, struct pcapng_test_hdr *);
+		cycles = (uint64_t)epb->timestamp_hi << 32;
+		cycles += epb->timestamp_lo;
+		cycles += (uint64_t)offsets[i] * hz;
+		epb->timestamp_hi = cycles >> 32;
+		epb->timestamp_lo = (uint32_t)cycles;
+
+		len = rte_pcapng_write_packets(pcapng, &mc, 1);
+		rte_pktmbuf_free(mc);
+		rte_pcapng_close(pcapng);
+		TEST_ASSERT(len > 0, "write failed at +%u s", offsets[i]);
+
+		ret = read_one_timestamp(file_name, &got_ns);
+		TEST_ASSERT(ret == 0, "can not read back +%u s", offsets[i]);
+
+		/* An overflow wraps and misses by the whole offset. */
+		want_ns = base_ns + (uint64_t)offsets[i] * NS_PER_S;
+		diff = (got_ns > want_ns) ? got_ns - want_ns : want_ns - got_ns;
+		if (diff > 2 * NS_PER_S)
+			printf("at +%u s: got %"PRIu64" want %"PRIu64"\n",
+			       offsets[i], got_ns, want_ns);
+		else
+			remove(file_name);
+
+		TEST_ASSERT(diff <= 2 * NS_PER_S,
+			    "timestamp off by %"PRIu64" ns at +%u s",
+			    diff, offsets[i]);
+	}
+
+	return 0;
+}
+
 static struct
 unit_test_suite test_pcapng_suite  = {
 	.setup = test_setup,
@@ -688,6 +825,7 @@ unit_test_suite test_pcapng_suite  = {
 		TEST_CASE(test_add_interface),
 		TEST_CASE(test_write_packets),
 		TEST_CASE(test_write_before_open),
+		TEST_CASE(test_long_timestamp),
 		TEST_CASES_END()
 	}
 };
-- 
2.53.0

Re: [PATCH 1/2] pcapng: revert use of reciprocal divide for timestamps

[Stephen Hemminger]

On Sun, 27 Sep 2026 12:50:14 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The use of recprocal divide caused in calculating timestamps
> caused overflow wraparound. This bug was introduced by confusion
> about bits, shift, and the test was too short to catch the
> problem.
> 
> Go back to just doing the divide which ends up faster on many
> than having to do 128 bit math on many CPU types.
> 
> This is not a pure git revert because there were other
> good things in that commit like handling earlier packets,
> and catching if TSC hz was zero.
> 
> Fixes: 4fc65615b274 ("pcapng: improve performance of timestamping")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Both applied to next-net