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]

Date:   Mon, 4 Dec 2017 10:43:58 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     David Miller <davem@...emloft.net>,
        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: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 think I would rather have something like a netdev private flag that
>> says LRO assembled frames are routable and just have this all run over
>> the LRO flag with a test for the private flag to avoid triggering the
>> LRO disable in the case of the flag being present. Really this is just
>> a clean LRO implementation anyway so maybe we should just go that
>> route where LRO is the hardware offload and GRO is the generic
>> software implementation of that offload. That way when GRO gets some
>> new feature that your hardware doesn't support we don't have to argue
>> about the differences since LRO is meant to be a limited
>> implementation anyway due to the nature of it being in hardware.
>
> Private flag will work.  But a standard feature flag is better since
> there are multiple drivers supporting this.  A standard way to turn
> this on/off is a better user experience.  It's also consistent with
> TSO/GSO on the transmit side.

I agree, and that is why I would prefer to see this use the LRO flag.
It is the flag that is normally used to indicating Rx coalescing in
hardware. Coming up with a custom feature flag for a form of LRO that
your hardware does doesn't make much sense to me. Otherwise I might as
well go modify ixgbe and rename the LRO it does to GRO_HW since I can
make it do most of what you are doing here.

>>
>> To me it just seems like this is an attempt to use the GRO name as a
>> marketing term and I really don't like the feel of it.
>>
>
> I disagree with this.  It's more than a marketing term.

Not really. It is a subset of GRO offload in hardware. In my mind that
is just LRO. I say subset since odds are you don't support all of the
same protocols and tunnels that GRO in software does.