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

[Mohammad Heib <mheib@redhat.com>]

On Mon, Sep 21, 2026 at 09:32:21PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The new devlink `atr_sample_rate` setter
>   (drivers/net/ethernet/intel/i40e/i40e_devlink.c:36-46) makes the value…
> - [Medium] The new documentation
>   (Documentation/networking/devlink/i40e.rst, lines 54-59) states that…
> - [Low] In the new documentation entry the lines 'Setting to ``0``
>   disables ATR sampling (no filters will be programmed)' and 'The…
> 
> --
> 
> i40e: add devlink parameter for Flow Director ATR sample rate
> 
> This adds a runtime devlink parameter, atr_sample_rate, that lets the
> Flow Director ATR sampling interval be changed without rebuilding the
> driver.  The value is stored in pf->atr_sample_rate and read by the Tx
> path, and a new entry documents it in the i40e devlink documentation.
> 
> > diff --git a/Documentation/networking/devlink/i40e.rst b/Documentation/networking/devlink/i40e.rst
> > index 51c887f0dc833..2cea98b631baa 100644
> > --- a/Documentation/networking/devlink/i40e.rst
> > +++ b/Documentation/networking/devlink/i40e.rst
> > @@ -40,6 +40,26 @@ Parameters
> >  
> >          The default value is ``0`` (internal calculation is used).
> >  
> > +.. list-table:: Driver specific parameters implemented
> > +    :widths: 5 5 90
> > +
> > +    * - Name
> > +      - Mode
> > +      - Description
> 
> [Severity: Low]
> This isn't a bug, but the third column here is labelled Description while
> the generic parameter table above it in the same file uses Notes.  Was the
> difference intended?
Fixed in v5, changed to Notes for consistency.
> 
> > +    * - ``atr_sample_rate``
> > +      - runtime
> > +      - Controls how frequently Flow Director ATR updates flow steering
> > +        information for active TCP flows.
> > +
> > +        ATR programs Flow Director entries based on sampled transmitted
> > +        packets. The sampling interval is specified as the number of
> > +        transmitted packets between ATR updates.
> > +
> > +        Lower values increase Flow Director programming activity, while
> > +        higher values reduce the update frequency.
> 
> [Severity: Medium]
> Is "the number of transmitted packets between ATR updates" the unit the
> code actually implements?  Three details in i40e_atr() seem to differ.
> 
> The counter compared against the configured value is per Tx ring, not
> per device:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	tx_ring->atr_count++;
> 
> 	/* sample on all syn/fin/rst packets or once every atr sample rate */
> 	if (!th->fin &&
> 	    !th->syn &&
> 	    !th->rst &&
> 	    (tx_ring->atr_count < sample_rate))
> 		return;
> 	...
> }
> 
> atr_count lives in struct i40e_ring, so on an N-queue device the
> aggregate programming rate is up to N times what a device-wide packet
> interval would suggest.  Given the commit message mentions "systems with
> a large number of queues", should the documentation say the interval is
> per Tx queue?
> 
> The increment is also only reached for TCP after the IP version and
> protocol checks:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	if (l4_proto != IPPROTO_TCP)
> 		return;
> 	...
> }
> 
> so non-TCP transmitted packets never advance the interval.
> 
> Finally, the threshold is bypassed for SYN, FIN and RST, so raising the
> value does not reduce programming activity for connection-churn
> workloads.  Should the text mention that exception?
> 
> > +
> > +        Setting to ``0`` disables ATR sampling (no filters will be programmed)
> > +        The default value is ``20``.
> 

Fixed in v5. The documentation now says "TCP packets per Tx queue" and
clarifies the interval is per queue, not per device.

