Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks

[John Fastabend <john.r.fastabend@intel.com>]

Herbert Xu wrote:
> On Sat, Feb 27, 2010 at 03:27:25AM -0800, David Miller wrote:
>   
>> If we have ip_summed == CHECKSUM_PARTIAL might some classifier
>> or packet scheduler action module require that the
>> transport header is setup properly before the SKB gets into
>> there?
>>     
>
> I think this is OK as the transport header setting was only there
> for backwards compatibility with certain drivers that relied on it.
> Its use was very much isolated.
>
> I just did a grep on net/sched and couldn't see anything obvious
> that uses transport_header.
>
>   
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index eb7f1a4..626124d 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>>>  {
>>>  	const struct net_device_ops *ops = dev->netdev_ops;
>>>  	int rc = NETDEV_TX_OK;
>>> +	int need_gso = netif_needs_gso(dev, skb);
>>> +
>>> +	if (!need_gso) {
>>> +		if (skb_has_frags(skb) &&
>>> +		    !(dev->features & NETIF_F_FRAGLIST) &&
>>> +		    __skb_linearize(skb))
>>> +			goto out_kfree_skb;
>>> +
>>> +		/* Fragmented skb is linearized if device does not support SG,
>>> +		 * or if at least one of fragments is in highmem and device
>>> +		 * does not support DMA from it.
>>> +		 */
>>> +		if (skb_shinfo(skb)->nr_frags &&
>>> +		    (!(dev->features & NETIF_F_SG) ||
>>> +		      illegal_highdma(dev, skb)) &&
>>> +		    __skb_linearize(skb))
>>> +			goto out_kfree_skb;
>>>       
>
> Please use skb_needs_linearize.
>
>   
>>> +		/* If packet is not checksummed and device does not support
>>> +		 * checksumming for this protocol, complete checksumming here.
>>> +		 */
>>> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>> +			skb_set_transport_header(skb, skb->csum_start -
>>> +				      skb_headroom(skb));
>>> +			if (!dev_can_checksum(dev, skb) &&
>>> +			     skb_checksum_help(skb))
>>> +				goto out_kfree_skb;
>>> +		}
>>> +	}
>>>  
>>>  	if (likely(!skb->next)) {
>>>  		if (!list_empty(&ptype_all))
>>>  			dev_queue_xmit_nit(skb, dev);
>>>  
>>> -		if (netif_needs_gso(dev, skb)) {
>>> +		if (need_gso) {
>>>  			if (unlikely(dev_gso_segment(skb)))
>>>  				goto out_kfree_skb;
>>>  			if (skb->next)
>>>       
>
> That whole if block should be moved into an else clause here:
>
> 		if (netif_needs_gso(dev, skb)) {
> 			if (unlikely(dev_gso_segment(skb)))
> 				goto out_kfree_skb;
> 			if (skb->next)
> 				goto gso;
> 		} else {
> 			do your thing
> 		}
>
> The reason is that the other paths only act on the fragments
> generated by GSO, so logically with your change we shouldn't
> apply any further software emulation to them, even if the device
> setting changed.
>
> Cheers,
>   
Herbert,

It looks like dev_gso_segment() could be used to "Verify header 
integrity only" according to the comment?  If this is true I think the 
logic should probably be

		if (netif_needs_gso(dev, skb)) {
			if (unlikely(dev_gso_segment(skb)))
				goto out_kfree_skb;
			if (skb->next)
				goto gso;
		} 
		do your thing

		

That way we linearize the skb if necessary in the case were 
dev_gso_segment() only verifies the header and does not return a list of 
segments.

thanks,
John.