From patchwork Wed Mar 14 23:23:17 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yi Zou X-Patchwork-Id: 146774 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id CD0DFB6EE7 for ; Thu, 15 Mar 2012 10:24:21 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760489Ab2CNXYT (ORCPT ); Wed, 14 Mar 2012 19:24:19 -0400 Received: from mga09.intel.com ([134.134.136.24]:45635 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751546Ab2CNXYS (ORCPT ); Wed, 14 Mar 2012 19:24:18 -0400 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 14 Mar 2012 16:24:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.67,351,1309762800"; d="scan'208";a="121116191" Received: from orsmsx604.amr.corp.intel.com ([10.22.226.87]) by orsmga002.jf.intel.com with ESMTP; 14 Mar 2012 16:23:18 -0700 Received: from orsmsx101.amr.corp.intel.com (10.22.225.128) by orsmsx604.amr.corp.intel.com (10.22.226.87) with Microsoft SMTP Server (TLS) id 8.2.255.0; Wed, 14 Mar 2012 16:23:17 -0700 Received: from orsmsx102.amr.corp.intel.com ([169.254.1.160]) by ORSMSX101.amr.corp.intel.com ([169.254.8.154]) with mapi id 14.01.0355.002; Wed, 14 Mar 2012 16:23:17 -0700 From: "Zou, Yi" To: Ben Hutchings CC: "Kirsher, Jeffrey T" , "davem@davemloft.net" , "netdev@vger.kernel.org" , "gospo@redhat.com" , "sassmann@redhat.com" Subject: RE: [net 2/2] net: fix a bug of dropping FCoE frames when disabling tx ip checksum Thread-Topic: [net 2/2] net: fix a bug of dropping FCoE frames when disabling tx ip checksum Thread-Index: AQHNAbBg6R8knyOJTkynqBNmIslsGZZqthOA//+LaCCAAJSJAP//lIsA Date: Wed, 14 Mar 2012 23:23:17 +0000 Message-ID: <138EA028228D124A900F5E6746F3C2160F6B557A@ORSMSX102.amr.corp.intel.com> References: <1331708518-28120-1-git-send-email-jeffrey.t.kirsher@intel.com> <1331708518-28120-2-git-send-email-jeffrey.t.kirsher@intel.com> <1331757355.2564.31.camel@bwh-desktop.uk.solarflarecom.com> <138EA028228D124A900F5E6746F3C2160F6B3176@ORSMSX102.amr.corp.intel.com> <1331764214.2564.33.camel@bwh-desktop.uk.solarflarecom.com> In-Reply-To: <1331764214.2564.33.camel@bwh-desktop.uk.solarflarecom.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org > On Wed, 2012-03-14 at 20:48 +0000, Zou, Yi wrote: > > > > > > On Wed, 2012-03-14 at 00:01 -0700, Jeff Kirsher wrote: > > > > From: Yi Zou > > > > > > > > Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip > > > checksum, > > > > FCoE CRC offload should not be impacte. The skb_checksum_help() is > > > needed > > > > only if it's not FCoE traffic for ip checksum, regardless of > ethtool > > > toggling > > > > the tx ip checksum on or off. > > > [...] > > > > > > I think the bug is more fundamental, and it's not just a problem for > > > FCoE. For the transmit path, the ip_summed values are specified as: > > > > > > [forwarding] > > > * COMPLETE: the most generic way. Device supplied checksum of > _all_ > > > * the packet as seen by netif_rx in skb->csum. > > > * NOTE: Even if device supports only some protocols, but > > > * is able to produce some skb->csum, it MUST use COMPLETE, > > > * not UNNECESSARY. > > > > > > [locally-generated] > > > * NONE: skb is checksummed by protocol or csum is not required. > > > * > > > * PARTIAL: device is required to csum packet as seen by > > > hard_start_xmit > > > * from skb->csum_start to the end and to record the checksum > > > * at skb->csum_start + skb->csum_offset. > > > > > > It's implicit that the checksum algorithm for CHECKSUM_PARTIAL is as > > > specified for TCP/IP. So none of those is correct when a different > > > algorithm is to be used. > > > > > > It seems like we may need another ip_summed value for FCoE, SCTP or > any > > > other protocol with a different checksum algorithm that will be > > > offloaded. Maybe allow CHECKSUM_UNNECESSARY to be used on output in > > > that case? > > > > > > Ben. > > > > CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx > path > > as well, > > I don't believe it is yet. > > > I think so but I am not 100% sure, it'd resolve this for fcoe and sctp > > like, downside is it's a bigger change that requires corresponding > changes in > > these protocol driver stacks as well. > > Yes, but this should be done properly rather than patched up with a > bunch of checks for specific protocols. > > Ben. Agreed, however, I will still have to fix the netif_needs_gso() like below to be able to use CHECKSUM_UNNECESSARY to not go down the dev_gso_segment() path for FCoE, which is using SKB_GSO_FCOE. So, anyone see this would break anything? i.e., using CHECKSUM_UNNECESSARY but still want to do dev_gso_segment? If ok, I will resend the patch to do the following. Thanks, yi > > -- > Ben Hutchings, Staff Engineer, Solarflare > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 0eac07c..c1b2b5f 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2636,7 +2636,8 @@ static inline int netif_needs_gso(struct sk_buff *skb, netdev_features_t features) { return skb_is_gso(skb) && (!skb_gso_ok(skb, features) || - unlikely(skb->ip_summed != CHECKSUM_PARTIAL)); + unlikely((skb->ip_summed != CHECKSUM_PARTIAL) && + (skb->ip_summed != CHECKSUM_UNNECESSARY))); }