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

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed,  2 Sep 2026 11:21:22 +0530
Rajesh Kumar <rajesh3.kumar@intel.com> wrote:

> +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

> +{
> +	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.

> +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.

> +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.

> +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.


> 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

AI had even more observations (Fable 5.1)

> +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.

> +static void eth_timesync_tx_slot_info_refresh(uint16_t port_id);

Move the definitions above first use instead.

> +	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.

> +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).

> +	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.

> + * -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.

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

Non-ASCII dash in source.

> +typedef int (*eth_timesync_tx_ts_get_caps_t)(struct rte_eth_dev *dev,
...
> +	eth_timesync_tx_ts_get_caps_t timesync_tx_ts_get_capabilities;
...
> +int rte_eth_timesync_tx_timestamp_slot_get_capabilities(uint16_t port_id,
...
> +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.

> +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.

> +++ 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.