Re: [PATCH net-next v10 4/5] openvswitch: add layer 3 flow/port support

On Mon, Jun 6, 2016 at 7:46 PM, Simon Horman [off-list ref] wrote:

On Thu, Jun 02, 2016 at 03:02:18PM -0700, pravin shelar wrote:

quoted

On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman
[off-list ref] wrote:

[...]

quoted

quoted

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 15f130e4c22b..5567529904fa 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c

@@ -300,6 +300,51 @@ static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *flow_key,
        return 0;
 }

+static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
+{
+       /* Pop outermost VLAN tag to skb metadata unless a VLAN tag
+        * is already present there.
+        */
+       if ((skb->protocol == htons(ETH_P_8021Q) ||
+            skb->protocol == htons(ETH_P_8021AD)) &&
+           !skb_vlan_tag_present(skb)) {
+               int err = skb_vlan_accel(skb);
+               if (unlikely(err))
+                       return err;
+       }
+

I do not think we can keep just the vlan tag and pop ethernet header.
There are multiple issues with this.
First networking stack can not handle suck packet. second issue even
after this patch OVS can not parse this type of packet. third this
patch does not allow pop-eth action on vlan tagged packet.
There is already separate vlan related actions in OVS so lets keep it simple.

I wonder if the best solution is to simply omit handling VLAN tags
in pop_eth for now. As you mention pop_eth is not permitted on such packets.

yes, lets just drop vlan support here.

quoted

quoted

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 0bb650f4f219..1e1392c3c0ed 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c

[...]

quoted

quoted

@@ -355,6 +359,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
        [OVS_KEY_ATTR_CT_ZONE]   = { .len = sizeof(u16) },
        [OVS_KEY_ATTR_CT_MARK]   = { .len = sizeof(u32) },
        [OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_labels) },
+       [OVS_KEY_ATTR_PACKET_ETHERTYPE] = { .len = sizeof(__be16) },
 };

I do not see need for OVS_KEY_ATTR_PACKET_ETHERTYPE, we can use
existing OVS_KEY_ATTR_ETHERTYPE to serialize the flow key. If there is
no OVS_KEY_ATTR_ETHERNET attribute then its l3 packet.

The idea of OVS_KEY_ATTR_PACKET_ETHERTYPE is to allow communication of
the L2 type of the packet which is not present in an L3 packet. In terms
of GRE (non-TEB) this correlates to the Protocol Type field in the GRE
header.

How about using OVS_KEY_ATTR_ETHERTYPE to communicate the protocol type?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

On Tue, Jun 07, 2016 at 03:45:58PM -0700, pravin shelar wrote:

On Mon, Jun 6, 2016 at 7:46 PM, Simon Horman [off-list ref] wrote:

quoted

On Thu, Jun 02, 2016 at 03:02:18PM -0700, pravin shelar wrote:

quoted

On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman
[off-list ref] wrote:

[...]

quoted

quoted

quoted

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 0bb650f4f219..1e1392c3c0ed 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c

[...]

quoted

quoted

@@ -355,6 +359,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
        [OVS_KEY_ATTR_CT_ZONE]   = { .len = sizeof(u16) },
        [OVS_KEY_ATTR_CT_MARK]   = { .len = sizeof(u32) },
        [OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_labels) },
+       [OVS_KEY_ATTR_PACKET_ETHERTYPE] = { .len = sizeof(__be16) },
 };

I do not see need for OVS_KEY_ATTR_PACKET_ETHERTYPE, we can use
existing OVS_KEY_ATTR_ETHERTYPE to serialize the flow key. If there is
no OVS_KEY_ATTR_ETHERNET attribute then its l3 packet.

The idea of OVS_KEY_ATTR_PACKET_ETHERTYPE is to allow communication of
the L2 type of the packet which is not present in an L3 packet. In terms
of GRE (non-TEB) this correlates to the Protocol Type field in the GRE
header.

How about using OVS_KEY_ATTR_ETHERTYPE to communicate the protocol type?

Yes, I believe that I now have that working locally. The assumption that
I am now working with is that OVS_KEY_ATTR_ETHERTYPE provides the type
metadata for Layer 3 packets if OVS_KEY_ATTR_ETHERNET (the Mac addresses)
is absent.