Re: [RFC PATCH v3 1/1] ethdev: add Tx timestamp slot management APIs

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 27 Aug 2026 17:51:59 +0530
Rajesh Kumar <rajesh3.kumar@intel.com> wrote:

> Extend ethdev timesync with a capability model for selecting between
> shared-register and per-packet Tx timestamping.
> 
> Add public and PMD interfaces to query timestamp capabilities, allocate
> timestamp slots, retrieve timestamps asynchronously, and release slots.
> Slots have port-global scope and can be used across Tx queues.
> 
> Add a dual-domain timestamp structure for reporting adjusted PHC time
> and raw hardware time independently through validity flags.
> 
> Add APIs to register and unregister the mbuf dynamic field and dynflag
> used to pass slot handles to the Tx datapath. Add helpers to associate
> a slot handle with an mbuf before transmission.
> 
> Keep the legacy Tx timestamp API for shared-register hardware and provide
> a compatibility alias for the mbuf stamping helper.
> 
> Document the timestamp capability model, slot lifecycle, and application
> workflow.
> 
> Signed-off-by: Rajesh Kumar <rajesh3.kumar@intel.com>
> ---

Lots of feedback for AI review that needs addressing (Claude Fable).
Also a feature like this needs some form of test coverage. Perhaps
mocking up something in null PMD or related.

Review of [RFC PATCH v3 1/1] ethdev: add Tx timestamp slot management APIs

Applied cleanly to current main and read post-apply. No build
performed (meson not available here); nothing in the diff looks like
it would fail to compile.

Errors:

1. Documentation and Doxygen claim rte_eth_timesync_enable() registers
   the dynfield/dynflag automatically. It does not. The patch does not
   touch rte_eth_timesync_enable(); post-apply it still just calls the
   PMD op. So the .. note:: in timesync.rst ("registers the dynamic
   field automatically ... Call ... explicitly only if creating
   mempools before enabling timesync") and the same statement in the
   header Doxygen for rte_eth_timesync_tx_slot_dynfield_register() are
   false. An application that follows the doc and relies on
   timesync_enable will get -ENOTSUP from stamp_mbuf (or worse, a late
   registration that fails after pools are created). Either add the
   call in rte_eth_timesync_enable() or drop the claim; given the
   "must register before pool create" constraint, dropping the claim
   and making the explicit call mandatory is the safer contract.

2. Commit message says "provide a compatibility alias for the mbuf
   stamping helper". No such alias exists in the diff. Either the
   alias was dropped between v2 and v3 and the message is stale, or
   it is missing. Fix one or the other.

3. Dead macros referencing a nonexistent structure:
     RTE_ETH_TIMESYNC_TX_TIMESTAMP_SLOT_INFO_MAX_VALID
     RTE_ETH_TIMESYNC_TX_TIMESTAMP_SLOT_INFO_FREE_VALID
   Doxygen says they are valid bits for
   rte_eth_timesync_tx_timestamp_slot_info.max_slots / .free_slots,
   but that struct is not defined anywhere. Leftover from an earlier
   revision; remove them.

4. The PMD contract is incomplete: the datapath needs the dynfield
   offset and dynflag bit, but they are file-static in rte_ethdev.c
   with no accessor in ethdev_driver.h. A PMD is forced to do its own
   rte_mbuf_dynfield_lookup()/dynflag_lookup() by name, which then
   cannot observe rte_eth_timesync_tx_slot_dynfield_unregister()
   resetting the library-side cache. So the documented behaviour that
   after unregister "PMD Tx datapaths fall back to port-level legacy
   mode" cannot actually happen: the PMD keeps testing the dynflag it
   looked up, and mbufs stamped before unregister still carry it.
   Either export an internal accessor (offset + flag) in
   ethdev_driver.h that PMDs must use, or drop the unregister API and
   its fallback claim. As written, the unregister function only
   changes library-local state and cannot deliver what its Doxygen
   promises.

Warnings:

5. rte_ethdev.h Doxygen for rte_eth_timesync_tx_slot_dynfield_unregister
   refers to "the port-level ptp_tx_index". That is an Intel driver
   internal, not an ethdev concept; a generic header should not
   reference it.

6. rte_eth_timesync_tx_timestamp_stamp_mbuf() calls
   rte_eth_timesync_tx_slot_dynfield_register() on every invocation.
   Post-registration this is just an int compare, but it also means
   the first stamp_mbuf call can silently register the dynfield after
   pools exist, which the register() Doxygen says may fail. It is also
   inconsistent with the "-ENOTSUP after unregister" contract: after
   unregister, stamp_mbuf will simply re-register (lookup succeeds)
   and go on working. Do the offset check inline and return -ENOTSUP
   if the offset is < 0 rather than re-registering.

7. Slot handles are uint32_t, but rte_eth_timesync_tx_ts_caps has no
   way to express the free-slot count, and slot_release() has no
   documented behaviour for double-release or release of a slot whose
   timestamp was never read. For an RFC that is acceptable, but the
   PMD contract section in timesync.rst should say what a PMD must do
   for an invalid or already-free slot_id (-EINVAL is the obvious
   answer, and the ethdev wrapper could enforce slot_id < max_slots
   if caps are cached).

8. Release notes and header call these experimental, but the new
   eth_dev_ops members are inserted in the middle of struct
   eth_dev_ops rather than at the end. eth_dev_ops is internal so
   this is not an ABI issue, but the ordering in the struct
   (alloc, get_capabilities, read_slot, release) does not match the
   typedef order or the order in features.rst
   (get_capabilities, alloc, read_slot, release). Make them consistent.

9. features.rst adds the ops and API names, but there is no PMD
   implementing them in this series and no testpmd hook or unit test
   exercising the new API. Per contributing guidelines a new ethdev
   API needs at least one driver implementation and a testpmd hook
   before it can be merged out of RFC.

Info:

10. timesync.rst: the "Clock Management & Control", "Rx Timestamp
    Extraction Workflow" and "PMD Implementation Requirements"
    sections use bullet lists with bold term + description; RST
    definition lists would render better. Several lines in the .rst
    are well over 100 columns; wrap at sentence boundaries.

11. In rte_eth_timesync_tx_slot_dynfield_register(), the inner
    braced block for flag_bit is unusual style in DPDK; declare
    flag_bit at function top or at point of use without the block.

12. Blank line separating the slot_release function from the
    "Internal process-local cache" comment is missing; there is a
    double blank line before stamp_mbuf.