netdev - Re: [PATCH net-next 01/15] net: add netdev->tso_ipv6_max

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]

Message-ID: <20220203105842.60c25d46@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 3 Feb 2022 10:58:42 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>, Coco Li <lixiaoyan@...gle.com>
Subject: Re: [PATCH net-next 01/15] net: add netdev->tso_ipv6_max_size
 attribute

On Thu, 3 Feb 2022 08:56:56 -0800 Eric Dumazet wrote:
> On Thu, Feb 3, 2022 at 8:34 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > On Wed,  2 Feb 2022 17:51:26 -0800 Eric Dumazet wrote:  
> > > From: Eric Dumazet <edumazet@...gle.com>
> > >
> > > Some NIC (or virtual devices) are LSOv2 compatible.
> > >
> > > BIG TCP plans using the large LSOv2 feature for IPv6.
> > >
> > > New netlink attribute IFLA_TSO_IPV6_MAX_SIZE is defined.
> > >
> > > Drivers should use netif_set_tso_ipv6_max_size() to advertize their limit.
> > >
> > > Unchanged drivers are not allowing big TSO packets to be sent.  
> >
> > Many drivers will have a limit on how many buffer descriptors they
> > can chain, not the size of the super frame, I'd think. Is that not
> > the case? We can't assume all pages but the first and last are full,
> > right?  
> 
> In our case, we have a 100Gbit Google NIC which has these limits:
> 
> - TX descriptor has a 16bit field filled with skb->len
> - No more than 21 frags per 'packet'
> 
> In order to support BIG TCP on it, we had to split the bigger TCP packets
> into smaller chunks, to satisfy both constraints (even if the second
> constraint is hardly hit once you chop to ~60KB packets, given our 4K
> MTU)
> 
> ndo_features_check() might help to take care of small oddities.

Makes sense, I was curious if we can do more in the core so that fewer
changes are required in the drivers. Both so that drivers don't have to
strip the header and so that drivers with limitations can be served 
pre-cooked smaller skbs.

> For instance I will insert the following in the next version of the series:
> 
> commit 26644be08edc2f14f6ec79f650cc4a5d380df498
> Author: Eric Dumazet <edumazet@...gle.com>
> Date:   Wed Feb 2 23:22:01 2022 -0800
> 
>     net: typhoon: implement ndo_features_check method
> 
>     Instead of disabling TSO if MAX_SKB_FRAGS > 32, implement
>     ndo_features_check() method for this driver.
> 
>     If skb has more than 32 frags, use the following heuristic:
> 
>     1) force GSO for gso packets
>     2) Otherwise force linearization.
> 
>     Most locally generated TCP packets will use a small number of fragments
>     anyway.
> 
>     Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> 
> diff --git a/drivers/net/ethernet/3com/typhoon.c
> b/drivers/net/ethernet/3com/typhoon.c
> index 8aec5d9fbfef2803c181387537300502a937caf0..216e26a49e9c272ba7483bfa06941ff11ea40e3c
> 100644
> --- a/drivers/net/ethernet/3com/typhoon.c
> +++ b/drivers/net/ethernet/3com/typhoon.c
> @@ -138,11 +138,6 @@ MODULE_PARM_DESC(use_mmio, "Use MMIO (1) or
> PIO(0) to access the NIC. "
>  module_param(rx_copybreak, int, 0);
>  module_param(use_mmio, int, 0);
> 
> -#if defined(NETIF_F_TSO) && MAX_SKB_FRAGS > 32
> -#warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO
> -#undef NETIF_F_TSO
> -#endif

I wonder how many drivers just assumed MAX_SKB_FRAGS will never 
change :S What do you think about a device-level check in the core 
for number of frags?