[ovs-dev] [PATCH v3 0/4] Add support for 802.1ad (QinQ tunneling)

Thomas F Herbert thomasfherbert at gmail.com
Fri Jul 15 12:08:33 UTC 2016


Eric and Xiao, I am cc'ing this to the ovs-dev list because other 
reviewers who reviewed the kernel module may want to weigh in.

--TFH

On 7/12/16 2:17 PM, Eric Garver wrote:
> This looks okay to me. This function was definitely missing a call to
> parse_flow_mask_nlattrs() for the masked case. My only question is why
> the original code has "if (unlikely(inner))".
>
> Tom,
> Can you speak to why this was checking for the inner tag?
Eric, please see my comments below.
>
> On Tue, Jul 12, 2016 at 11:37:32PM +0800, Xiao Liang wrote:
>> Hi Eric,
>>
>> Nice to hear that we could work together on this feature, and many
>> thanks for you effort. Hope we could get it work in the upstream soon.
>>
>> By the way, for the kernel patch, I changed the last lines in
>> __parse_vlan_from_nlattrs:
>>          if (unlikely(inner)) {
>>                  err = parse_flow_nlattrs(encap, a, en_attrs, log);
>>                  if (err)
>>                          return err;
>>          }
>>          return 0;
>> to:
>>          if (is_mask)
>>                  err = parse_flow_mask_nlattrs(encap, a, en_attrs, log);
>>          else
>>                  err = parse_flow_nlattrs(encap, a, en_attrs, log);
>>
>>          return err;
>>
>> Is it reasonable?
Yes, I think you are correct. The mask case seems to be missing from my 
V20 kernel module patch. Maybe when I refactored the code to use a 
common function for inner and outer vlans, I dropped the mask case. 
However, unlikely(inner) may be OK though because the single tagged vlan 
is the more common case.

A complication is that the OF spec says that matching should be on the 
outer tag only. (See section 7.2.3.8.) I don't recall but the intent of 
this code may have been to check for a mask on the outer tag only. 
However, it makes sense to code it in such a way that both tags are 
handled symmetrically.
>>
>> Thanks,
>> Xiao
>>
>>
>> On Mon, Jul 11, 2016 at 9:45 PM, Eric Garver <egarver at redhat.com> wrote:
>>> Hi Xiao,
>>>
>>> I hope it's okay to unicast you. I wanted to let you know that I'm
>>> working with Thomas to get 802.1ad support accepted upstream (OVS and
>>> kernel).
>>>
>>> Your patchset here looks well done. I will continue testing and
>>> providing feedback.
>>>
>>> Regarding the kernel, I'm currently in progress of rebasing, testing,
>>> and fixing a couple issues. When I think it's ready I will post it to
>>> the netdev mailing list for review.
>>>
>>> Hopefully together we can get both user space and kernel support
>>> upstream in the very near future.
>>>
>>> Take care.
>>> Eric.
>>>
>>> On Sun, Jul 03, 2016 at 08:47:05AM +0800, Xiao Liang wrote:
>>>> These patches enable tpid 0x88a8, multiple VLAN headers, and add new vlan_mode "dot1q-tunnel". See commit message of patch1 for details.
>>>>
>>>> v2: Add VLAN handling test cases for dot1q-tunnel vlan_mode.
>>>>      Adjust VLAN+MPLS test cases for multi VLAN.
>>>> v3: Set default max_vlan_headers of netdev and test-odp (fix test failure found by Tom).
>>>>      Fix loop condition when parsing mask in odp-util.c.
>>>>      Clean up comments.
>>>>
>>>>
>>>> Xiao Liang (4):
>>>>    Add support for 802.1ad (QinQ tunneling)
>>>>    tests: Add tests for QinQ VLAN handling
>>>>    tests: Adjust VLAN+MPLS handling cases for QinQ
>>>>    tests: Set default max_vlan_headers
>>>>
>>>>   include/openvswitch/flow.h        |  13 +-
>>>>   include/openvswitch/ofp-actions.h |  10 +-
>>>>   include/openvswitch/packets.h     |   5 +
>>>>   lib/dpctl.c                       |  29 ++-
>>>>   lib/dpif-netdev.c                 |   8 +-
>>>>   lib/flow.c                        | 109 ++++++----
>>>>   lib/flow.h                        |   6 +-
>>>>   lib/match.c                       |  47 ++--
>>>>   lib/meta-flow.c                   |  22 +-
>>>>   lib/nx-match.c                    |  14 +-
>>>>   lib/odp-util.c                    | 227 ++++++++++++--------
>>>>   lib/odp-util.h                    |   4 +-
>>>>   lib/ofp-actions.c                 |  61 +++---
>>>>   lib/ofp-util.c                    |  56 ++---
>>>>   lib/tnl-ports.c                   |   2 +-
>>>>   ofproto/bond.c                    |   2 +-
>>>>   ofproto/ofproto-dpif-ipfix.c      |   6 +-
>>>>   ofproto/ofproto-dpif-rid.h        |   2 +-
>>>>   ofproto/ofproto-dpif-sflow.c      |   4 +-
>>>>   ofproto/ofproto-dpif-xlate.c      | 436 ++++++++++++++++++++++++++------------
>>>>   ofproto/ofproto-dpif-xlate.h      |   6 +-
>>>>   ofproto/ofproto-dpif.c            |  74 ++++++-
>>>>   ofproto/ofproto.h                 |   8 +-
>>>>   ovn/controller/pinctrl.c          |   5 +-
>>>>   tests/ofproto-dpif.at             | 238 ++++++++++++---------
>>>>   tests/test-classifier.c           |  15 +-
>>>>   tests/test-odp.c                  |   1 +
>>>>   utilities/ovs-ofctl.c             |  29 +--
>>>>   vswitchd/bridge.c                 |  27 ++-
>>>>   vswitchd/vswitch.ovsschema        |  16 +-
>>>>   vswitchd/vswitch.xml              |  31 +++
>>>>   31 files changed, 1002 insertions(+), 511 deletions(-)
>>>>
>>>> --
>>>> 2.9.0
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev at openvswitch.org
>>>> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list