On Mon, Jun 2, 2014 at 9:04 PM, Simon Horman <ho...@verge.net.au> wrote: > Hi Jesse, > > thanks for your feedback. > > On Mon, Jun 02, 2014 at 05:58:10PM -0700, Jesse Gross wrote: >> On Sun, May 25, 2014 at 5:22 PM, Simon Horman <ho...@verge.net.au> wrote: >> > diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c >> > index 803a94c..8ce596c 100644 >> > --- a/datapath/flow_netlink.c >> > +++ b/datapath/flow_netlink.c >> > + case OVS_ACTION_ATTR_POP_MPLS: >> > + if (!eth_p_mpls(eth_type)) >> > + return -EINVAL; >> >> Should this also take into account the VLAN tag? It's really part of >> the EtherType although it has been stripped out here. Actually, maybe >> it's better to not track the vlan_tci separately at all during >> validation but just fold it into the EtherType. >> >> The practical implication of this is that you wouldn't be able to pop >> out from underneath a VLAN. This may be a good thing if we are trying >> to avoid tag order issues - after all, you can't push underneath a >> VLAN either. I'm not sure what effects this has on the need to track >> mac_len, if any. > > My thinking is that the ordering problem only surfaces in relation > to push MPLS actions where should it go in relation to VLAN tags. > For pop actions it seems to me that the outermost tag should be removed > regardless of its position in relation to other tags. > > So I think that the code above is safe. Though now you mention > it I do notice that it only allows pop MPLS if there is at most > one VLAN tag present. > > That said, I would not mind particularly disabling pop MPLS in the > presence of VLAN tags. At the very least it is related to the > painful issue of tag ordering.
I agree that it is safe but my thought was the it avoids a number of potential corner cases such as: * Difference between push and pop underneath vlan tags. * Pop with multiple vlan tags * Differences with varying EtherTypes used for vlans > I explored your idea of tracking only eth_type rather than both > it and vlan_tci. I did this by adding the following logic to > ovs_nla_copy_actions(). > > if (key->eth.tci & htons(VLAN_TAG_PRESENT)) > eth_type = htons(ETH_P_8021Q); > else > eth_type = key->eth.type; > > I then updated the usage of eth_type in ovs_nla_copy_actions__() accordingly. > One problem that I have run into is what to do about pop VLAN. > > I don't believe its possible to know what the new eth type is. > This makes subsequent checks of the eth type for validate_set() > a little awkward. And seems to indicate that an extra parameter would > be needed. > > For this reason I am inclined to leave the eth_type and vlan_tci > parameters in place. In this case there is no problem with pop VLAN > as the ether type inside the VLAN tag should be the value of eth_type. Can't we populate eth_type with the EtherType from the flow key in pop_vlan? This doesn't provide us with the ability to look arbitrarily deep into the packet but it should at least retain the functionality that we have today. >> Otherwise, I'm happy with this. I think that we need to conclude the >> discussion on the other patch and update this appropriately first. > > Yes, lets get that sorted out. > > Assuming the other patch is accepted do you want me to increase the > coverage of the compatibility code (in this patch) up to whichever version > of the kernel the other patch is included in? It seems logical to me but I > do not have strong feelings about it. Yes, I think that probably makes sense. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
