netdev - Re: [RFC PATCH 06/21] mlx5: add header

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200618202526.zcbuzzxtln2ljawn@lion.mk-sys.cz>
Date:   Thu, 18 Jun 2020 22:25:26 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Jonathan Lemon <jonathan.lemon@...il.com>, netdev@...r.kernel.org,
        kernel-team@...com, axboe@...nel.dk,
        Govindarajulu Varadarajan <gvaradar@...co.com>
Subject: Re: [RFC PATCH 06/21] mlx5: add header_split flag

On Thu, Jun 18, 2020 at 11:12:57AM -0700, Eric Dumazet wrote:
> On 6/18/20 9:09 AM, Jonathan Lemon wrote:
> > Adds a "rx_hd_split" private flag parameter to ethtool.
> > 
> > This enables header splitting, and sets up the fragment mappings.
> > The feature is currently only enabled for netgpu channels.
> 
> We are using a similar idea (pseudo header split) to implement 4096+(headers) MTU at Google,
> to enable TCP RX zerocopy on x86.
> 
> Patch for mlx4 has not been sent upstream yet.
> 
> For mlx4, we are using a single buffer of 128*(number_of_slots_per_RX_RING),
> and 86 bytes for the first frag, so that the payload exactly fits a 4096 bytes page.
> 
> (In our case, most of our data TCP packets only have 12 bytes of TCP options)
> 
> I suggest that instead of a flag, you use a tunable, that can be set by ethtool,
> so that the exact number of bytes can be tuned, instead of hard coded in the driver.

I fully agree that such generic parameter would be a better solution
than a private flag. But I have my doubts about adding more tunables.
The point is that the concept of tunables looks like a workaround for
the lack of extensibility of the ioctl interface where the space for
adding new parameters to existing subcommands was limited (or none).

With netlink, adding new parameters is much easier and as only three
tunables were added in 6 years (or four with your proposal), we don't
have to worry about having too many different attributes (current code
isn't even designed to scale well to many tunables).

This new header split parameter could IMHO be naturally put together
with rx-copybreak and tx-copybreak and possibly any future parameters
to control how packet contents is passed between NIC/driver and
networking stack.

> (Patch for the counter part of [1] was resent 10 days ago on netdev@ by Govindarajulu Varadarajan)
> (Not sure if this has been merged yet)

Not yet, I want to take another look in the rest of this week.

Michal

> [1]
> 
> commit f0db9b073415848709dd59a6394969882f517da9
> Author: Govindarajulu Varadarajan <_govind@....com>
> Date:   Wed Sep 3 03:17:20 2014 +0530
> 
>     ethtool: Add generic options for tunables
>     
>     This patch adds new ethtool cmd, ETHTOOL_GTUNABLE & ETHTOOL_STUNABLE for getting
>     tunable values from driver.
>     
>     Add get_tunable and set_tunable to ethtool_ops. Driver implements these
>     functions for getting/setting tunable value.
>     
>     Signed-off-by: Govindarajulu Varadarajan <_govind@....com>
>     Signed-off-by: David S. Miller <davem@...emloft.net>