Re: [PATCH net-next v12] net: reduce ARFS flow updates by checking LLC affinity

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] set_rps_cpu() (net/core/dev.c:5281-5284) decides whether to
  skip ARFS hardware steering by comparing next_cpu with old_cpu.

--

net: reduce ARFS flow updates by checking LLC affinity

This patch makes set_rps_cpu() skip the ARFS ndo_rx_flow_steer() call
when the CPU recorded for the flow and the new target CPU share an LLC.
rflow->cpu is still updated. A new sysctl, net.core.arfs_llc_affinity,
turns this on and off, and it is off by default.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3b60b15e2175e..0011cec296402 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -5273,6 +5277,12 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>  		if (!skb_rx_queue_recorded(skb) || !dev->rx_cpu_rmap ||
>  		    !(dev->features & NETIF_F_NTUPLE))
>  			goto out;
> +
> +		if (static_branch_unlikely(&arfs_llc_affinity) &&
> +		    old_cpu < nr_cpu_ids && cpu_online(old_cpu) &&
> +		    cpus_share_cache(old_cpu, next_cpu))
> +			goto out;
> +

[Severity: Medium]
Does rflow->cpu reliably show which LLC services the hardware queue the
flow is arriving on?

get_rps_cpu() takes old_cpu from the hash slot in the arrival queue's
table:

	tcpu = rflow->cpu;
	...
		rflow = set_rps_cpu(dev, skb, rflow, tcpu, next_cpu, hash);

That value is software bookkeeping, and other flows can share the slot.
The new check runs before the cpu_rmap_lookup_index() /
skb_get_rx_queue() comparison. So it can suppress ndo_rx_flow_steer()
even when the arrival queue's IRQ/NAPI CPU is in a different LLC from
next_cpu.

The skip path still reaches:

	WRITE_ONCE(rflow->cpu, next_cpu);

sd_llc_id equality is transitive, so every later migration inside that
LLC would skip again. Could the flow then stay on a far queue
indefinitely?

Here are some ways rflow->cpu can drift away from the real hardware
queue:

- After a successful steer from queue A to queue B, packets still in
  flight on A go through set_rps_cpu() again and hit:

		if (hash != READ_ONCE(tmp_rflow->hash) ||
		    next_cpu == tmp_cpu)
			goto out;

  This records next_cpu (LLC X) in A's slot, but A's IRQ may be in LLC
  Y. Two later cases then hit the new skip, with the application on
  another CPU in LLC X. One is a flow whose hash collides with that slot
  and which RSS delivers on A. The other is the original flow coming
  back on A after rps_may_expire_flow() let the driver drop its rule.
  No failure is needed for this to happen.

- The existing goto out paths still record next_cpu without moving the
  hardware queue. These are ndo_rx_flow_steer() returning rc < 0 (for
  example -ENOMEM, -EPERM or -EPROTONOSUPPORT from
  mlx5e_rx_flow_steer()), a target queue with no flow table, and an
  active colliding filter. Before this patch the next migration retried
  the steer. With the sysctl on, migrations within the same LLC never
  retry.

- irqbalance or an admin moves queue A's IRQ to another LLC. cpu_rmap
  gets updated, but the skip happens before cpu_rmap is consulted.

- A driver drops all of its hardware rules without touching
  rps_dev_flow, for example mlx5e_arfs_disable()->arfs_del_rules().

In each case NAPI would process the packets on a remote LLC, and they
would then go through the RPS backlog to the application's CPU with an
IPI.

This also seems to conflict with the new text in
Documentation/networking/scaling.rst, and with the similar wording in the
commit message:

  "since the application's cache locality is preserved either way"

The reported ~99% drop in mlx5e_rx_flow_steer calls cannot tell avoided
redundant updates apart from steering that is wrongly suppressed.

Would it be more accurate to compare next_cpu with the CPU servicing the
arrival queue instead of the previously recorded rflow->cpu? For example,
that could be smp_processor_id() for non-threaded NAPI, or the CPU tied
to the queue from skb_get_rx_queue(skb).

>  		rxq_index = cpu_rmap_lookup_index(dev->rx_cpu_rmap, next_cpu);
>  		if (rxq_index == skb_get_rx_queue(skb))
>  			goto out;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109030042.2956-1-nashuiliang%40gmail.com