Re: [PATCH 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes

On Fri, Sep 25, 2015 at 3:35 PM, Thomas F Herbert
[off-list ref] wrote:

Pravin,

Another comment and question. Please seen inline below.

Thanks,

--Tom

On 9/24/15 7:42 PM, Pravin Shelar wrote:

quoted

On Thu, Sep 24, 2015 at 10:58 AM, Thomas F Herbert
[off-list ref] wrote:

quoted

Add support for 802.1ad including the ability to push and pop double
tagged vlans. Add support for 802.1ad to netlink parsing and flow
conversion. Uses double nested encap attributes to represent double
tagged vlan. Inner TPID encoded along with ctci in nested attributes.

Signed-off-by: Thomas F Herbert <redacted>
---
  net/openvswitch/flow.c         |  83 +++++++++++++++++----
  net/openvswitch/flow.h         |   5 ++
  net/openvswitch/flow_netlink.c | 166
++++++++++++++++++++++++++++++++++++++---
  3 files changed, 230 insertions(+), 24 deletions(-)

...

quoted

quoted

@@ -1320,6 +1437,7 @@ static int __ovs_nla_put_key(const struct

sw_flow_key *swkey,
  {
         struct ovs_key_ethernet *eth_key;
         struct nlattr *nla, *encap;
+       struct nlattr *in_encap = NULL;

         if (nla_put_u32(skb, OVS_KEY_ATTR_RECIRC_ID, output->recirc_id))
                 goto nla_put_failure;

@@ -1368,17 +1486,42 @@ static int __ovs_nla_put_key(const struct

sw_flow_key *swkey,
         ether_addr_copy(eth_key->eth_src, output->eth.src);
         ether_addr_copy(eth_key->eth_dst, output->eth.dst);

-       if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021Q)) {
+       if (swkey->eth.tci || eth_type_vlan(swkey->eth.type)) {
                 __be16 eth_type;
-               eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff);
+
+               if (swkey->eth.cvlan.ctci ||
+                   eth_type_vlan(swkey->eth.cvlan.c_tpid))
+                       eth_type = !is_mask ? htons(ETH_P_8021AD) :
+                                             htons(0xffff);
+               else
+                       eth_type = !is_mask ? htons(ETH_P_8021Q) :
+                                             htons(0xffff);
+

Here we can directly dump output->eth.type to netlink. No need to
check for inner encap.

The eth.type is set to the inner encapsulated protocol not to the tpid. We
don't "know" what the outer tpid so I assume it is 802.1Q. To address this
situation, do you think I should add the outer tpid to sw_flow_key?
Also see comment above in flow.h.

With the addition of nested vlan, we need to add outer tpid. This will
simplify vlan netlink serialization too.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

On 9/29/15 6:56 PM, Pravin Shelar wrote:

On Fri, Sep 25, 2015 at 3:35 PM, Thomas F Herbert
[off-list ref] wrote:

quoted

Pravin,

Another comment and question. Please seen inline below.

Thanks,

--Tom

On 9/24/15 7:42 PM, Pravin Shelar wrote:

quoted

On Thu, Sep 24, 2015 at 10:58 AM, Thomas F Herbert
[off-list ref] wrote:

quoted

Add support for 802.1ad including the ability to push and pop double
tagged vlans. Add support for 802.1ad to netlink parsing and flow
conversion. Uses double nested encap attributes to represent double
tagged vlan. Inner TPID encoded along with ctci in nested attributes.

Signed-off-by: Thomas F Herbert <redacted>
---
   net/openvswitch/flow.c         |  83 +++++++++++++++++----
   net/openvswitch/flow.h         |   5 ++
   net/openvswitch/flow_netlink.c | 166
++++++++++++++++++++++++++++++++++++++---
   3 files changed, 230 insertions(+), 24 deletions(-)

...

quoted

quoted

quoted

@@ -1320,6 +1437,7 @@ static int __ovs_nla_put_key(const struct

sw_flow_key *swkey,
   {
          struct ovs_key_ethernet *eth_key;
          struct nlattr *nla, *encap;
+       struct nlattr *in_encap = NULL;

          if (nla_put_u32(skb, OVS_KEY_ATTR_RECIRC_ID, output->recirc_id))
                  goto nla_put_failure;

@@ -1368,17 +1486,42 @@ static int __ovs_nla_put_key(const struct

sw_flow_key *swkey,
          ether_addr_copy(eth_key->eth_src, output->eth.src);
          ether_addr_copy(eth_key->eth_dst, output->eth.dst);

-       if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021Q)) {
+       if (swkey->eth.tci || eth_type_vlan(swkey->eth.type)) {
                  __be16 eth_type;
-               eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff);
+
+               if (swkey->eth.cvlan.ctci ||
+                   eth_type_vlan(swkey->eth.cvlan.c_tpid))
+                       eth_type = !is_mask ? htons(ETH_P_8021AD) :
+                                             htons(0xffff);
+               else
+                       eth_type = !is_mask ? htons(ETH_P_8021Q) :
+                                             htons(0xffff);
+

Here we can directly dump output->eth.type to netlink. No need to
check for inner encap.

The eth.type is set to the inner encapsulated protocol not to the tpid. We
don't "know" what the outer tpid so I assume it is 802.1Q. To address this
situation, do you think I should add the outer tpid to sw_flow_key?
Also see comment above in flow.h.

With the addition of nested vlan, we need to add outer tpid. This will
simplify vlan netlink serialization too.

Yes, thanks. I agree that this is the sensible approach.