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

On Fri, 07 Nov 2014 14:07:36 Vick, Matthew wrote:

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

quoted

Oh, I suppose we need to check the gso_type too. More like this?

+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;
+}

It seems like the skb_shinfo(skb)->gso_type check should be in some more
generic ndo_gso_check that drivers can default to/extend. Then, we could
end up with something like

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb))
		return false;

	return true;
}

This could even be simplified and still legible as

static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
{
	return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb));
}

What do you think of this approach?

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?

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.

Joe