Skip to contentSkip to conversation

net: avoid indirect memory pressure calls

PATCH In review Florian Westphal 4 participants 9 messages Feb 24, 2023 – Mar 1, 2023

Subsystems (3)
Networking Network drivers Sockets
lore
Share & tools
lore

Florian Westphal

author
#0 lore raw

PATCH 1/1 net: avoid indirect memory pressure calls

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

"iperf3 -c $peer -t 60 -Z -f g", connected via veth in another netns.

Fixes 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
Signed-off-by Florian Westphal <[email protected]>
---
 net/core/sock.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
net/core/sock.c +13 -9
index 341c565dbc26..45d247112aa5 100644
@@ -2809,22 +2809,26 @@ EXPORT_SYMBOL(sock_cmsg_send);
 
 static void sk_enter_memory_pressure(struct sock *sk)
 {
-	if (!sk->sk_prot->enter_memory_pressure)
+	unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
+
+	if (!memory_pressure || READ_ONCE(*memory_pressure))
 		return;
 
-	sk->sk_prot->enter_memory_pressure(sk);
+	if (sk->sk_prot->enter_memory_pressure)
+		sk->sk_prot->enter_memory_pressure(sk);
 }
 
 static void sk_leave_memory_pressure(struct sock *sk)
 {
-	if (sk->sk_prot->leave_memory_pressure) {
-		sk->sk_prot->leave_memory_pressure(sk);
-	} else {
-		unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
+	unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
 
-		if (memory_pressure && READ_ONCE(*memory_pressure))
-			WRITE_ONCE(*memory_pressure, 0);
-	}
+	if (!memory_pressure || READ_ONCE(*memory_pressure) == 0)
+		return;
+
+	if (sk->sk_prot->leave_memory_pressure)
+		sk->sk_prot->leave_memory_pressure(sk);
+	else
+		WRITE_ONCE(*memory_pressure, 0);
 }
 
 DEFINE_STATIC_KEY_FALSE(net_high_order_alloc_disable_key);
Signature · 1 line
2.39.2
maintainer
#1 lore raw
Quoted 15 lines · On Fri, 24 Feb 2023 19:46:06 +0100 Florian Westphal wrote:

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

Quoted 7 lines

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

"iperf3 -c $peer -t 60 -Z -f g", connected via veth in another netns.

Fixes 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
Signed-off-by Florian Westphal <[email protected]>

Looks acceptable, Eric?

#2 lore raw

From: Jakub Kicinski <[email protected]>
Date: Mon, 27 Feb 2023 15:27:41 -0800

Quoted 19 lines · On Fri, 24 Feb 2023 19:46:06 +0100 Florian Westphal wrote:
Quoted 18 lines

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

Quoted 7 lines
Quoted 4 lines

"iperf3 -c $peer -t 60 -Z -f g", connected via veth in another netns.

Fixes 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
Signed-off-by Florian Westphal <[email protected]>

Looks acceptable, Eric?

I'm no Eric, but I'd only change this:

+ if (!memory_pressure || READ_ONCE(*memory_pressure) == 0)

to

+ if (!memory_pressure || !READ_ONCE(*memory_pressure))

:p

The perf boost looks gross, love that *_*

Thanks,
Olek

author
#3 lore raw
Quoted 35 lines · Alexander Lobakin

From: Jakub Kicinski <[email protected]>
Date: Mon, 27 Feb 2023 15:27:41 -0800

Quoted 19 lines · On Fri, 24 Feb 2023 19:46:06 +0100 Florian Westphal wrote:
Quoted 18 lines

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

Quoted 7 lines
Quoted 4 lines

"iperf3 -c $peer -t 60 -Z -f g", connected via veth in another netns.

Fixes 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
Signed-off-by Florian Westphal <[email protected]>

Looks acceptable, Eric?

I'm no Eric, but I'd only change this:

net/core/sock.c +1 -0
+	if (!memory_pressure || READ_ONCE(*memory_pressure) == 0)

to

Review

Quoted 1 line
(quoted diff) +1 -0
+	if (!memory_pressure || !READ_ONCE(*memory_pressure))

I intentioanlly used '== 0', i found it too easy to miss the '!' before 'R'. But maybe I just need better glasses.

#4 lore raw
Quoted 33 lines · Florian Westphal
Quoted 31 lines · Alexander Lobakin

From: Jakub Kicinski <[email protected]>
Date: Mon, 27 Feb 2023 15:27:41 -0800

Quoted 19 lines · On Fri, 24 Feb 2023 19:46:06 +0100 Florian Westphal wrote:
Quoted 18 lines

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

Quoted 7 lines
Quoted 4 lines

"iperf3 -c $peer -t 60 -Z -f g", connected via veth in another netns.

Fixes 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
Signed-off-by Florian Westphal <[email protected]>

Looks acceptable, Eric?

I'm no Eric, but I'd only change this:

Quoted 8 lines
Quoted 5 lines

+ if (!memory_pressure || READ_ONCE(*memory_pressure) == 0)

to

