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

Pravin Shelar pshelar at nicira.com
Fri Oct 23 19:26:44 UTC 2015


On Wed, Oct 21, 2015 at 7:39 AM, Thomas F Herbert <therbert at redhat.com> wrote:
>
>
> On 10/20/15 4:34 PM, Pravin Shelar wrote:
>>
>> 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.
>
> Yes, you are right. The code in master never encoded the tpid in the key and
> I didn't add it when I modified the swkey for the addition of the vlan
> struct.
>
> Yes, you are right the code is wrong. The intention was to leave the old
> code which left the outer tci in the key for later processing. In the code
> in master, after checking proper vlan the function ovs_key_from_nlattrs()
> encoded the tci.
>
> Now I see that I must refactor the code in ovs_nla_get_match() and
> parse_vlan_from_nlattrs to explicitly encode the outer vlan including tpid
> before the inner vlan.
>>
>> Call to parse_flow_nlattrs  in parse_vlan_from_nlattrs() has no effect
>> so it should be removed
>
> parse_flow_nlattrs() is called for each level of encapsulation. It sets
> v_attrs with the keys found in the flow key. This is to insure that I am
> looking only at the keys for the current level of encapsulation. Then it is
> called a final time to parse out the l3 attributes for encoding.

ovs_nla_get_match() calls parse_flow_nlattrs() and then again in
parse_vlan_from_nlattrs(), second parse_flow_nlattr() can potentially
overwrite vlan attributes. Therefore it should called after the outer
vlan attributes are read.



More information about the dev mailing list