Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert <thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 5/14/15 3:33 AM, Pravin Shelar wrote:
> On Tue, May 12, 2015 at 5:06 PM, Thomas F Herbert
> <thomasfherbert@gmail.com> wrote:
>> Add support for 802.1ad including the ability to push and pop double
>> tagged vlans.
>>
>> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
...
>> +               if (is_mask)
>> +                       SW_FLOW_KEY_PUT(match, eth.ctci, htons(0xffff),
>> +                                       is_mask);
>> +               else
>
> 8021AD mask from user parameters is ignored and 0xffff is set. You
> need to set default 0xffff mask for ctci and then override it with
> user mask if given in the key.
Pravin, once again, thanks for your review.

I am thinking you are correct. I did it this way because it is the way 
single tagged vlans are handled in the original vlan code. Which raises 
two issues. 1. When I change this, I should also change the tci code for 
consistency and should that be a separate patch? 2. The implication is 
that so far all vlan vid mask matching has been done in user space only.
>
>> +                       SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
>> +       }
...
>> +{
>> +       return _ovs_vlan_from_nlattrs(match, attrs, a, true, log);
>> +}
>> +
> I do not see value in these functions. Can you directly call
> _ovs_vlan_from_nlattrs().
OK
>
>>   static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>>                                  const struct nlattr **a, bool is_mask,
>>                                  bool log)
>> @@ -1024,6 +1069,113 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
>>          nlattr_set(attr, val, ovs_key_lens);
>>   }
>>
>> +static int _parse_vlan_from_nlattrs(const struct nlattr *nla,
>> +                                   struct sw_flow_match *match,
>> +                                   u64 *key_attrs,
>> +                                   const struct nlattr **a, bool is_mask,
>> +                                   bool log)
>> +{
>> +       int err;
>> +       __be16 tci;
>> +
>> +       if (!is_mask) {
>> +               u64 v_attrs = 0;
>> +
>> +               tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>> +
>> +               if (tci & htons(VLAN_TAG_PRESENT)) {
>> +                       if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
>> +                                     htons(ETH_P_8021AD)))) {
>> +                               err = parse_flow_nlattrs(nla, a, &v_attrs, log);
>> +                               if (err)
>> +                                       return err;
>> +                               if (v_attrs) {
>> +                                       err = ovs_vlan_from_nlattrs(match,
>> +                                                                   v_attrs, a,
>> +                                                                   log);
>> +                                       if (err)
>> +                                               return err;
>> +                               }
>> +                               /* Insure that tci key attribute isn't
>> +                                * overwritten by encapsulated customer tci.
>> +                                */
>> +                               v_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
>
> We also need to clear v_attrs when key has single vlan tag which is
> else part of this block.
This code is implemented this way because I have been using only a 
single encapsulation level for double tagged vlans. I sneak the inner 
tag into the encap and and then clear it here because the flow key has 
only one attribute type for vlan and v_attrs is only used inside the 
encapsulation.
Along with this change, I can also update 
Documentation/networking/openvswitch.txt to show the double nested 
encapsulation flow key.
>
>> +                               *key_attrs |= v_attrs;
>> +                       } else {
>> +                               err = parse_flow_nlattrs(nla, a, key_attrs,
>> +                                                        log);
>> +                               if (err)
>> +                                       return err;
>> +                       }
>> +               } else if (!tci) {
>> +                       /* Corner case for truncated 802.1Q header. */
>> +                       if (nla_len(nla)) {
>> +                               OVS_NLERR(log, "Truncated 802.1Q header has non-zero encap attribute.");
>> +                               return -EINVAL;
>> +                       }
>> +               } else {
>> +                       OVS_NLERR(log, "Encap attr is set for non-VLAN frame");
>> +                       return  -EINVAL;
>> +               }
>> +
> For double vlan tag case we need to have double encap attributes in
> flow key; one for each tag. So flow key should look like:
>
> eth_type(0x88A8),vlan(vid=10),encap(eth_type(0x08100), vlan(vid=20),
> encap(eth_type(0x0800), ...))
>
> Can you adjust vlan parsing code according ?
Yes, I think you are right. I should change the code to use two levels 
of encapsulation for double tagged vlans. This also would be the best 
for consistency with future implementation of 802.1ah.
>
>
...
>> +{
>> +       return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, false, log);
>> +}
> You can move the key parsing block from _parse_vlan_from_nlattrs()
> here and move mask block in function bellow and get ride of
> _parse_vlan_from_nlattrs() function.
OK. I think this will make the code more straight forward.
>
>> +
>> +static int parse_vlan_mask_from_nlattrs(const struct nlattr *nla,
>> +                                       struct sw_flow_match *match,
>> +                                       u64 *key_attrs,
>> +                                       const struct nlattr **a,
>> +                                       bool log)
>> +{
>> +       return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, true, log);
>> +}
>> +
>>   /**


-- 
Thomas F. Herbert
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev