Re: [RFC PATCH v5 0/5] ethdev: add Tx timestamp slot APIs

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 6 Oct 2026 15:58:29 +0530
"Kumar, Rajesh" <rajesh3.kumar@intel.com> wrote:

> Hi Stephen,
> I wanted to quickly check if you have any further comments or feedback 
> on this v5 series?
> If you are satisfied with the current changes, we can move forward to 
> the next steps for this patch.
> 
> Regards,
> 
> Rajesh
> 
> On 21-09-2026

Lots of open issues from AI review.
The biggest issue is that this overlaps with existing metering API
and seems  specific to Intel NIC's. I strongly discourage API's that
can only be used on one vendor NIC.


Series: [RFC PATCH v5 0/5] ethdev: add Tx timestamp slot APIs

Applied to main 49bb9a5 (release_26_11.rst conflict only). Each
commit builds with -Dwerror=true (net/intel/ice, test-pmd); no bisect
issues.

Series-level:

The main problem is in the ice implementation. Slot state is never
cleaned up before a slot is handed out, so the read path can return
a timestamp that belongs to an earlier packet as if it were the
current one (patch 4, Error).

The legacy static index and the slot allocator also overlap on ice.

On the API side, the caller has to set two separate mbuf flags for
one action. The caps query returns -ENOTSUP on every PMD except ice,
so it cannot be used to choose a mechanism as the doc says.

The patch 1 commit message and the cover letter describe an
unregister / "process-local reset" API that is not in this version.


Patch 1/5 ethdev: add Tx timestamp slot management APIs

Warning:

- The commit message says "Add APIs to register and unregister the
  mbuf dynamic field", and the cover letter lists "process-local
  reset". Only rte_eth_timesync_tx_slot_dynfield_register() exists.

- rte_eth_timesync_tx_slot_dynfield_register() falls back to
  rte_mbuf_dynfield_lookup() / rte_mbuf_dynflag_lookup() after a
  registration failure. Register already returns the existing offset
  when the name and params match, including in a secondary process.
  The only extra case the lookup covers is EEXIST with different
  params, and there it accepts a field of the wrong size or
  alignment. It also returns -ENOTSUP for every failure, losing
  rte_errno (ENOMEM, EEXIST). Drop both lookups and return
  -rte_errno.

- The register doc says "Must be called before the first
  rte_pktmbuf_pool_create()". rte_mbuf_dyn.h says "The registration
  can be done at any moment"; pool creation has no bearing on it.
  The same wrong claim appears in the patch 3 note.

- rte_eth_timesync_tx_slot_caps() returns -ENOTSUP for every in-tree
  PMD except ice, including all PMDs that implement
  timesync_read_tx_timestamp. Nothing ever reports
  RTE_ETH_TIMESYNC_TX_SLOT_SINGLE_REG, so "Applications use this to
  select the mechanism supported by the port" does not hold. Either
  have ethdev report SINGLE_REG when timesync_tx_slot_get_caps is
  NULL and timesync_read_tx_timestamp is set, or drop SINGLE_REG.

- rte_eth_timesync_tx_slot_stamp() sets the dynflag but not
  RTE_MBUF_F_TX_IEEE1588_TMST. The patch 3 example has to set TMST
  separately. If the application forgets it, ice takes the
  "else if (ol_flags & RTE_MBUF_F_TX_IEEE1588_TMST)" path, no
  timestamp is requested, and the slot read returns -EAGAIN forever.
  stamp() should set both flags.

- The release note says the new APIs "support both shared-register
  and slot-bank usage models through the rte_eth_timesync_tx_slot_*
  interface family". The shared-register model is still served only
  by rte_eth_timesync_read_tx_timestamp().

- There is no functional test. dynfield_register() and stamp() are
  pure software and can be tested in app/test without hardware.

Info:

- The read and release doxygen says "is undefined behaviour - PMDs
  should return -EINVAL"; pick one. Those two lines (rte_ethdev.h
  5448, 5472) also use non-ASCII em-dashes. The release doc still
  refers to "stamp_mbuf", the old function name.

