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

["Kumar, Rajesh" <rajesh3.kumar@intel.com>]

On 02-09-2026 07:43 pm, Stephen Hemminger wrote:
> 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
Acked. Fixed in v5.
>
>> +{
>> +	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.
>
>> +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.

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

>
>> +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.
>
>
>> 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)
>
>> +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.
Acked. this function is dropped in v5.
>
>> +	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
>
>> +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.
>
>> +	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.

>
>> + * -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.
Acked. Fixed in v5.
>
>> +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.

Acked. Fixed in v5.

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