I think these are all acked now, if not then they are now: Acked-by: Ben Pfaff <b...@ovn.org>
Thanks a lot for implementing this. On Wed, May 04, 2016 at 01:12:11PM -0700, Jarno Rajahalme wrote: > Series pushed to master unto this point, waiting for a review from Ben for > the rest. > > Jarno > > > On Apr 22, 2016, at 8:00 PM, Jarno Rajahalme <ja...@ovn.org> wrote: > > > > This optimization applied when a staged lookup index would narrow down > > to a single rule, which happens sometimes is simple test cases, but > > presumably less often in more populated flow tables. The result of > > this optimization allowed a bit more general megaflows, but the bit > > patterns produced were sometimes cryptic. Finally, a later fix to a > > more important performance problem does not allow for this > > optimization any more, so remove it now. > > > > Signed-off-by: Jarno Rajahalme <ja...@ovn.org> > > Acked-by: Ryan Moats <rmo...@us.ibm.com> > > Acked-by: Ben Pfaff <b...@ovn.org> > > --- > > lib/classifier.c | 75 > > +-------------------------------------------------- > > tests/classifier.at | 10 +++---- > > tests/ofproto-dpif.at | 14 +++++----- > > 3 files changed, 13 insertions(+), 86 deletions(-) > > > > diff --git a/lib/classifier.c b/lib/classifier.c > > index a62a2bd..1557f6a 100644 > > --- a/lib/classifier.c > > +++ b/lib/classifier.c > > @@ -1658,54 +1658,6 @@ find_match(const struct cls_subtable *subtable, > > cls_version_t version, > > return NULL; > > } > > > > -/* Returns true if 'target' satisifies 'flow'/'mask', that is, if each bit > > - * for which 'flow', for which 'mask' has a bit set, specifies a particular > > - * value has the correct value in 'target'. > > - * > > - * This function is equivalent to miniflow_and_mask_matches_flow() but this > > - * version fills in the mask bits in 'wc'. */ > > -static inline bool > > -miniflow_and_mask_matches_flow_wc(const struct miniflow *flow, > > - const struct minimask *mask, > > - const struct flow *target, > > - struct flow_wildcards *wc) > > -{ > > - const uint64_t *flowp = miniflow_get_values(flow); > > - const uint64_t *maskp = miniflow_get_values(&mask->masks); > > - const uint64_t *target_u64 = (const uint64_t *)target; > > - uint64_t *wc_u64 = (uint64_t *)&wc->masks; > > - uint64_t diff; > > - size_t idx; > > - map_t map; > > - > > - FLOWMAP_FOR_EACH_MAP (map, mask->masks.map) { > > - MAP_FOR_EACH_INDEX(idx, map) { > > - uint64_t msk = *maskp++; > > - > > - diff = (*flowp++ ^ target_u64[idx]) & msk; > > - if (diff) { > > - goto out; > > - } > > - > > - /* Fill in the bits that were looked at. */ > > - wc_u64[idx] |= msk; > > - } > > - target_u64 += MAP_T_BITS; > > - wc_u64 += MAP_T_BITS; > > - } > > - return true; > > - > > -out: > > - /* Only unwildcard if none of the differing bits is already > > - * exact-matched. */ > > - if (!(wc_u64[idx] & diff)) { > > - /* Keep one bit of the difference. The selected bit may be > > - * different in big-endian v.s. little-endian systems. */ > > - wc_u64[idx] |= rightmost_1bit(diff); > > - } > > - return false; > > -} > > - > > static const struct cls_match * > > find_match_wc(const struct cls_subtable *subtable, cls_version_t version, > > const struct flow *flow, struct trie_ctx > > trie_ctx[CLS_MAX_TRIES], > > @@ -1724,8 +1676,6 @@ find_match_wc(const struct cls_subtable *subtable, > > cls_version_t version, > > > > /* Try to finish early by checking fields in segments. */ > > for (i = 0; i < subtable->n_indices; i++) { > > - const struct cmap_node *inode; > > - > > if (check_tries(trie_ctx, n_tries, subtable->trie_plen, > > subtable->index_maps[i], flow, wc)) { > > /* 'wc' bits for the trie field set, now unwildcard the > > preceding > > @@ -1740,32 +1690,9 @@ find_match_wc(const struct cls_subtable *subtable, > > cls_version_t version, > > subtable->index_maps[i], > > &mask_offset, &basis); > > > > - inode = cmap_find(&subtable->indices[i], hash); > > - if (!inode) { > > + if (!cmap_find(&subtable->indices[i], hash)) { > > goto no_match; > > } > > - > > - /* If we have narrowed down to a single rule already, check whether > > - * that rule matches. Either way, we're done. > > - * > > - * (Rare) hash collisions may cause us to miss the opportunity for > > this > > - * optimization. */ > > - if (!cmap_node_next(inode)) { > > - const struct cls_match *head; > > - > > - ASSIGN_CONTAINER(head, inode - i, index_nodes); > > - if (miniflow_and_mask_matches_flow_wc(&head->flow, > > &subtable->mask, > > - flow, wc)) { > > - /* Return highest priority rule that is visible. */ > > - CLS_MATCH_FOR_EACH (rule, head) { > > - if (OVS_LIKELY(cls_match_visible_in_version(rule, > > - version))) > > { > > - return rule; > > - } > > - } > > - } > > - return NULL; > > - } > > } > > /* Trie check for the final range. */ > > if (check_tries(trie_ctx, n_tries, subtable->trie_plen, > > diff --git a/tests/classifier.at b/tests/classifier.at > > index b110508..e56ba3a 100644 > > --- a/tests/classifier.at > > +++ b/tests/classifier.at > > @@ -49,7 +49,7 @@ Datapath actions: 1 > > ]) > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=11.1.2.15,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=80'], > > [0], [stdout]) > > AT_CHECK([tail -2 stdout], [0], > > - [Megaflow: recirc_id=0,ip,in_port=1,nw_dst=1.0.0.0/1.0.0.0,nw_frag=no > > + [Megaflow: recirc_id=0,ip,in_port=1,nw_dst=11.0.0.0/8,nw_frag=no > > Datapath actions: drop > > ]) > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=10.1.2.15,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=80'], > > [0], [stdout]) > > @@ -59,7 +59,7 @@ Datapath actions: drop > > ]) > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=10.1.2.15,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=79'], > > [0], [stdout]) > > AT_CHECK([tail -2 stdout], [0], > > - [Megaflow: > > recirc_id=0,tcp,in_port=1,nw_dst=10.1.2.15,nw_frag=no,tp_dst=0x1/0x1 > > + [Megaflow: > > recirc_id=0,tcp,in_port=1,nw_dst=10.1.2.15,nw_frag=no,tp_dst=0x40/0xfff0 > > Datapath actions: 2 > > ]) > > OVS_VSWITCHD_STOP > > @@ -87,7 +87,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > > # nw_dst and nw_src should be on by default > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=80'], > > [0], [stdout]) > > AT_CHECK([tail -2 stdout], [0], > > - [Megaflow: recirc_id=0,ip,in_port=1,nw_dst=192.168.0.0/16,nw_frag=no > > + [Megaflow: recirc_id=0,tcp,in_port=1,nw_dst=192.168.0.0/16,nw_frag=no > > Datapath actions: drop > > ]) > > > > @@ -102,7 +102,7 @@ AT_CHECK([ovs-vsctl set Flow_Table t0 > > prefixes=nw_dst,nw_dst], [1], [], > > AT_CHECK([ovs-vsctl set Flow_Table t0 prefixes=nw_dst], [0]) > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=80'], > > [0], [stdout]) > > AT_CHECK([tail -2 stdout], [0], > > - [Megaflow: recirc_id=0,ip,in_port=1,nw_dst=192.168.0.0/16,nw_frag=no > > + [Megaflow: recirc_id=0,tcp,in_port=1,nw_dst=192.168.0.0/16,nw_frag=no > > Datapath actions: drop > > ]) > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=2,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=80'], > > [0], [stdout]) > > @@ -117,7 +117,7 @@ Datapath actions: drop > > ]) > > AT_CHECK([ovs-appctl ofproto/trace br0 > > 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=10.1.2.15,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=79'], > > [0], [stdout]) > > AT_CHECK([tail -2 stdout], [0], > > - [Megaflow: > > recirc_id=0,tcp,in_port=1,nw_dst=10.1.2.15,nw_frag=no,tp_src=0x0/0x1,tp_dst=0x40/0xfff0 > > + [Megaflow: > > recirc_id=0,tcp,in_port=1,nw_dst=10.1.2.15,nw_frag=no,tp_src=0x0/0xffc0,tp_dst=0x40/0xfff0 > > Datapath actions: 3 > > ]) > > AT_CHECK([ovs-vsctl set Flow_Table t0 prefixes=none], [0]) > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 9ac2e2a..99a6560 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -6315,7 +6315,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 > > 'in_port(1),eth(src=50:54:00:00:00: > > sleep 1 > > AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl > > recirc_id=0,ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,nw_frag=no, > > actions: <del> > > -recirc_id=0,ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:0b/ff:ff:00:00:00:02,nw_frag=no, > > actions: <del> > > +recirc_id=0,ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:0b,nw_frag=no, > > actions: <del> > > ]) > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > @@ -6334,7 +6334,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 > > 'in_port(1),eth(src=50:54:00:00:00: > > sleep 1 > > AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl > > recirc_id=0,icmp,in_port=1,vlan_tci=0x0000,nw_src=10.0.0.4,nw_frag=no, > > actions: <del> > > -recirc_id=0,ip,in_port=1,vlan_tci=0x0000,nw_src=10.0.0.2/0.0.0.2,nw_frag=no, > > actions: <del> > > +recirc_id=0,ip,in_port=1,vlan_tci=0x0000,nw_src=10.0.0.2/30,nw_frag=no, > > actions: <del> > > ]) > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > @@ -6353,7 +6353,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 > > 'in_port(1),eth(src=50:54:00:00:00: > > sleep 1 > > AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl > > recirc_id=0,ipv6,in_port=1,vlan_tci=0x0000,ipv6_src=2001:db8:3c4d:1:2:3:4:5,nw_frag=no, > > actions: <del> > > -recirc_id=0,ipv6,in_port=1,vlan_tci=0x0000,ipv6_src=2001:db8:3c4d:5:4:3:2:1/0:0:0:4::,nw_frag=no, > > actions: <del> > > +recirc_id=0,ipv6,in_port=1,vlan_tci=0x0000,ipv6_src=2001:db8:3c4d:5:4:3:2:1/62,nw_frag=no, > > actions: <del> > > ]) > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > @@ -6541,7 +6541,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 > > 'in_port(1),eth(src=50:54:00:00:00: > > sleep 1 > > AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl > > recirc_id=0,ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,nw_frag=no, > > actions: <del> > > -recirc_id=0,ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:0b/ff:ff:00:00:00:02,nw_frag=no, > > actions: <del> > > +recirc_id=0,ip,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:0b,nw_frag=no, > > actions: <del> > > ]) > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > @@ -6744,7 +6744,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 > > 'in_port(1),eth(src=50:54:00:00:00: > > sleep 1 > > AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl > > recirc_id=0,icmp,in_port=1,vlan_tci=0x0000,nw_src=10.0.0.4,nw_ttl=64,nw_frag=no, > > actions: <del> > > -recirc_id=0,ip,in_port=1,vlan_tci=0x0000,nw_src=10.0.0.2/0.0.0.2,nw_frag=no, > > actions: <del> > > +recirc_id=0,ip,in_port=1,vlan_tci=0x0000,nw_src=10.0.0.2/30,nw_frag=no, > > actions: <del> > > ]) > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > @@ -7398,7 +7398,7 @@ done > > > > AT_CHECK([ovs-appctl dpif/dump-flows br0 | strip_ufid | strip_used | sort], > > [0], [dnl > > recirc_id(0),in_port(1),eth_type(0x1234), packets:8, bytes:480, used:0.0s, > > actions:100 > > -recirc_id(0),in_port(1),eth_type(0x8100),vlan(vid=99/0x0,pcp=7/0x0),encap(eth_type(0x1234)), > > packets:2, bytes:120, used:0.0s, actions:drop > > +recirc_id(0),in_port(1),eth_type(0x8100),vlan(vid=99,pcp=7/0x0),encap(eth_type(0x1234)), > > packets:2, bytes:120, used:0.0s, actions:drop > > ]) > > > > # Check that the new flow matches the CFI bit, while both vid and pcp > > @@ -7407,7 +7407,7 @@ AT_CHECK([grep '\(modify\)\|\(flow_add\)' > > ovs-vswitchd.log | strip_ufid ], [0], > > dpif_netdev|DBG|flow_add: > > recirc_id=0,in_port=1,vlan_tci=0x0000,dl_type=0x1234, actions:100 > > dpif|DBG|dummy@ovs-dummy: put[[modify]] > > skb_priority(0/0),skb_mark(0/0),recirc_id(0),dp_hash(0/0),in_port(1),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234) > > dpif|DBG|dummy@ovs-dummy: put[[modify]] > > skb_priority(0/0),skb_mark(0/0),recirc_id(0),dp_hash(0/0),in_port(1),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234), > > actions:100 > > -dpif_netdev|DBG|flow_add: > > recirc_id=0,in_port=1,vlan_tci=0xf063/0x1000,dl_type=0x1234, actions:drop > > +dpif_netdev|DBG|flow_add: recirc_id=0,in_port=1,dl_vlan=99,dl_type=0x1234, > > actions:drop > > ]) > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > -- > > 2.1.4 > > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev