[ovs-dev] [PATCH net-next V17 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes

Pravin Shelar pshelar at nicira.com
Tue Oct 20 20:34:46 UTC 2015


On Tue, Oct 20, 2015 at 7:26 AM, Thomas F Herbert
<thomasfherbert at gmail.com> wrote:
> On 10/19/15 2:28 PM, Pravin Shelar wrote:
>>
>> On Sat, Oct 17, 2015 at 6:12 PM, Thomas F Herbert
>> <thomasfherbert at gmail.com> wrote:
>>>
>>> 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 <thomasfherbert at gmail.com>
>>> ---
>>>   net/openvswitch/actions.c      |   6 +-
>>>   net/openvswitch/flow.c         |  76 +++++++++++++-----
>>>   net/openvswitch/flow.h         |   8 +-
>>>   net/openvswitch/flow_netlink.c | 172
>>> +++++++++++++++++++++++++++++++++++++----
>>>   net/openvswitch/vport-netdev.c |   4 +-
>>>   5 files changed, 227 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index 315f533..09cc1c9 100644
>>
>> ...
>>
>>> diff --git a/net/openvswitch/flow_netlink.c
>>> b/net/openvswitch/flow_netlink.c
>>> index c92d6a2..97a6d12 100644
>>> --- a/net/openvswitch/flow_netlink.c
>>> +++ b/net/openvswitch/flow_netlink.c
>>
>> ...
>>
>>> @@ -1320,6 +1443,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 +1492,29 @@ 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)) {
>>> -               __be16 eth_type;
>>> -               eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff);
>>> -               if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type)
>>> ||
>>> -                   nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
>>> output->eth.tci))
>>> +       if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) {
>>> +               if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>>> +                                output->eth.vlan.tpid) ||
>>> +                   nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
>>> output->eth.vlan.tci))
>>>                          goto nla_put_failure;
>>>                  encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
>>> -               if (!swkey->eth.tci)
>>> +               if (!swkey->eth.vlan.tci)
>>>                          goto unencap;
>>> -       } else
>>> +               if (swkey->eth.cvlan.tci) {
>>> +                       /* Customer tci is nested but uses same key
>>> attribute.
>>> +                        */
>>> +                       if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
>>> +                                        output->eth.cvlan.tpid) ||
>>> +                           nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
>>> +                                        output->eth.cvlan.tci))
>>> +                               goto nla_put_failure;
>>> +                       in_encap = nla_nest_start(skb,
>>> OVS_KEY_ATTR_ENCAP);
>>> +                       if (!swkey->eth.cvlan.tci)
>>> +                               goto unencap;
>>> +               }
>>> +       } else {
>>>                  encap = NULL;
>>> +       }
>>
>> After the vlan parsing changes, we need to encode cvlan in outer
>> netlink attribute and then encode regular vlan.
>
> I don't understand this. cvlan is inner vlan, why would it be encoded in
> outer vlan without the 2nd layer of encapsulation?
>
Lets start with double tagged packet.
packet: eth, vlan 10, inner vlan 20, ip.
flow key would be:
flow_key: vlan = 10, cvlan = 20
That would get serialize over netlink like:
eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x8100),vlan(vid=20,pcp=0),
encap(eth_type(0x0800),ipv4(frag=no))...
So far looks good.

Now userspace sends back same key and installs flow in kernel
datapath. But ovs_nla_get_match() parses netink key and sets vlan in
reverse order. After parsing netlink in ovs_nla_get_match() vlans
flow-key would look like:
flow_key: vlan = 20, cvlan = 10. This is not what we started with.

Now I think rather than fixing __ovs_nla_put_key (), we need to fix
ovs_nla_get_match() to keep vlan order.
I also noticed that eth.vlan.tpid is never initialized from netlink attributes.
Call to parse_flow_nlattrs  in parse_vlan_from_nlattrs() has no effect
so it should be removed.


> One think I see I should have done is check for eth_type_vlan() on the inner
> tag as well.
>>
>> Currently we are
>> reversing netlink encoding while sending back the vlan attributes.
>> cvlan and vlan is independent,
>
> Are you talking about a corner case where the incoming packet has an inner
> vlan but no outer vlN?
>>
nope, this is usual case.

>> therefore we need to check
>> swkey->eth.cvlan.tc outside of swkey->eth.vlan.tci block. Also
>> redundant check for swkey->eth.cvlan.tci should be removed
>
> I must be missing something because I don't understand this either. First my
> patch encodes the outer vlan and then sets the encap bit and then encodes
> the inner vlan. The encoding shows up correctly in user space.
>>
Once ovs_nla_get_match() is fixed, there is no need to change this
function to fix that ordering.

I was talking about following redundant check:

                if (swkey->eth.cvlan.tci) {
                        /* Customer tci is nested but uses same key attribute.
                         */
                        if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
                                         output->eth.cvlan.tpid) ||
                            nla_put_be16(skb, OVS_KEY_ATTR_VLAN,
                                         output->eth.cvlan.tci))
                                goto nla_put_failure;
                        in_encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
>>>>>>>>               if (!swkey->eth.cvlan.tci)  <<<<<<<<<<<<<
                                goto unencap;
                }

>>
>> Can you also post changes for userspace vswitchd so that I can try next
>> patch.
>
> I posted a patch for user space (V14) to ovs dev on October 2nd. I haven't
> made any changes since then.
> http://openvswitch.org/pipermail/dev/2015-October/060874.html
>
That patch does not apply on OVS master. You could also send me link
to ovs userpsace github repo with these patches applied so that I can
try kernel datapath changes.



More information about the dev mailing list