* [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path
@ 2013-02-06 2:36 Cong Wang
2013-02-06 9:23 ` David Laight
2013-02-06 20:58 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Cong Wang @ 2013-02-06 2:36 UTC (permalink / raw)
To: netdev; +Cc: Jesse Gross, David S. Miller, Cong Wang
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(-)
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);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path
2013-02-06 2:36 [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path Cong Wang
@ 2013-02-06 9:23 ` David Laight
2013-02-06 9:30 ` Cong Wang
2013-02-06 20:58 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: David Laight @ 2013-02-06 9:23 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: Jesse Gross, David S. Miller
> +/* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path
2013-02-06 9:23 ` David Laight
@ 2013-02-06 9:30 ` Cong Wang
2013-02-06 9:48 ` David Laight
0 siblings, 1 reply; 5+ messages in thread
From: Cong Wang @ 2013-02-06 9:30 UTC (permalink / raw)
To: David Laight; +Cc: netdev, Jesse Gross, David S. Miller
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().
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path
2013-02-06 9:30 ` Cong Wang
@ 2013-02-06 9:48 ` David Laight
0 siblings, 0 replies; 5+ messages in thread
From: David Laight @ 2013-02-06 9:48 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Jesse Gross, David S. Miller
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path
2013-02-06 2:36 [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path Cong Wang
2013-02-06 9:23 ` David Laight
@ 2013-02-06 20:58 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2013-02-06 20:58 UTC (permalink / raw)
To: amwang; +Cc: netdev, jesse
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-02-06 20:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-06 2:36 [PATCH net-next v2] net: adjust skb_gso_segment() for calling in rx path Cong Wang
2013-02-06 9:23 ` David Laight
2013-02-06 9:30 ` Cong Wang
2013-02-06 9:48 ` David Laight
2013-02-06 20:58 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).