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

[Martin Spinler]

On Mon, 2026-01-26 at 16:34 -0800, Stephen Hemminger wrote:
> On Wed, 21 Jan 2026 18:01:17 +0100
> spinler at cesnet.cz wrote:
> 
> > From: Martin Spinler <spinler at 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 at cesnet.cz>
> 

Together with the comment in another email thread

> 5. **Patch 8**: Release notes mention timestamp changes not present in this series
> Good to address.

it seems, that the "[v4,3/6] net/nfb: update timestamp calculation to
meaningful value" from "net/nfb: code cleanup" series was my rash act,
as it resolves just one of many issues.
Would it be possible to revert back this series to "Change Requested"
state? I would remove just this one patch from series and resubmit. Or
can you suggest a better solution?


> 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.

1. I completely agree, I usually don't touch code that I don't need to
modify (except comprehensive cleaning), but I will address this issue
in one of the next series (concerning queues), which I already have
semi-prepared; there is some cleaning up anyway.

> 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.

2. I see, perhaps the original author hoped, that always inline would
be faster this way? :)

> 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.

4. Agree, I needed/have a shared function in my next series (offloads
and new/alternative queue driver) already.

>    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?

Unfortunately the firmware is not prepared to Y2038: needs more thought
in our team to find the solution.


> Something like?
> 
> struct nfb_meta {
>        rte_le32_t rx_hisecs;
>        rte_le32_t rx_losecs;
>        rte_le32_t rx_nsecs;
> };

Makes more sense.

> 
> 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.

10. you are right. I'm addressing that in other series.

> 
> 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?

UTC time is good hint for read_clock. I think it should work quite
well. But I need to think it through/test more.
(There is separate NFB tool/daemon, which configures/synchronises the
Timestamp Unit inside Firmware and also selects between external PPS
and CPU time. The usage varies for different applications, but the
values are still in nsecs.)

> 
> Transmit has seem inline madness.
> 	

Thank you very much for all the other comments (3, 5, 6, 7 ,8, 9), they
are all relevant and I will try to incorporate them.