On Tue, 18 Aug 2026 00:54:14 +0530
Rajesh Kumar <[email protected]> wrote:
> The current DPDK ethdev time synchronization framework is architected
> around a single, shared hardware latch. The existing API,
> `rte_eth_timesync_read_tx_timestamp()`, assumes a serialization model
> where only one TX timestamp is outstanding at any given time.
>
> This model creates severe limitations for modern high-throughput network
> interface cards (NICs). When multiple packets requiring precise
> transmit timestamps are sent concurrently, the shared latch becomes a
> race-condition bottleneck. It makes timestamp retrieval unreliable and
> drops accuracy. Furthermore, Poll Mode Drivers (PMDs) backed by
> hardware that supports independent, per-packet timestamping slots have
> no way to expose this capability to the user.
>
> To solve this, this RFC introduces a formal slot-based lifecycle API
> for per-packet Tx timestamp management. The API decouples timestamp
> tracking from the global latch model, enabling true asynchronous,
> parallel hardware timestamping.
Lots of reasonable AI feedback to the design.
Review of the RFC. Design issues first since that's what they're asking for,
then code defects.
Design
No capability discovery or exhaustion semantics. Nothing reports how many
slots exist, whether they're per-port or per-queue, and slot_alloc() doesn't
document what it returns when slots run out (-ENOSPC? -EAGAIN?). That's the
first thing an application hits. Needs a rte_eth_dev_info field or query, and a
defined out-of-slots errno.
Queue asymmetry: alloc() takes tx_queue_id but read() and release() don't.
Either slot_id is port-global (then why does alloc need the queue?) or it's
per-queue (then read/release are ambiguous). Pick one and document it. Also
tx_queue_id is never validated against nb_tx_queues in the ethdev layer.
Interaction with the existing mechanism is undefined. Does the app still
set RTE_MBUF_F_TX_IEEE1588_TMST? Can the legacy latch API and the slot API
coexist on one port? PMDs today key tx timestamping off that flag; the RFC
needs to say what supersedes what.
Fast-path cost contradicts the stated motivation. The cover letter argues
high-throughput concurrent timestamping, but the lifecycle is three dev_ops
indirect calls plus a dynfield write per packet, all through the slow path.
Fine for PTP rates; if the claim is more than that, alloc/release want burst
variants or the intended rate should be stated.
cycles_ns is self-contradictory: is it raw counter cycles or nanoseconds
from the free-running clock? If cycles, drop the _ns and expose the frequency;
if ns, call it raw_ns or free_ns. Also this struct switches to int64 ns while
every other timesync call uses struct timespec; probably the right move but
justify it in the cover letter.
PMDs can't consume the dynfield as written. The offset and flag are static
in rte_ethdev.c and not exposed to drivers. A PMD has to re-lookup by name, and
the dynflag name only exists as a string concat inside the .c file, so drivers
would hardcode "..._flag". Define the flag name macro in the header and provide
a lookup helper, following the RTE_MBUF_DYNFIELD_TIMESTAMP_NAME pattern.
Naming: rte_eth_timesync_tx_timestamp_stamp_mbuf stutters.
..._tx_slot_set_mbuf or similar.
Defects
Silent dynflag failure in rte_eth_timesync_tx_slot_dynfield_register(). If
both rte_mbuf_dynflag_register() and the lookup fail,
rte_eth_timesync_tx_slot_dynflag stays 0, the function returns 0, and
stamp_mbuf() ORs 0 into ol_flags and reports success. The PMD never sees the
request. Must return error. Worse, the early return on offset >= 0 means the
flag is never retried on subsequent calls, so one transient failure is
permanent.
Likely doesn't compile as posted: the diff adds no includes, but uses
struct rte_mbuf_dynfield, rte_mbuf_dynflag_register() (needs rte_mbuf_dyn.h)
and alignof (needs stdalign.h pre-C23). Check whether rte_ethdev.c already
pulls those in; I don't believe it does.
stamp_mbuf() takes port_id and ignores it, documented as "reserved for
future PMD use". Either validate it or drop it; a parameter whose semantics
arrive later is an API trap. Dropping it also removes the false implication
that the call is port-scoped.
Nits
stamp_mbuf() doxygen deviates from the file's param style, omits the
-EINVAL return the code actually produces, and contains an em-dash. The
dual_domain_timestamp struct fields lack doxygen comments.
v1 needs rel_notes and prog_guide (ptp section) updates; RFC is fine
without.
