From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sabrina Dubroca Subject: Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW Date: Fri, 22 Dec 2017 15:57:58 +0100 Message-ID: <20171222145758.GA685@bistromath.localdomain> References: <1513411784-17653-1-git-send-email-michael.chan@broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: davem@davemloft.net, netdev@vger.kernel.org, andrew.gospodarek@broadcom.com, alexander.duyck@gmail.com To: Michael Chan Return-path: Received: from mx1.redhat.com ([209.132.183.28]:48596 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753048AbdLVO6D (ORCPT ); Fri, 22 Dec 2017 09:58:03 -0500 Content-Disposition: inline In-Reply-To: <1513411784-17653-1-git-send-email-michael.chan@broadcom.com> Sender: netdev-owner@vger.kernel.org List-ID: Hello, Sorry for commenting late. 2017-12-16, 03:09:39 -0500, Michael Chan wrote: > Introduce NETIF_F_GRO_HW feature flag and convert drivers that support > hardware GRO to use the new flag. > > v5: > - Documentation changes requested by Alexander Duyck. > - bnx2x changes requested by Manish Chopra to enable LRO by default, and > disable GRO_HW if disable_tpa module parameter is set. > > v4: > - more changes requested by Alexander Duyck: > - check GRO_HW/GRO dependency in drivers's ndo_fix_features(). > - Reverse the order of RXCSUM and GRO_HW dependency check in > netdev_fix_features(). > - No propagation in netdev_disable_gro_hw(). 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. I don't think drivers should be allowed to say "GRO_HW doesn't depend on GRO". 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 Thanks, -- Sabrina