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

On 02-09-2026 07:43 pm, Stephen Hemminger wrote:

On Wed,  2 Sep 2026 11:21:22 +0530
Rajesh Kumar [off-list ref] wrote:

quoted

+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_timestamp_slot_alloc, 26.11)
+int
+rte_eth_timesync_tx_timestamp_slot_alloc(uint16_t port_id,
+					 uint32_t *slot_id)

Could join to one line, max line line is now 100

Acked. Fixed in v5.

quoted

+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (slot_id == NULL) {
+		RTE_ETHDEV_LOG_LINE(ERR,
+			"Cannot allocate ethdev port %u Tx timestamp slot to NULL",
+			port_id);

Minor nit the wording of that error message is awkward.
Similar problem in other messages.

Acked. Fixed in v5.

quoted

+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_slot_dynfield_register, 26.11)
+int
+rte_eth_timesync_tx_slot_dynfield_register(void)
+{
+	const struct rte_mbuf_dynfield slot_dynfield = {
+		.name  = RTE_ETH_TIMESYNC_TX_SLOT_DYNFIELD_NAME,
+		.size  = sizeof(uint32_t),
+		.align = alignof(uint32_t),
+	};
+	uint16_t port_id;
+
+	if (rte_eth_timesync_tx_slot_dynfield_offset >= 0)
+		return 0;
+
+	rte_eth_timesync_tx_slot_dynfield_offset =
+			rte_mbuf_dynfield_register(&slot_dynfield);
+	if (rte_eth_timesync_tx_slot_dynfield_offset < 0)
+		rte_eth_timesync_tx_slot_dynfield_offset =
+				rte_mbuf_dynfield_lookup(
+					RTE_ETH_TIMESYNC_TX_SLOT_DYNFIELD_NAME, NULL);
+	if (rte_eth_timesync_tx_slot_dynfield_offset < 0)
+		return -ENOTSUP;
+
+	{
+		int flag_bit = rte_mbuf_dynflag_register(
+			&(const struct rte_mbuf_dynflag){
+				.name = RTE_ETH_TIMESYNC_TX_SLOT_DYNFLAG_NAME});
+		if (flag_bit < 0)
+			flag_bit = rte_mbuf_dynflag_lookup(
+				RTE_ETH_TIMESYNC_TX_SLOT_DYNFLAG_NAME, NULL);
+		if (flag_bit < 0)
+			return -ENOTSUP;
+		rte_eth_timesync_tx_slot_dynflag = RTE_BIT64(flag_bit);
+	}

No need for basic block {} here.

Acked. Fixed in v5.

quoted

+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_slot_dynfield_unregister, 26.11)
+int
+rte_eth_timesync_tx_slot_dynfield_unregister(void)
+{
+	uint16_t port_id;
+
+	/* Reset cached state without freeing dynamic-field bytes. */
+	rte_eth_timesync_tx_slot_dynfield_offset = -1;
+	rte_eth_timesync_tx_slot_dynflag = 0;
+
+	RTE_ETH_FOREACH_VALID_DEV(port_id)
+		eth_timesync_tx_slot_info_refresh(port_id);
+
+	return 0;
+}
+

If it always returns 0 why not void.
Not sure what the point of this function is. It doesn't really do anything.

Acked, removed this function in v5.

quoted

+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_timestamp_stamp_mbuf, 26.11)
+int
+rte_eth_timesync_tx_timestamp_stamp_mbuf(uint16_t port_id,
+						 uint32_t slot_id, struct rte_mbuf *m)
+{
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	if (m == NULL)
+		return -EINVAL;
+	if (rte_eth_timesync_tx_slot_dynfield_register() != 0)
+		return -ENOTSUP;
+	*RTE_MBUF_DYNFIELD(m, rte_eth_timesync_tx_slot_dynfield_offset,
+			   uint32_t *) = slot_id;
+	m->ol_flags |= rte_eth_timesync_tx_slot_dynflag;
+	return 0;
+}

This is possibly in data path, use unlikely() here.

Acked. Fixed in v5.

quoted

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index ee400b386f..bde391dea4 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h

