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

[chuang]

On Tue, Oct 13, 2026 at 3:07 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
>
> On Tue, Oct 13, 2026 at 12:00 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 5 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 · Medium: 1 · 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] The new LLC skip in set_rps_cpu() (net/core/dev.c:5196-5199)
> >   assumes that `old_cpu` shows where the NIC currently delivers the flow.
> > - [Low] The new user-visible sysctl net.core.arfs_llc_affinity
> >   (net/core/sysctl_net_core.c:563-569) has no documentation.
> > - [Low] The config guards for the sysctl and its only user do not match.
> > - [Low] The commit message says the results "demonstrate that filtering
> >   updates by LLC affinity significantly reduces driver lock contention…
> > - [Low] In this tree, cpus_share_cache() (kernel/sched/core.c:4008-4014)
> >   returns `per_cpu(sd_llc_id, a) == per_cpu(sd_llc_id, b)`.
> >
> > --
> >
> > net: reduce ARFS flow updates by checking LLC affinity
> >
> > This commit adds an opt-in sysctl, net.core.arfs_llc_affinity. When it is
> > set, set_rps_cpu() skips the ARFS hardware steering update
> > (ndo_rx_flow_steer) if the flow's previous RFS CPU and the new one share
> > an LLC. The rflow->cpu bookkeeping is still updated, so software RFS keeps
> > steering packets to the CPU the application runs on.
> >
> > > These results demonstrate that filtering updates by LLC affinity
> > > significantly reduces driver lock contention and improves overall
> > > CPU efficiency under heavy network load.
> >
> > [Severity: Low]
> >
> > Does the data above support this claim? The only number given is the
> > mlx5e_rx_flow_steer() call rate, which drops from about 200k/s to about
> > 10/s. There are no throughput, latency, CPU utilisation or lock
> > contention numbers.
> >
> > The commit message also leaves out the trade-off. When the hardware
> > update is skipped, the flow stays on its old queue. get_rps_cpu() then
> > hands every packet to next_cpu with a remote enqueue_to_backlog() plus an
> > RPS IPI. After a completed ARFS steer, the packet would instead be
> > handled on the queue's own IRQ CPU.
> >
> > The phrase "subsequent packets take the fast path in get_rps_cpu()
> > (tcpu == next_cpu)" could be read as local processing. The packets are
> > still moved to a remote CPU.
> >
> > The opening paragraph names rps_record_sock_flow() as the motivation.
> > Since v10, though, the patch only changes set_rps_cpu() and get_rps_cpu().
> > Could that paragraph be reworded to match?
> >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index a6951ea08961e..434ad5499906d 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> >
> > [ ... ]
> >
> > > @@ -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;
> > > +
> > > +             /*
> > > +              * ARFS LLC Affinity Feature:
> > > +              * Reduce 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).
> > > +              *
> > > +              * Skip the expensive hardware steering update if the flow's previous
> > > +              * target CPU and the new one share the last level cache: cache locality
> > > +              * for the application is preserved either way.
> > > +              */
> > > +             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]
> >
> > Is old_cpu a reliable sign of where the NIC currently delivers this flow?
> >
> > get_rps_cpu() passes in tcpu = rflow->cpu. That value is read from the
> > hashed slot in the flow table of the queue the packet arrived on. It only
> > records the last software RFS target for that slot, and it may have no
> > hardware filter behind it. For example:
> >
> > - ndo_rx_flow_steer() returned rc < 0, but set_rps_cpu() still did
> >   WRITE_ONCE(rflow->cpu, next_cpu) on the arrival queue's slot.
> >   mlx5e_rx_flow_steer() can return -EPROTONOSUPPORT, -ENOMEM, or -EPERM
> >   while MLX5E_ARFS_STATE_ENABLED is clear.
> >
> > - The tmp_rflow collision check or the !q_tag_ptr check took goto out.
> >
> > - The slot still holds a CPU left by an earlier flow that hashed to the
> >   same rfs_slot(). store_rps_dev_flow_table_cnt() sets RPS_NO_CPU only
> >   when the table is allocated.
> >
> > - The driver flushed or re-created its filters.
> >
> > In each of these cases, if old_cpu and next_cpu share an LLC, the new
> > check skips ndo_rx_flow_steer() and then reaches:
> >
> >         WRITE_ONCE(rflow->cpu, next_cpu);
> >
> > On later packets tcpu == next_cpu, so get_rps_cpu() never calls
> > set_rps_cpu() again. Hardware steering is not retried while the
> > application stays within that LLC. A new flow that lands on a stale slot
> > in the same LLC would never get an ARFS filter at all.
> >
> > Before this patch, every CPU change triggered a new steering attempt, so
> > a failed or missing filter recovered on its own.
> >
> > Without a filter, the flow stays on its RSS queue, which may be serviced
> > in another LLC or NUMA node. Every packet is then handed over by software
> > RFS. cpu_rmap_update() ranks queues by sibling, package and NUMA node,
> > not by LLC. Nothing guarantees an LLC relationship between the receiving
> > queue and old_cpu. Doesn't that contradict "cache locality for the
> > application is preserved either way"?
> >
> > Could the skip be limited to the case where this flow has a live
> > hardware filter, for example rflow->filter != RPS_NO_FILTER &&
> > rflow->hash == hash? Another option would be to compare next_cpu with
> > the CPU that actually services the receiving queue, rather than with
> > rflow->cpu.
>
> Eric used raw_smp_processor_id() in the v9 thread:
> https://lore.kernel.org/netdev/CANn89iJkPPXruitnot9D28Xe5LWnD-irtgCUEOpmFfVkG4M=-g@xxxxxxxxxxxxxx/

