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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 18 Jun 2020 15:45:22 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        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 6/18/20 1:25 PM, Michal Kubecek wrote:
> 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.

This is what I suggested, maybe this was not clear.

Currently known tunables are :

enum tunable_id {
	ETHTOOL_ID_UNSPEC,
	ETHTOOL_RX_COPYBREAK,
	ETHTOOL_TX_COPYBREAK,
	ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
	/*
	 * Add your fresh new tunable attribute above and remember to update
	 * tunable_strings[] in net/core/ethtool.c
	 */
	__ETHTOOL_TUNABLE_COUNT,
};

Ie add a new ETHTOOL_RX_HEADER_SPLIT value.


Or maybe I am misunderstanding your point.

> 
>> (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>