On Wed, Mar 15, 2017 at 03:06:39PM -0700, Andy Zhou wrote:
> On Wed, Mar 1, 2017 at 2:47 PM, Eric Garver <[email protected]> wrote:
> > Flow key handling 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.
> >
> > Refactor 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.
> >
> > Use other_config:vlan-limit in table Open_vSwitch to limit maximum VLANs
> > that can be matched. This allows us to preserve backwards compatibility.
> >
> > Add test cases for VLAN depth limit, Multi-VLAN actions and QinQ VLAN
> > handling
> >
> > Co-authored-by: Thomas F Herbert <[email protected]>
> > Signed-off-by: Thomas F Herbert <[email protected]>
> > Co-authored-by: Xiao Liang <[email protected]>
> > Signed-off-by: Xiao Liang <[email protected]>
> > Signed-off-by: Eric Garver <[email protected]>
>
> Thanks for working on this. And sorry it took a while to get those reviewed.
>
> The patch does not apply cleanly to master. Do you mind rebase and repost?
> I suppose you will need to this anyways for them to be pushed.
Hmm, by coincidence I was also looking at this patch, so I'm going to
respond directly to some of your comments.
> > @@ -103,7 +109,8 @@ struct flow {
> > struct eth_addr dl_dst; /* Ethernet destination address. */
> > struct eth_addr dl_src; /* Ethernet source address. */
> > ovs_be16 dl_type; /* Ethernet frame type. */
> > - ovs_be16 vlan_tci; /* If 802.1Q, TCI | VLAN_CFI; otherwise 0.
> > */
> > + uint8_t pad2[2]; /* Pad to 64 bits. */
> Do we need 4 more bytes for 64 bit alignment?
I don't think so, this looks correct to me. Assuming dl_dst is properly
aligned, there are 6+6+2+2 == 16 bytes from it to the new vlans[] member.
> > +void
> > +flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
> > +{
> > + int n = flow_count_vlan_headers(flow);
> > + if (n == 0) {
> > + return;
> > + }
> Can n be 0? or should this be an assert instead?
The use in ofpact_check__() suggests to me that this is correct as-is,
unless we want to change the interface.
> > + if (wc) {
> > + memset(&wc->masks.vlans[1], 0xff,
> > + sizeof(union flow_vlan_hdr) * (n - 1));
> > + }
>
> Should we also set the mask for wc->masks.vlans[0]?
Wildcards are mostly about read dependencies. This only writes to
vlans[0] so I don't think it's necessary.
flow_pop_mpls() follows the same pattern.
> > + memmove(&flow->vlans[0], &flow->vlans[1],
> > + sizeof(union flow_vlan_hdr) * (n - 1));
> > + memset(&flow->vlans[n - 1], 0, sizeof(union flow_vlan_hdr));
> > +}
> > +
> > +void
> > +flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
> > +{
> > + int n = flow_count_vlan_headers(flow);
>
> Do we always know there is room in the flow?
It looks like we don't, but the one caller where it matters has
essentially the same failure case beforehand.
> > + if (wc) {
> > + memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> > + }
> Should we set mask for the vlan fields that is about to be pushed?
I think that this does not introduce a read dependency.
> > + memmove(&flow->vlans[1], &flow->vlans[0],
> > + sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
> > + memset(&flow->vlans[0], 0, sizeof(union flow_vlan_hdr));
> The function syas "_uninit", I don't think the last memset() is needed.
Hmm. The caller still needs to initialize it, otherwise it's invalid
since the CFI bit isn't set. You're right that it's zeroed. This is
not a perfect situation here.
Anyway, I'm pretty happy with this patch and I'm likely to push it (with
changes) pretty soon.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
