[PATCH] geneve: restore vlan bits in xmit path

[PATCH] geneve: restore vlan bits in xmit path

[John W. Linville]

These seem to have been accidentally dropped in commit 371bd1061d29
("geneve: Consolidate Geneve functionality in single module.").

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/geneve.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index a917ae1cfbf3..0aaf302cc31b 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/hash.h>
 #include <net/dst_metadata.h>
 #include <net/gro_cells.h>
@@ -532,13 +533,20 @@ static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb,
 	int err;
 
 	min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len
-			+ GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr);
+			+ GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr)
+			+ (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
 	err = skb_cow_head(skb, min_headroom);
 	if (unlikely(err)) {
 		kfree_skb(skb);
 		goto free_rt;
 	}
 
+	skb = vlan_hwaccel_push_inside(skb);
+	if (unlikely(!skb)) {
+		err = -ENOMEM;
+		goto free_rt;
+	}
+
 	skb = udp_tunnel_handle_offloads(skb, csum);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
-- 
2.4.3

Re: [PATCH] geneve: restore vlan bits in xmit path

[Pravin Shelar]

On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
<linville@tuxdriver.com> wrote:
> These seem to have been accidentally dropped in commit 371bd1061d29
> ("geneve: Consolidate Geneve functionality in single module.").
>
Geneve should not export vxlan feature. So that it never sees vxlan
tagged packets. Can you turn off the vlan feature?

Re: [PATCH] geneve: restore vlan bits in xmit path

[John W. Linville]

On Thu, Sep 17, 2015 at 11:45:58AM -0700, Pravin Shelar wrote:
> On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
> <linville@tuxdriver.com> wrote:
> > These seem to have been accidentally dropped in commit 371bd1061d29
> > ("geneve: Consolidate Geneve functionality in single module.").
> >
> Geneve should not export vxlan feature. So that it never sees vxlan
> tagged packets. Can you turn off the vlan feature?

I'm not sure I understand...?  This is vlan, not vxlan.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] geneve: restore vlan bits in xmit path

[Jesse Gross]

On Thu, Sep 17, 2015 at 12:25 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Thu, Sep 17, 2015 at 11:45:58AM -0700, Pravin Shelar wrote:
>> On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
>> <linville@tuxdriver.com> wrote:
>> > These seem to have been accidentally dropped in commit 371bd1061d29
>> > ("geneve: Consolidate Geneve functionality in single module.").
>> >
>> Geneve should not export vxlan feature. So that it never sees vxlan
>> tagged packets. Can you turn off the vlan feature?
>
> I'm not sure I understand...?  This is vlan, not vxlan.

I think he just mean vlan. If you remove the line where
dev->vlan_features are set then the core stack will handle this and we
don't need to do anything special here.

Re: [PATCH] geneve: restore vlan bits in xmit path

[Pravin Shelar]

On Thu, Sep 17, 2015 at 12:48 PM, Jesse Gross <jesse@nicira.com> wrote:
> On Thu, Sep 17, 2015 at 12:25 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
>> On Thu, Sep 17, 2015 at 11:45:58AM -0700, Pravin Shelar wrote:
>>> On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
>>> <linville@tuxdriver.com> wrote:
>>> > These seem to have been accidentally dropped in commit 371bd1061d29
>>> > ("geneve: Consolidate Geneve functionality in single module.").
>>> >
>>> Geneve should not export vxlan feature. So that it never sees vxlan
>>> tagged packets. Can you turn off the vlan feature?
>>
>> I'm not sure I understand...?  This is vlan, not vxlan.
>
> I think he just mean vlan. If you remove the line where
> dev->vlan_features are set then the core stack will handle this and we
> don't need to do anything special here.

Yes, I meant vlan, sorry for confusion.

Re: [PATCH] geneve: restore vlan bits in xmit path

[John W. Linville]

On Thu, Sep 17, 2015 at 12:48:56PM -0700, Jesse Gross wrote:
> On Thu, Sep 17, 2015 at 12:25 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
> > On Thu, Sep 17, 2015 at 11:45:58AM -0700, Pravin Shelar wrote:
> >> On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
> >> <linville@tuxdriver.com> wrote:
> >> > These seem to have been accidentally dropped in commit 371bd1061d29
> >> > ("geneve: Consolidate Geneve functionality in single module.").
> >> >
> >> Geneve should not export vxlan feature. So that it never sees vxlan
> >> tagged packets. Can you turn off the vlan feature?
> >
> > I'm not sure I understand...?  This is vlan, not vxlan.
> 
> I think he just mean vlan. If you remove the line where
> dev->vlan_features are set then the core stack will handle this and we
> don't need to do anything special here.

Is that preferrable to this patch?  Tunneling vlan-tagged frames
seems weird, but I would hate to disallow it if some crazy person
wanted to do that...

I guess the other way would slightly improve performance, and this
could be added back later.  What about the VLAN-related bits in
dev->features and ->hw_features?  Should they go as well?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] geneve: restore vlan bits in xmit path

