[ovs-dev] [RFC PATCH v5] Add support for 802.1ad (QinQ tunneling)

Eric Garver e at erig.me
Wed Oct 26 13:24:45 UTC 2016


Hi Ben and Xiao,

One comment below.

On Mon, Oct 17, 2016 at 11:15:28AM +0800, Xiao Liang wrote:
> Hi Ben,
> 
> Please see inline.
> 
> And another question:
> In datapath/README.md:
>   - If the userspace flow key includes more fields than the
>     kernel's, for example if userspace decoded an IPv6 header but
>     the kernel stopped at the Ethernet type, then userspace can
>     forward the packet manually, without setting up a flow in the
>     kernel.
> Is there an example?
> 
> Thanks,
> Xiao
> 
> 
> On Wed, Oct 5, 2016 at 4:08 AM, Ben Pfaff <blp at ovn.org> wrote:
> > On Wed, Sep 14, 2016 at 12:34:36PM +0800, Xiao Liang wrote:
> >> Flow key handleing changes:
> >>  - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
> >>    headers.
> >>  - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
> >>    increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> >>
> >> Refacter VLAN handling in dpif-xlate:
> >>  - Introduce 'xvlan' to track VLAN stack during flow processing.
> >>  - Input and output VLAN translation according to the xbundle type.
> >>
> >> Push VLAN action support:
> >>  - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
> >>  - Support push_vlan on dot1q packets.
> >>
> >> Add new port VLAN mode "dot1q-tunnel":
> >>  - Example:
> >>      ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
> >>    Pushes another VLAN 100 header on packets (tagged and untagged) on ingress,
> >>    and pops it on egress.
> >>  - Customer VLAN check:
> >>      ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
> >>    Only customer VLAN of 10 and 20 are allowed.
> >>
> >> Use other_config:vlan-limit in table Open_vSwitch to limit maxium VLANs
> >> that can be matched
> >>
> >> Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN handling
> >>
> >> Signed-off-by: Xiao Liang <shaw.leon at gmail.com>
> >
> > Thanks for working on this.
> >
> > "sparse" says:
> >     ../lib/odp-util.c:5052:42: warning: restricted ovs_be16 degrades to integer
> >
> > The fix is:
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 527cd7a..12ebda1 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -5049,7 +5049,7 @@ parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
> >
> >      while (encaps < max_vlan_headers &&
> >             (is_mask ?
> > -            (src_flow->vlans[encaps].tci & htons(VLAN_CFI)) :
> > +            (src_flow->vlans[encaps].tci & htons(VLAN_CFI)) != 0 :
> >              eth_type_vlan(flow->dl_type))) {
> >          /* Calculate fitness of outer attributes. */
> >          encap  = (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)
> >
> > In miniflow_extract(), the following code always adds an extra 64-bit
> > word to the miniflow, even if there are no vlans.  It should avoid that
> > if the vlan doubleword is going to be all-0s.  (Previously, this
> > optimization was not necessary because the VLAN was in the same
> > doubleword as the Ethertype, which is always nonzero.)
> 
> Yes.
> 
> >
> >         /* dl_type */
> >         dl_type = parse_ethertype(&data, &size);
> >         miniflow_push_be16(mf, dl_type, dl_type);
> >         miniflow_pad_to_64(mf, dl_type);
> >         miniflow_push_words_32(mf, vlans, vlans, FLOW_MAX_VLAN_HEADERS);
> >
> > In flow_wildcards_init_for_packet(), I think that the safer thing to do
> > would be to implement the FIXME:
> > +    /* No need to set mask of inner VLANs that doesn't exist.
> > +     * FIXME: set mask of the first zero VLAN? */
> > +    WC_MASK_FIELD(wc, vlans[0]);
> > +    for (int i = 1; i < FLOW_MAX_VLAN_HEADERS; i++) {
> > +        if (flow->vlans[i].tci == htons(0)) {
> > +            break;
> > +        }
> > +        WC_MASK_FIELD(wc, vlans[i]);
> > +    }
> 
> Agree. But match_format will then print an extra vlan_tci1, so many
> many tests need change.
> 
> >
> > I don't think that a max_vlan_headers global variable is a good idea.
> > Different datapaths might have different limits.
> 
> There's currently no datapath table in ovsdb. Is it possible to
> configure limits for different datapath?
> 

The global max_vlan_headers is for Open_vSwitch table
other_config:vlan_limit. There is another max_vlan_headers in struct
odp_support for probing the datapath. The global should be renamed to
something like flow_vlan_limit.

I think it'd be nice if vlan_limit was a bridge level other_config, but
miniflow_extract() currently has no context. It only gets a packet.



More information about the dev mailing list