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

[chuang]

Hi,

On Thu, Aug 6, 2026 at 7:11 PM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> 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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804024053.765-1-nashuiliang@xxxxxxxxx?part=1
> ---
> --- Patch [1]: [PATCH net-next v7] net: reduce RFS/ARFS flow updates by checking LLC affinity ---
> commit a4fc10ca836e4630c08de840b94b9f47a375f4d6
> Author: Chuang Wang <nashuiliang@xxxxxxxxx>
>
> net: reduce RFS/ARFS flow updates by checking LLC affinity
>
> This patch makes rps_record_sock_flow() skip the flow table update when the
> new CPU shares an LLC with the previously recorded one, gated by a new
> sysctl net.core.rps_feat_llc_affinity. It also converts
> sock_rps_record_flow_hash() and sock_rps_record_flow() from static inlines
> into exported out-of-line functions.
>
> A few questions below.
>
> > diff --git a/include/net/rps.h b/include/net/rps.h
> > index e33c6a2fa8bbc..6dacf0888a6c2 100644
> > --- a/include/net/rps.h
> > +++ b/include/net/rps.h
>
> [ ... ]
>
> > @@ -55,11 +56,14 @@ struct rps_sock_flow_table {
> >
> >  #define RPS_NO_CPU 0xffff
> >
> > +bool rps_llc_check(u32 old_val, u32 new_val);
> > +
> >  static inline void rps_record_sock_flow(rps_tag_ptr tag_ptr, u32 hash)
> >  {
> >       unsigned int index = hash & rps_tag_to_mask(tag_ptr);
> >       u32 val = hash & ~net_hotdata.rps_cpu_mask;
> >       struct rps_sock_flow_table *table;
> > +     u32 old_val;
> >
> >       /* We only give a hint, preemption can change CPU under us */
> >       val |= raw_smp_processor_id();
> > @@ -68,7 +72,8 @@ static inline void rps_record_sock_flow(rps_tag_ptr tag_ptr, u32 hash)
> >       /* The following WRITE_ONCE() is paired with the READ_ONCE()
> >        * here, and another one in get_rps_cpu().
> >        */
> > -     if (READ_ONCE(table[index].ent) != val)
> > +     old_val = READ_ONCE(table[index].ent);
> > +     if (old_val != val && rps_llc_check(old_val, val))
> >               WRITE_ONCE(table[index].ent, val);
>
> [Severity: Medium]
> [Finding: main-2-4]
> [Sources: opus-5]
> Should the skip decision also require that old_val belongs to the same
> flow?  The slot index here is only a masked hash:
>
>         unsigned int index = hash & rps_tag_to_mask(tag_ptr);
>
> so distinct flows share slots.  get_rps_cpu() re-validates the stored hash
> bits for exactly that reason:
>
>         ident = READ_ONCE(sock_flow_table[flow_id].ent);
>         if ((ident ^ hash) & ~net_hotdata.rps_cpu_mask)
>                 goto try_rps;
>
> With the CPU extraction fixed (see the comment on rps_llc_check() below),
> if flow A owns the slot with (hashA, cpu3) and flow B then records from a
> CPU sharing an LLC with cpu3, rps_llc_check() returns false, the slot keeps
> hashA, and every packet of B fails the hash test in get_rps_cpu() and falls
> back to plain RPS.  Flow B can then never install its own entry.

This issue will not occur, and it will not degrade to plain RPS. If flow A
and flow B share the same slot (i.e., same masked hash bits) and their CPUs
are in the same LLC, it will reuse the existing flow A configuration.

> The same applies to the sentinel.  rps_sock_flow_sysctl() initializes every
> entry:
>
>         for (i = 0; i < size; i++)
>                 sock_table[i].ent = RPS_NO_CPU;
>
> and _sock_rps_delete_flow() writes RPS_NO_CPU back on teardown.  With the
> intended mask, RPS_NO_CPU & rps_cpu_mask decodes to nr_cpu_ids - 1 on
> power-of-two CPU counts, which is a valid and usually active CPU, so the
> first record for a flow can be suppressed and the slot can stay at
> RPS_NO_CPU.
>
> Would something like this before the CPU comparison be more correct?
>
>         if ((old_val ^ new_val) & ~net_hotdata.rps_cpu_mask)
>                 return true;
>
> >  }
> >
> > @@ -136,25 +141,8 @@ static inline bool rfs_is_needed(void)
> >  #endif
> >  }
> >
> > -static inline void sock_rps_record_flow_hash(__u32 hash)
> > -{
> > -#ifdef CONFIG_RPS
> > -     if (!rfs_is_needed())
> > -             return;
> > -
> > -     _sock_rps_record_flow_hash(hash);
> > -#endif
> > -}
> > -
> > -static inline void sock_rps_record_flow(const struct sock *sk)
> > -{
> > -#ifdef CONFIG_RPS
> > -     if (!rfs_is_needed())
> > -             return;
> > -
> > -     _sock_rps_record_flow(sk);
> > -#endif
> > -}
> > +void sock_rps_record_flow_hash(__u32 hash);
> > +void sock_rps_record_flow(const struct sock *sk);
>
> [Severity: Medium]
> [Finding: main-1-1]
> [Sources: opus-5]
> The changelog says:
>
>   "Additionally, export sock_rps_record_flow_hash() and
>   sock_rps_record_flow(). This resolves a symbol visibility compilation
>   error triggered by 'tun' using sock_rps_record_flow_hash() in
>   tun_flow_update() when CONFIG_TUN is built as a module."
>
> Is that error pre-existing?  Before this patch both helpers were static
> inline in include/net/rps.h, so tun.ko and sctp.ko referenced no external
> symbol.  The undefined-symbol reports quoted in the v4/v5 notes look like
> they come from this patch adding a call to the non-exported rps_llc_check()
> inside the inline rps_record_sock_flow().
>
> Should the changelog say that instead?