- Drop "This function does not make the PMD hardware lifecycle safe
  by itself." It gives the reader nothing to act on (also in
  patch 2).

- The new wrappers have no trace points. The existing timesync calls
  do, e.g. rte_eth_trace_timesync_read_tx_timestamp().

- Unrelated changes: a blank line removed before
  rte_eth_add_rx_callback, and the log text reworded in
  rte_eth_timesync_read_tx_timestamp().

- The definitions of alloc, release, dynfield_register and stamp put
  the return type on the same line as the name. caps and read in the
  same patch follow DPDK style.

- Nothing in the series sets raw_ns or
  RTE_ETH_TIMESYNC_DUAL_DOMAIN_TIMESTAMP_RAW_VALID. Consider leaving
  them out until a PMD produces them.

- The dynfield name "rte_eth_timesync_tx_slot" does not follow the
  rte_<libname>_dynfield_<name> convention in rte_mbuf_dyn.h.


Patch 2/5 doc: describe ethdev timesync clock and Rx timestamp API

Info:

- The commit message says the patch documents existing behaviour
  only. The "must ensure no Tx timestamp operations are in flight
  before disabling" requirement is new in patch 1; move it to
  patch 3.

- The flags argument of rte_eth_timesync_read_rx_timestamp() is not
  explained. ice uses it as the Rx queue index.


Patch 3/5 doc: describe ethdev Tx timestamp slot API

Warning:

- "* **Single Shared Register** (...):" and "* **Adjusted Domain**"
  are term/description bullet lists. Use RST definition lists. The
  same applies to the numbered "1. **Clock Control & Adjustment**:"
  list in patch 2.

Info:

- Indentation is inconsistent in the step 1 and step 4 code
  examples.

- Document how late a slot read may happen. ice extends a 32-bit ns
  value against the current PHC time, so the result is only correct
  within about 2.1 s of capture (see patch 4).


Patch 4/5 net/ice: support per-packet Tx timestamp slots

Error:

- Stale timestamps are returned as valid.

  E810 has no ready register: ice_get_phy_tx_tstamp_ready_e810()
  does "*tstamp_ready = 0xFFFFFFFFFFFFFFFF;". In
  ice_ptp_read_tx_dual_timestamp() the ready-bit test therefore
  never fires, and "if (ret || tstamp == 0)" is the only readiness
  check.

  Nothing clears a slot before it is handed out.
  ice_ptp_alloc_tx_slot() only sets the bitmap bit.
  ice_timesync_enable() does not reset the PHY timestamp memory.
  ice_timesync_disable() clears index 0 only:
  "ice_clear_phy_tstamp(hw, lport, 0);".

  So a slot can still hold a value from a process that exited
  without releasing it, or one the hardware wrote after the
  application timed out and released the slot. The next owner reads
  that value as its own, before its packet is even sent.

  On E822 and ETH56G, release does not touch the PHY at all
  ("if (hw->phy_model == ICE_PHY_E810)"). A timestamp that arrives
  after a timeout leaves the ready bit set for the next owner.

  Fix: in timesync_enable, reset this PF's slot range in PHY memory
  and clear ts_slot_bitmap. In alloc, discard any entry already
  present in the chosen slot.

Warning:

- The legacy index overlaps the allocator. ice_ptp_init_info() sets
  "ad->ptp_tx_index = 0;" on E810, E830 and ETH56G, and base_slot on
  E822. "slot = (uint8_t)rte_ctz64(free_in_range);" therefore hands
  out the legacy slot first.

  A TMST packet without the dynflag uses ad->ptp_tx_index and lands
  in that same slot. On E810, ice_timesync_read_tx_timestamp() now
  also clears that slot after reading it. Reserve the legacy bit in
  ts_slot_bitmap at init.

- "caps->max_slots = 64;" is unconditional, but the allocator limits
  E822 to "64 / ppq" slots per PF. The debug log "all 64 timestamp
  slots busy" is wrong on E822 for the same reason. Compute the
  range once, store it in the adapter, and use it for caps, alloc,
  read and release.

