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: <CAKgT0Udj45tba+bKa9ON5QAEmoikL7MxZy9uL5LgiAwD7SSfJA@mail.gmail.com>
Date:   Mon, 4 Dec 2017 11:36:03 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        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 11:26 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Mon, 2017-12-04 at 13:59 -0500, David Miller 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@...adco
>> m.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@...adc
>> om.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.
>>
>> LRO gets disabled when bridging or routing is enabled, and HW GRO
>> should not take this penalty.
>
> Also having separate flags means that one can decide to disable HW GRO
> and enable (linux) GRO if he wants to. Or the opposite.
>
> I definitely like this idea.

Yeah, I get that. More specifically it is the bit where you have to
disable the hardware GRO due to a bug and then that also prevents you
from using the software GRO. The reuse of the GRO flag to represent a
hardware offload in general was a poor decision.

I just don't get why these drivers have to support both LRO or GRO_HW
at the same time. It seems like it is just overkill. I would rather
see them just use the LRO feature to represent hardware aggregation of
frames and then let the stack know if LRO is reversible or not for a
given device.