* [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
@ 2014-06-02 4:43 Simon Horman
2014-06-02 16:21 ` Thomas Graf
0 siblings, 1 reply; 9+ messages in thread
From: Simon Horman @ 2014-06-02 4:43 UTC (permalink / raw)
To: David Miller, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: dev-yBygre7rU0TnMu66kgdUjQ, YAMAMOTO Takashi
If an MPLS packet requires segmentation then use mpls_features
to determine if the software implementation should be used.
As no driver advertises MPLS GSO segmentation this will always be
the case.
I had not noticed that this was necessary before as software MPLS GSO
segmentation was already being used in my test environment. I believe that
the reason for that is the skbs in question always had fragments and the
driver I used does not advertise NETIF_F_FRAGLIST (which seems to be the
case for most drivers). Thus software segmentation was activated by
skb_gso_ok().
Thanks to Jesse Gross for prompting me to investigate this.
Acked-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Acked-by: YAMAMOTO Takashi <yamamoto-jCdQPDEk3idL9jVzuh4AOg@public.gmane.org>
Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
---
v4
* Correct typos in comment
* Added Ack from YAMAMOTO Takashi
v3
* As requested by David Miller
- Do not mark net_mpls_features as inline
- Correct alignment of parameters
v2
* Added Ack from Jesse Gross
* Removed duplicate 'Thus' from changelog
---
net/core/dev.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 0355ca5..24289a0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2540,6 +2540,42 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
}
EXPORT_SYMBOL(netif_skb_features);
+/* If MPLS offload request, verify we are testing hardware MPLS features
+ * instead of standard features for the netdev.
+ */
+#ifdef CONFIG_NET_MPLS_GSO
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
+{
+ /* There is no support for MPLS LRO. So the only way that
+ * an MPLS skb could require GSO segmentation is if it
+ * was received as a non-MPLS skb and then became an MPLS skb.
+ * This may be effected by Open vSwitch in which case the
+ * mac_len will non-zero and not equal to skb_network_offset
+ * as the former indicates the end of L2 while the latter indicates
+ * the beginning of L3 and there is a gap between them occupied
+ * by the MPLS label stack.
+ *
+ * Thus it is possible to avoid traversing any VLAN tags that are
+ * present to determine if the ethtype is MPLS. Instead the
+ * inequality of mac_len and skb_network_offset are used to
+ * determine if a packet is MPLS for the purpose of determining
+ * offload features.
+ */
+ if (skb->mac_len && skb->mac_len != skb_network_offset(skb))
+ features &= dev->mpls_features;
+ return features;
+}
+#else
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
+{
+ return features;
+}
+#endif
+
int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
struct netdev_queue *txq)
{
@@ -2576,6 +2612,8 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
if (skb->encapsulation)
features &= dev->hw_enc_features;
+ features = net_mpls_features(skb, dev, features);
+
if (netif_needs_gso(skb, features)) {
if (unlikely(dev_gso_segment(skb, features)))
goto out_kfree_skb;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
2014-06-02 4:43 [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation Simon Horman
@ 2014-06-02 16:21 ` Thomas Graf
2014-06-03 0:16 ` Simon Horman
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Graf @ 2014-06-02 16:21 UTC (permalink / raw)
To: Simon Horman; +Cc: David Miller, netdev, Jesse Gross, YAMAMOTO Takashi, dev
On 06/02/14 at 01:43pm, Simon Horman wrote:
> +#ifdef CONFIG_NET_MPLS_GSO
> +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> +{
> + /* There is no support for MPLS LRO. So the only way that
> + * an MPLS skb could require GSO segmentation is if it
> + * was received as a non-MPLS skb and then became an MPLS skb.
> + * This may be effected by Open vSwitch in which case the
> + * mac_len will non-zero and not equal to skb_network_offset
> + * as the former indicates the end of L2 while the latter indicates
> + * the beginning of L3 and there is a gap between them occupied
> + * by the MPLS label stack.
> + *
> + * Thus it is possible to avoid traversing any VLAN tags that are
> + * present to determine if the ethtype is MPLS. Instead the
> + * inequality of mac_len and skb_network_offset are used to
> + * determine if a packet is MPLS for the purpose of determining
> + * offload features.
> + */
> + if (skb->mac_len && skb->mac_len != skb_network_offset(skb))
> + features &= dev->mpls_features;
> + return features;
> +}
Could you elaborate a bit on the safety of this? What about
GRE GSO which sets mac_len to the inner network offset?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
2014-06-02 16:21 ` Thomas Graf
@ 2014-06-03 0:16 ` Simon Horman
[not found] ` <20140603001640.GA31821-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Simon Horman @ 2014-06-03 0:16 UTC (permalink / raw)
To: Thomas Graf; +Cc: David Miller, netdev, Jesse Gross, YAMAMOTO Takashi, dev
On Mon, Jun 02, 2014 at 05:21:45PM +0100, Thomas Graf wrote:
> On 06/02/14 at 01:43pm, Simon Horman wrote:
> > +#ifdef CONFIG_NET_MPLS_GSO
> > +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> > + struct net_device *dev,
> > + netdev_features_t features)
> > +{
> > + /* There is no support for MPLS LRO. So the only way that
> > + * an MPLS skb could require GSO segmentation is if it
> > + * was received as a non-MPLS skb and then became an MPLS skb.
> > + * This may be effected by Open vSwitch in which case the
> > + * mac_len will non-zero and not equal to skb_network_offset
> > + * as the former indicates the end of L2 while the latter indicates
> > + * the beginning of L3 and there is a gap between them occupied
> > + * by the MPLS label stack.
> > + *
> > + * Thus it is possible to avoid traversing any VLAN tags that are
> > + * present to determine if the ethtype is MPLS. Instead the
> > + * inequality of mac_len and skb_network_offset are used to
> > + * determine if a packet is MPLS for the purpose of determining
> > + * offload features.
> > + */
> > + if (skb->mac_len && skb->mac_len != skb_network_offset(skb))
> > + features &= dev->mpls_features;
> > + return features;
> > +}
>
> Could you elaborate a bit on the safety of this? What about
> GRE GSO which sets mac_len to the inner network offset?
Hi Thomas,
thanks for pointing that out.
It seems to me that I made an error in extending an assumption
that is true inside the (unmerged MPLS patch for) the Open vSwitch
datapath to code outside of the datapath. I had thought this
would be safe as the check should only trigger for packets
manipulated by the datapath.
I now think that its possible that the GRE GSO code could kick in: if the
datapath outputs to GRE. And even if that is not the case it seems to me
that adding an assumption in code in net/core/dev.c to the way mac_len is
set which has not been universally adopted throughout net/ is asking for
trouble.
My _untested_ alternate approach as illustrated below is to check the
ethernet type for MPLS, using skb_network_protocol to account for TEB and
VLANs.
I am slightly concerned about the performance implications of this
approach. I notice harmonize_features() already makes a call to
skb_network_protocol(). So if performance is a problem perhaps that call
could be leveraged somehow.
diff --git a/net/core/dev.c b/net/core/dev.c
index 0355ca5..736c48c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2540,6 +2540,33 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
}
EXPORT_SYMBOL(netif_skb_features);
+/* If MPLS offload request, verify we are testing hardware MPLS features
+ * instead of standard features for the netdev.
+ */
+#ifdef CONFIG_NET_MPLS_GSO
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
+{
+ __be16 type;
+ int depth;
+
+ type = skb_network_protocol(skb, &depth);
+ if (type == cpu_to_be16(ETH_P_MPLS_UC) ||
+ type == cpu_to_be16(ETH_P_MPLS_MC))
+ features &= dev->mpls_features;
+
+ return features;
+}
+#else
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
+{
+ return features;
+}
+#endif
+
int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
struct netdev_queue *txq)
{
@@ -2576,6 +2603,8 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
if (skb->encapsulation)
features &= dev->hw_enc_features;
+ features = net_mpls_features(skb, dev, features);
+
if (netif_needs_gso(skb, features)) {
if (unlikely(dev_gso_segment(skb, features)))
goto out_kfree_skb;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
[not found] ` <20140603001640.GA31821-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2014-06-03 0:45 ` Jesse Gross
2014-06-03 2:38 ` Simon Horman
0 siblings, 1 reply; 9+ messages in thread
From: Jesse Gross @ 2014-06-03 0:45 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, YAMAMOTO Takashi, dev-yBygre7rU0TnMu66kgdUjQ, David Miller
On Mon, Jun 2, 2014 at 5:16 PM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> On Mon, Jun 02, 2014 at 05:21:45PM +0100, Thomas Graf wrote:
>> On 06/02/14 at 01:43pm, Simon Horman wrote:
>> > +#ifdef CONFIG_NET_MPLS_GSO
>> > +static netdev_features_t net_mpls_features(struct sk_buff *skb,
>> > + struct net_device *dev,
>> > + netdev_features_t features)
>> > +{
>> > + /* There is no support for MPLS LRO. So the only way that
>> > + * an MPLS skb could require GSO segmentation is if it
>> > + * was received as a non-MPLS skb and then became an MPLS skb.
>> > + * This may be effected by Open vSwitch in which case the
>> > + * mac_len will non-zero and not equal to skb_network_offset
>> > + * as the former indicates the end of L2 while the latter indicates
>> > + * the beginning of L3 and there is a gap between them occupied
>> > + * by the MPLS label stack.
>> > + *
>> > + * Thus it is possible to avoid traversing any VLAN tags that are
>> > + * present to determine if the ethtype is MPLS. Instead the
>> > + * inequality of mac_len and skb_network_offset are used to
>> > + * determine if a packet is MPLS for the purpose of determining
>> > + * offload features.
>> > + */
>> > + if (skb->mac_len && skb->mac_len != skb_network_offset(skb))
>> > + features &= dev->mpls_features;
>> > + return features;
>> > +}
>>
>> Could you elaborate a bit on the safety of this? What about
>> GRE GSO which sets mac_len to the inner network offset?
>
> Hi Thomas,
>
> thanks for pointing that out.
>
> It seems to me that I made an error in extending an assumption
> that is true inside the (unmerged MPLS patch for) the Open vSwitch
> datapath to code outside of the datapath. I had thought this
> would be safe as the check should only trigger for packets
> manipulated by the datapath.
>
> I now think that its possible that the GRE GSO code could kick in: if the
> datapath outputs to GRE. And even if that is not the case it seems to me
> that adding an assumption in code in net/core/dev.c to the way mac_len is
> set which has not been universally adopted throughout net/ is asking for
> trouble.
>
> My _untested_ alternate approach as illustrated below is to check the
> ethernet type for MPLS, using skb_network_protocol to account for TEB and
> VLANs.
>
> I am slightly concerned about the performance implications of this
> approach. I notice harmonize_features() already makes a call to
> skb_network_protocol(). So if performance is a problem perhaps that call
> could be leveraged somehow.
To be honest, I think this actually really belongs as part of
netif_skb_features()/harmonize_features(). The point of those
functions is to return the offloading features that are available for
a given packet, so it's not clear why they wouldn't take MPLS into
account. If we merged them then it would both be cleaner and should
avoid any performance issues.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
2014-06-03 0:45 ` Jesse Gross
@ 2014-06-03 2:38 ` Simon Horman
[not found] ` <20140603023852.GA20728-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Simon Horman @ 2014-06-03 2:38 UTC (permalink / raw)
To: Jesse Gross; +Cc: Thomas Graf, David Miller, netdev, YAMAMOTO Takashi, dev
On Mon, Jun 02, 2014 at 05:45:22PM -0700, Jesse Gross wrote:
> On Mon, Jun 2, 2014 at 5:16 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Jun 02, 2014 at 05:21:45PM +0100, Thomas Graf wrote:
> >> On 06/02/14 at 01:43pm, Simon Horman wrote:
> >> > +#ifdef CONFIG_NET_MPLS_GSO
> >> > +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> >> > + struct net_device *dev,
> >> > + netdev_features_t features)
> >> > +{
> >> > + /* There is no support for MPLS LRO. So the only way that
> >> > + * an MPLS skb could require GSO segmentation is if it
> >> > + * was received as a non-MPLS skb and then became an MPLS skb.
> >> > + * This may be effected by Open vSwitch in which case the
> >> > + * mac_len will non-zero and not equal to skb_network_offset
> >> > + * as the former indicates the end of L2 while the latter indicates
> >> > + * the beginning of L3 and there is a gap between them occupied
> >> > + * by the MPLS label stack.
> >> > + *
> >> > + * Thus it is possible to avoid traversing any VLAN tags that are
> >> > + * present to determine if the ethtype is MPLS. Instead the
> >> > + * inequality of mac_len and skb_network_offset are used to
> >> > + * determine if a packet is MPLS for the purpose of determining
> >> > + * offload features.
> >> > + */
> >> > + if (skb->mac_len && skb->mac_len != skb_network_offset(skb))
> >> > + features &= dev->mpls_features;
> >> > + return features;
> >> > +}
> >>
> >> Could you elaborate a bit on the safety of this? What about
> >> GRE GSO which sets mac_len to the inner network offset?
> >
> > Hi Thomas,
> >
> > thanks for pointing that out.
> >
> > It seems to me that I made an error in extending an assumption
> > that is true inside the (unmerged MPLS patch for) the Open vSwitch
> > datapath to code outside of the datapath. I had thought this
> > would be safe as the check should only trigger for packets
> > manipulated by the datapath.
> >
> > I now think that its possible that the GRE GSO code could kick in: if the
> > datapath outputs to GRE. And even if that is not the case it seems to me
> > that adding an assumption in code in net/core/dev.c to the way mac_len is
> > set which has not been universally adopted throughout net/ is asking for
> > trouble.
> >
> > My _untested_ alternate approach as illustrated below is to check the
> > ethernet type for MPLS, using skb_network_protocol to account for TEB and
> > VLANs.
> >
> > I am slightly concerned about the performance implications of this
> > approach. I notice harmonize_features() already makes a call to
> > skb_network_protocol(). So if performance is a problem perhaps that call
> > could be leveraged somehow.
>
> To be honest, I think this actually really belongs as part of
> netif_skb_features()/harmonize_features(). The point of those
> functions is to return the offloading features that are available for
> a given packet, so it's not clear why they wouldn't take MPLS into
> account. If we merged them then it would both be cleaner and should
> avoid any performance issues.
I think that the reason that I didn't do this initially
was that I wanted to handle mpls_features in a similar way
to that of hw_enc_features.
In light of the feedback from you and Thomas I do agree that
it seems to make sense to handle things in
netif_skb_features()/harmonize_features().
As per your suggestion I have tested the following
revised patch.
From: Simon Horman <horms@verge.net.au>
[PATCH v4.1] MPLS: Use mpls_features to activate software MPLS GSO segmentation
If an MPLS packet requires segmentation then use mpls_features
to determine if the software implementation should be used.
As no driver advertises MPLS GSO segmentation this will always be
the case.
I had not noticed that this was necessary before as software MPLS GSO
segmentation was already being used in my test environment. I believe that
the reason for that is the skbs in question always had fragments and the
driver I used does not advertise NETIF_F_FRAGLIST (which seems to be the
case for most drivers). Thus software segmentation was activated by
skb_gso_ok().
This introduces the overhead of an extra call to skb_network_protocol()
in the case where where CONFIG_NET_MPLS_GSO is set and
skb->ip_summed == CHECKSUM_NONE.
Thanks to Jesse Gross for prompting me to investigate this.
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v4.1
* Use ethertype of packet to detect MPLS rather than
relying on mac_len indicating a gap between the end of L2
and the beginning of L3. That assumption seems to
be broken by the GRE GSO code.
* Move mpls_features handling into harmonize_features()
This allows an existing call in there to skb_network_protocol()
to be leveraged.
* Removed acks as the patch has now changed in a material way
v4
* Correct typos in comment
* Added Ack from YAMAMOTO Takashi
v3
* As requested by David Miller
- Do not mark net_mpls_features as inline
- Correct alignment of parameters
v2
* Added Ack from Jesse Gross
* Removed duplicate 'Thus' from changelog
---
net/core/dev.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 0355ca5..0fc92ee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2498,11 +2498,38 @@ static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
return 0;
}
+/* If MPLS offload request, verify we are testing hardware MPLS features
+ * instead of standard features for the netdev.
+ */
+#ifdef CONFIG_NET_MPLS_GSO
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ netdev_features_t features)
+{
+ int tmp;
+ __be16 type;
+
+ type = skb_network_protocol(skb, &tmp);
+ if (unlikely(type == cpu_to_be16(ETH_P_MPLS_UC) ||
+ type == cpu_to_be16(ETH_P_MPLS_MC)))
+ features &= skb->dev->mpls_features;
+
+ return features;
+}
+#else
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ netdev_features_t features)
+{
+ return features;
+}
+#endif
+
static netdev_features_t harmonize_features(struct sk_buff *skb,
netdev_features_t features)
{
int tmp;
+ features = net_mpls_features(skb, features);
+
if (skb->ip_summed != CHECKSUM_NONE &&
!can_checksum_protocol(features, skb_network_protocol(skb, &tmp))) {
features &= ~NETIF_F_ALL_CSUM;
--
2.0.0.rc2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
[not found] ` <20140603023852.GA20728-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2014-06-03 3:42 ` Eric Dumazet
2014-06-03 4:30 ` Simon Horman
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2014-06-03 3:42 UTC (permalink / raw)
To: Simon Horman
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev, YAMAMOTO Takashi, David Miller
Hi Simon
On Tue, 2014-06-03 at 11:38 +0900, Simon Horman wrote:
> +/* If MPLS offload request, verify we are testing hardware MPLS features
> + * instead of standard features for the netdev.
> + */
> +#ifdef CONFIG_NET_MPLS_GSO
> +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> + netdev_features_t features)
> +{
> + int tmp;
> + __be16 type;
> +
> + type = skb_network_protocol(skb, &tmp);
> + if (unlikely(type == cpu_to_be16(ETH_P_MPLS_UC) ||
> + type == cpu_to_be16(ETH_P_MPLS_MC)))
I believe net/core/dev.c prefers to use htons(ETH_P_MPLS_UC)
(check netif_skb_features())
> + features &= skb->dev->mpls_features;
> +
> + return features;
> +}
> +#else
> +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> + netdev_features_t features)
> +{
> + return features;
> +}
> +#endif
> +
> static netdev_features_t harmonize_features(struct sk_buff *skb,
> netdev_features_t features)
> {
> int tmp;
_be16 type;
type = skb_network_protocol(skb, &tmp);
>
> + features = net_mpls_features(skb, features);
features = net_mpls_features(skb, features, type);
> +
> if (skb->ip_summed != CHECKSUM_NONE &&
> !can_checksum_protocol(features, skb_network_protocol(skb, &tmp))) {
!can_checksum_protocol(features, type)) {
> features &= ~NETIF_F_ALL_CSUM;
I guess CONFIG_NET_MPLS_GSO will be set by all distros, right ?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
2014-06-03 3:42 ` Eric Dumazet
@ 2014-06-03 4:30 ` Simon Horman
2014-06-03 4:46 ` [ovs-dev] " YAMAMOTO Takashi
0 siblings, 1 reply; 9+ messages in thread
From: Simon Horman @ 2014-06-03 4:30 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jesse Gross, Thomas Graf, David Miller, netdev, YAMAMOTO Takashi, dev
Hi Eric,
On Mon, Jun 02, 2014 at 08:42:02PM -0700, Eric Dumazet wrote:
> Hi Simon
>
> On Tue, 2014-06-03 at 11:38 +0900, Simon Horman wrote:
> > +/* If MPLS offload request, verify we are testing hardware MPLS features
> > + * instead of standard features for the netdev.
> > + */
> > +#ifdef CONFIG_NET_MPLS_GSO
> > +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> > + netdev_features_t features)
> > +{
> > + int tmp;
> > + __be16 type;
> > +
> > + type = skb_network_protocol(skb, &tmp);
> > + if (unlikely(type == cpu_to_be16(ETH_P_MPLS_UC) ||
> > + type == cpu_to_be16(ETH_P_MPLS_MC)))
>
> I believe net/core/dev.c prefers to use htons(ETH_P_MPLS_UC)
Thanks, I will fix that.
> (check netif_skb_features())
>
> > + features &= skb->dev->mpls_features;
> > +
> > + return features;
> > +}
> > +#else
> > +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> > + netdev_features_t features)
> > +{
> > + return features;
> > +}
> > +#endif
> > +
> > static netdev_features_t harmonize_features(struct sk_buff *skb,
> > netdev_features_t features)
> > {
> > int tmp;
>
> _be16 type;
>
> type = skb_network_protocol(skb, &tmp);
>
> >
> > + features = net_mpls_features(skb, features);
>
> features = net_mpls_features(skb, features, type);
>
> > +
> > if (skb->ip_summed != CHECKSUM_NONE &&
> > !can_checksum_protocol(features, skb_network_protocol(skb, &tmp))) {
>
> !can_checksum_protocol(features, type)) {
>
> > features &= ~NETIF_F_ALL_CSUM;
Thanks, I'll switch things around as you suggest.
> I guess CONFIG_NET_MPLS_GSO will be set by all distros, right ?
That seems likely to me.
From: Simon Horman <horms@verge.net.au>
MPLS: Use mpls_features to activate software MPLS GSO segmentation
If an MPLS packet requires segmentation then use mpls_features
to determine if the software implementation should be used.
As no driver advertises MPLS GSO segmentation this will always be
the case.
I had not noticed that this was necessary before as software MPLS GSO
segmentation was already being used in my test environment. I believe that
the reason for that is the skbs in question always had fragments and the
driver I used does not advertise NETIF_F_FRAGLIST (which seems to be the
case for most drivers). Thus software segmentation was activated by
skb_gso_ok().
This introduces the overhead of an extra call to skb_network_protocol()
in the case where where CONFIG_NET_MPLS_GSO is set and
skb->ip_summed == CHECKSUM_NONE.
Thanks to Jesse Gross for prompting me to investigate this.
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v4.2
* As suggested by Eric Dumazet
- Use htons() instead of cpu_to_be16()
- Refactor code to pass type to net_mpls_features.
v4.1
* Following fedback from Thomas Graff and Jesse Gross
- Use ethertype of packet to detect MPLS rather than
relying on mac_len indicating a gap between the end of L2
and the beginning of L3. That assumption seems to
be broken by the GRE GSO code.
- Move mpls_features handling into harmonize_features()
This allows an existing call in there to skb_network_protocol()
to be leveraged.
- Removed acks as the patch has now changed in a material way
v4
* As suggested by YAMAMOTO Takashi
- Correct typos in comment
* Added Ack from YAMAMOTO Takashi
v3
* As requested by David Miller
- Do not mark net_mpls_features as inline
- Correct alignment of parameters
v2
* Added Ack from Jesse Gross
* Removed duplicate 'Thus' from changelog
---
net/core/dev.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 0355ca5..7c063ac 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2498,13 +2498,42 @@ static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
return 0;
}
+/* If MPLS offload request, verify we are testing hardware MPLS features
+ * instead of standard features for the netdev.
+ */
+#ifdef CONFIG_NET_MPLS_GSO
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ netdev_features_t features,
+ __be16 type)
+{
+ int tmp;
+
+ if (unlikely(type == htons(ETH_P_MPLS_UC) ||
+ type == htons(ETH_P_MPLS_MC)))
+ features &= skb->dev->mpls_features;
+
+ return features;
+}
+#else
+static netdev_features_t net_mpls_features(struct sk_buff *skb,
+ netdev_features_t features,
+ __be16 type)
+{
+ return features;
+}
+#endif
+
static netdev_features_t harmonize_features(struct sk_buff *skb,
netdev_features_t features)
{
int tmp;
+ __be16 type;
+
+ type = skb_network_protocol(skb, &tmp);
+ features = net_mpls_features(skb, features, type);
if (skb->ip_summed != CHECKSUM_NONE &&
- !can_checksum_protocol(features, skb_network_protocol(skb, &tmp))) {
+ !can_checksum_protocol(features, type)) {
features &= ~NETIF_F_ALL_CSUM;
} else if (illegal_highdma(skb->dev, skb)) {
features &= ~NETIF_F_SG;
--
2.0.0.rc2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [ovs-dev] [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
2014-06-03 4:30 ` Simon Horman
@ 2014-06-03 4:46 ` YAMAMOTO Takashi
[not found] ` <20140603044611.C2B0970BA7-0CV7wKnmZOB82hYKe6nXyg@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: YAMAMOTO Takashi @ 2014-06-03 4:46 UTC (permalink / raw)
To: horms; +Cc: eric.dumazet, dev, netdev, davem
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0355ca5..7c063ac 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2498,13 +2498,42 @@ static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
> return 0;
> }
>
> +/* If MPLS offload request, verify we are testing hardware MPLS features
> + * instead of standard features for the netdev.
> + */
> +#ifdef CONFIG_NET_MPLS_GSO
> +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> + netdev_features_t features,
> + __be16 type)
> +{
> + int tmp;
this variable seems no longer used.
> +
> + if (unlikely(type == htons(ETH_P_MPLS_UC) ||
> + type == htons(ETH_P_MPLS_MC)))
why unlikely?
otherwise,
Acked-by: YAMAMOTO Takashi <yamamoto@valinux.co.jp>
YAMAMOTO Takashi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation
[not found] ` <20140603044611.C2B0970BA7-0CV7wKnmZOB82hYKe6nXyg@public.gmane.org>
@ 2014-06-03 4:49 ` Simon Horman
0 siblings, 0 replies; 9+ messages in thread
From: Simon Horman @ 2014-06-03 4:49 UTC (permalink / raw)
To: YAMAMOTO Takashi
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
On Tue, Jun 03, 2014 at 01:46:11PM +0900, YAMAMOTO Takashi wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 0355ca5..7c063ac 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -2498,13 +2498,42 @@ static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
> > return 0;
> > }
> >
> > +/* If MPLS offload request, verify we are testing hardware MPLS features
> > + * instead of standard features for the netdev.
> > + */
> > +#ifdef CONFIG_NET_MPLS_GSO
> > +static netdev_features_t net_mpls_features(struct sk_buff *skb,
> > + netdev_features_t features,
> > + __be16 type)
> > +{
> > + int tmp;
>
> this variable seems no longer used.
Thanks, I will remove it.
> > +
> > + if (unlikely(type == htons(ETH_P_MPLS_UC) ||
> > + type == htons(ETH_P_MPLS_MC)))
>
> why unlikely?
Because packets are not likely to be MPLS (IMHO).
I'm happy to remove the unlikely() if you like.
>
> otherwise,
> Acked-by: YAMAMOTO Takashi <yamamoto-jCdQPDEk3idL9jVzuh4AOg@public.gmane.org>
>
> YAMAMOTO Takashi
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-06-03 4:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-02 4:43 [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation Simon Horman
2014-06-02 16:21 ` Thomas Graf
2014-06-03 0:16 ` Simon Horman
[not found] ` <20140603001640.GA31821-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2014-06-03 0:45 ` Jesse Gross
2014-06-03 2:38 ` Simon Horman
[not found] ` <20140603023852.GA20728-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2014-06-03 3:42 ` Eric Dumazet
2014-06-03 4:30 ` Simon Horman
2014-06-03 4:46 ` [ovs-dev] " YAMAMOTO Takashi
[not found] ` <20140603044611.C2B0970BA7-0CV7wKnmZOB82hYKe6nXyg@public.gmane.org>
2014-06-03 4:49 ` Simon Horman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.