netdev - Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO

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: <CAKgT0Udzdn2K7mnVjYLHGUAe33G93Kvb09MqVdQrNr9Y4ZkjCw@mail.gmail.com>
Date:   Mon, 4 Dec 2017 11:24:43 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Michael Chan <michael.chan@...adcom.com>,
        Netdev <netdev@...r.kernel.org>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        everest-linux-l2@...ium.com
Subject: Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

On Mon, Dec 4, 2017 at 10:59 AM, David Miller <davem@...emloft.net> wrote:
> From: Alexander Duyck <alexander.duyck@...il.com>
> Date: Mon, 4 Dec 2017 10:43:58 -0800
>
>> On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@...adcom.com> wrote:
>>> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
>>> <alexander.duyck@...il.com> wrote:
>>>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@...adcom.com> wrote:
>>>>> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
>>>>> GRO.  With this flag, we can now independently turn on or off hardware
>>>>> GRO when GRO is on.  Hardware GRO guarantees that packets can be
>>>>> re-segmented by TSO/GSO to reconstruct the original packet stream.
>>>>>
>>>>> Cc: Ariel Elior <Ariel.Elior@...ium.com>
>>>>> Cc: everest-linux-l2@...ium.com
>>>>> Signed-off-by: Michael Chan <michael.chan@...adcom.com>
>>>>
>>>> Do we really need yet another feature bit for this? We already have
>>>> LRO and GRO and now we have to add something that isn't quite either
>>>> one?
>>>
>>> I think so, to be consistent with TSO/GSO on the transmit side.  On
>>> the receive side, we have LRO/GRO_HW/GRO.  There is difference between
>>> LRO/GRO_HW that we need to distinguish between the 2.
>>
>> I don't really see the difference. Your GRO_HW likely doens't do all
>> of the stuff GRO can do. Neither does LRO. Both occur in the hardware
>> normally. It would make sense to reuse the LRO flag for this instead
>> of coming up with a new feature flag that makes things confusing by
>> saying you are doing a software offload in hardware.
>>
>> I view LRO as a subset of what GRO can handle, that is performed in
>> hardware. From the stack perspective the only thing that really
>> matters is that the frames can be segmented back into what they were
>> before they were assembled. That is why I think it would be better to
>> add a flag indicating that the LRO is reversible instead of adding yet
>> another feature bit that the user has to toggle. That way if at some
>> point in the future an issue is found where your "GRO in hardware"
>> feature has a bug that isn't reversible it is just a matter of
>> clearing the privage flag bit and the mechanisms already in place for
>> dealing with assembly and routing can take over.
>
> I don't think they should use the LRO flag.
>
> If their HW GRO stream is fully reversible, which it is, then it's not
> LRO.

I get all that. I was just hoping that we could phase out the old LRO
over time and push it more towards a GRO friendly implementation. It
is going to be annoying when a marketing person gets a hold of the
term because what you are going to end up with is people pushing for
LRO implementations to be tweaked to work like GRO just so they can
use the feature flag.

> LRO gets disabled when bridging or routing is enabled, and HW GRO
> should not take this penalty.

That is why I suggested a private flag indicating that LRO on this
device is reversible. Basically if it is reversible then we don't need
to disable it when routing/bridging.

If we insist on using the name GRO for this I would prefer GRO_PARTIAL
instead of GRO_HW. It would help to keep things symmetric as we have
been saying since GSO_PARTIAL is a partial implementation of GSO in
the hardware and this would be a partial implementation of GRO in the
hardware as it would be missing some protocols and functionality.

In addition if anything can be done so that the hardware GRO
implementation doesn't prevent software GRO from aggregating things
even larger if possible that would be preferred.

Anyway that is just my $.02 on this.

- Alex