|SUCCESS| [v1,2/2] net/iavf: reuse device alarm for PHC sync

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162821

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-02

git sha 20260402152137.1527322-2-soumyadeep.hore at intel.com
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>
Subject: net/iavf: remove PHC polling from Rx datapath

This patch removes periodic PHC read/update checks from scalar and
vector flex RX paths, keeping timestamp conversion based on queue PHC
state. This avoids hot-path PHC polling overhead and preserves the
latency fix for RX timestamp-enabled traffic.

> diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
> index e621d4bf47..76615f39e8 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx.c

[ ... ]

> @@ -1585,7 +1575,6 @@ iavf_recv_pkts_flex_rxd(void *rx_queue,
>  				rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high));
>  
>  			rxq->phc_time = ts_ns;
> -			rxq->hw_time_update = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);

Does this code still update rxq->hw_time_update anywhere after removing
this line? If the alarm-based sync in patch 2/2 is the only update path,
and if it only updates rxq[0], does that leave hw_time_update stale for
other queues?

> @@ -1755,7 +1734,6 @@ iavf_recv_scattered_pkts_flex_rxd(void *rx_queue, struct rte_mbuf **rx_pkts,
>  				rte_le_to_cpu_32(rxd.wb.flex_ts.ts_high));
>  
>  			rxq->phc_time = ts_ns;
> -			rxq->hw_time_update = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);

Same concern as above in iavf_recv_scattered_pkts_flex_rxd().

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> index 2e18be3616..a688ad4230 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c

[ ... ]

> @@ -514,18 +514,10 @@ _iavf_recv_raw_pkts_vec_avx2_flex_rxd(struct ci_rx_queue *rxq,
>  	if (!(rxdp->wb.status_error0 &
>  			rte_cpu_to_le_32(1 << IAVF_RX_FLEX_DESC_STATUS0_DD_S)))
>  		return 0;
> -	bool is_tsinit = false;
>  	uint8_t inflection_point = 0;
>  	__m256i hw_low_last = _mm256_set_epi32(0, 0, 0, 0, 0, 0, 0, rxq->phc_time);
>  	if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> -		uint64_t sw_cur_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
> -
> -		if (unlikely(sw_cur_time - rxq->hw_time_update > 4)) {
> -			hw_low_last = _mm256_setzero_si256();
> -			is_tsinit = 1;
> -		} else {
> -			hw_low_last = _mm256_set_epi32(0, 0, 0, 0, 0, 0, 0, rxq->phc_time);
> -		}
> +		hw_low_last = _mm256_set_epi32(0, 0, 0, 0, 0, 0, 0, rxq->phc_time);
>  	}

The removed code guarded against stale rxq->phc_time by checking
hw_time_update. Without this check, does stale PHC data get used for
timestamp conversion if the alarm hasn't fired recently?

