Patchwork [net-next,v2] net: adjust skb_gso_segment() for calling in rx path

login
register
mail settings
Submitter Amerigo Wang
Date Feb. 6, 2013, 2:36 a.m.
Message ID <1360118198-25440-1-git-send-email-amwang@redhat.com>
Download mbox | patch
Permalink /patch/218479/
State Accepted
Delegated to: David Miller
Headers show

Comments

Amerigo Wang - Feb. 6, 2013, 2:36 a.m.
From: Cong Wang <amwang@redhat.com>

skb_gso_segment() is almost always called in tx path,
except for openvswitch. It calls this function when
it receives the packet and tries to queue it to user-space.
In this special case, the ->ip_summed check inside
skb_gso_segment() is no longer true, as ->ip_summed value
has different meanings on rx path.

This patch adjusts skb_gso_segment() so that we can at least
avoid such warnings on checksum.

Cc: Jesse Gross <jesse@nicira.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 include/linux/netdevice.h  |   11 +++++++++--
 net/core/dev.c             |   21 ++++++++++++++++-----
 net/openvswitch/datapath.c |    2 +-
 3 files changed, 26 insertions(+), 8 deletions(-)
David Laight - Feb. 6, 2013, 9:23 a.m.
> +/* openvswitch calls this on rx path, so we need a different check.
> + */
> +static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)
> +{
> +	if (tx_path)
> +		return skb->ip_summed != CHECKSUM_PARTIAL;
> +	else
> +		return skb->ip_summed == CHECKSUM_NONE;
> +}
> +

That code wants a lot of unlikely() added.

It seems wrong to be adding code a very common path for one
obscure caller.

Perhaps the caller should be modifying the ip_summed field
(etc) to match the values expected for a tx skb.

	David


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang - Feb. 6, 2013, 9:30 a.m.
On Wed, 2013-02-06 at 09:23 +0000, David Laight wrote:
> > +/* openvswitch calls this on rx path, so we need a different check.
> > + */
> > +static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)
> > +{
> > +	if (tx_path)
> > +		return skb->ip_summed != CHECKSUM_PARTIAL;
> > +	else
> > +		return skb->ip_summed == CHECKSUM_NONE;
> > +}
> > +
> 
> That code wants a lot of unlikely() added.
> 
> It seems wrong to be adding code a very common path for one
> obscure caller.

Its caller does:

+       if (unlikely(skb_needs_check(skb, tx_path))) {

> 
> Perhaps the caller should be modifying the ip_summed field
> (etc) to match the values expected for a tx skb.
> 

This is even uglier, you need to restore ip_summed after calling
skb_gso_segment().


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Feb. 6, 2013, 9:48 a.m.
> On Wed, 2013-02-06 at 09:23 +0000, David Laight wrote:

> > > +/* openvswitch calls this on rx path, so we need a different check.

> > > + */

> > > +static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)

> > > +{

> > > +	if (tx_path)

> > > +		return skb->ip_summed != CHECKSUM_PARTIAL;

> > > +	else

> > > +		return skb->ip_summed == CHECKSUM_NONE;

> > > +}

> > > +

> >

> > That code wants a lot of unlikely() added.

> >

> > It seems wrong to be adding code a very common path for one

> > obscure caller.

> 

> Its caller does:

> 

> +       if (unlikely(skb_needs_check(skb, tx_path))) {


In my experience that isn't enough to get all the sub-tests
of complex conditionals compiled in the desired way.

	David
David Miller - Feb. 6, 2013, 8:58 p.m.
From: Cong Wang <amwang@redhat.com>
Date: Wed,  6 Feb 2013 10:36:38 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> skb_gso_segment() is almost always called in tx path,
> except for openvswitch. It calls this function when
> it receives the packet and tries to queue it to user-space.
> In this special case, the ->ip_summed check inside
> skb_gso_segment() is no longer true, as ->ip_summed value
> has different meanings on rx path.
> 
> This patch adjusts skb_gso_segment() so that we can at least
> avoid such warnings on checksum.
> 
> Cc: Jesse Gross <jesse@nicira.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

This looks perfect, applied, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 85b0949..ab2774e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2662,8 +2662,15 @@  extern int netdev_master_upper_dev_link(struct net_device *dev,
 extern void netdev_upper_dev_unlink(struct net_device *dev,
 				    struct net_device *upper_dev);
 extern int skb_checksum_help(struct sk_buff *skb);
-extern struct sk_buff *skb_gso_segment(struct sk_buff *skb,
-	netdev_features_t features);
+extern struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
+	netdev_features_t features, bool tx_path);
+
+static inline
+struct sk_buff *skb_gso_segment(struct sk_buff *skb, netdev_features_t features)
+{
+	return __skb_gso_segment(skb, features, true);
+}
+
 #ifdef CONFIG_BUG
 extern void netdev_rx_csum_fault(struct net_device *dev);
 #else
diff --git a/net/core/dev.c b/net/core/dev.c
index a87bc74..ac9a97a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2302,18 +2302,29 @@  out:
 }
 EXPORT_SYMBOL(skb_checksum_help);
 
+/* openvswitch calls this on rx path, so we need a different check.
+ */
+static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)
+{
+	if (tx_path)
+		return skb->ip_summed != CHECKSUM_PARTIAL;
+	else
+		return skb->ip_summed == CHECKSUM_NONE;
+}
+
 /**
- *	skb_gso_segment - Perform segmentation on skb.
+ *	__skb_gso_segment - Perform segmentation on skb.
  *	@skb: buffer to segment
  *	@features: features for the output path (see dev->features)
+ *	@tx_path: whether it is called in TX path
  *
  *	This function segments the given skb and returns a list of segments.
  *
  *	It may return NULL if the skb requires no segmentation.  This is
  *	only possible when GSO is used for verifying header integrity.
  */
-struct sk_buff *skb_gso_segment(struct sk_buff *skb,
-	netdev_features_t features)
+struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
+				  netdev_features_t features, bool tx_path)
 {
 	struct sk_buff *segs = ERR_PTR(-EPROTONOSUPPORT);
 	struct packet_offload *ptype;
@@ -2336,7 +2347,7 @@  struct sk_buff *skb_gso_segment(struct sk_buff *skb,
 	skb->mac_len = skb->network_header - skb->mac_header;
 	__skb_pull(skb, skb->mac_len);
 
-	if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
+	if (unlikely(skb_needs_check(skb, tx_path))) {
 		skb_warn_bad_offload(skb);
 
 		if (skb_header_cloned(skb) &&
@@ -2365,7 +2376,7 @@  struct sk_buff *skb_gso_segment(struct sk_buff *skb,
 
 	return segs;
 }
-EXPORT_SYMBOL(skb_gso_segment);
+EXPORT_SYMBOL(__skb_gso_segment);
 
 /* Take action when hardware reception checksum errors are detected. */
 #ifdef CONFIG_BUG
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index d8c13a9..9dc537d 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -301,7 +301,7 @@  static int queue_gso_packets(struct net *net, int dp_ifindex,
 	struct sk_buff *segs, *nskb;
 	int err;
 
-	segs = skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM);
+	segs = __skb_gso_segment(skb, NETIF_F_SG | NETIF_F_HW_CSUM, false);
 	if (IS_ERR(segs))
 		return PTR_ERR(segs);