Re: [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 21 Jan 2026 18:01:17 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> The resulting timestamp wasn't monotonically increasing value.
> 
> Now the timestamp is value in nanoseconds (Unix epoch).
> 
> Unfortunately there's currently no safe mechanism in nfb-framework
> to get ticks from hardware to implement read_clock function.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>

While review this code, noticed some things that are related.

1. Why is nfb_eth_ndp_rx in a header file, it is only called one place.
   C code should be in the .c file.

2. It is marked as always inline, but since it is called by an indirect
   function pointer, dev->rx_burst it is never going to be inlined anyway.

3. This could be simplified.

	/* returns either all or nothing */
	i = rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts);
	if (unlikely(i != 0))
		return 0;

	/* returns either all or nothing */
	if (unlikely(rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts) != 0))
		return 0;

4. You are fighting against line length here:
			if (nfb_timestamp_dynfield_offset >= 0) {
				rte_mbuf_timestamp_t timestamp;

				/* seconds */
				timestamp =
					rte_le_to_cpu_32(*((uint32_t *)
					(packets[i].header + 8)));
				timestamp *= NSEC_PER_SEC;
				/* nanoseconds */
				timestamp +=
					rte_le_to_cpu_32(*((uint32_t *)
					(packets[i].header + 4)));
				*nfb_timestamp_dynfield(mbuf) = timestamp;
				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
			}
   Either add a helper function expand since current DPDK max line length is 100.
   It looks like you are using pointer math rather than a structure.
   Also does the driver just use timespec? I.e is it safe in 2038 with a 64 bit seconds?

Something like?

struct nfb_meta {
       rte_le32_t rx_hisecs;
       rte_le32_t rx_losecs;
       rte_le32_t rx_nsecs;
};

static inline void
nfb_set_timestamp(struct rte_mbuf *mbuf, const void *header)
{
	const struct nfb_meta *hdr = header;
	rte_mbuf_timestamp_t timestamp;

	timestamp = (((uint64_t) rte_le_to_cpu_32(hdr->rx_hisecs) << 32) 
		    + (uint64_t) rte_le_to_cpu_32(hdr->rx_losec)) * NS_PER_SEC;
	timestamp += rte_le_to_cpu_32(hdr->rx_nsecs);
	
	*nfb_timestamp_dynfield(mbuf) = timestamp;
	mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
}

5. Using volatile for statistics is unnecessary and slows down
   code. Since only one thread can update them there is no need.
   It seems that lots of drivers cut-paste this off old code.
   The one exception would be if the driver supported TX lockfree
   but in that case it would need real atomics.

6. Driver should consider getting rid of variable length arrays.
   VLA's are a bug trap and a GNU extension. It is perfectly reasonable
   to have an upper bound on burst size of something like 64 bytes.

7. Driver could use rte_pktmbuf_free_bulk instead of loops in partial rx case.

8. offload flags should already be set correctly when mbufs are allocated.
   doing mbuf->ol_flags = 0 is not needed (but harmless).

9. You might consider using rte_pktmbuf_append() which would standardize
   update to mbuf and avoid the current situation where if packet received
   is bigger than space in the mbuf pool, the driver will corrupt the mbuf.

10. The driver probably haven't been tested with multiple ports.
    But the expected behavior is that timestamp is configure per-port.
    Most drivers have a flag and only add timestamp if it has been
    configured per-port.

Since driver is using time since 1/1/1970, you can just use UTC time
for read_clock. Except if there is no time sync between CPU and NIC.
Maybe just compute offset once on first packet received?

Transmit has seem inline madness.