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