On Thu, Jun 12, 2014 at 9:08 AM, Thomas Graf <tg...@suug.ch> wrote: > On 06/12/14 at 09:04am, Jesse Gross wrote: >> On Wed, Jun 11, 2014 at 11:06 PM, Avinash <avinashand...@gmail.com> wrote: >> > On 6/11/14, Jesse Gross <je...@nicira.com> wrote: >> >> On Wed, Jun 11, 2014 at 7:26 AM, Avinash <avinashand...@gmail.com> wrote: >> >>> On 6/10/14, Thomas Graf <tg...@suug.ch> wrote: >> >>>> On 06/10/14 at 07:03pm, Avinash wrote: >> >>>>> @@ -471,10 +497,14 @@ int ovs_flow_extract(struct sk_buff *skb, u16 >> >>>>> in_port, struct sw_flow_key *key) >> >>>>> >> >>>>> if (vlan_tx_tag_present(skb)) >> >>>>> key->eth.tci = htons(vlan_get_tci(skb)); >> >>>>> - else if (eth->h_proto == htons(ETH_P_8021Q)) >> >>>>> + else if (eth->h_proto == htons(ETH_P_8021Q) || >> >>>>> + eth->h_proto == htons(ETH_P_8021AD)) >> >>>>> if (unlikely(parse_vlan(skb, key))) >> >>>>> return -ENOMEM; >> >>>>> >> >>>>> + if (unlikely(parse_remaining_vlans(skb))) >> >>>>> + return -ENOMEM; >> >>>> >> >>>> How about adding the functionality of parse_remaining_vlans() to >> >>>> parse_vlan() directly instead of checking every single packet? >> >>>> >> >>> >> >>> Combining parse_remaining_vlans() with the parse_vlan() might not skip >> >>> the inner VLANs present in the packet when the execution enters the >> >>> condition vlan_tx_tag_present(). >> >> >> >> You shouldn't blindly skip vlan tags anyways. This patch also >> >> misreports EtherTypes in Netlink since it doesn't actually store them >> >> in the flow. You need to find a cleaner approach that will work for >> >> all situations and be possible to extend in the future. >> >> >> > >> > Storing the inner vlans in the flow might not be useful as these are not >> > processed to the user space. Userspace makes decision considering only >> > the outer vlan. Also for supporting stacked VLANs (2 or more vlans), >> > storing >> > may requires an array of vlans. >> > >> > Do you see any issue storing only the outer VLAN in the flow ? >> >> It's fine to only look at the outer tag, that's what is done today. >> What's not fine is parsing the packet and throwing the information >> away which you have done with both the VLAN TPI and further tags. At a >> minimum, this changes existing behavior and makes it difficult to >> extend in the future, which is why I said that you need to think this >> through more. > > I think it would make sense to separate out the ETH_P_8021AD tag > matching from the patch since that is of obvious good value and then > discuss the stacked tag behaviour separately.
I agree that this definitely makes sense. I think there are potentially some tricky compatibility issues to be worked out even with just the new EtherType, so it would be good to tackle that first. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
