[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ba7f6c19-989a-c5af-6ca7-12614680af59@gmail.com>
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>
Powered by blists - more mailing lists