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

[Thomas F Herbert <thomasfherbert@gmail.com>]

On 9/29/15 6:56 PM, Pravin Shelar wrote:
> On Fri, Sep 25, 2015 at 3:35 PM, Thomas F Herbert
> <thomasfherbert@gmail.com> wrote:
>> Pravin,
>>
>> Another comment and question. Please seen inline below.
>>
>> Thanks,
>>
>> --Tom
>>
>> On 9/24/15 7:42 PM, Pravin Shelar wrote:
>>> On Thu, Sep 24, 2015 at 10:58 AM, Thomas F Herbert
>>> <thomasfherbert@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@gmail.com>
>>>> ---
>>>>    net/openvswitch/flow.c         |  83 +++++++++++++++++----
>>>>    net/openvswitch/flow.h         |   5 ++
>>>>    net/openvswitch/flow_netlink.c | 166
>>>> ++++++++++++++++++++++++++++++++++++++---
>>>>    3 files changed, 230 insertions(+), 24 deletions(-)
>>>>
> ...
>
>>>> @@ -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.