[PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Simon Horman]

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

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Thomas Graf]

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?

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Simon Horman]

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;

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Jesse Gross]

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.

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Simon Horman]

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

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Eric Dumazet]

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 ?

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Simon Horman]

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

Re: [ovs-dev] [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[YAMAMOTO Takashi]

> 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

Re: [PATCH v4 net-next] MPLS: Use mpls_features to activate software MPLS GSO segmentation

[Simon Horman]

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
>