Re: [PATCH net 1/2] tcp: Limit number of segments generated by GSO per skb

[Stephen Hemminger <shemminger@vyatta.com>]

On Mon, 30 Jul 2012 23:20:57 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Mon, 2012-07-30 at 14:46 -0700, David Miller wrote:
> > From: Ben Hutchings <bhutchings@solarflare.com>
> > Date: Mon, 30 Jul 2012 20:35:52 +0100
> > 
> > > On Mon, 2012-07-30 at 19:31 +0200, Eric Dumazet wrote:
> > >> Or you could introduce a new wk->sk_gso_max_segments, that your sfc
> > >> driver sets to whatever limit ?
> > > 
> > > Yes, that's another option.
> > 
> > This is how I want this handled.
> 
> How should that be applied in the GRO-forwarding case?
> 
> Ben.
> 
Why not make max_frags a property of the device?

Something like the following untested idea:
diff --git a/net/core/dev.c b/net/core/dev.c
index 0ebaea1..bfb005b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2159,14 +2159,16 @@ EXPORT_SYMBOL(netif_skb_features);
  *	   at least one of fragments is in highmem and device does not
  *	   support DMA from it.
  */
-static inline int skb_needs_linearize(struct sk_buff *skb,
-				      int features)
+static inline bool skb_needs_linearize(struct sk_buff *skb,
+				       int features, unsigned int maxfrags)
 {
-	return skb_is_nonlinear(skb) &&
-			((skb_has_frag_list(skb) &&
-				!(features & NETIF_F_FRAGLIST)) ||
-			(skb_shinfo(skb)->nr_frags &&
-				!(features & NETIF_F_SG)));
+	if (!skb_is_nonlinear(skb))
+		return false;
+
+	if (skb_has_frag_list(skb))
+		return !(features & NETIF_F_FRAGLIST);
+	else
+		return skb_shinfo(skb)->nr_frags > maxfrags;
 }
 
 int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
@@ -2206,7 +2208,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 			if (skb->next)
 				goto gso;
 		} else {
-			if (skb_needs_linearize(skb, features) &&
+			if (skb_needs_linearize(skb, features, dev->max_frags) &&
 			    __skb_linearize(skb))
 				goto out_kfree_skb;
 
@@ -5544,6 +5546,20 @@ int register_netdevice(struct net_device *dev)
 	dev->features |= NETIF_F_SOFT_FEATURES;
 	dev->wanted_features = dev->features & dev->hw_features;
 
+	if (dev->max_frags > 0) {
+		if (!(features & NETIF_F_SG)) {
+			netdev_dbg(dev,
+				   "Resetting max fragments since no NETIF_F_SG\n");
+			dev->max_frags = 0;
+		}
+	} else {
+		/* If device has not set maximum number of fragments
+		 * then assume it can take any number of them
+		 */
+		if (features & NETIF_F_SG)
+			dev->max_frags = MAX_SKB_FRAGS;
+	}
+
 	/* Turn on no cache copy if HW is doing checksum */
 	if (!(dev->flags & IFF_LOOPBACK)) {
 		dev->hw_features |= NETIF_F_NOCACHE_COPY;