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

On Mon, Dec 4, 2017 at 11:26 AM, Eric Dumazet [off-list ref] wrote:

On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote:

quoted

From: Alexander Duyck <redacted>
Date: Mon, 4 Dec 2017 10:43:58 -0800

quoted

On Mon, Dec 4, 2017 at 10:23 AM, Michael Chan <michael.chan@broadco

m.com> wrote:

quoted

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 <michael.chan@broadc

om.com> wrote:

quoted

quoted

quoted

quoted

Introduce NETIF_F_GRO_HW feature flag for NICs that support

hardware

quoted

quoted

quoted

quoted

GRO.  With this flag, we can now independently turn on or off

hardware

quoted

quoted

quoted

quoted

GRO when GRO is on.  Hardware GRO guarantees that packets can be
re-segmented by TSO/GSO to reconstruct the original packet

stream.

quoted

quoted

quoted

quoted

Cc: Ariel Elior <redacted>
Cc: <redacted>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Do we really need yet another feature bit for this? We already

have

quoted

quoted

quoted

LRO and GRO and now we have to add something that isn't quite

either

quoted

quoted

quoted

one?

I think so, to be consistent with TSO/GSO on the transmit side.

On

quoted

quoted

the receive side, we have LRO/GRO_HW/GRO.  There is difference

between

quoted

quoted

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

quoted

of the stuff GRO can do. Neither does LRO. Both occur in the

hardware

quoted

normally. It would make sense to reuse the LRO flag for this

instead

quoted

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

quoted

before they were assembled. That is why I think it would be better

to

quoted

add a flag indicating that the LRO is reversible instead of adding

yet

quoted

another feature bit that the user has to toggle. That way if at

some

quoted

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

quoted

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.

Yeah, I get that. More specifically it is the bit where you have to
disable the hardware GRO due to a bug and then that also prevents you
from using the software GRO. The reuse of the GRO flag to represent a
hardware offload in general was a poor decision.

I just don't get why these drivers have to support both LRO or GRO_HW
at the same time. It seems like it is just overkill. I would rather
see them just use the LRO feature to represent hardware aggregation of
frames and then let the stack know if LRO is reversible or not for a
given device.