openvswitch: make vlan handling consistent
Subsystems (3)
v3 cover openvswitch: make vlan handling consistent
Always keep the first vlan tag "accelerated", i.e. in skb->vlan_tci.
Unfortunately, with all the changes since v2, this patchset no longer has the nice deletions > insertions diffstat. I still think it's worth it, as it makes things more consistent overall.
Patch 3 is valid on its own. Patch 6 is needed in one form or other; with the changes in this set, it's a simple deletion. Otherwise we'd need more elaborate checks.
Jiri Benc (6):
openvswitch: make skb modifiable in ovs_flow_key_extract*
openvswitch: normalize vlan rx path
openvswitch: add NETIF_F_HW_VLAN_STAG_TX to internal dev
openvswitch: keep vlan tag accelerated on internal device
openvswitch: remove unreachable code in vlan parsing
openvswitch: fix vlan subtraction from packet length
net/openvswitch/datapath.c | 9 ++-- net/openvswitch/flow.c | 83 ++++++++++++++++++++++++------------ net/openvswitch/flow.h | 14 +++--- net/openvswitch/vport-internal_dev.c | 9 +++- net/openvswitch/vport.c | 18 +++----- 5 files changed, 83 insertions(+), 50 deletions(-)
Signature · 1 line
1.8.3.1
PATCH v3 1/6 openvswitch: make skb modifiable in ovs_flow_key_extract*
Allow ovs_flow_key_extract and ovs_flow_key_extract_userspace to modify the skb. This will be used by the following patch to move vlan tag to the vlan_tci field.
---
v3: new in v3
--- net/openvswitch/datapath.c | 9 ++++++--- net/openvswitch/flow.c | 33 +++++++++++++++++++++++++-------- net/openvswitch/flow.h | 14 ++++++++------ net/openvswitch/vport.c | 8 +++----- 4 files changed, 42 insertions(+), 22 deletions(-)
net/openvswitch/datapath.c +6 -3
@@ -607,10 +607,13 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)if (IS_ERR (flow)) goto err_kfree_skb; - err =ovs_flow_key_extract_userspace (net, a[OVS_PACKET_ATTR_KEY ], - packet, &flow->key, log); -if (err) + packet =ovs_flow_key_extract_userspace (net, a[OVS_PACKET_ATTR_KEY ], + packet, &flow->key, log); +if (IS_ERR (packet)) { + err =PTR_ERR (packet); + packet =NULL ; goto err_flow_free; + } err =ovs_nla_copy_actions (net, a[OVS_PACKET_ATTR_ACTIONS ], &flow->key, &acts, log);
net/openvswitch/flow.c +25 -8
@@ -721,9 +721,12 @@ int ovs_flow_key_update(struct sk_buff *skb, struct sw_flow_key *key)return key_extract (skb, key); } -int ovs_flow_key_extract (const struct ip_tunnel_info *tun_info, -struct sk_buff *skb,struct sw_flow_key *key) +struct sk_buff *ovs_flow_key_extract (const struct ip_tunnel_info *tun_info, +struct sk_buff *skb, +struct sw_flow_key *key) { +int err; +/* Extract metadata from packet. */ if (tun_info) { key->tun_proto =ip_tunnel_info_af (tun_info);
@@ -753,19 +756,33 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info, key->ovs_flow_hash =0 ; key->recirc_id =0 ; -return key_extract (skb, key); + err =key_extract (skb, key); +if (err) { +kfree_skb (skb); +return ERR_PTR (err); + } +return skb; } -int ovs_flow_key_extract_userspace (struct net *net,const struct nlattr *attr, -struct sk_buff *skb, -struct sw_flow_key *key,bool log) +struct sk_buff *ovs_flow_key_extract_userspace (struct net *net, +const struct nlattr *attr, +struct sk_buff *skb, +struct sw_flow_key *key, +bool log) {int err;/* Extract metadata from netlink attributes. */ err =ovs_nla_get_flow_metadata (net, attr, key, log);if (err) -return err; + goto err_free; -return key_extract (skb, key); + err =key_extract (skb, key); +if (err) + goto err_free; +return skb; + +err_free: +kfree_skb (skb); +return ERR_PTR (err); }
net/openvswitch/flow.h +8 -6
@@ -224,12 +224,14 @@ void ovs_flow_stats_clear(struct sw_flow *);u64 ovs_flow_used_time (unsigned long flow_jiffies);int ovs_flow_key_update (struct sk_buff *skb,struct sw_flow_key *key); -int ovs_flow_key_extract (const struct ip_tunnel_info *tun_info, -struct sk_buff *skb, -struct sw_flow_key *key); +struct sk_buff *ovs_flow_key_extract (const struct ip_tunnel_info *tun_info, +struct sk_buff *skb, +struct sw_flow_key *key);/* Extract key from packet coming from userspace. */ -int ovs_flow_key_extract_userspace (struct net *net,const struct nlattr *attr, -struct sk_buff *skb, -struct sw_flow_key *key,bool log); +struct sk_buff *ovs_flow_key_extract_userspace (struct net *net, +const struct nlattr *attr, +struct sk_buff *skb, +struct sw_flow_key *key, +bool log);#endif /* flow.h */
net/openvswitch/vport.c +3 -5
@@ -455,11 +455,9 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb, }/* Extract flow from 'skb' into 'key'. */ - error =ovs_flow_key_extract (tun_info, skb, &key); -if (unlikely (error)) { -kfree_skb (skb); -return error; - } + skb =ovs_flow_key_extract (tun_info, skb, &key); +if (IS_ERR (skb)) +return PTR_ERR (skb);ovs_dp_process_packet (skb, &key);return 0 ; }
Signature · 1 line
1.8.3.1
PATCH v3 2/6 openvswitch: normalize vlan rx path
In reply to Jiri BencSimilarly to how the core networking stack behaves, let the first vlan tag be always stored in skb->vlan_tci. This is already ensured in __netif_receive_skb_core for packets that were received from the kernel and honored by skb_vlan_push and skb_vlan_pop. There is a couple of paths where a packet with vlan header inside the packet data can be received:
(1) Packets received from the user space.
(2) Packets received via internal device, either injected through AF_PACKET or with vlan acceleration turned off.
In addition, there will be a third path when support for ETH_P_TEB packets is added.
To catch all of these cases, untag the vlan frame in ovs_flow_key_extract and ovs_flow_key_extract_userspace.
---
v3: moved the untagging to ovs_flow_key_extract* to catch all cases
--- net/openvswitch/flow.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
net/openvswitch/flow.c +22 -0
@@ -716,6 +716,20 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)return 0 ; } +static struct sk_buff *vlan_untag (struct sk_buff *skb) +{ +if (eth_type_vlan (skb->protocol)) { +__skb_pull (skb,ETH_HLEN ); +skb_reset_network_header (skb); +skb_reset_mac_len (skb); + skb =skb_vlan_untag (skb); +if (unlikely (!skb)) +return NULL ; +skb_push (skb,ETH_HLEN ); + } +return skb; +} +int ovs_flow_key_update (struct sk_buff *skb,struct sw_flow_key *key) {return key_extract (skb, key);
@@ -727,6 +741,10 @@ struct sk_buff *ovs_flow_key_extract(const struct ip_tunnel_info *tun_info, {int err; + skb =vlan_untag (skb); +if (!skb) +return ERR_PTR (-ENOMEM ); +/* Extract metadata from packet. */ if (tun_info) { key->tun_proto =ip_tunnel_info_af (tun_info);
@@ -772,6 +790,10 @@ struct sk_buff *ovs_flow_key_extract_userspace(struct net *net, {int err; + skb =vlan_untag (skb); +if (!skb) +return ERR_PTR (-ENOMEM ); +/* Extract metadata from netlink attributes. */ err =ovs_nla_get_flow_metadata (net, attr, key, log);if (err)
Signature · 1 line
1.8.3.1
PATCH v3 3/6 openvswitch: add NETIF_F_HW_VLAN_STAG_TX to internal dev
In reply to Jiri BencThe internal device does support 802.1AD offloading.
---
v3: new in v3
--- net/openvswitch/vport-internal_dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/openvswitch/vport-internal_dev.c +1 -1
@@ -176,7 +176,7 @@ static void do_setup(struct net_device *netdev) netdev->vlan_features = netdev->features; netdev->hw_enc_features = netdev->features; - netdev->features |=NETIF_F_HW_VLAN_CTAG_TX ; + netdev->features |=NETIF_F_HW_VLAN_CTAG_TX |NETIF_F_HW_VLAN_STAG_TX ; netdev->hw_features = netdev->features & ~NETIF_F_LLTX ;eth_hw_addr_random (netdev);
Signature · 1 line
1.8.3.1
PATCH v3 4/6 openvswitch: keep vlan tag accelerated on internal device
In reply to Jiri BencDisallow turning off of vlan acceleration on internal ports. We need the vlan tag to be in skb->vlan_tci; otherwise, we would pull it back in ovs_flow_key_extract, defeating the purpose of setting the vlan acceleration off in the first place.
---
v3: new in v3
--- net/openvswitch/vport-internal_dev.c | 7 +++++++ 1 file changed, 7 insertions(+)
net/openvswitch/vport-internal_dev.c +7 -0
@@ -143,6 +143,12 @@ static void internal_set_rx_headroom(struct net_device *dev, int new_hr) dev->needed_headroom = new_hr <0 ?0 : new_hr; } +static netdev_features_t internal_fix_features (struct net_device *dev, +netdev_features_t features) +{ +return features |NETIF_F_HW_VLAN_CTAG_TX |NETIF_F_HW_VLAN_STAG_TX ; +} +static const struct net_device_ops internal_dev_netdev_ops = { .ndo_open = internal_dev_open, .ndo_stop = internal_dev_stop,
@@ -151,6 +157,7 @@ static const struct net_device_ops internal_dev_netdev_ops = { .ndo_change_mtu = internal_dev_change_mtu, .ndo_get_stats64 = internal_get_stats, .ndo_set_rx_headroom = internal_set_rx_headroom, + .ndo_fix_features = internal_fix_features, };static struct rtnl_link_ops internal_dev_link_ops__read_mostly = {
Signature · 1 line
1.8.3.1
PATCH v3 5/6 openvswitch: remove unreachable code in vlan parsing
In reply to Jiri BencNow when the first vlan tag is always in skb->vlan_tci, drop code that assumed it might not be the case.
This patch also removes the wrong likely() statement around skb_vlan_tag_present introduced by 018c1dda5ff1 ("openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes"). This code is called whenever flow key is being extracted from the packet, the packet may be as likely vlan tagged as not.
---
v3: unchanged
--- net/openvswitch/flow.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
net/openvswitch/flow.c +8 -20
@@ -308,9 +308,7 @@ static bool icmp6hdr_ok(struct sk_buff *skb)/** * Parse vlan tag from vlan header. -* Returns ERROR on memory error. -* Returns 0 if it encounters a non-vlan or incomplete packet. -* Returns 1 after successfully parsing vlan tag. +* Returns ERROR on memory error, 0 otherwise. */ static int parse_vlan_tag (struct sk_buff *skb,struct vlan_head *key_vh) {
@@ -331,34 +329,24 @@ static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh) key_vh->tpid = vh->tpid;__skb_pull (skb,sizeof (struct vlan_head )); -return 1 ; +return 0 ; }static int parse_vlan (struct sk_buff *skb,struct sw_flow_key *key) { -int res; - key->eth.vlan.tci =0 ; key->eth.vlan.tpid =0 ; key->eth.cvlan.tci =0 ; key->eth.cvlan.tpid =0 ; -if (likely (skb_vlan_tag_present (skb))) { - key->eth.vlan.tci =htons (skb->vlan_tci); - key->eth.vlan.tpid = skb->vlan_proto; - }else { -/* Parse outer vlan tag in the non-accelerated case. */ - res =parse_vlan_tag (skb, &key->eth.vlan); -if (res <=0 ) -return res; - } +if (!skb_vlan_tag_present (skb)) +return 0 ; -/* Parse inner vlan tag. */ - res =parse_vlan_tag (skb, &key->eth.cvlan); -if (res <=0 ) -return res; + key->eth.vlan.tci =htons (skb->vlan_tci); + key->eth.vlan.tpid = skb->vlan_proto; -return 0 ; +/* Parse inner vlan tag. */ +return parse_vlan_tag (skb, &key->eth.cvlan); }static __be16 parse_ethertype (struct sk_buff *skb)
Signature · 1 line
1.8.3.1
PATCH v3 6/6 openvswitch: fix vlan subtraction from packet length
In reply to Jiri BencWhen the packet has its vlan tag in skb->vlan_tci, the length of the VLAN header is not counted in skb->len. It doesn't make sense to subtract it.
In addition, to honor the comment below the code, the VLAN header length should not be subtracted if there's a vlan tag in skb->vlan_tci. This leads to the code simply subtracting the Ethernet header length.
---
v3: unchanged
--- net/openvswitch/vport.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
net/openvswitch/vport.c +3 -7
@@ -481,17 +481,13 @@ EXPORT_SYMBOL_GPL(ovs_vport_deferred_free);static unsigned int packet_length (const struct sk_buff *skb) { -unsigned int length = skb->len -ETH_HLEN ; - -if (skb_vlan_tagged (skb)) - length -=VLAN_HLEN ; -/* Don't subtract for multiple VLAN tags. Most (all?) drivers allow * (ETH_LEN + VLAN_HLEN) in addition to the mtu value, but almost none * account for 802.1ad. e.g. is_skb_forwardable(). +* Note that the first VLAN tag is always in skb->vlan_tci, thus not +* accounted for in skb->len. */ - -return length; +return skb->len -ETH_HLEN ; }void ovs_vport_send (struct vport *vport,struct sk_buff *skb)
Signature · 1 line
1.8.3.1
Quoted 6 lines · Jiri Benc
Always keep the first vlan tag "accelerated", i.e. in
skb->vlan_tci.Unfortunately, with all the changes since v2, this patchset no longer has the nice deletions > insertions diffstat. I still think it's worth it, as it makes things more consistent overall.
After looking at the changes, I am not sure about the value. These patches are making code bit complicated by processing vlan header twice rather than once in current code.
Quoted 4 lines
Patch 3 is valid on its own. Patch 6 is needed in one form or other; with the changes in this set, it's a simple deletion. Otherwise we'd need more elaborate checks.
As far as patch 6 is concerned I think we could do MTU checks similar to the rest of networking stack (for example is_skb_forwardable()). That would simplify things here.
Quoted 18 lines
Jiri Benc (6):
openvswitch: make skb modifiable in ovs_flow_key_extract*
openvswitch: normalize vlan rx path
openvswitch: addNETIF_F_HW_VLAN_STAG_TXto internal dev
openvswitch: keep vlan tag accelerated on internal device
openvswitch: remove unreachable code in vlan parsing
openvswitch: fix vlan subtraction from packet lengthnet/openvswitch/datapath.c | 9 ++-- net/openvswitch/flow.c | 83 ++++++++++++++++++++++++------------ net/openvswitch/flow.h | 14 +++--- net/openvswitch/vport-internal_dev.c | 9 +++- net/openvswitch/vport.c | 18 +++----- 5 files changed, 83 insertions(+), 50 deletions(-)--
1.8.3.1
Quoted 10 lines · Pravin Shelar
Quoted 6 lines · Jiri Benc
Always keep the first vlan tag "accelerated", i.e. in
skb->vlan_tci.Unfortunately, with all the changes since v2, this patchset no longer has the nice deletions > insertions diffstat. I still think it's worth it, as it makes things more consistent overall.
After looking at the changes, I am not sure about the value. These patches are making code bit complicated by processing vlan header twice rather than once in current code.
Yes, this is a trade-off. A bit more complexity on packet ingress, less complexity on packet processing.
My main motivation was L3 packets where the code in packet_length became more complicated than I'd like to to cover all possible cases. Normalizing the vlan tags looked as a pretty obvious improvement. But I'm not that thrilled with what it evolved into. I think it's slightly better than what we have now but I can understand how you may think opposite.
I'll rip the fixes (patches 3 and 6) off this patchset and send them separately.
Quoted 3 lines
As far as patch 6 is concerned I think we could do MTU checks similar to the rest of networking stack (for example
is_skb_forwardable()). That would simplify things here.
Fixing the current code is not that hard. The real problem is the added complexity with L3 packets. I'll look more into the possible solutions for the L3 patchset.
Jiri