Re: [PATCH iwl-next v3 2/2] i40e: add devlink parameter for Flow Director ATR sample rate

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
i40e: add devlink parameter for Flow Director ATR sample rate

This commit introduces a runtime devlink parameter to tune the Flow Director
ATR sample rate. This allows dynamic adjustments to the sampling interval
without rebuilding the driver, which can help reduce TCP packet reordering
on systems with high-rate workloads.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> index 229179ccc1319..cf487efdd803d 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
[ ... ]
> @@ -33,12 +33,48 @@ static int i40e_max_mac_per_vf_get(struct devlink *devlink,
>  	return 0;
>  }
>  
> +static int i40e_atr_sample_rate_set(struct devlink *devlink,
> +				    u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct i40e_pf *pf = devlink_priv(devlink);
> +	u32 sample_rate = ctx->val.vu32;
> +
> +	pf->atr_sample_rate = sample_rate;

[Severity: Low]
Does modifying pf->atr_sample_rate concurrently with the Tx fast path
introduce a data race?

In the Tx fast path, i40e_atr() reads this value locklessly. If the devlink
callback sets this to 0 between the two reads in i40e_atr():

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
        if (!pf->atr_sample_rate)
                return;
        ...
        if (tx_ring->atr_count < pf->atr_sample_rate)
        ...
}

Could this race lead to a situation where the second check evaluates
tx_ring->atr_count against 0, unintentionally programming an extra Flow
Director filter?

While the functional impact might just be a single extra sampled packet,
should we use READ_ONCE() in the fast path and WRITE_ONCE() here to avoid
KCSAN data race warnings and ensure a consistent value is used throughout
the function?

> +	return 0;
> +}

[ ... ]