> @@ -1152,10 +1144,8 @@ _iavf_recv_raw_pkts_vec_avx2_flex_rxd(struct ci_rx_queue *rxq,
>  					*RTE_MBUF_DYNFIELD(rx_pkts[i + 7],
>  						iavf_timestamp_dynfield_offset, uint32_t *) = _mm256_extract_epi32(ts_low1, 7);
>  
> -					if (unlikely(is_tsinit)) {
> +					{
>  						uint32_t in_timestamp;
> -						if (iavf_get_phc_time(rxq))
> -							PMD_DRV_LOG(ERR, "get physical time failed");
>  						in_timestamp = *RTE_MBUF_DYNFIELD(rx_pkts[i + 0],
>  								iavf_timestamp_dynfield_offset, uint32_t *);
>  						rxq->phc_time = iavf_tstamp_convert_32b_64b(rxq->phc_time, in_timestamp);

Does this code now always run the timestamp conversion logic even when
the previous is_tsinit guard is removed? The unconditional braces hint
that dead code is being left behind.

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> index 9a93a0b062..7fc3ba8956 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c

[ ... ]

> @@ -1343,11 +1335,9 @@ _iavf_recv_raw_pkts_vec_avx512_flex_rxd(struct ci_rx_queue *rxq,
>  					*RTE_MBUF_DYNFIELD(rx_pkts[i + 7],
>  						iavf_timestamp_dynfield_offset, uint32_t *) = _mm256_extract_epi32(ts_low1, 7);
>  
> -					if (unlikely(is_tsinit)) {
> +					{
>  						uint32_t in_timestamp;
>  
> -						if (iavf_get_phc_time(rxq))
> -							PMD_DRV_LOG(ERR, "get physical time failed");
>  						in_timestamp = *RTE_MBUF_DYNFIELD(rx_pkts[i + 0],
>  										iavf_timestamp_dynfield_offset, uint32_t *);
>  						rxq->phc_time = iavf_tstamp_convert_32b_64b(rxq->phc_time, in_timestamp);

Same unconditional execution concern as in the avx2 path. Does removing
the is_tsinit check leave dead or unconditionally executed conversion
code?

---

git sha 20260402152137.1527322-3-soumyadeep.hore at intel.com
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>
Subject: net/iavf: reuse device alarm for PHC sync

This patch reuses existing iavf device alarm cadence to drive periodic
PHC sync instead of a dedicated PHC alarm callback. Keep PHC start/stop
hooks as pause/resume controls around queue reconfiguration and device
lifecycle paths.

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index 39949acc11..caba5b49cd 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h

[ ... ]

> @@ -383,6 +384,8 @@ struct iavf_adapter {
>  	enum iavf_rx_func_type rx_func_type;
>  	enum iavf_tx_func_type tx_func_type;
>  	uint16_t fdir_ref_cnt;
> +	uint8_t phc_sync_ticks;
> +	bool phc_sync_paused;
>  	struct iavf_devargs devargs;
>  };

Does the phc_sync_ticks counter overflow after 255 iterations of the
alarm handler? With IAVF_ALARM_INTERVAL at 50000 us, this seems like
it would happen.

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 802e095174..1cb78e2f36 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -2741,10 +2749,70 @@ iavf_dev_alarm_handler(void *param)
>  
>  	iavf_enable_irq0(hw);
>  
> +	if (iavf_phc_sync_alarm_needed(dev) && !adapter->phc_sync_paused) {
> +		adapter->phc_sync_ticks++;
> +		if (adapter->phc_sync_ticks >=
> +		    IAVF_PHC_SYNC_ALARM_INTERVAL_US / IAVF_ALARM_INTERVAL) {
> +			struct ci_rx_queue *rxq = dev->data->rx_queues[0];
> +
> +			adapter->phc_sync_ticks = 0;
> +			if (iavf_get_phc_time(rxq) == 0)
> +				rxq->hw_time_update = rte_get_timer_cycles() /
> +					(rte_get_timer_hz() / 1000);
> +		}
> +	} else {
> +		adapter->phc_sync_ticks = 0;
> +	}

Does this code only update PHC state for dev->data->rx_queues[0]? What
happens if dev->data->rx_queues[0] is not configured or if other queues
need the PHC update?

Does iavf_phc_sync_alarm_needed()->rx_queues[0] check occur before or
after this code dereferences rx_queues[0]? Could there be a race
condition or null pointer dereference?

Does the condition (iavf_get_phc_time(rxq) == 0) silently ignore PHC
read failures by not logging them, unlike the old code paths that called
PMD_DRV_LOG(ERR, "get physical time failed")?

> +static bool
> +iavf_phc_sync_alarm_needed(struct rte_eth_dev *dev)
> +{
> +	struct iavf_adapter *adapter;
> +
> +	adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	if (adapter->closed || adapter->stopped)
> +		return false;
> +
> +	if (!(dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP))
> +		return false;
> +
> +	if (dev->data->nb_rx_queues == 0 || dev->data->rx_queues[0] == NULL)
> +		return false;
> +
> +	return true;
> +}

Does this function properly handle the case where rx_queues[0] is NULL
even though nb_rx_queues > 0? Does checking only rx_queues[0] cover all
configurations?

> +void
> +iavf_phc_sync_alarm_start(struct rte_eth_dev *dev)
> +{
> +	struct iavf_adapter *adapter;
> +
> +	if (!iavf_phc_sync_alarm_needed(dev))
> +		return;
> +
> +	adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	adapter->phc_sync_paused = false;
> +	adapter->phc_sync_ticks = 0;
> +}

Does iavf_phc_sync_alarm_start() need any error handling or
synchronization when unsetting phc_sync_paused? Could there be a race
with iavf_dev_alarm_handler() checking phc_sync_paused?

> +void
> +iavf_phc_sync_alarm_stop(struct rte_eth_dev *dev)
> +{
> +	struct iavf_adapter *adapter;
> +
> +	if (dev == NULL || dev->data == NULL || dev->data->dev_private == NULL)
> +		return;
> +
> +	adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	adapter->phc_sync_paused = true;
> +	adapter->phc_sync_ticks = 0;
> +}

Does iavf_phc_sync_alarm_stop() need synchronization to prevent a race
with iavf_dev_alarm_handler() that might be in the middle of updating
PHC state when phc_sync_paused is set?

> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
> index 9ad39300c6..4bd51dcc21 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c
> @@ -2091,12 +2091,16 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
>  	args.out_size = IAVF_AQ_BUF_SZ;
>  
>  	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
> +		iavf_phc_sync_alarm_stop(dev);
>  		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
> +		iavf_phc_sync_alarm_start(dev);
>  	} else {
>  		rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
> +		iavf_phc_sync_alarm_stop(dev);
>  		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
>  		rte_eal_alarm_set(IAVF_ALARM_INTERVAL,
>