On Mon, 2026-01-26 at 16:34 -0800, Stephen Hemminger wrote:
> 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]>
>
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.
