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 <b...@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.l...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