@@ -5513,6 +5513,19 @@ int rte_eth_timesync_read_rx_timestamp(uint16_t port_id,
  /**
   * Read an IEEE1588/802.1AS Tx timestamp from an Ethernet device.
   *
+ * This is the legacy Tx timestamp API and is intended for register-based
+ * timestamp reads. It does not provide per-packet correlation.
+ *

Rather than weak guidance which will get ignored and stale.
   1. Convert all in-tree uses of old API
   2. Announce deprecation in this release
   3. Mark legacy API as deprecated

The slot APIs are additive, not a drop-in replacement for 
rte_eth_timesync_read_tx_timestamp(). Existing hardware/PMDs may expose 
only a single TX timestamp latch and cannot implement per-packet slots. 
We will not label or deprecate the existing API in this series. Instead, 
the new capability-query API lets applications select slot-based 
timestamping when supported. Removed the “legacy” wording from the 
existing API documentation.

AI had even more observations (Fable 5.1)

quoted

+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_slot_infos, 26.11)
+struct rte_eth_timesync_tx_slot_info
+rte_eth_timesync_tx_slot_infos[RTE_MAX_ETHPORTS];

Exporting a RTE_MAX_ETHPORTS sized array from the public header bakes
build config into ABI. rte_eth_fp_ops lives in ethdev_driver.h, this
should too; only PMDs read it.

The per-port array also has no per-port content. offset and dynflag are
process globals; the only per-port part is "caps say PER_PACKET". Put
the two globals in ethdev_driver.h and let the PMD that implements
slots check them. Drops the array, the refresh loop, and the forward
declaration.

quoted

+static void eth_timesync_tx_slot_info_refresh(uint16_t port_id);

Move the definitions above first use instead.

Acked. this function is dropped in v5.

quoted

+	ret = eth_err(port_id, dev->dev_ops->timesync_enable(dev));
+	if (ret == 0)
+		eth_timesync_tx_slot_info_refresh(port_id);

No matching reset in timesync_disable. Info stays stale after disable.

Acked. in v5, eth_timesync_tx_slot_info_refresh itself is dropped 
entirely, we don't need to call in timesync enable/disable

quoted

+int
+rte_eth_timesync_tx_timestamp_stamp_mbuf(uint16_t port_id,
+						 uint32_t slot_id, struct rte_mbuf *m)
+{
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	if (m == NULL)
+		return -EINVAL;
+	if (rte_eth_timesync_tx_slot_dynfield_register() != 0)
+		return -ENOTSUP;

Calling register from the per-packet path is wrong. First call takes
the mbuf dyn lock and walks every port calling into driver dev_ops.
Header says "safe for concurrent callers"; it is not, two threads
racing on first stamp both run registration on plain globals.

port_id is validated but otherwise unused. Stamping a SINGLE_REG port
succeeds and sets a flag nothing reads. Check the port's slot info,
return -ENOTSUP if dynflag == 0, and require the app to have called
register up front (which the doc already says it must, before pool
create).

Acked. in v5 register is no longer being called from per-packet path.

quoted

+	rte_eth_timesync_tx_slot_dynfield_offset = -1;
+	rte_eth_timesync_tx_slot_dynflag = 0;

Written unlocked, read from Tx datapath on other cores. Also cannot
free the dynfield. Agree with dropping unregister entirely.

Acked. Droped the unregister entirely in v5.

quoted

+ * -ENOTSUP and the PMD TX path falls back to the port-level ptp_tx_index
+ * (legacy mode) on every port.

ptp_tx_index is an Intel driver internal. Does not belong in rte_ethdev.h.

quoted

+ * The underlying DPDK dynfield bytes are NOT freed — DPDK provides no dynfield

Non-ASCII dash in source.

Acked. Fixed in v5.

quoted

+typedef int (*eth_timesync_tx_ts_get_caps_t)(struct rte_eth_dev *dev,

...

quoted

+	eth_timesync_tx_ts_get_caps_t timesync_tx_ts_get_capabilities;

...

quoted

+int rte_eth_timesync_tx_timestamp_slot_get_capabilities(uint16_t port_id,

...

quoted

+int rte_eth_timesync_read_tx_timestamp_slot(uint16_t port_id,

Three spellings of the same op, and the read function breaks the
rte_eth_timesync_tx_timestamp_slot_* prefix the release note
advertises. One prefix for all of it, rte_eth_timesync_tx_slot_{caps,
alloc,read,release,stamp} is shorter and consistent.

Also TX/Tx mixed throughout comments and docs. Tx.

Acked. Fixed in v5.

quoted

+struct rte_eth_timesync_dual_domain_timestamp {
+	int64_t adjusted_ns;
+	int64_t raw_ns;
+	uint32_t valid_mask;
+};

4 byte tail hole. Fine for experimental, but say so or reorder before
it goes stable.

Acked. added 4 byte reserved field to fix 4 byte tail hole.

quoted

+++ b/doc/guides/prog_guide/ethdev/timesync.rst

Lines up to 150+ chars. Doc guideline is one sentence per line. Half
of this file documents existing clock/Rx API, which is a separate
patch from the slot feature.

  Acked. Created a separate patch in the series to add documentation for 
existing clock/Rx API