netdev - Re: [PATCH v2 net-next] net: introduce a config option to tweak MAX_SKB

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:   Wed, 22 Mar 2023 12:14:33 +0200
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH v2 net-next] net: introduce a config option to tweak
 MAX_SKB_FRAGS

On 21/03/2023 18:35, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> 
> Currently, MAX_SKB_FRAGS value is 17.
> 
> For standard tcp sendmsg() traffic, no big deal because tcp_sendmsg()
> attempts order-3 allocations, stuffing 32768 bytes per frag.
> 
> But with zero copy, we use order-0 pages.
> 
> For BIG TCP to show its full potential, we add a config option
> to be able to fit up to 45 segments per skb.
> 
> This is also needed for BIG TCP rx zerocopy, as zerocopy currently
> does not support skbs with frag list.
> 
> We have used MAX_SKB_FRAGS=45 value for years at Google before
> we deployed 4K MTU, with no adverse effect, other than
> a recent issue in mlx4, fixed in commit 26782aad00cc
> ("net/mlx4: MLX4_TX_BOUNCE_BUFFER_SIZE depends on MAX_SKB_FRAGS")
> 
> Back then, goal was to be able to receive full size (64KB) GRO
> packets without the frag_list overhead.
> 
> Note that /proc/sys/net/core/max_skb_frags can also be used to limit
> the number of fragments TCP can use in tx packets.
> 
> By default we keep the old/legacy value of 17 until we get
> more coverage for the updated values.
> 
> Sizes of struct skb_shared_info on 64bit arches
> 
> MAX_SKB_FRAGS | sizeof(struct skb_shared_info):
> ==============================================
>          17     320
>          21     320+64  = 384
>          25     320+128 = 448
>          29     320+192 = 512
>          33     320+256 = 576
>          37     320+320 = 640
>          41     320+384 = 704
>          45     320+448 = 768
> 
> This inflation might cause problems for drivers assuming they could pack
> both the incoming packet and skb_shared_info in half a page, using build_skb().
> 
> v2: fix two build errors assuming MAX_SKB_FRAGS was "unsigned long"
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  drivers/scsi/cxgbi/libcxgbi.c |  4 ++--
>  include/linux/skbuff.h        | 14 ++------------
>  net/Kconfig                   | 12 ++++++++++++
>  net/packet/af_packet.c        |  4 ++--
>  4 files changed, 18 insertions(+), 16 deletions(-)
> 

Nice! I was statically increasing it for our datapath performance tests
w/ BIG TCP and zerocopy, had to implement custom header-data split
for mlx to get it all working but the improvements are impressive as
expected.

FWIW,
Reviewed-by: Nikolay Aleksandrov <razor@...ckwall.org>