Re: [PATCH net-next V10 3/4] 802.1AD: Flow handling, actions and vlan parsing

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

On 6/4/15 1:45 PM, Pravin Shelar wrote:
> On Tue, Jun 2, 2015 at 10:50 AM, 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>
>> ---
>>   net/openvswitch/flow.c | 82 ++++++++++++++++++++++++++++++++++++++++++--------
>>   net/openvswitch/flow.h |  3 ++
>>   2 files changed, 73 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 2dacc7b..9c73a2e 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -298,21 +298,78 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>   static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>   {
>>          struct qtag_prefix {
>> -               __be16 eth_type; /* ETH_P_8021Q */
>> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>>                  __be16 tci;
>>          };
>> -       struct qtag_prefix *qp;
>> +       struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
>>
>> -       if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
>> +       struct qinqtag_prefix {
>> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>> +               __be16 tci;
>> +               __be16 inner_tpid; /* ETH_P_8021Q */
>> +               __be16 ctci;
>> +       };
>> +
> ...
>
>>
>> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
>> index a076e44..fa83c61 100644
>> --- a/net/openvswitch/flow.h
>> +++ b/net/openvswitch/flow.h
>> @@ -134,6 +134,9 @@ struct sw_flow_key {
>>                  u8     src[ETH_ALEN];   /* Ethernet source address. */
>>                  u8     dst[ETH_ALEN];   /* Ethernet destination address. */
>>                  __be16 tci;             /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
>> +               __be16 ctci;            /* 0 if no CVLAN, VLAN_TAG_PRESENT set
>> +                                        * otherwise.
>> +                                        */
>>                  __be16 type;            /* Ethernet frame type. */
>>          } eth;
>>          union {
>> --
>> 2.1.0
>>
> Currently you have restricted the datapath implementation to support
> only 8021AD. We can extend this to support double tagging by adding
> inner_tpid field to struct sw_flow_key. OVS netlink interface already
> allows this type of configuration. So is there reason not to do it in
> this series?
Pravin, thanks for the review.

In the original implementation, I thought I would make only the 
minimally necessary changes and I have been carrying that forward 
through each revision.

If only one tag, tci is present that implies tpid of 0x8100, if two tags 
are present, both tci and ctci, that implies a tpid of 0x88a8 and this 
is in fact how the code works and is how it supports both 802.1q and 
802.1ad. The tpid's are not strictly necessary in the flow key for both 
802.1q and 802.1ad to work. OF doesn't support the ability to push and 
pop vlans with non valid tpid's so there's no ambiguity.

Contrarily and despite my comment above, although not strictly 
necessary, I don't have any objection to adding the tpid and to the flow 
key and I can make it work either way.

--TFH
>


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