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