|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

Review of patch series for net/iavf PHC polling removal and alarm reuse:

Commit: 20260402152137.1527322-2-soumyadeep.hore at intel.com
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>
Subject: [PATCH v1 1/2] 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. The
intent is to avoid hot-path PHC polling overhead while preserving 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
> @@ -1507,16 +1507,6 @@ iavf_recv_pkts_flex_rxd(void *rx_queue,
>  	rx_ring = rxq->rx_flex_ring;
>  	ptype_tbl = rxq->iavf_vsi->adapter->ptype_tbl;
>  
> -	if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> -		uint64_t sw_cur_time = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
> -
> -		if (sw_cur_time - rxq->hw_time_update > 4) {
> -			if (iavf_get_phc_time(rxq))
> -				PMD_DRV_LOG(ERR, "get physical time failed");
> -			rxq->hw_time_update = sw_cur_time;
> -		}
> -	}
> -

[ ... ]

> @@ -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);

This removal of the hw_time_update assignment appears in multiple
iavf_recv_* functions. Does this leave rxq->hw_time_update stale
indefinitely when timestamps are not present in incoming packets? If the
background alarm from patch 2/2 is somehow delayed or missed, can
timestamp conversion operate correctly with an unbounded staleness in
rxq->phc_time?

[ ... ]

> 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);
>  	}

[ ... ]

> @@ -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");

The removal of is_tsinit and the unconditional braces looks unusual. Does
the newly unconditional block unnecessarily perform the inflection point
scan and wrap detection on every burst? What is the expected performance
impact of removing the is_tsinit guard?

[ ... ]

> @@ -1388,8 +1378,6 @@ _iavf_recv_raw_pkts_vec_avx2_flex_rxd(struct ci_rx_queue *rxq,
>  				PMD_DRV_LOG(ERR, "invalid inflection point for rx timestamp");
>  				break;
>  			}
> -
> -			rxq->hw_time_update = rte_get_timer_cycles() / (rte_get_timer_hz() / 1000);
>  		}

Similar concern: does this removal of hw_time_update assignment leave the
field stale for an arbitrary time? Can timestamp conversions drift or
fail silently when hw_time_update is not refreshed until the background
alarm runs?

[ ... ]

Commit: 20260402152137.1527322-3-soumyadeep.hore at intel.com
Author: Soumyadeep Hore <soumyadeep.hore at intel.com>
Subject: [PATCH v1 2/2] 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, and adds 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
> @@ -76,6 +76,7 @@
>  #define IAVF_QUEUE_ITR_INTERVAL_MAX     8160 /* 8160 us */
>  
>  #define IAVF_ALARM_INTERVAL 50000 /* us */
> +#define IAVF_PHC_SYNC_ALARM_INTERVAL_US 200000
>  
>  /* The overhead from MTU to max frame size.
>   * Considering QinQ packet, the VLAN tag needs to be counted twice.
> @@ -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;

Does the phc_sync_paused boolean require any synchronization or memory
ordering guarantees? Can iavf_dev_alarm_handler()->iavf_phc_sync_alarm_needed()
race with iavf_phc_sync_alarm_stop() if stop is called from a control path
while the alarm callback is executing?

[ ... ]

> 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
> @@ -21,6 +21,7 @@
>  #include <rte_pci.h>
>  #include <rte_alarm.h>
>  #include <rte_atomic.h>
> +#include <rte_cycles.h>

[ ... ]

> @@ -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 iavf_phc_sync_alarm_needed()->dev->data->rx_queues[0] check prevent
a NULL pointer dereference here? If queue 0 gets freed or reassigned
between the check and the rxq dereference, can a use-after-free occur?

Can iavf_dev_alarm_handler() execute concurrently with queue
reconfiguration paths that modify dev->data->rx_queues? If so, does the
code require locking or ordering to prevent observing a partially updated
queue pointer?

[ ... ]

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

Is the check for dev->data->rx_queues[0] != NULL sufficient to prevent
use-after-free in the caller? Can rx_queues[0] become NULL or point to
freed memory after this function returns but before the caller dereferences
it?

[ ... ]

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

If iavf_phc_sync_alarm_start() executes after iavf_dev_alarm_handler() has
already read phc_sync_paused as true but before it re-arms the alarm, can
the phc_sync_ticks reset cause a skipped PHC sync interval?

[ ... ]

> 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,
>  				  iavf_dev_alarm_handler, dev);
> +		iavf_phc_sync_alarm_start(dev);
>  	}

In the else branch, does iavf_phc_sync_alarm_stop() execute after
rte_eal_alarm_cancel() prevent the alarm from running? Can the alarm
callback execute in a narrow window between alarm_cancel and
phc_sync_alarm_stop, potentially observing phc_sync_paused==false?

Does the ordering of alarm_set() followed by phc_sync_alarm_start() ensure
the alarm callback sees the updated phc_sync_paused state, or can a race
occur if the callback runs before phc_sync_alarm_start() completes?