> [Severity: Low]
> These two lines have no blank line between them and the first one has no
> terminating period, so reStructuredText joins them into a single
> paragraph and renders as "... (no filters will be programmed) The default
> value is 20.".  The max_mac_per_vf entry just above puts a blank line
> before its "The default value is ..." sentence.
> 
> >  
> >  Info versions
> >  =============
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_devlink.c b/drivers/net/ethernet/intel/i40e/i40e_devlink.c
> > index 229179ccc1319..6df7eb217073d 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;
> > +
> > +	WRITE_ONCE(pf->atr_sample_rate, sample_rate);
> > +	return 0;
> > +}
> 
> [Severity: Medium]
> Setting the value to 0 is now reachable at runtime while
> I40E_FLAG_FD_ATR_ENA stays set.  Does this leave already programmed ATR
> entries resident in the Flow Director table with no way to remove them?
> 

Fixed in v5. The devlink setter now flushes existing ATR entries when
rate is set to 0, using the same mechanism as the ethtool ATR-disable
path (__I40E_FD_ATR_AUTO_DISABLED + __I40E_FD_FLUSH_REQUESTED).
> In i40e_atr() the zero-rate return happens before the FIN/RST handling
> and before the descriptor build that emits the remove command:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	/* if sampling is disabled do nothing */
> 	sample_rate = READ_ONCE(pf->atr_sample_rate);
> 	if (!sample_rate)
> 		return;
> 	...
> 	dtype_cmd |= (th->fin || th->rst) ?
> 		     (I40E_FILTER_PROGRAM_DESC_PCMD_REMOVE <<
> 		      I40E_TXD_FLTR_QW1_PCMD_SHIFT) :
> 	...
> }
> 
> That PCMD_REMOVE descriptor is the only software removal path, and it is
> only redundant when hardware eviction is available:
> 
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_atr() {
> 	...
> 	if (test_bit(I40E_FLAG_HW_ATR_EVICT_ENA, pf->flags)) {
> 		/* HW ATR eviction will take care of removing filters on FIN
> 		 * and RST packets.
> 		 */
> 	...
> }
> 
> I40E_FLAG_HW_ATR_EVICT_ENA is only set in i40e_sw_init() when
> pf->hw.caps has I40E_HW_CAP_ATR_EVICT, and it can also be cleared by the
> hw-atr-eviction ethtool private flag.  On such a device, after any TCP
> traffic has programmed ATR filters, a
> 
>   devlink dev param set pci/<bdf> name atr_sample_rate value 0 cmode runtime
> 
> appears to leave those entries steering received packets for flows that
> have since closed, while fd_atr_cnt and i40e_get_current_atr_cnt() still
> report ATR as active.
> 
> The existing way to turn ATR off via the flow-director-atr private flag
> does the teardown explicitly:
> 
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c:i40e_set_priv_flags() {
> 	...
> 	/* Flush current ATR settings if ATR was disabled */
> 	if (test_bit(I40E_FLAG_FD_ATR_ENA, changed_flags) &&
> 	    !test_bit(I40E_FLAG_FD_ATR_ENA, new_flags)) {
> 		set_bit(__I40E_FD_ATR_AUTO_DISABLED, pf->state);
> 		set_bit(__I40E_FD_FLUSH_REQUESTED, pf->state);
> 	}
> 	...
> }
> 
> Should the devlink setter do the same when the new rate is 0, or should
> 0 be rejected by a validate callback instead?  Note the later patch in
> the series that triggers a PF reset when re-enabling ATR via ethtool only
> touches i40e_set_priv_flags(), so it does not add this flush.
> 
> > +
> > +static int i40e_atr_sample_rate_get(struct devlink *devlink,
> 
> [ ... ]
> 
> >  static const struct devlink_param i40e_dl_params[] = {
> 
> [ ... ]
> 
> > +	DEVLINK_PARAM_DRIVER(I40E_DEVLINK_PARAM_ID_ATR_SAMPLE_RATE,
> > +			     "atr_sample_rate",
> > +			     DEVLINK_PARAM_TYPE_U32,
> > +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> > +			     i40e_atr_sample_rate_get,
> > +			     i40e_atr_sample_rate_set,
> > +			     NULL),
> >  };
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917204541.3072415-1-anthony.l.nguyen%40intel.com