On Mon, Dec 4, 2017 at 11:52 AM, Michael Chan [off-list ref] wrote:

On Mon, Dec 4, 2017 at 10:43 AM, Alexander Duyck
[off-list ref] wrote:

quoted

On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan [off-list ref] wrote:

quoted

On Mon, Dec 4, 2017 at 8:47 AM, Alexander Duyck
[off-list ref] wrote:

quoted

On Mon, Dec 4, 2017 at 3:12 AM, Michael Chan [off-list ref] wrote:

quoted

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 <redacted>
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 agree it is a little confusing, but LRO + private flag is just as
confusing in my opinion.

quoted

I view LRO as a subset of what GRO can handle, that is performed in
hardware.

I don't view LRO as a subset of GRO.  LRO loses information that
cannot be reconstructed back once an LRO frame is aggregated.  LRO is
just different from GRO.  GRO_HW must provide enough information to
reconstruct the original frames.

I think we can just agree to disagree on this. Defining GRO_HW as
reversible LRO is the way I view it. In my mind LRO is assembly in
hardware, the non-reversible part is an unfortunate side effect of
existing implementations. From the sound of things you and Dave view
the non-reversible part as a core bit of LRO and the hardware/driver
implementation of it is just a secondary thing.

quoted

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.

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.

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.

quoted

quoted

quoted

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.

quoted

quoted

quoted

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?