Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

On Friday, November 07, 2014 11:49:38 Vick, Matthew wrote:

On 11/6/14, 9:05 PM, "Joe Stringer" [off-list ref] wrote:

quoted

Let's merge both discussions into one thread (pick here or there). We
have
this suggestion or the one which simply checks for tunnels and
inner+outer
header lengths. Do you have a preference between them?

Agreed with merging the thread--consider it merged!

Reflecting on this some more, I prefer the latter option (checking tunnels
and header lengths). I'm leaning towards pushing the header length check
into fm10k_tx_encap_offload and then making fm10k_gso_check call that with
the gso_type. So, it's really the most recent version of the patch you
proposed:

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
	    !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}


plus the header length being checked in fm10k_tx_encap_offload. The only
nit would be that I'd just return the conditional instead of having
"return true" or "return false" lines.

OK, that sounds reasonable.

The tunnel length check really should be there in fm10k_tx_encap_offload
anyway, so I'll get a patch together for that one.

Thanks.

quoted

We could introduce an "skb_is_gso_encap()" or similar for this purpose.
Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to
what
fm10k_tx_encap_offload() checks for though; it might not even make sense
to call
it if some of the other SKB_GSO_* flags are raised.

A fair point. On the other hand, we have to check the header length both
in the GSO and non-GSO cases anyway, so only having the check in
fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as
duplicative. What do you think about that approach?

Sure, I think fm10k_tx_encap_offload() is a good place for the header length 
check. Separately, my question above was regarding the idea of a helper for 
SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the fm10k 
driver is because all encap is checked in the fm10k_tx_encap_offload() function. 
Other drivers don't seem to handle different tunnels together like this though, 
so I'm inclined to stick with the below for now.
                                                                                
static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)        
{                                                                               
        return (!(skb_shinfo(skb)->gso_type &                                   
                  (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) ||                        
                fm10k_tx_encap_offload(skb));                                   
}

Cheers,
Joe