[Pravin Shelar]

On Thu, Sep 17, 2015 at 1:15 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Thu, Sep 17, 2015 at 12:48:56PM -0700, Jesse Gross wrote:
>> On Thu, Sep 17, 2015 at 12:25 PM, John W. Linville
>> <linville@tuxdriver.com> wrote:
>> > On Thu, Sep 17, 2015 at 11:45:58AM -0700, Pravin Shelar wrote:
>> >> On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
>> >> <linville@tuxdriver.com> wrote:
>> >> > These seem to have been accidentally dropped in commit 371bd1061d29
>> >> > ("geneve: Consolidate Geneve functionality in single module.").
>> >> >
>> >> Geneve should not export vxlan feature. So that it never sees vxlan
>> >> tagged packets. Can you turn off the vlan feature?
>> >
>> > I'm not sure I understand...?  This is vlan, not vxlan.
>>
>> I think he just mean vlan. If you remove the line where
>> dev->vlan_features are set then the core stack will handle this and we
>> don't need to do anything special here.
>
> Is that preferrable to this patch?  Tunneling vlan-tagged frames
> seems weird, but I would hate to disallow it if some crazy person
> wanted to do that...
>

To support vlan-offload in geneve we end up doing same as networking
stack does in case of software fallback for vlan-offload. So by not
setting the feature, we can avoid the duplicate code in geneve module.

> I guess the other way would slightly improve performance, and this
> could be added back later.  What about the VLAN-related bits in
> dev->features and ->hw_features?  Should they go as well?
>
No need to set vlan feature in any of device features.

Re: [PATCH] geneve: restore vlan bits in xmit path

[Jesse Gross]

On Thu, Sep 17, 2015 at 1:15 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Thu, Sep 17, 2015 at 12:48:56PM -0700, Jesse Gross wrote:
>> On Thu, Sep 17, 2015 at 12:25 PM, John W. Linville
>> <linville@tuxdriver.com> wrote:
>> > On Thu, Sep 17, 2015 at 11:45:58AM -0700, Pravin Shelar wrote:
>> >> On Thu, Sep 17, 2015 at 10:18 AM, John W. Linville
>> >> <linville@tuxdriver.com> wrote:
>> >> > These seem to have been accidentally dropped in commit 371bd1061d29
>> >> > ("geneve: Consolidate Geneve functionality in single module.").
>> >> >
>> >> Geneve should not export vxlan feature. So that it never sees vxlan
>> >> tagged packets. Can you turn off the vlan feature?
>> >
>> > I'm not sure I understand...?  This is vlan, not vxlan.
>>
>> I think he just mean vlan. If you remove the line where
>> dev->vlan_features are set then the core stack will handle this and we
>> don't need to do anything special here.
>
> Is that preferrable to this patch?  Tunneling vlan-tagged frames
> seems weird, but I would hate to disallow it if some crazy person
> wanted to do that...

This doesn't actually prevent sending VLAN tagged frames inside the
tunnel. It just means that the stack won't send VLAN accelerated
frames to the driver and will put them directly in the frame instead.
That's the same as what this code here is emulating so there's no
difference, just code cleanliness.

[PATCH v2] geneve: remove vlan-related feature assignment

[John W. Linville]

The code handling vlan tag insertion was dropped in commit 371bd1061d29
("geneve: Consolidate Geneve functionality in single module.").  Now we
need to drop the related vlan feature bits in the netdev structure.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
v2 -- replace earlier patch "geneve: restore vlan bits in xmit path"

 drivers/net/geneve.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 549febac0579..63fc4eb85cbe 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -748,12 +748,8 @@ static void geneve_setup(struct net_device *dev)
 	dev->features    |= NETIF_F_RXCSUM;
 	dev->features    |= NETIF_F_GSO_SOFTWARE;
 
-	dev->vlan_features = dev->features;
-	dev->features    |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
-
 	dev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	dev->hw_features |= NETIF_F_GSO_SOFTWARE;
-	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 
 	netif_keep_dst(dev);
 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
-- 
2.4.3

Re: [PATCH v2] geneve: remove vlan-related feature assignment

[Jesse Gross]

On Fri, Sep 18, 2015 at 1:20 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> The code handling vlan tag insertion was dropped in commit 371bd1061d29
> ("geneve: Consolidate Geneve functionality in single module.").  Now we
> need to drop the related vlan feature bits in the netdev structure.
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

Reviewed-by: Jesse Gross <jesse@nicira.com>

Re: [PATCH v2] geneve: remove vlan-related feature assignment

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 18 Sep 2015 16:20:32 -0400

> The code handling vlan tag insertion was dropped in commit 371bd1061d29
> ("geneve: Consolidate Geneve functionality in single module.").  Now we
> need to drop the related vlan feature bits in the netdev structure.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

Applied, thanks.