[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 at dpdk.org

Signed-off-by: Stephen Hemminger <stephen at 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