On Wed, 21 Jan 2026 18:01:17 +0100
[email protected] wrote:
> From: Martin Spinler <[email protected]>
>
> 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 <[email protected]>
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.
