Re: [RFC PATCH net-next v1 2/2] Documentation: networking: document SO_NO_RFS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stale rps_sock_flow_table entry when SO_NO_RFS set on active socket
--

commit 42cabc51ee3ece5cddea8fc01572ac23fc041dc0
Author: Vitaly Zuevsky <vitaly.zuevsky@gcore.com>

Documentation: networking: document SO_NO_RFS

This commit documents the SO_NO_RFS socket option in the networking scaling
documentation, describing how it disables RFS and accelerated RFS for
individual sockets and is inherited across accept().

> diff --git a/Documentation/networking/scaling.rst b/Documentation/networking/scaling.rst
> index 6c9836000a8d1..54c7a72da5632 100644
> --- a/Documentation/networking/scaling.rst
> +++ b/Documentation/networking/scaling.rst
> @@ -428,6 +428,20 @@ queues. So for instance, if rps_sock_flow_entries is set to 131072 and there
>  are 16 configured receive queues, rps_flow_cnt for each queue might be
>  configured as 8192.
>  
> +Excluding individual sockets
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +RFS (and therefore accelerated RFS, see below) can be switched off for
> +a single socket with the SO_NO_RFS socket option. Flows of such
> +sockets are never recorded in rps_sock_flow_table and stay on the CPU
> +selected by RSS or RPS.

[Severity: Medium]
Does enabling SO_NO_RFS on an already-accepted or active single socket
actually prevent its flow from being present in rps_sock_flow_table?

When a passive connection is accepted from a listening socket that does not
have SO_NO_RFS set, __inet_accept() records the flow before returning the new
socket to user space:

net/ipv4/af_inet.c:__inet_accept() {
	...
	sock_rps_record_flow(newsk);
	...
}

Similarly, any connected TCP, UDP, SCTP, or MPTCP socket records its flow in
net_hotdata.rps_sock_flow_table as soon as poll(), sendmsg(), or recvmsg()
calls sock_rps_record_flow().

If the application then enables SO_NO_RFS on that socket, sk_setsockopt()
only sets the SOCK_NO_RFS flag without deleting the existing entry from
rps_sock_flow_table:

net/core/sock.c:sk_setsockopt() {
	...
	case SO_NO_RFS:
		sock_valbool_flag(sk, SOCK_NO_RFS, valbool);
		break;
	...
}

Because the slot in rps_sock_flow_table remains populated, get_rps_cpu()
continues matching the socket's flow hash on incoming packets and continues
software RFS steering and ndo_rx_flow_steer() hardware filter programming to
the previously recorded CPU, while _sock_rps_record_flow() returns early on
subsequent CPU migrations.

Furthermore, when the socket is later closed and inet_unhash(),
udp_lib_unhash(), or sctp_unhash() calls sock_rps_delete_flow(),
_sock_rps_delete_flow() sees SOCK_NO_RFS set and returns early without
resetting the table slot to RPS_NO_CPU:

include/net/rps.h:_sock_rps_delete_flow() {
	...
	/* Never recorded, so never clear a slot that may belong to
	 * another flow.
	 */
	if (sock_flag(sk, SOCK_NO_RFS))
		return;
	...
}

Should sk_setsockopt() call sock_rps_delete_flow(sk) before setting
SOCK_NO_RFS (or restrict SO_NO_RFS to unestablished sockets), and should the
documentation clarify the behavior when SO_NO_RFS is set after a flow has
already been recorded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031113330.45704-1-vitaly.zuevsky@gcore.com?part=2