On Mon, Dec 4, 2017 at 11:26 AM, Eric Dumazet <eric.duma...@gmail.com> wrote: > On Mon, 2017-12-04 at 13:59 -0500, David Miller wrote: >> From: Alexander Duyck <alexander.du...@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.du...@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.el...@cavium.com> >> >>>> Cc: everest-linux...@cavium.com >> >>>> Signed-off-by: Michael Chan <michael.c...@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.
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.
