[RFC PATCH net-next v1 0/2] net: per-socket opt-out from RFS/aRFS

[RFC PATCH net-next v1 0/2] net: per-socket opt-out from RFS/aRFS

[Vitaly Zuevsky]

RFS and accelerated RFS are enabled per host (rps_sock_flow_entries)
and per rx queue (rps_flow_cnt, NETIF_F_NTUPLE). Whether steering a
given flow is worth it is a property of the flow, and a process using
the flows of different traffic patterns cannot express it.

Motivating workload: a CDN edge running nginx with workers and IRQs
pinned, with deliberately more NIC queue -> CPU lanes than workers.
Outbound connections to peers and origins are long-lived and benefit
from aRFS. The larger population of client connections is
short-lived, arrives on an RSS-chosen CPU and is served from there;
steering them gains nothing, yet each is recorded on its first
recvmsg() and pushed into the NIC via ndo_rx_flow_steer(). On mlx5
that is a GFP_ATOMIC allocation and a per-device spinlock in softirq,
a firmware command on a single-threaded workqueue, and an O(rules)
expiry walk under the same lock. Enabling aRFS on such a host more
than doubled softirq CPU and, as traffic grew, ended in softirq
livelock and a forced reboot. Aligning accept() CPUs with RSS through
reuseport does not apply because the queue:worker mapping is
intentionally not 1:1.

This series adds SO_NO_RFS, a boolean socket option. When set,
sock_rps_record_flow() does not record the socket's flow in
rps_sock_flow_table, so get_rps_cpu() finds no match and neither
software RFS nor aRFS acts on the flow; it stays on the CPU chosen by
RSS (plain RPS is per queue and unaffected). The bit lives in sk_flags
and is therefore copied by sk_clone_lock(): set once on a listening
socket, every accept()ed child is excluded, while connect()ed sockets
of the same process keep RFS/aRFS.

Cost on the record path is one sock_flag() test, reached only when
the rfs_needed static key is on and sk_state == TCP_ESTABLISHED;
sk_flags is already read by tcp_recvmsg() so the line is hot.

Status: compile-tested on x86_64 with CONFIG_RPS=y and =n, not yet
runtime-tested. Sent as RFC to get agreement on the interface and
semantics first. The "before" state on the motivating host is a
production outage and cannot be re-run; testing on comparable
workloads by others would be very welcome.

Vitaly Zuevsky (2):
  net: add SO_NO_RFS to exclude a socket from RFS and aRFS
  Documentation: networking: document SO_NO_RFS

 Documentation/networking/scaling.rst  | 14 ++++++++++++++
 arch/alpha/include/uapi/asm/socket.h  |  2 ++
 arch/mips/include/uapi/asm/socket.h   |  2 ++
 arch/parisc/include/uapi/asm/socket.h |  2 ++
 arch/sparc/include/uapi/asm/socket.h  |  2 ++
 include/net/rps.h                     | 13 +++++++++++++
 include/net/sock.h                    |  1 +
 include/uapi/asm-generic/socket.h     |  2 ++
 net/core/sock.c                       |  8 ++++++++
 9 files changed, 46 insertions(+)


base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.34.1

[RFC PATCH net-next v1 1/2] net: add SO_NO_RFS to exclude a socket from RFS and aRFS

[Vitaly Zuevsky]

RFS and accelerated RFS are enabled per host and per rx queue, but
whether steering helps is a property of the flow. A host that carries
a few long-lived flows which benefit from steering alongside a large
churn of short-lived flows has no way to express that today.

Add a boolean socket option, SO_NO_RFS. When set, sock_rps_record_flow()
does not record the socket in rps_sock_flow_table, so get_rps_cpu()
finds no match and neither software RFS nor ndo_rx_flow_steer() acts
on the flow; it stays on the CPU chosen by RSS or RPS.

The option is kept in sk_flags so that sk_clone_lock() copies it to
accepted children: setting it on a listening socket excludes every
connection accepted from it, while sockets the same process connects
itself remain eligible.

sock_rps_delete_flow() is skipped as well, because a socket that never
recorded itself must not clear a table slot that may belong to another
flow.

The test sits after the TCP_ESTABLISHED check and inside the
rfs_needed static branch, so hosts without RFS pay nothing and hosts
with RFS pay one test_bit() on a cache line tcp_recvmsg() already
touches.

Signed-off-by: Vitaly Zuevsky <vitaly.zuevsky@gcore.com>
---
 arch/alpha/include/uapi/asm/socket.h  |  2 ++
 arch/mips/include/uapi/asm/socket.h   |  2 ++
 arch/parisc/include/uapi/asm/socket.h |  2 ++
 arch/sparc/include/uapi/asm/socket.h  |  2 ++
 include/net/rps.h                     | 13 +++++++++++++
 include/net/sock.h                    |  1 +
 include/uapi/asm-generic/socket.h     |  2 ++
 net/core/sock.c                       |  8 ++++++++
 8 files changed, 32 insertions(+)

diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
index 5d3524c26b2b..256d37f5b9b0 100644
--- a/arch/alpha/include/uapi/asm/socket.h
+++ b/arch/alpha/include/uapi/asm/socket.h
@@ -161,6 +161,8 @@
 
 #define SO_PEERPIDFD_THREAD    87
 
+#define SO_NO_RFS              88
+
 #if !defined(__KERNEL__)
 
 #if __BITS_PER_LONG == 64
diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
index 245a43f52fb1..5b477de463a3 100644
--- a/arch/mips/include/uapi/asm/socket.h
+++ b/arch/mips/include/uapi/asm/socket.h
@@ -172,6 +172,8 @@
 
 #define SO_PEERPIDFD_THREAD    87
 
+#define SO_NO_RFS              88
+
 #if !defined(__KERNEL__)
 
 #if __BITS_PER_LONG == 64
diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
index f23710e1c671..b3a4cc2cbc50 100644
--- a/arch/parisc/include/uapi/asm/socket.h
+++ b/arch/parisc/include/uapi/asm/socket.h
@@ -153,6 +153,8 @@
 
 #define SO_PEERPIDFD_THREAD	0x4055
 
+#define SO_NO_RFS		0x4056
+
 #if !defined(__KERNEL__)
 
 #if __BITS_PER_LONG == 64
diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
index b35b25bdefc2..ba5ed7942570 100644
--- a/arch/sparc/include/uapi/asm/socket.h
+++ b/arch/sparc/include/uapi/asm/socket.h
@@ -154,6 +154,8 @@
 
 #define SO_PEERPIDFD_THREAD      0x0060
 
+#define SO_NO_RFS                0x0061
+
 #if !defined(__KERNEL__)
 
 
