netdev - Re: [PATCH net] net: avoid indirect memory pressure calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Feb 2023 17:44:38 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Florian Westphal <fw@...len.de>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, shakeelb@...gle.com, soheil@...gle.com
Subject: Re: [PATCH net] net: avoid indirect memory pressure calls

On Fri, Feb 24, 2023 at 7:49 PM Florian Westphal <fw@...len.de> 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
>
> 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 <fw@...len.de>
> ---
>  net/core/sock.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 341c565dbc26..45d247112aa5 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -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.

>  {
> -       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);
>  }
>

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]
diff --git a/net/core/sock.c b/net/core/sock.c
index 341c565dbc262fcece1c5b410609d910a68edcb0..0472f8559a10136672ce6647f133367c81a93cb7
100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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;
        }