+ if (!memory_pressure || !READ_ONCE(*memory_pressure))

I intentioanlly used '== 0', i found it too easy to miss the '!' before 'R'. But maybe I just need better glasses.

Sorry for the delay, I will take a look.

#5 lore raw
In reply to Florian Westphal
Quoted 32 lines · Florian Westphal

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

"iperf3 -c $peer -t 60 -Z -f g", connected via veth in another netns.

Fixes 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
Signed-off-by Florian Westphal <[email protected]>
---
 net/core/sock.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
net/core/sock.c +0 -0
index 341c565dbc26..45d247112aa5 100644

net/core/sock.c EXPORT_SYMBOL(sock_cmsg_send); · line 2810

Quoted 2 lines
net/core/sock.c +0 -0
@@ -2809,22 +2809,26 @@ EXPORT_SYMBOL(sock_cmsg_send);
 
 static void sk_enter_memory_pressure(struct sock *sk)

This one is probably not called under normal circumstances.

Quoted 22 lines
net/core/sock.c +6 -8
 {
-       if (!sk->sk_prot->enter_memory_pressure)
+       unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
+
+       if (!memory_pressure || READ_ONCE(*memory_pressure))
                return;
 
-       sk->sk_prot->enter_memory_pressure(sk);
+       if (sk->sk_prot->enter_memory_pressure)
+               sk->sk_prot->enter_memory_pressure(sk);
 }
 
 static void sk_leave_memory_pressure(struct sock *sk)
 {
-       if (sk->sk_prot->leave_memory_pressure) {
-               sk->sk_prot->leave_memory_pressure(sk);
-       } else {
-               unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
+       unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
 
-               if (memory_pressure && READ_ONCE(*memory_pressure))
-                       WRITE_ONCE(*memory_pressure, 0);

net/core/sock.c EXPORT_SYMBOL(sock_cmsg_send); · line 2833

Quoted 10 lines
net/core/sock.c +7 -1
-       }
+       if (!memory_pressure || READ_ONCE(*memory_pressure) == 0)
+               return;
+
+       if (sk->sk_prot->leave_memory_pressure)
+               sk->sk_prot->leave_memory_pressure(sk);
+       else
+               WRITE_ONCE(*memory_pressure, 0);
 }

For this one, we have too callers.

First one (__sk_mem_reduce_allocated) is using

if (sk_under_memory_pressure(sk)

     ...

So I wonder if for consistency we could use the same heuristic [1] ?

Note that [1] is memcg aware/ready.

Refactoring of sk_enter_memory_pressure() and
sk_leave_memory_pressure() could wait net-next maybe...

[1]

net/core/sock.c +0 -0
index 341c565dbc262fcece1c5b410609d910a68edcb0..0472f8559a10136672ce6647f133367c81a93cb7

100644

net/core/sock.c +0 -0
@@ -2993,7 +2993,8 @@ int __sk_mem_raise_allocated(struct sock *sk,

int size, int amt, int kind)

        /* Under limit. */
        if (allocated <= sk_prot_mem_limits(sk, 0)) {
-               sk_leave_memory_pressure(sk);
+               if (sk_under_memory_pressure(sk))
+                       sk_leave_memory_pressure(sk);
                return 1;
        }
#6 lore raw
Quoted 15 lines · Eric Dumazet
Quoted 13 lines · Florian Westphal

There is a noticeable tcp performance regression (loopback or cross-netns), seen with iperf3 -Z (sendfile mode) when generic retpolines are needed.

With SK_RECLAIM_THRESHOLD checks gone number of calls to enter/leave memory pressure happen much more often. For TCP indirect calls are used.

We can't remove the if-set-return short-circuit check in tcp_enter_memory_pressure because there are callers other than sk_enter_memory_pressure. Doing a check in the sk wrapper too reduces the indirect calls enough to recover some performance.

Quoted 6 lines
Quoted 6 lines

Before,
0.00-60.00 sec 322 GBytes 46.1 Gbits/sec receiver

After:
0.00-60.04 sec 359 GBytes 51.4 Gbits/sec receiver

BTW I was curious why Google was not seeing this, and it appears Brian Vasquez forgot to upstream this change...

commit 5ea2f21d6c1078d2c563cb455ad5877b4ada94e1

Author: Brian Vazquez <[email protected]>
Date:   Thu Mar 3 19:09:49 2022 -0800

PRODKERNEL: net-directcall: annotate tcp_leave_memory_pressure and tcp_getsockopt

Switch to upstream infra to make rebase easier

Tested: built and booted on lab machine

Upstream-Plan: 150254871
Effort: net-directcall

net/core/sock.c +0 -0
index 05032b399c873984e5297898d647905ca9f21f2e..54cb989dc162f3982380ac12cf5a150214e209a2

100644

net/core/sock.c +4 -1
@@ -2647,10 +2647,13 @@ static void sk_enter_memory_pressure(struct sock *sk)
        sk->sk_prot->enter_memory_pressure(sk);
 }
 