- Read and release only check "slot_id > 63".

  Releasing an unallocated or already-released slot clears a bit
  that another thread may now own, and on E810 zeroes its PHY entry.
  The ethdev header asks PMDs to return -EINVAL in that case.

  On E822, reading a slot outside this PF's range reads another PF's
  quad entry. Per the ice_clear_phy_tstamp_e822() comment, that read
  auto-clears the entry, so this PF steals the other PF's timestamp.

  Check the slot against the PF range and the bitmap.
  rte_atomic_fetch_and_explicit() returns the old value for the
  ownership test.

- "if (ret) return -EAGAIN;" after ice_get_phy_tx_tstamp_ready() and
  ice_read_phy_tstamp() turns sideband and register failures into
  "retry". The caller then polls forever. Return -EIO.

- ice_timesync_tx_slot_alloc() does not check ad->ptp_ena. With
  timesync disabled, or with txpp_ena set (ice_timesync_enable()
  returns 0 without enabling), alloc succeeds and read returns
  -EAGAIN forever.

- The E810 ice_clear_phy_tstamp() call added to the timeout branch
  of ice_timesync_read_tx_timestamp() is unreachable. tstamp_ready
  is all ones on E810, so
  "!(tstamp_ready & BIT_ULL(ad->ptp_tx_index))" is never true. The
  "clear ... after ... a timeout" fix claimed in the commit message
  does not happen.

- No release note entry under "Updated Intel ice driver".

- ice_timesync_tx_slot_alloc() -> ice_get_next_tx_desc_idx() ->
  ice_ptp_alloc_tx_slot() repeats the dev, slot_id and dual NULL
  checks at every level, after ethdev has already validated them.
  ice_get_next_tx_desc_idx is also misnamed; it allocates a slot,
  not a descriptor index. Collapse the layers.

Info:

- get_context_desc() evaluates
  "if (ol_flags & rte_eth_timesync_tx_slot_dynflag)" for every
  packet in the scalar path, before the ice_calc_context_desc() == 0
  early return. That is an exported-variable load (via the GOT in a
  shared build) per packet. Move the lookup into the TMST branch.

- Pre-existing: TSYN is in the "else if" after TSO, so TMST is
  ignored on TSO packets. With slots, the allocated slot then stays
  at -EAGAIN.

- Pre-existing: the vector Tx paths, including the AVX2 and AVX512
  ctx offload paths, never set CI_TX_CTX_DESC_TSYN.

  Tx path selection does not consider ptp_ena, while Rx does
  (ice_set_rx_function). caps still reports PER_PACKET, so a stamped
  packet on a vector path never gets a timestamp.

- Pre-existing: ice_tstamp_convert_32b_64b(hw, ad, 1, ...) writes the
  shared ad->time_hw without synchronization. The slot read adds a
  concurrent writer alongside the Rx path.

- On E810 the legacy read now hits "Failed to read phy timestamp" at
  ERR on every poll until the hardware writes the slot. ptpclient
  and ieee1588fwd both poll in loops, so this floods the log. Log at
  DEBUG and return -EAGAIN.

- "(uint32_t)(uint16_t)idx" double cast. Magic 64 and 63 should be a
  define. Missing space in "RTE_ATOMIC(uint64_t)ts_slot_bitmap;".


Patch 5/5 app/testpmd: add Tx timestamp capabilities command

Warning:

- The "show" command calls rte_eth_timesync_tx_slot_alloc() and
  release(). A show command should not change device state; on E810
  the release zeroes a PHY entry. If a forwarding engine holds every
  slot, the output is "Alloc test: failed (-28)". Drop the
  round-trip.

- testpmd_funcs.rst and the testpmd help text are not updated.

- stamp, read and dynfield_register cannot be reached from testpmd.
  ieee1588fwd is the natural place to use the slot API when the port
  reports PER_PACKET.

Info:

- There is no port_id_is_invalid(res->port_id, ENABLED_WARN) check.
  Print errors with strerror(-ret).