diff --git a/include/net/rps.h b/include/net/rps.h
index e33c6a2fa8bb..5afa48868236 100644
--- a/include/net/rps.h
+++ b/include/net/rps.h
@@ -98,6 +98,13 @@ static inline void _sock_rps_record_flow(const struct sock *sk)
 	 * [1] : sk_state and sk_prot are in the same cache line.
 	 */
 	if (sk->sk_state == TCP_ESTABLISHED) {
+		/* SO_NO_RFS: application asked us to never steer this flow.
+		 * Tested after sk_state so the common case stays on one
+		 * cache line.
+		 */
+		if (unlikely(sock_flag(sk, SOCK_NO_RFS)))
+			return;
+
 		/* This READ_ONCE() is paired with the WRITE_ONCE()
 		 * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
 		 */
@@ -111,6 +118,12 @@ static inline void _sock_rps_delete_flow(const struct sock *sk)
 	rps_tag_ptr tag_ptr;
 	u32 hash, index;
 
+	/* Never recorded, so never clear a slot that may belong to
+	 * another flow.
+	 */
+	if (sock_flag(sk, SOCK_NO_RFS))
+		return;
+
 	hash = READ_ONCE(sk->sk_rxhash);
 	if (!hash)
 		return;
diff --git a/include/net/sock.h b/include/net/sock.h
index 173b37b2b8bb..4472dc4ed28e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1024,6 +1024,7 @@ enum sock_flags {
 	SOCK_RCVMARK, /* Receive SO_MARK  ancillary data with packet */
 	SOCK_RCVPRIORITY, /* Receive SO_PRIORITY ancillary data with packet */
 	SOCK_TIMESTAMPING_ANY, /* Copy of sk_tsflags & TSFLAGS_ANY */
+	SOCK_NO_RFS, /* SO_NO_RFS: never record this flow for RFS/aRFS */
 };
 
 #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index 56fed7ab27ab..9bf415b0d470 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -156,6 +156,8 @@
 
 #define SO_PEERPIDFD_THREAD	87
 
+#define SO_NO_RFS		88
+
 #if !defined(__KERNEL__)
 
 #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
diff --git a/net/core/sock.c b/net/core/sock.c
index e7405be8941f..02b8cc4bc1c4 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1597,6 +1597,10 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
 		reuseport_update_incoming_cpu(sk, val);
 		break;
 
+	case SO_NO_RFS:
+		sock_valbool_flag(sk, SOCK_NO_RFS, valbool);
+		break;
+
 	case SO_CNX_ADVICE:
 		if (val == 1)
 			dst_negative_advice(sk);
@@ -2118,6 +2122,10 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
 		v.val = READ_ONCE(sk->sk_incoming_cpu);
 		break;
 
+	case SO_NO_RFS:
+		v.val = sock_flag(sk, SOCK_NO_RFS);
+		break;
+
 	case SO_MEMINFO:
 	{
 		u32 meminfo[SK_MEMINFO_VARS];

base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.34.1

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

[Vitaly Zuevsky]

Describe the new per-socket opt-out next to the RFS configuration
knobs it complements.

Signed-off-by: Vitaly Zuevsky <vitaly.zuevsky@gcore.com>
---
 Documentation/networking/scaling.rst | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/networking/scaling.rst b/Documentation/networking/scaling.rst
index 6c9836000a8d..54c7a72da563 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. The setting is inherited by sockets returned
+from accept(), so applying it to a listening socket excludes all
+connections accepted from it while connections the same process
+initiates with connect() continue to be steered. This is useful when
+one host handles both a small set of long-lived flows that benefit
+from steering and a large churn of short-lived flows for which the
+cost of programming the NIC exceeds the benefit.
+
 
 Accelerated RFS
 ===============
-- 
2.34.1

Re: [RFC PATCH net-next v1 1/2] net: add SO_NO_RFS to exclude a socket from RFS and aRFS

[Willem de Bruijn]

Vitaly Zuevsky wrote:
> RFS and accelerated RFS are enabled per host and per rx queue, but
> whether steering helps is a property of the flow. A host that carries
> a few long-lived flows which benefit from steering alongside a large
> churn of short-lived flows has no way to express that today.
> 
> Add a boolean socket option, SO_NO_RFS. When set, sock_rps_record_flow()
> does not record the socket in rps_sock_flow_table, so get_rps_cpu()
> finds no match and neither software RFS nor ndo_rx_flow_steer() acts
> on the flow; it stays on the CPU chosen by RSS or RPS.
> 
> The option is kept in sk_flags so that sk_clone_lock() copies it to
> accepted children: setting it on a listening socket excludes every
> connection accepted from it, while sockets the same process connects
> itself remain eligible.
> 
> sock_rps_delete_flow() is skipped as well, because a socket that never
> recorded itself must not clear a table slot that may belong to another
> flow.
> 
> The test sits after the TCP_ESTABLISHED check and inside the
> rfs_needed static branch, so hosts without RFS pay nothing and hosts
> with RFS pay one test_bit() on a cache line tcp_recvmsg() already
> touches.
> 
> Signed-off-by: Vitaly Zuevsky <vitaly.zuevsky@gcore.com>

The Documentation change can/should be included in the main patch.

> ---
>  arch/alpha/include/uapi/asm/socket.h  |  2 ++
>  arch/mips/include/uapi/asm/socket.h   |  2 ++
>  arch/parisc/include/uapi/asm/socket.h |  2 ++
>  arch/sparc/include/uapi/asm/socket.h  |  2 ++
>  include/net/rps.h                     | 13 +++++++++++++
>  include/net/sock.h                    |  1 +
>  include/uapi/asm-generic/socket.h     |  2 ++
>  net/core/sock.c                       |  8 ++++++++
>  8 files changed, 32 insertions(+)
> 

> diff --git a/include/net/rps.h b/include/net/rps.h
> index e33c6a2fa8bb..5afa48868236 100644
> --- a/include/net/rps.h
> +++ b/include/net/rps.h
> @@ -98,6 +98,13 @@ static inline void _sock_rps_record_flow(const struct sock *sk)
>  	 * [1] : sk_state and sk_prot are in the same cache line.
>  	 */
>  	if (sk->sk_state == TCP_ESTABLISHED) {
> +		/* SO_NO_RFS: application asked us to never steer this flow.
> +		 * Tested after sk_state so the common case stays on one
> +		 * cache line.
> +		 */

Avoid verbose comments. Bots comment a lot more than kernel style.

Also arguably incorrect as TCP_ESTABLISHED is the common case.

> +		if (unlikely(sock_flag(sk, SOCK_NO_RFS)))
> +			return;
> +
>  		/* This READ_ONCE() is paired with the WRITE_ONCE()
>  		 * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
>  		 */
> @@ -111,6 +118,12 @@ static inline void _sock_rps_delete_flow(const struct sock *sk)
>  	rps_tag_ptr tag_ptr;
>  	u32 hash, index;
>  
> +	/* Never recorded, so never clear a slot that may belong to
> +	 * another flow.

Never is too strong here: can be set during socket lifetime.

Even if it had been correct, still another example of unnecessary
verbosity.

> +	 */
> +	if (sock_flag(sk, SOCK_NO_RFS))
> +		return;
> +

Both here and _sock_rfs_record_flow: probably should be in their
callers, like the other precondition check rfs_is_needed.

>  	hash = READ_ONCE(sk->sk_rxhash);

Optionally maybe also skip writing sk_rxhash if this is the only user.

>  	if (!hash)
>  		return;