+INDIRECT_CALLABLE_DECLARE(void tcp_leave_memory_pressure(struct sock *sk));
+
 static void sk_leave_memory_pressure(struct sock *sk)
 {
        if (sk->sk_prot->leave_memory_pressure) {
-               sk->sk_prot->leave_memory_pressure(sk);
+               INDIRECT_CALL_1(sk->sk_prot->leave_memory_pressure,
+                               tcp_leave_memory_pressure, sk);
        } else {
                unsigned long *memory_pressure = sk->sk_prot->memory_pressure;
 

@@ -3439,6 +3442,10 @@ int sock_recv_errqueue(struct sock *sk, struct

msghdr *msg, int len,
}
EXPORT_SYMBOL(sock_recv_errqueue);

+INDIRECT_CALLABLE_DECLARE(int tcp_getsockopt(struct sock *sk, int level,
+ int optname, char __user *optval,
+ int __user *optlen));
+
/*
* Get a socket option on an socket.
*
@@ -3451,7 +3458,8 @@ int sock_common_getsockopt(struct socket *sock, int level, int optname,

 {
        struct sock *sk = sock->sk;

- return sk->sk_prot->getsockopt(sk, level, optname, optval, optlen);
+ return INDIRECT_CALL_1(sk->sk_prot->getsockopt, tcp_getsockopt,
+ sk, level, optname, optval, optlen);
}
EXPORT_SYMBOL(sock_common_getsockopt);

author
#7 lore raw
Quoted 19 lines · Eric Dumazet

BTW I was curious why Google was not seeing this, and it appears Brian Vasquez forgot to upstream this change...

commit 5ea2f21d6c1078d2c563cb455ad5877b4ada94e1

Author: Brian Vazquez <[email protected]>
Date:   Thu Mar 3 19:09:49 2022 -0800

PRODKERNEL: net-directcall: annotate tcp_leave_memory_pressure and tcp_getsockopt

net/core/sock.c +0 -0
index 05032b399c873984e5297898d647905ca9f21f2e..54cb989dc162f3982380ac12cf5a150214e209a2

100644

net/core/sock.c +0 -0
@@ -2647,10 +2647,13 @@ static void sk_enter_memory_pressure(struct sock *sk)
        sk->sk_prot->enter_memory_pressure(sk);
 }

net/core/sock.c static void sk_enter_memory_pressure(struct sock *sk) · line 2658

Quoted 10 lines
net/core/sock.c +4 -1
+INDIRECT_CALLABLE_DECLARE(void tcp_leave_memory_pressure(struct sock *sk));
+
 static void sk_leave_memory_pressure(struct sock *sk)
 {
        if (sk->sk_prot->leave_memory_pressure) {
-               sk->sk_prot->leave_memory_pressure(sk);
+               INDIRECT_CALL_1(sk->sk_prot->leave_memory_pressure,
+                               tcp_leave_memory_pressure, sk);
        } else {
                unsigned long *memory_pressure = sk->sk_prot->memory_pressure;

re-tested: this change also resolves the regression i was seeing.

If you prefer to upstream this instead of the proposed change then I'm fine with that.

Thanks.

#8 lore raw
Quoted 34 lines · Florian Westphal
Quoted 19 lines · Eric Dumazet

BTW I was curious why Google was not seeing this, and it appears Brian Vasquez forgot to upstream this change...

commit 5ea2f21d6c1078d2c563cb455ad5877b4ada94e1

Author: Brian Vazquez <[email protected]>
Date:   Thu Mar 3 19:09:49 2022 -0800

PRODKERNEL: net-directcall: annotate tcp_leave_memory_pressure and tcp_getsockopt

net/core/sock.c +0 -0
index 05032b399c873984e5297898d647905ca9f21f2e..54cb989dc162f3982380ac12cf5a150214e209a2

100644

net/core/sock.c +0 -0
@@ -2647,10 +2647,13 @@ static void sk_enter_memory_pressure(struct sock *sk)
        sk->sk_prot->enter_memory_pressure(sk);
 }

net/core/sock.c static void sk_enter_memory_pressure(struct sock *sk) · line 2658

Quoted 10 lines
net/core/sock.c +4 -1
+INDIRECT_CALLABLE_DECLARE(void tcp_leave_memory_pressure(struct sock *sk));
+
 static void sk_leave_memory_pressure(struct sock *sk)
 {
        if (sk->sk_prot->leave_memory_pressure) {
-               sk->sk_prot->leave_memory_pressure(sk);
+               INDIRECT_CALL_1(sk->sk_prot->leave_memory_pressure,
+                               tcp_leave_memory_pressure, sk);
        } else {
                unsigned long *memory_pressure = sk->sk_prot->memory_pressure;

re-tested: this change also resolves the regression i was seeing.

Quoted 2 lines

If you prefer to upstream this instead of the proposed change then I'm fine with that.

This seems a bit less risky, if the plan is to add this to stable trees. ( We had this mitigation for ~4 years at Google)

I will rebase Brian patch (only the tcp_leave_memory_pressure part) and send it.

Thanks !