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

[Eric Dumazet <eric.dumazet@gmail.com>]

On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote:
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Date: Mon, 4 Dec 2017 10:43:58 -0800
> 
> > On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadco
> m.com> wrote:
> >> On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
> >> <alexander.duyck@gmail.com> wrote:
> >>> On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan <michael.chan@broadc
> 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@cavium.com>
> >>>> Cc: everest-linux-l2@cavium.com
> >>>> Signed-off-by: Michael Chan <michael.chan@broadcom.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.