Patchwork [net,2/2] net: fix a bug of dropping FCoE frames when disabling tx ip checksum

login
register
mail settings
Submitter Yi Zou
Date March 14, 2012, 11:23 p.m.
Message ID <138EA028228D124A900F5E6746F3C2160F6B557A@ORSMSX102.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/146774/
State RFC
Delegated to: David Miller
Headers show

Comments

Yi Zou - March 14, 2012, 11:23 p.m.
> 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 <yi.zou@intel.com>

> > > >

> > > > 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.

Patch

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)));

 }