You are correct, the error was not pre-existing; it was introduced by
adding rps_llc_check().

>
> Also, does turning these into out-of-line calls change the RFS fast path?
> The rfs_is_needed() static-key test previously compiled to a nop at each
> call site (tcp_recvmsg, tcp_sendmsg, inet_accept, sctp, tun), and on
> CONFIG_RPS=n the whole thing compiled away.  Now every call site performs a
> real call, including on CONFIG_RPS=n where the new definitions sit after
> "#endif /* CONFIG_RPS */" with empty bodies.  An alternative would be to
> keep the inlines and export rps_llc_check() only.
>

If we only export rps_llc_check(), it doesn't clearly explain why tun/sctp
are calling it, since they actually use rps_record_sock_flow(). Exporting
only rps_llc_check() hides this call dependency.

I believe exporting rps_record_sock_flow() and sock_rps_record_flow_hash()
makes the issue and the dependency much clearer to describe.

> >
> >  static inline void sock_rps_delete_flow(const struct sock *sk)
> >  {
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index c1c1be1a6962f..ab74616f95892 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
>
> [ ... ]
>
> > @@ -5218,6 +5220,55 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
> >       return cpu;
> >  }
> >
> > +/**
> > + * rps_llc_check - determine if RPS flow table should be updated.
> > + * @old_val: previous flow record value.
> > + * @new_val: target flow record value.
> > + *
> > + * Return: true if the record needs an update, false otherwise.
> > + */
> > +bool rps_llc_check(u32 old_val, u32 new_val)
> > +{
> > +     u32 old_cpu = old_val & ~net_hotdata.rps_cpu_mask;
> > +     u32 new_cpu = new_val & ~net_hotdata.rps_cpu_mask;
>
> [Severity: Critical]
> [Finding: main-1-0]
> [Sources: opus-5]
> Is the mask polarity inverted here?  The entry format comment in
> include/net/rps.h says the upper part holds the high-order hash bits and
> the lower part holds the CPU number, and both the producer and the existing
> consumer agree with that:
>
> rps_record_sock_flow():
>         u32 val = hash & ~net_hotdata.rps_cpu_mask;
>         val |= raw_smp_processor_id();
>
> get_rps_cpu():
>         if ((ident ^ hash) & ~net_hotdata.rps_cpu_mask)
>                 goto try_rps;
>         next_cpu = ident & net_hotdata.rps_cpu_mask;
>
> So old_cpu and new_cpu as computed above are flow hash bits, not CPU ids.
> Two consequences seem to follow.
>
> First, hash & ~rps_cpu_mask is either 0 or a multiple of rps_cpu_mask + 1,
> which equals roundup_pow_of_two(nr_cpu_ids), so the first guard
>
>         if (old_cpu >= nr_cpu_ids || !cpu_active(old_cpu))
>                 return true;
>
> is true for every non-zero value and rps_llc_check() returns true
> unconditionally.  Does the LLC filtering ever run?  If it does not, where
> does the ~93% reduction in mlx5e_rx_flow_steer reported in the changelog
> come from?

You are absolutely right about the mask polarity. I made a mistake when
preparing this patch and will fix it by using `val & net_hotdata.rps_cpu_mask`
for the CPU extractions. The ~93% reduction data reported in the changelog
was actually tested with this fix applied on the linux-6.1 kernel.

> Second, in the remaining case where the stored hash bits happen to be zero
> (recorded hash smaller than roundup_pow_of_two(nr_cpu_ids)), old_cpu is 0,
> which is active and normally present in current->cpus_ptr, so the code
> reaches cpus_share_cache(old_cpu, new_cpu) with new_cpu being an arbitrary
> hash-derived value that is never range checked:
>
> 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);
> }
>
> Can this index __per_cpu_offset[] out of bounds and dereference the
> resulting address?  The hashes come from remote 4-tuples and this runs on
> the socket receive/transmit path.
>
> Would using val & net_hotdata.rps_cpu_mask for both extractions, plus a
> new_cpu < nr_cpu_ids check before cpus_share_cache(), be the intended
> logic?

