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

[Pravin Shelar]

On Tue, Oct 27, 2015 at 9:45 AM, Thomas F Herbert
<thomasfherbert at gmail.com> wrote:
> On 10/26/15 10:10 PM, Pravin Shelar wrote:
> Thanks for the review.
>>
>> On Sun, Oct 25, 2015 at 5:11 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.
>>> Outer
>>> TPID is also encoded in the flow key.
>>>
>>> Signed-off-by: Thomas F Herbert <thomasfherbert at gmail.com>
>>
>> This patch does not apply on current master due to conflicts related
>> net-branch merge.
>
> OK, I will rebase.
>
>>
>>> ---
>>>   net/openvswitch/actions.c      |   6 +-
>>>   net/openvswitch/flow.c         |  76 ++++++++++++----
>>>   net/openvswitch/flow.h         |   8 +-
>>>   net/openvswitch/flow_netlink.c | 199
>>> +++++++++++++++++++++++++++++++++++++----
>>>   net/openvswitch/vport-netdev.c |   4 +-
>>>   5 files changed, 252 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>>> index c8db44a..ed19e2b 100644
>>> --- a/net/openvswitch/flow.c
>>> +++ b/net/openvswitch/flow.c
>>> @@ -302,24 +302,68 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>>                                    sizeof(struct icmp6hdr));
>>>   }
>>>
>>> -static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>> +/* Parse vlan tag from vlan header.
>>> + * Returns ERROR on memory error.
>>> + * Returns 0 if it encounters a non-vlan or incomplete packet.
>>> + * Returns 1 after successfully parsing vlan tag.
>>> + */
>>> +
>>> +static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *vlan)
>>>   {
>>> -       struct qtag_prefix {
>>> -               __be16 eth_type; /* ETH_P_8021Q */
>>> -               __be16 tci;
>>> -       };
>>> -       struct qtag_prefix *qp;
>>> +       struct vlan_head *qp = (struct vlan_head *)skb->data;
>>> +
>>> +       if (likely(!eth_type_vlan(qp->tpid)))
>>> +               return 0;
>>>
>>> -       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> sizeof(__be16)))
>>> +       if (unlikely(skb->len < sizeof(struct vlan_head) +
>>> sizeof(__be16)))
>>>                  return 0;
>>
>> Why do we need extra sizeof(__be16) bytes here?
>
> I don't have an answer to your question. I didn't write this code and have
> wondered about why the extra two bytes were reserved. I don't know why it
> should be necessarily for inner or outer vlans or the HW accelerated case or
> for the non-accelerated case. If no reviewer can state a case for it, I will
> remove it with the next version of this patch.
>
Looks like it is optimization for parsing ethertype, So lets keep it.

>>>
>>>                  } else if (!tci) {
>>>                          /* Corner case for truncated 802.1Q header. */
>>>                          if (nla_len(encap)) {
>>> @@ -1169,7 +1312,7 @@ int ovs_nla_get_match(struct net *net, struct
>>> sw_flow_match *match,
>>>                          goto free_newmask;
>>>
>>>                  /* Always match on tci. */
>>> -               SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>> +               SW_FLOW_KEY_PUT(match, eth.vlan.tci, htons(0xffff),
>>> true);
>>
>> Also need to exact match on inner tci.
>
> This code sets a match on tci even if no vlan is present. Is this is for the
> case where there is no explicit mask specified in the netlink encoded flow?
> If that is correct, then it does need to be done for the inner vlan too.

Yes, By default it needs to be matched. userspace can overwrite it
with different wildcard.