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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 4

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] When the new LLC shortcut in set_rps_cpu()
  (net/core/dev.c:5196-5199) fires, it skips ndo_rx_flow_steer().
- [Medium] The new check in set_rps_cpu() relies on
  cpus_share_cache(old_cpu, next_cpu), which in this tree compares raw…
- [Low] The new comment in set_rps_cpu() says 'Reduce RFS/ARFS flow
  updates by checking LLC affinity' and 'This strategy only updates the…
- [Low] The patch adds a user-visible sysctl,
  /proc/sys/net/core/rps_feat_llc_affinity (net_core_table,…
- [Low] The aRFS section of Documentation/networking/scaling.rst (lines
  443-448) says the stack 'automatically calls this function…
- [Low] The rps_feat_llc_affinity static key (dev.c:5125) and its sysctl
  (sysctl_net_core.c:563-569) are built under CONFIG_RPS.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but writing 1 to
  /sys/class/net/<dev>/queues/rx-N/rps_flow_cnt gives a flow table whose…

--

net: reduce ARFS flow updates by checking LLC affinity

With this commit, set_rps_cpu() skips the ndo_rx_flow_steer() call when
the flow's previous target CPU and the new one share an LLC. rflow->cpu
is still updated in that case. A new sysctl,
net.core.rps_feat_llc_affinity, backed by a static key and off by
default, turns the behaviour on.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index f587645e930a1..8b046100525a4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5122,6 +5122,7 @@ struct static_key_false rps_needed __read_mostly;
>  EXPORT_SYMBOL(rps_needed);
>  struct static_key_false rfs_needed __read_mostly;
>  EXPORT_SYMBOL(rfs_needed);
> +struct static_key_false rps_feat_llc_affinity __read_mostly;
>
>  static u32 rfs_slot(u32 hash, rps_tag_ptr tag_ptr)
>  {

[Severity: High]
This regression wasn't introduced by this patch, but can rfs_slot()
return an index past the end of a one-entry flow table?

Writing 1 to /sys/class/net/<dev>/queues/rx-N/rps_flow_cnt gives
mask = 0 in store_rps_dev_flow_table_cnt(). The runtime ilog2(0) is -1,
so the encoded log becomes 0:

    tag_ptr |= (ilog2(mask) + 1);

rfs_slot() then calls hash_32(hash, 0), which does:

    return __hash_32(val) >> (32 - bits);

Shifting a u32 by 32 is undefined. On x86-64 and arm64 the shift count
is masked, so the full 32-bit hash product comes back instead of 0.

get_rps_cpu() uses that value directly:

    rflow = flow_table + flow_id;

set_rps_cpu() later writes through the same pointer with
WRITE_ONCE(rflow->cpu, next_cpu) and the last_qtail update. On the aRFS
path it also writes to the target queue's tmp_rflow.

rps_may_expire_flow() checks flow_id against 1UL << log, but
get_rps_cpu() and set_rps_cpu() don't.

Could this cause out-of-bounds reads and writes in NET_RX softirq? That
would need CAP_NET_ADMIN to configure such a table and
rps_sock_flow_entries to be set. It looks like this goes back to commit
a435163d3100b0.

[ ... ]

> @@ -5179,6 +5180,24 @@ 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;
> +
> +		/*
> +		 * RPS LLC Affinity Feature:
> +		 * Reduce RFS/ARFS flow updates by checking LLC affinity.
> +		 *
> +		 * Frequent flow table updates can trigger constant hardware steering
> +		 * reconfigurations (e.g., ndo_rx_flow_steer), leading to significant
> +		 * contention on driver internal locks (like mlx5's arfs_lock).
> +		 *
> +		 * This strategy only updates the flow record if it migrates across LLC
> +		 * boundaries. This minimizes expensive hardware updates while
> +		 * preserving cache locality for the application.
> +		 */

[Severity: Low]
This isn't a bug, but does this comment still match the code now that
the check lives in set_rps_cpu()? After goto out, the code still runs:

    head = READ_ONCE(per_cpu(softnet_data, next_cpu).input_queue_head);
    rps_input_queue_tail_save(&rflow->last_qtail, head);
    ...
    WRITE_ONCE(rflow->cpu, next_cpu);

So the flow record is updated every time, and only the
ndo_rx_flow_steer() call is skipped. The commit message agrees: "The
per-queue flow bookkeeping (rflow->cpu) is still updated".

The comment looks left over from v9, where the check was in
rps_record_sock_flow().

> +		if (static_branch_unlikely(&rps_feat_llc_affinity) &&
> +		    old_cpu < nr_cpu_ids && cpu_online(old_cpu) &&
> +			cpus_share_cache(old_cpu, next_cpu))
> +			goto out;

[Severity: Medium]
Does old_cpu reliably show where the hardware is delivering this flow
right now?

old_cpu is the tcpu that get_rps_cpu() reads from rflow->cpu in the
arrival queue's software table. set_rps_cpu() writes next_cpu into
rflow->cpu on every goto out path, including paths that never install a
hardware filter. One is a driver failure:

    rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
                                            rxq_index, flow_id);
    if (rc < 0)
        goto out;

For example, mlx5e_rx_flow_steer() returns -EPERM when ARFS is disabled
and -ENOMEM when the GFP_ATOMIC allocation fails. Another is the
collision skip:

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

Hash slots are also shared between flows and never reset, so the
arrival queue's slot can hold a stale or colliding CPU.

Here is an example. Flow F arrives by RSS on queue R, whose IRQ CPU is
in LLC A. The application runs on CPU Y in LLC B, and the earlier steer
to Y failed or collided. When the application moves to CPU Z, also in
LLC B:

get_rps_cpu()
  set_rps_cpu(dev, skb, rflow, Y, Z, hash)
    cpus_share_cache(Y, Z) is true
    goto out
  rflow->cpu = Z

>From then on tcpu == next_cpu, so set_rps_cpu() is not called again for
this flow. Any later move inside LLC B takes the same shortcut.

Could F stay on the remote queue R indefinitely, with every packet
crossing LLCs through an RPS backlog enqueue and an IPI? Before this
patch, every CPU change retried ndo_rx_flow_steer().

The commit message says "cache locality for the application is
preserved either way". That seems to hold only when the hardware
already delivers to old_cpu's LLC, and nothing here checks that.

Would it be better to compare next_cpu with the CPU that services the
arrival queue (smp_processor_id() on this path, or the rmap CPU for
skb_get_rx_queue())? Another option is to skip only when a filter is
known to be installed.

[Severity: Medium]
How does this check behave for CPUs that have no sched domain?

cpus_share_cache() compares the raw sd_llc_id values:

    return per_cpu(sd_llc_id, this_cpu) == per_cpu(sd_llc_id, that_cpu);

sd_llc_id defaults to -1. It is only assigned for CPUs in the map
passed to build_sched_domains(), and sched_init_domains() masks that
map with housekeeping_cpumask(HK_TYPE_DOMAIN). CPUs isolated with
isolcpus=domain therefore keep -1.

sched_cpu_deactivate() also resets sd_llc_id to -1, through
sched_domains_free_llc_id() -> __sched_domains_free_llc_id(), while
the CPU is still cpu_online().

If so, any two isolated CPUs compare as sharing an LLC, even when they
are on different sockets. With rps_feat_llc_affinity enabled, a flow
whose consumer moves between isolated CPUs on different LLCs would skip
ndo_rx_flow_steer() and record rflow->cpu = next_cpu. Wouldn't that
flow then never be re-steered?

A short-lived version of this looks possible during hotplug. A
deactivating old_cpu (still online, sd_llc_id == -1) could be compared
with an isolated or offline next_cpu. Nothing here checks whether
next_cpu is online.

[Severity: Low]
Should Documentation/networking/scaling.rst be updated as well? Its
Accelerated RFS section says the stack "automatically calls this
function every time a flow entry in rps_dev_flow_table is updated".

With rps_feat_llc_affinity enabled, set_rps_cpu() updates the entry
(last_qtail and cpu) but skips ndo_rx_flow_steer() when old_cpu and
next_cpu share an LLC. The hardware filter then keeps pointing at the
old queue, and software RFS forwards packets to next_cpu.

[ ... ]

> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index 473c162736d69..6043ccb52cffa 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -560,6 +560,13 @@ static struct ctl_table net_core_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= rps_sock_flow_sysctl
>  	},
> +	{
> +		.procname	= "rps_feat_llc_affinity",
> +		.data		= &rps_feat_llc_affinity.key,
> +		.maxlen		= sizeof(rps_feat_llc_affinity.key),
> +		.mode		= 0644,
> +		.proc_handler	= proc_do_static_key
> +	},
>  #endif

[Severity: Low]
Should this new sysctl get an entry in
Documentation/admin-guide/sysctl/net.rst? high_order_alloc_disable,
another proc_do_static_key knob in this table, is documented there.
rps_sock_flow_entries is covered in Documentation/networking/scaling.rst.
rps_feat_llc_affinity has no entry anywhere under Documentation/.

The name also suggests it affects plain RPS. In fact it only takes
effect on the aRFS path, when skb_rx_queue_recorded(), dev->rx_cpu_rmap
and NETIF_F_NTUPLE are all set. Could the documentation say this, and
also give the default (off) and the accepted values (0/1)?

[Severity: Low]
This entry and the rps_feat_llc_affinity key are built under
CONFIG_RPS. The key's only reader is in set_rps_cpu(), inside
#ifdef CONFIG_RFS_ACCEL, and RFS_ACCEL can be turned off on its own:

    config RFS_ACCEL
        bool "Hardware acceleration of RFS"
        depends on RPS

On a kernel with RPS=y and RFS_ACCEL=n, won't this sysctl accept writes
and toggle a static key that nothing reads? The new old_cpu argument to
set_rps_cpu() is also unused in that configuration.

Would it make sense to put the key and this sysctl entry under
CONFIG_RFS_ACCEL?

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