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: <CACKFLi=9d2yPftR11RggDkr37Zb3T5rbrPK5xny8vqEUYdMy1A@mail.gmail.com>
Date:   Mon, 4 Dec 2017 15:05:23 -0800
From:   Michael Chan <michael.chan@...adcom.com>
To:     Alexander Duyck <alexander.duyck@...il.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 12:58 PM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> On Mon, Dec 4, 2017 at 11:52 AM, Michael Chan <michael.chan@...adcom.com> wrote:

>> NETIF_F_GRO_HW is a flag that depends on NETIF_F_GRO.  In some ways,
>> it is similar to a private flag that depends on NETIF_F_LRO.  But I
>> think a standard flag is better.
>
> Why would you make it dependent on NETIF_F_GRO? That doesn't make any
> sense to me. It gets in the way of GRO anyway as it can't assemble an
> already aggregated frame.

GRO_HW is basically hardware accelerated GRO.  If the user doesn't
want GRO at all (let's say he is running generic XDP, or he wants
tcpdump to show the packets on the wire), disabling GRO should disable
everything.  It's more intuitive that way.

>
> It seems more like the two features should be able to co-exist with
> either one being able to be disabled/enabled independently. It makes
> it much easier to debug things this way. Otherwise there is no way to
> tell if a given issue is software or hardware GRO since disabling
> software disables them both.
>
>>>
>>>>>
>>>>> 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.
>>
>> Again, there is enough difference between LRO and hardware GRO that it
>> makes sense to add a new flag.  Functionally, a private flag will work
>> too, but a standard flag makes more intuitive sense to me and to
>> users.
>>
>> Yeah, the idea is that any vendor can use GRO_HW.  Today, there are 3
>> drivers supporting it.  I'm sure there will be other drivers
>> supporting this in the future.  For something that is supported by
>> multiple vendors, that's another reason to use a standard flag.
>>
>>>
>>>>>
>>>>> 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.
>>
>> Of course, hardware has some limitations, such as the number of TCP
>> connections it can aggregate, etc.  But again, it is different from
>> LRO.
>
> The bit I don't like about this is that if you bond a device that is
> running LRO with one that is running GSO_HW you now have to disable
> two flags on the bond in order to disable hardware aggregation.
>
> Admittedly I haven't take a look at the entire patch set, but did you
> also take care of the flag sychronization issues this is going to
> cause with upper devices such as vlan, macvlan, bond, etc?

I will take another look.  Thanks.