Message-ID: <CAKgT0Ufzvnf=0Sg3S9jkGKK+ze+7j_sLdqkw4q=-OdEcyRU=GA@mail.gmail.com>
Date:   Fri, 29 Dec 2017 07:12:36 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     Michael Chan <michael.chan@...adcom.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>
Subject: Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

On Fri, Dec 29, 2017 at 4:43 AM, Sabrina Dubroca <sd@...asysnail.net> wrote:
> 2017-12-22, 10:14:32 -0800, Alexander Duyck wrote:
>> On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@...asysnail.net> wrote:
>> > IIUC, with the patches that were applied, each driver can define
>> > whether GRO_HW depends on GRO? From a user's perspective, this
>> > inconsistent behavior is going to be quite confusing.
>> >
>> > Worse than inconsistent behavior, it looks like a driver deciding that
>> > GRO_HW doesn't depend on GRO is going to introduce a change of
>> > behavior.  Previously, when GRO was disabled, there wouldn't be any
>> > packet over MTU handed to the network stack.  Now, even if GRO is
>> > disabled, GRO_HW might still be enabled, so we might get over-MTU
>> > packets because of hardware GRO.
>>
>> This isn't actually true. LRO was still handling packets larger than
>> MTU over even when GRO was disabled.
>
> Sure, LRO will also cause that, but we're speaking in the context of
> GRO here, which means no LRO.

We are talking about GRO_HW. Which is basically aggregation being
performed in hardware. The choice of name is unfortunate though since
it implies this is GRO when what is actually happening is just
GRO-like. Really the only difference between LRO and GRO_HW is that
the frames are better formed in the final result. It is a matter of
what is performed versus where it is performed.

>> > I don't think drivers should be allowed to say "GRO_HW doesn't depend
>> > on GRO".
>>
>> Why not, it doesn't. In my mind GRO_HW is closer to LRO than it is to
>> GRO.
>
> Why do you say that? It looks like GRO to me. These drivers are
> calling tcp_gro_complete() for example.

The final frame output in the case of frames that are aggregated will
be the same as GRO. However LRO could theoretically do the same thing
if the hardware had been implemented correctly in the first place.

>> The only ugly bit as I see it is that these devices were exposing
>> the feature via the GRO flag in the first place. So for the sake of
>> legacy they might want to carry around the dependency.
>>
>> > I think it's reasonable to be able to disable software GRO even if
>> > hardware GRO is enabled. Thus, I would propose:
>> > - keep the current GRO flag
>> > - add a GRO_HW flag, depending on GRO, enforced by the core as in
>> >   earlier versions of these patches
>> > - add a GRO_SW flag, also depending on GRO
>>
>> This seems like a bunch of extra overhead for not much gain. Do we
>> really need to fork GRO into 3 bits? I would argue that GRO_HW really
>> should have been branded something like FORWARDABLE_LRO, but nobody
>> wanted to touch the name LRO since it apparently has some negative
>> stigma to it. If we had used a name like that we probably wouldn't be
>> going through all these extra hoops. The only real reason why this is
>> even being associated with GRO in the first place is that is how this
>> feature was hidden by the drivers so they got around having to deal
>> with the LRO being disabled for routing/forwarding issue. Those are
>> the parts that want to keep it associated with GRO since that is how
>> they exposed it in their devices originally.
>
> I think it wouldn't have hidden behind GRO if it wasn't GRO. Again,
> why do you think it's not GRO?

The only real piece I see as missing the propagation bits for GRO_HW
to upper devices. The argument was that GRO doesn't have to do that,
but I think we are going to have to get there at some point so that
when we encounter the first piece of hardware that does this wrong we
have a switch ready to allow us to disable it for debugging. If we are
insistent on having a switch that binds together GRO and GRO_HW one
thought I had was to change the meaning of GRO_HW for virtual devices
to indicate that we allow GRO to happen on the lower devices
associated with this device. My thought was that GRO_HW allows GRO to
take place on the physical hardware below the netdev, so why couldn't
we say that GRO_HW also applies to upper devices and them indicating
they don't want GRO, GRO_HW, or LRO for lower devices. Thoughts? The
only reason it occurred to me is that there is currently no way to
propagate a disable of GRO so GRO_HW might be a way to do that for
virtual devices. It would make GRO_HW more about the location of the
GRO feature instead of the feature itself. Basically if you clear it
then nothing below that point should be doing any sort of aggregation.

Thanks.

- Alex

Powered by blists - more mailing lists