Eric was only giving an example there. When adding the LLC check to
set_rps_cpu(), it is old_cpu (rflow->cpu) that should enter the
comparison, not the current CPU.
>
> >
> > [Severity: Low]
> >
> > Can cpus_share_cache() return true here for CPUs in different LLCs? In
> > this tree it is:
> >
> > kernel/sched/core.c:cpus_share_cache() {
> >         if (this_cpu == that_cpu)
> >                 return true;
> >
> >         return per_cpu(sd_llc_id, this_cpu) == per_cpu(sd_llc_id, that_cpu);
> > }
> >
> > sd_llc_id defaults to -1. It is assigned only in build_sched_domains()
> > and reset to -1 by __sched_domains_free_llc_id().
> >
> > sched_cpu_deactivate() calls sched_domains_free_llc_id(cpu) while the CPU
> > is still in cpu_online_mask. CPUs outside any sched domain, such as
> > isolcpus=domain CPUs, keep -1 permanently.
> >
> > The new check guards old_cpu only with cpu_online(), and next_cpu only
> > with nr_cpu_ids. If both CPUs carry the -1 sentinel, they compare as
> > sharing an LLC and ndo_rx_flow_steer() is skipped. That could happen when
> > an application migrates between two isolated CPUs in different LLCs, or
> > during a hotplug window.
> >
> > The sentinel handling belongs to the scheduler, but should this new
> > networking user guard against it?
> >
> > > +
> > >               rxq_index = cpu_rmap_lookup_index(dev->rx_cpu_rmap, next_cpu);
> > >               if (rxq_index == skb_get_rx_queue(skb))
> > >                       goto out;
> >
> > [ ... ]
> >
> > > diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> > > index 473c162736d69..9ce0bbe454c83 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       = "arfs_llc_affinity",
> > > +             .data           = &arfs_llc_affinity.key,
> > > +             .maxlen         = sizeof(arfs_llc_affinity.key),
> > > +             .mode           = 0644,
> > > +             .proc_handler   = proc_do_static_key
> > > +     },
> > >  #endif
> >
> > [Severity: Low]
> >
> > Should this entry, and the arfs_llc_affinity definition in
> > net/core/dev.c, be under CONFIG_RFS_ACCEL instead of CONFIG_RPS?
> >
> > The only reader, static_branch_unlikely(&arfs_llc_affinity) in
> > set_rps_cpu(), sits inside #ifdef CONFIG_RFS_ACCEL. RFS_ACCEL can be
> > turned off on its own ("depends on RPS" in net/Kconfig).
> >
> > On CONFIG_RPS=y, CONFIG_RFS_ACCEL=n kernels,
> > /proc/sys/net/core/arfs_llc_affinity is still registered and accepts
> > writes that flip the key, but nothing changes.
> >
> > [Severity: Low]
> >
> > This isn't a bug, but net.core.arfs_llc_affinity isn't documented in
> > Documentation/admin-guide/sysctl/net.rst. That file covers other
> > net.core knobs, such as rps_default_mask and high_order_alloc_disable.
> >
> > The Accelerated RFS section of Documentation/networking/scaling.rst also
> > still says:
> >
> >   The network stack automatically calls this function every time a flow
> >   entry in rps_dev_flow_table is updated.
> >
> > With the knob enabled, set_rps_cpu() now updates rflow->cpu and
> > rflow->last_qtail without calling ndo_rx_flow_steer() whenever the two
> > CPUs share an LLC.
> >
> > Could the documentation describe the new knob, its default (0) and
> > accepted values, and this exception in scaling.rst?
> >
> > --
> > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012060152.2930-1-nashuiliang%40gmail.com