On Thu, 27 Aug 2026 17:51:59 +0530 Rajesh Kumar <[email protected]> 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 <[email protected]> > --- 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.