This will not occur. When `rps_llc_check()` is called, `new_val` is already
a valid CPU ID.

> > +
> > +     /*
> > +      * 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.
> > +      */
> > +     if (static_branch_unlikely(&rps_feat_llc_affinity)) {
> > +             /* Force update if the recorded CPU is invalid or has gone offline */
> > +             if (old_cpu >= nr_cpu_ids || !cpu_active(old_cpu))
> > +                     return true;
> > +
> > +             /*
> > +              * Force an update if the current task is no longer permitted
> > +              * to run on the old_cpu.
> > +              */
> > +             if (!cpumask_test_cpu(old_cpu, current->cpus_ptr))
> > +                     return true;
>
> [Severity: Medium]
> [Finding: main-5-11]
> [Sources: opus-5]
> Is current the task that owns this flow?  tun_flow_update() calls
> sock_rps_record_flow_hash(e->rps_rxhash) and its callers are tun_get_user()
> and tun_xdp_one(), so current there is the tun or vhost writer thread, not
> the application that will recvmsg() the flow.  The affinity of that
> unrelated task does not say where the consumer runs.
>
> Also, p->cpus_ptr is not always &p->cpus_mask:
>
> kernel/sched/core.c:set_cpus_allowed_common() {
>         if (ctx->flags & (SCA_MIGRATE_ENABLE | SCA_MIGRATE_DISABLE)) {
>                 p->cpus_ptr = ctx->new_mask;
>                 return;
>         }
>         ...
> }
>
> Inside a migrate_disable() region (including PREEMPT_RT spin_lock
> sections), cpus_ptr is cpumask_of(rq->cpu), so this test is false for every
> old_cpu other than the current one and rps_llc_check() degenerates to
> "always update".  The mask is also read with no serialization against a
> concurrent sched_setaffinity() or cpuset change.
>
> > +
> > +             /*
> > +              * If CPUs do not share a cache, allow the update to prevent
> > +              * expensive remote memory accesses and cache misses.
> > +              */
> > +             if (!cpus_share_cache(old_cpu, new_cpu))
> > +                     return true;
> > +
> > +             return false;
> > +     }
> > +
> > +     return true;
> > +}
> > +
> >  #ifdef CONFIG_RFS_ACCEL
> >
> >  /**

I will remove this check in the next version.

> [ ... ]
>
> > diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> > index b508618bfc123..b6d4ebcbb6a68 100644
> > --- a/net/core/sysctl_net_core.c
> > +++ b/net/core/sysctl_net_core.c
> > @@ -210,6 +210,33 @@ static int rps_sock_flow_sysctl(const struct ctl_table *table, int write,
> >       kvfree_rcu_mightsleep(tofree);
> >       return ret;
> >  }
> > +
> > +static int rps_feat_llc_affinity_sysctl(const struct ctl_table *table, int write,
> > +                                     void *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > +     u8 curr_state;
> > +     int ret;
> > +     const struct ctl_table tmp = {
> > +             .data = &curr_state,
> > +             .maxlen = sizeof(curr_state),
> > +             .mode = table->mode,
> > +             .extra1 = table->extra1,
> > +             .extra2 = table->extra2
> > +     };
> > +
> > +     curr_state = static_branch_unlikely(&rps_feat_llc_affinity) ? 1 : 0;
> > +
> > +     ret = proc_dou8vec_minmax(&tmp, write, buffer, lenp, ppos);
> > +     if (write && ret == 0) {
> > +             if (curr_state && !static_branch_unlikely(&rps_feat_llc_affinity))
> > +                     static_branch_enable(&rps_feat_llc_affinity);
> > +             else if (!curr_state && static_branch_unlikely(&rps_feat_llc_affinity))
> > +                     static_branch_disable(&rps_feat_llc_affinity);
> > +     }
> > +
> > +     return ret;
> > +}
>
> [Severity: Medium]
> [Finding: main-5-9]
> [Sources: opus-5]
> Can two concurrent writers to this file race?  The snapshot, the parse and
> the toggle are done with no serialization, while the in-tree helper for the
> same job takes a mutex:
>
> kernel/sysctl.c:proc_do_static_key() {
>         static DEFINE_MUTEX(static_key_mutex);
>         ...
>         mutex_lock(&static_key_mutex);
>         val = static_key_enabled(key);
>         ret = proc_dointvec_minmax(&tmp, dir, buffer, lenp, ppos);
>         ...
>         mutex_unlock(&static_key_mutex);
> }
>
> If CPU0 is inside static_key_enable_cpuslocked():
>
>         if (atomic_read(&key->enabled) == 0) {
>                 atomic_set(&key->enabled, -1);
>                 jump_label_update(key);
>                 atomic_set_release(&key->enabled, 1);
>         }
>
> then CPU1 entering static_key_disable_cpuslocked() reads the transient -1
> before taking jump_label_lock():
>
>         if (atomic_read(&key->enabled) != 1) {
>                 WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
>                 return;
>         }
>
> which warns and drops the disable request, leaving the key in the state the
> administrator did not ask for.  Would reusing proc_do_static_key() be
> simpler here?  It also enforces the CAP_SYS_ADMIN check that this handler
> omits.
>
> The "if (curr_state && !static_branch_unlikely(...))" pre-checks look
> redundant since enable and disable are idempotent, and they only widen the
> window.
>
> > +
> >  #endif /* CONFIG_RPS */
> >
> >  #ifdef CONFIG_NET_FLOW_LIMIT
> > @@ -554,6 +581,14 @@ static struct ctl_table net_core_table[] = {
> >               .mode           = 0644,
> >               .proc_handler   = rps_sock_flow_sysctl
> >       },
> > +     {
> > +             .procname   = "rps_feat_llc_affinity",
> > +             .maxlen     = sizeof(u8),
> > +             .mode       = 0644,
> > +             .proc_handler   = rps_feat_llc_affinity_sysctl,
> > +             .extra1     = SYSCTL_ZERO,
> > +             .extra2     = SYSCTL_ONE
> > +     },

I will fix this in the next version by reusing the standard
`proc_do_static_key()` helper.

> [Severity: Low]
> [Finding: main-1-2]
> [Sources: opus-5]
> Could this new sysctl get a documentation entry?  Neither the tunable list
> in the RFS Configuration section of Documentation/networking/scaling.rst nor
> Documentation/admin-guide/sysctl/net.rst mentions rps_feat_llc_affinity, its
> semantics or its default.
>
> The same file also states:
>
>   "rps_sock_flow_table is a global flow table that contains the *desired*
>   CPU for flows: the CPU that is currently processing the flow in
>   userspace.  Each table value is a CPU index that is updated during calls
>   to recvmsg and sendmsg (specifically, inet_recvmsg(), inet_sendmsg() and
>   tcp_splice_read())."
>
> With the feature enabled the entry is deliberately left stale whenever
> rps_llc_check() returns false, so that description no longer holds.  Should
> it be updated in the same patch?
>
> >  #endif
> >  #ifdef CONFIG_NET_FLOW_LIMIT
> >       {

I will fix this in the next version by adding the documentation entry for
`rps_feat_llc_affinity` and updating the RFS description in
`Documentation/networking/scaling.rst`.