| Message ID | 20220106180704.2388658-1-aconole@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] Revert "odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP." | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 6 Jan 2022, at 19:07, Aaron Conole wrote: > This reverts commit c645550bb249 ("odp-util: Always report > ODP_FIT_TOO_LITTLE for IGMP.") > > Always forcing a slow path action can result in some over-broad > flows which swallow all traffic and force them to userspace, as reported > in the thread at > https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html > and at > https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html > > Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically. > Additionally, remove the userspace wc mask to prevent revalidator from > cycling flows. > > Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") > Signed-off-by: Aaron Conole <aconole@redhat.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > v1->v2: Add unit test for coverage of mcast traffic when not doing snooping > v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the > matching issue. > v3->v4: Fix up the unit tests to be consistent when reporting up flows. Split > IGMP unit test into two different tests. Use a multicast eth addr for > the FLOOD under NORMAL action case. I reviewed and tested the patch (even run the test cases in a 50 times loop), and all looks fine! Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 1/6/22 19:07, Aaron Conole wrote: > This reverts commit c645550bb249 ("odp-util: Always report > ODP_FIT_TOO_LITTLE for IGMP.") > > Always forcing a slow path action can result in some over-broad > flows which swallow all traffic and force them to userspace, as reported > in the thread at > https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html > and at > https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html > > Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically. > Additionally, remove the userspace wc mask to prevent revalidator from > cycling flows. > > Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") > Signed-off-by: Aaron Conole <aconole@redhat.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > v1->v2: Add unit test for coverage of mcast traffic when not doing snooping > v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the > matching issue. > v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split > IGMP unit test into two different tests. Use a multicast eth addr for > the FLOOD under NORMAL action case. > > lib/odp-util.c | 5 --- > ofproto/ofproto-dpif-xlate.c | 1 - > tests/mcast-snooping.at | 68 ++++++++++++++++++++++++++++++ > tests/ofproto-macros.at | 15 +++++++ > tests/system-traffic.at | 80 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 163 insertions(+), 6 deletions(-) > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index fbdfc7ad83..0f21ffe639 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], > } > } > } > - } else if (src_flow->nw_proto == IPPROTO_IGMP > - && src_flow->dl_type == htons(ETH_TYPE_IP)) { > - /* OVS userspace parses the IGMP type, code, and group, but its > - * datapaths do not, so there is always missing information. */ > - return ODP_FIT_TOO_LITTLE; > } > if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { > if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index cafcd014ad..29d26cf9ac 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx) > */ > ctx->xout->slow |= SLOW_ACTION; > > - memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > if (mcast_snooping_is_membership(flow->tp_src) || > mcast_snooping_is_query(flow->tp_src)) { > if (ctx->xin->allow_side_effects && ctx->xin->packet) { > diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at > index 757cf7186e..e24f293a0c 100644 > --- a/tests/mcast-snooping.at > +++ b/tests/mcast-snooping.at > @@ -216,3 +216,70 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl > ]) > > AT_CLEANUP > + > + > +AT_SETUP([mcast - igmp flood for non-snoop enabled]) > +OVS_VSWITCHD_START([]) > + > +AT_CHECK([ > + ovs-vsctl set bridge br0 \ > + datapath_type=dummy], [0]) > + > +add_of_ports br0 1 2 > + > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > + > +ovs-appctl time/stop > + > +dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP > +dnl in reverse direction > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ > + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) > +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \ > + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) > + > + > +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl > + strip_stats | strip_used | strip_recirc | dnl > + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], > + [0], [dnl > +recirc_id(<recirc>),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:100,2 > +recirc_id(<recirc>),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1 > +]) > + > +ovs-appctl time/warp 100000 > + > +dnl Next we should clear the flows and install a complex case > +AT_CHECK([ovs-ofctl del-flows br0]) > + > +AT_DATA([flows.txt], [dnl > +table=0, arp actions=NORMAL > +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) > +table=0, in_port=2 actions=output:1 > +table=1, ip,ct_state=+trk+inv actions=drop > +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 > +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) > +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +ovs-appctl time/warp 100000 > + > +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ > + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ > + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) > + > + > +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl > + strip_stats | strip_used | strip_recirc | dnl > + sed 's/pid=[[0-9]]*,// > + s/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], > + [0], [dnl > +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 > +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) > +recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) > +]) I tried to backport the patch to earlier branches, but this test fails on 2.14 and 2.13 (works on 2.15+) with the following result: --- - 2022-01-26 21:33:15.713282778 +0000 +++ /home/runner/work/ovs/ovs/openvswitch-2.13.7/_build/sub/tests/testsuite.dir/at-groups/2302/stdout 2022-01-26 21:33:15.708679313 +0000 @@ -1,4 +1,4 @@ +ct_state(+inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:drop ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 -ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) Packets seems to be marked +inv by conntrack. Is it some kind of conntrack issue that got fixed in 2.15 that we should backport? Can we modify the test somehow to make it work on 2.13 ? > + > +AT_CLEANUP > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 736d9809cb..f906b5c3b5 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -134,6 +134,21 @@ strip_ufid () { > sed 's/mega_ufid:[[-0-9a-f]]* // > s/ufid:[[-0-9a-f]]* //' > } > + > +# Strips packets: and bytes: from output > +strip_stats () { > + sed 's/packets:[[0-9]]*/packets:0/ > + s/bytes:[[0-9]]*/bytes:0/' > +} > + > +# Changes all 'recirc(...)' and 'recirc=...' to say 'recirc(<recirc_id>)' and > +# 'recirc=<recirc_id>' respectively. This should make output easier to > +# compare. > +strip_recirc() { > + sed 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/ > + s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ > + s/recirc([[x0-9]]*)/recirc(<recirc>)/' > +} > m4_divert_pop([PREPARE_TESTS]) > > m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index d79753a99a..5d9449fada 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -6287,6 +6287,86 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_BANNER([IGMP]) > + > +AT_SETUP([IGMP - flood under normal action]) > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") > +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") > + > +AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"]) > + > +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 01 00 5e 01 01 03 dnl > +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl > +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl > +00 00 00 00 > /dev/null]) > + > +AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl > + br0: > + br0 65534/1: (internal) This line is failing the system-userspace testsuite, because userspace datapath reports 'tap' instead of 'internal'. Suggesting a following change: diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 1ba28f7ee..7711f14b0 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -6681,18 +6681,11 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl 00 00 00 00 > /dev/null]) -AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl - br0: - br0 65534/1: (internal) - ovs-p1 1/2: (system) - ovs-p2 2/3: (system) -]) - -AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl strip_stats | strip_used | strip_recirc | dnl sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], [0], [dnl -recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1,3 +recirc_id(<recirc>),in_port(ovs-p1),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:br0,ovs-p2 ]) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP --- What do you think? > + ovs-p1 1/2: (system) > + ovs-p2 2/3: (system) > +]) > + > +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl > + strip_stats | strip_used | strip_recirc | dnl > + sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], > + [0], [dnl > +recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1,3 > +]) > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > +AT_SETUP([IGMP - forward with ICMP]) > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") > +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") > + > +AT_DATA([flows.txt], [dnl > +table=0, arp actions=NORMAL > +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) > +table=0, in_port=2 actions=output:1 > +table=1, ip,ct_state=+trk+inv actions=drop > +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 > +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) > +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 > +]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole > + > +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl > +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl > +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl > +00 00 00 00 > /dev/null]) > + > +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl > +f0 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a dnl > +01 01 02 08 00 f7 ff ff ff ff ff > /dev/null]) > + > +sleep 1 > + > +AT_CHECK([ovs-ofctl dump-flows br0 | grep -v NXST | dnl > + strip_duration | grep -v arp | grep -v n_packets=0 | dnl > + grep -v 'in_port=2 actions=output:1' | dnl > + sed 's/n_bytes=[[0-9]]*/n_bytes=0/ > + s/idle_age=[[0-9]]*/idle_age=0/ > + s/n_packets=[[1-9]]/n_packets=0/' | sort], [0], [dnl > + cookie=0x0, table=0, n_packets=0, n_bytes=0, idle_age=0, ip,in_port=1 actions=ct(table=1,zone=64000) > + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,icmp,in_port=1 actions=output:2 > + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,ip,in_port=1 actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) > +]) These are OpenFlow rules. Shouldn't we dump datapath flows, or am I missing something? > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_BANNER([802.1ad]) > > AT_SETUP([802.1ad - vlan_limit]) >
Ilya Maximets <i.maximets@ovn.org> writes: > On 1/6/22 19:07, Aaron Conole wrote: >> This reverts commit c645550bb249 ("odp-util: Always report >> ODP_FIT_TOO_LITTLE for IGMP.") >> >> Always forcing a slow path action can result in some over-broad >> flows which swallow all traffic and force them to userspace, as reported >> in the thread at >> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html >> and at >> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html >> >> Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically. >> Additionally, remove the userspace wc mask to prevent revalidator from >> cycling flows. >> >> Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> v1->v2: Add unit test for coverage of mcast traffic when not doing snooping >> v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the >> matching issue. >> v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split >> IGMP unit test into two different tests. Use a multicast eth addr for >> the FLOOD under NORMAL action case. >> >> lib/odp-util.c | 5 --- >> ofproto/ofproto-dpif-xlate.c | 1 - >> tests/mcast-snooping.at | 68 ++++++++++++++++++++++++++++++ >> tests/ofproto-macros.at | 15 +++++++ >> tests/system-traffic.at | 80 ++++++++++++++++++++++++++++++++++++ >> 5 files changed, 163 insertions(+), 6 deletions(-) >> >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index fbdfc7ad83..0f21ffe639 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], >> } >> } >> } >> - } else if (src_flow->nw_proto == IPPROTO_IGMP >> - && src_flow->dl_type == htons(ETH_TYPE_IP)) { >> - /* OVS userspace parses the IGMP type, code, and group, but its >> - * datapaths do not, so there is always missing information. */ >> - return ODP_FIT_TOO_LITTLE; >> } >> if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { >> if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index cafcd014ad..29d26cf9ac 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx) >> */ >> ctx->xout->slow |= SLOW_ACTION; >> >> - memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); >> if (mcast_snooping_is_membership(flow->tp_src) || >> mcast_snooping_is_query(flow->tp_src)) { >> if (ctx->xin->allow_side_effects && ctx->xin->packet) { >> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at >> index 757cf7186e..e24f293a0c 100644 >> --- a/tests/mcast-snooping.at >> +++ b/tests/mcast-snooping.at >> @@ -216,3 +216,70 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl >> ]) >> >> AT_CLEANUP >> + >> + >> +AT_SETUP([mcast - igmp flood for non-snoop enabled]) >> +OVS_VSWITCHD_START([]) >> + >> +AT_CHECK([ >> + ovs-vsctl set bridge br0 \ >> + datapath_type=dummy], [0]) >> + >> +add_of_ports br0 1 2 >> + >> +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) >> + >> +ovs-appctl time/stop >> + >> +dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP >> +dnl in reverse direction >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \ >> + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) >> + >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >> + strip_stats | strip_used | strip_recirc | dnl >> + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >> + [0], [dnl >> +recirc_id(<recirc>),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:100,2 >> +recirc_id(<recirc>),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1 >> +]) >> + >> +ovs-appctl time/warp 100000 >> + >> +dnl Next we should clear the flows and install a complex case >> +AT_CHECK([ovs-ofctl del-flows br0]) >> + >> +AT_DATA([flows.txt], [dnl >> +table=0, arp actions=NORMAL >> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) >> +table=0, in_port=2 actions=output:1 >> +table=1, ip,ct_state=+trk+inv actions=drop >> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 >> +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 >> +]) >> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> + >> +ovs-appctl time/warp 100000 >> + >> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >> + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) >> + >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >> + strip_stats | strip_used | strip_recirc | dnl >> + sed 's/pid=[[0-9]]*,// >> + s/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >> + [0], [dnl >> +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 >> +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) >> +recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) >> +]) > > I tried to backport the patch to earlier branches, but this test fails > on 2.14 and 2.13 (works on 2.15+) with the following result: > > --- - 2022-01-26 21:33:15.713282778 +0000 > +++ > /home/runner/work/ovs/ovs/openvswitch-2.13.7/_build/sub/tests/testsuite.dir/at-groups/2302/stdout > 2022-01-26 21:33:15.708679313 +0000 > @@ -1,4 +1,4 @@ > +ct_state(+inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), > packets:0, bytes:0, used:never, actions:drop > ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), > packets:0, bytes:0, used:never, actions:2 > -ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), > packets:0, bytes:0, used:never, > actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) > recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), > packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) > > Packets seems to be marked +inv by conntrack. Is it some kind > of conntrack issue that got fixed in 2.15 that we should backport? > Can we modify the test somehow to make it work on 2.13 ? I'll look into it. Thanks. >> + >> +AT_CLEANUP >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index 736d9809cb..f906b5c3b5 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -134,6 +134,21 @@ strip_ufid () { >> sed 's/mega_ufid:[[-0-9a-f]]* // >> s/ufid:[[-0-9a-f]]* //' >> } >> + >> +# Strips packets: and bytes: from output >> +strip_stats () { >> + sed 's/packets:[[0-9]]*/packets:0/ >> + s/bytes:[[0-9]]*/bytes:0/' >> +} >> + >> +# Changes all 'recirc(...)' and 'recirc=...' to say 'recirc(<recirc_id>)' and >> +# 'recirc=<recirc_id>' respectively. This should make output easier to >> +# compare. >> +strip_recirc() { >> + sed 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/ >> + s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ >> + s/recirc([[x0-9]]*)/recirc(<recirc>)/' >> +} >> m4_divert_pop([PREPARE_TESTS]) >> >> m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index d79753a99a..5d9449fada 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -6287,6 +6287,86 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_BANNER([IGMP]) >> + >> +AT_SETUP([IGMP - flood under normal action]) >> + >> +OVS_TRAFFIC_VSWITCHD_START() >> +ADD_NAMESPACES(at_ns0, at_ns1) >> + >> +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") >> +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") >> + >> +AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"]) >> + >> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 01 00 5e 01 01 03 dnl >> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >> +00 00 00 00 > /dev/null]) >> + >> +AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl >> + br0: >> + br0 65534/1: (internal) > > This line is failing the system-userspace testsuite, because userspace > datapath reports 'tap' instead of 'internal'. Suggesting a following > change: > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 1ba28f7ee..7711f14b0 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -6681,18 +6681,11 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl > 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl > 00 00 00 00 > /dev/null]) > > -AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl > - br0: > - br0 65534/1: (internal) > - ovs-p1 1/2: (system) > - ovs-p2 2/3: (system) > -]) > - > -AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl > +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl > strip_stats | strip_used | strip_recirc | dnl > sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], > [0], [dnl > -recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1,3 > +recirc_id(<recirc>),in_port(ovs-p1),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:br0,ovs-p2 > ]) > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > --- > > What do you think? ACK - I'll pull that in. > >> + ovs-p1 1/2: (system) >> + ovs-p2 2/3: (system) >> +]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >> + strip_stats | strip_used | strip_recirc | dnl >> + sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >> + [0], [dnl >> +recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), >> packets:0, bytes:0, used:never, actions:1,3 >> +]) >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> + >> +AT_SETUP([IGMP - forward with ICMP]) >> + >> +OVS_TRAFFIC_VSWITCHD_START() >> +ADD_NAMESPACES(at_ns0, at_ns1) >> + >> +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") >> +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") >> + >> +AT_DATA([flows.txt], [dnl >> +table=0, arp actions=NORMAL >> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) >> +table=0, in_port=2 actions=output:1 >> +table=1, ip,ct_state=+trk+inv actions=drop >> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 >> +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 >> +]) >> +AT_CHECK([ovs-ofctl del-flows br0]) >> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> + >> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole >> + >> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl >> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >> +00 00 00 00 > /dev/null]) >> + >> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl >> +f0 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a dnl >> +01 01 02 08 00 f7 ff ff ff ff ff > /dev/null]) >> + >> +sleep 1 >> + >> +AT_CHECK([ovs-ofctl dump-flows br0 | grep -v NXST | dnl >> + strip_duration | grep -v arp | grep -v n_packets=0 | dnl >> + grep -v 'in_port=2 actions=output:1' | dnl >> + sed 's/n_bytes=[[0-9]]*/n_bytes=0/ >> + s/idle_age=[[0-9]]*/idle_age=0/ >> + s/n_packets=[[1-9]]/n_packets=0/' | sort], [0], [dnl >> + cookie=0x0, table=0, n_packets=0, n_bytes=0, idle_age=0, >> ip,in_port=1 actions=ct(table=1,zone=64000) >> + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, >> ct_state=+new+trk,icmp,in_port=1 actions=output:2 >> + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, >> ct_state=+new+trk,ip,in_port=1 >> actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >> +]) > > These are OpenFlow rules. Shouldn't we dump datapath flows, or > am I missing something? IIRC, we use the openflow rules because they are wildly different datapath rules between the two, so we rely on these rules being hit. Additionally, in the kernel DP case, if ipv6 is enabled then some ND will occur and that will tamper with n_packets/n_bytes counters (which is why I clear it before doing the comparison). I will look at the datapath flows again, but I believe the datapath action set is different because of the way controller() is implemented between the two (in the userspace DP, it can be a direct call, but in the kerneldp it needs to be a userspace call first). So I guess I will look at this, but expect to keep it as-is. >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_BANNER([802.1ad]) >> >> AT_SETUP([802.1ad - vlan_limit]) >>
On 2/23/22 19:58, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> On 1/6/22 19:07, Aaron Conole wrote: >>> This reverts commit c645550bb249 ("odp-util: Always report >>> ODP_FIT_TOO_LITTLE for IGMP.") >>> >>> Always forcing a slow path action can result in some over-broad >>> flows which swallow all traffic and force them to userspace, as reported >>> in the thread at >>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html >>> and at >>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html >>> >>> Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically. >>> Additionally, remove the userspace wc mask to prevent revalidator from >>> cycling flows. >>> >>> Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") >>> Signed-off-by: Aaron Conole <aconole@redhat.com> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> v1->v2: Add unit test for coverage of mcast traffic when not doing snooping >>> v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the >>> matching issue. >>> v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split >>> IGMP unit test into two different tests. Use a multicast eth addr for >>> the FLOOD under NORMAL action case. >>> >>> lib/odp-util.c | 5 --- >>> ofproto/ofproto-dpif-xlate.c | 1 - >>> tests/mcast-snooping.at | 68 ++++++++++++++++++++++++++++++ >>> tests/ofproto-macros.at | 15 +++++++ >>> tests/system-traffic.at | 80 ++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 163 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/odp-util.c b/lib/odp-util.c >>> index fbdfc7ad83..0f21ffe639 100644 >>> --- a/lib/odp-util.c >>> +++ b/lib/odp-util.c >>> @@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], >>> } >>> } >>> } >>> - } else if (src_flow->nw_proto == IPPROTO_IGMP >>> - && src_flow->dl_type == htons(ETH_TYPE_IP)) { >>> - /* OVS userspace parses the IGMP type, code, and group, but its >>> - * datapaths do not, so there is always missing information. */ >>> - return ODP_FIT_TOO_LITTLE; >>> } >>> if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { >>> if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index cafcd014ad..29d26cf9ac 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx) >>> */ >>> ctx->xout->slow |= SLOW_ACTION; >>> >>> - memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); >>> if (mcast_snooping_is_membership(flow->tp_src) || >>> mcast_snooping_is_query(flow->tp_src)) { >>> if (ctx->xin->allow_side_effects && ctx->xin->packet) { >>> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at >>> index 757cf7186e..e24f293a0c 100644 >>> --- a/tests/mcast-snooping.at >>> +++ b/tests/mcast-snooping.at >>> @@ -216,3 +216,70 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl >>> ]) >>> >>> AT_CLEANUP >>> + >>> + >>> +AT_SETUP([mcast - igmp flood for non-snoop enabled]) >>> +OVS_VSWITCHD_START([]) >>> + >>> +AT_CHECK([ >>> + ovs-vsctl set bridge br0 \ >>> + datapath_type=dummy], [0]) >>> + >>> +add_of_ports br0 1 2 >>> + >>> +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) >>> + >>> +ovs-appctl time/stop >>> + >>> +dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP >>> +dnl in reverse direction >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >>> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \ >>> + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) >>> + >>> + >>> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>> + strip_stats | strip_used | strip_recirc | dnl >>> + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>> + [0], [dnl >>> +recirc_id(<recirc>),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:100,2 >>> +recirc_id(<recirc>),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1 >>> +]) >>> + >>> +ovs-appctl time/warp 100000 >>> + >>> +dnl Next we should clear the flows and install a complex case >>> +AT_CHECK([ovs-ofctl del-flows br0]) >>> + >>> +AT_DATA([flows.txt], [dnl >>> +table=0, arp actions=NORMAL >>> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) >>> +table=0, in_port=2 actions=output:1 >>> +table=1, ip,ct_state=+trk+inv actions=drop >>> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 >>> +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >>> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 >>> +]) >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>> + >>> +ovs-appctl time/warp 100000 >>> + >>> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >>> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >>> + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) >>> + >>> + >>> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>> + strip_stats | strip_used | strip_recirc | dnl >>> + sed 's/pid=[[0-9]]*,// >>> + s/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>> + [0], [dnl >>> +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 >>> +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) >>> +recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) >>> +]) >> >> I tried to backport the patch to earlier branches, but this test fails >> on 2.14 and 2.13 (works on 2.15+) with the following result: >> >> --- - 2022-01-26 21:33:15.713282778 +0000 >> +++ >> /home/runner/work/ovs/ovs/openvswitch-2.13.7/_build/sub/tests/testsuite.dir/at-groups/2302/stdout >> 2022-01-26 21:33:15.708679313 +0000 >> @@ -1,4 +1,4 @@ >> +ct_state(+inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), >> packets:0, bytes:0, used:never, actions:drop >> ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), >> packets:0, bytes:0, used:never, actions:2 >> -ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), >> packets:0, bytes:0, used:never, >> actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) >> recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), >> packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) >> >> Packets seems to be marked +inv by conntrack. Is it some kind >> of conntrack issue that got fixed in 2.15 that we should backport? >> Can we modify the test somehow to make it work on 2.13 ? > > I'll look into it. Thanks. > >>> + >>> +AT_CLEANUP >>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >>> index 736d9809cb..f906b5c3b5 100644 >>> --- a/tests/ofproto-macros.at >>> +++ b/tests/ofproto-macros.at >>> @@ -134,6 +134,21 @@ strip_ufid () { >>> sed 's/mega_ufid:[[-0-9a-f]]* // >>> s/ufid:[[-0-9a-f]]* //' >>> } >>> + >>> +# Strips packets: and bytes: from output >>> +strip_stats () { >>> + sed 's/packets:[[0-9]]*/packets:0/ >>> + s/bytes:[[0-9]]*/bytes:0/' >>> +} >>> + >>> +# Changes all 'recirc(...)' and 'recirc=...' to say 'recirc(<recirc_id>)' and >>> +# 'recirc=<recirc_id>' respectively. This should make output easier to >>> +# compare. >>> +strip_recirc() { >>> + sed 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/ >>> + s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ >>> + s/recirc([[x0-9]]*)/recirc(<recirc>)/' >>> +} >>> m4_divert_pop([PREPARE_TESTS]) >>> >>> m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index d79753a99a..5d9449fada 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -6287,6 +6287,86 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> +AT_BANNER([IGMP]) >>> + >>> +AT_SETUP([IGMP - flood under normal action]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_START() >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") >>> +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") >>> + >>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 01 00 5e 01 01 03 dnl >>> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >>> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >>> +00 00 00 00 > /dev/null]) >>> + >>> +AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl >>> + br0: >>> + br0 65534/1: (internal) >> >> This line is failing the system-userspace testsuite, because userspace >> datapath reports 'tap' instead of 'internal'. Suggesting a following >> change: >> >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 1ba28f7ee..7711f14b0 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -6681,18 +6681,11 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >> 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >> 00 00 00 00 > /dev/null]) >> >> -AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl >> - br0: >> - br0 65534/1: (internal) >> - ovs-p1 1/2: (system) >> - ovs-p2 2/3: (system) >> -]) >> - >> -AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl >> strip_stats | strip_used | strip_recirc | dnl >> sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >> [0], [dnl >> -recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1,3 >> +recirc_id(<recirc>),in_port(ovs-p1),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:br0,ovs-p2 >> ]) >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> --- >> >> What do you think? > > ACK - I'll pull that in. > >> >>> + ovs-p1 1/2: (system) >>> + ovs-p2 2/3: (system) >>> +]) >>> + >>> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>> + strip_stats | strip_used | strip_recirc | dnl >>> + sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>> + [0], [dnl >>> +recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), >>> packets:0, bytes:0, used:never, actions:1,3 >>> +]) >>> +OVS_TRAFFIC_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> +AT_SETUP([IGMP - forward with ICMP]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_START() >>> +ADD_NAMESPACES(at_ns0, at_ns1) >>> + >>> +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") >>> +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") >>> + >>> +AT_DATA([flows.txt], [dnl >>> +table=0, arp actions=NORMAL >>> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) >>> +table=0, in_port=2 actions=output:1 >>> +table=1, ip,ct_state=+trk+inv actions=drop >>> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 >>> +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >>> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 >>> +]) >>> +AT_CHECK([ovs-ofctl del-flows br0]) >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>> + >>> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole >>> + >>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl >>> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >>> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >>> +00 00 00 00 > /dev/null]) >>> + >>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl >>> +f0 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a dnl >>> +01 01 02 08 00 f7 ff ff ff ff ff > /dev/null]) >>> + >>> +sleep 1 >>> + >>> +AT_CHECK([ovs-ofctl dump-flows br0 | grep -v NXST | dnl >>> + strip_duration | grep -v arp | grep -v n_packets=0 | dnl >>> + grep -v 'in_port=2 actions=output:1' | dnl >>> + sed 's/n_bytes=[[0-9]]*/n_bytes=0/ >>> + s/idle_age=[[0-9]]*/idle_age=0/ >>> + s/n_packets=[[1-9]]/n_packets=0/' | sort], [0], [dnl >>> + cookie=0x0, table=0, n_packets=0, n_bytes=0, idle_age=0, >>> ip,in_port=1 actions=ct(table=1,zone=64000) >>> + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, >>> ct_state=+new+trk,icmp,in_port=1 actions=output:2 >>> + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, >>> ct_state=+new+trk,ip,in_port=1 >>> actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >>> +]) >> >> These are OpenFlow rules. Shouldn't we dump datapath flows, or >> am I missing something? > > IIRC, we use the openflow rules because they are wildly different > datapath rules between the two, so we rely on these rules being hit. > Additionally, in the kernel DP case, if ipv6 is enabled then some ND > will occur and that will tamper with n_packets/n_bytes counters (which > is why I clear it before doing the comparison). > > I will look at the datapath flows again, but I believe the datapath > action set is different because of the way controller() is implemented > between the two (in the userspace DP, it can be a direct call, but in > the kerneldp it needs to be a userspace call first). > > So I guess I will look at this, but expect to keep it as-is. OK. I guess, the main check here is n_packets=[[1-9]]. Is that correct? Maybe just a comment would be enough in that case. It's not clear what exactly we're testing here otherwise. Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 2/23/22 19:58, Aaron Conole wrote: >> Ilya Maximets <i.maximets@ovn.org> writes: >> >>> On 1/6/22 19:07, Aaron Conole wrote: >>>> This reverts commit c645550bb249 ("odp-util: Always report >>>> ODP_FIT_TOO_LITTLE for IGMP.") >>>> >>>> Always forcing a slow path action can result in some over-broad >>>> flows which swallow all traffic and force them to userspace, as reported >>>> in the thread at >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html >>>> and at >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html >>>> >>>> Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically. >>>> Additionally, remove the userspace wc mask to prevent revalidator from >>>> cycling flows. >>>> >>>> Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") >>>> Signed-off-by: Aaron Conole <aconole@redhat.com> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>> --- >>>> v1->v2: Add unit test for coverage of mcast traffic when not doing snooping >>>> v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the >>>> matching issue. >>>> v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split >>>> IGMP unit test into two different tests. Use a multicast eth addr for >>>> the FLOOD under NORMAL action case. >>>> >>>> lib/odp-util.c | 5 --- >>>> ofproto/ofproto-dpif-xlate.c | 1 - >>>> tests/mcast-snooping.at | 68 ++++++++++++++++++++++++++++++ >>>> tests/ofproto-macros.at | 15 +++++++ >>>> tests/system-traffic.at | 80 ++++++++++++++++++++++++++++++++++++ >>>> 5 files changed, 163 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/lib/odp-util.c b/lib/odp-util.c >>>> index fbdfc7ad83..0f21ffe639 100644 >>>> --- a/lib/odp-util.c >>>> +++ b/lib/odp-util.c >>>> @@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], >>>> } >>>> } >>>> } >>>> - } else if (src_flow->nw_proto == IPPROTO_IGMP >>>> - && src_flow->dl_type == htons(ETH_TYPE_IP)) { >>>> - /* OVS userspace parses the IGMP type, code, and group, but its >>>> - * datapaths do not, so there is always missing information. */ >>>> - return ODP_FIT_TOO_LITTLE; >>>> } >>>> if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { >>>> if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { >>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>>> index cafcd014ad..29d26cf9ac 100644 >>>> --- a/ofproto/ofproto-dpif-xlate.c >>>> +++ b/ofproto/ofproto-dpif-xlate.c >>>> @@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx) >>>> */ >>>> ctx->xout->slow |= SLOW_ACTION; >>>> >>>> - memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); >>>> if (mcast_snooping_is_membership(flow->tp_src) || >>>> mcast_snooping_is_query(flow->tp_src)) { >>>> if (ctx->xin->allow_side_effects && ctx->xin->packet) { >>>> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at >>>> index 757cf7186e..e24f293a0c 100644 >>>> --- a/tests/mcast-snooping.at >>>> +++ b/tests/mcast-snooping.at >>>> @@ -216,3 +216,70 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl >>>> ]) >>>> >>>> AT_CLEANUP >>>> + >>>> + >>>> +AT_SETUP([mcast - igmp flood for non-snoop enabled]) >>>> +OVS_VSWITCHD_START([]) >>>> + >>>> +AT_CHECK([ >>>> + ovs-vsctl set bridge br0 \ >>>> + datapath_type=dummy], [0]) >>>> + >>>> +add_of_ports br0 1 2 >>>> + >>>> +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) >>>> + >>>> +ovs-appctl time/stop >>>> + >>>> +dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP >>>> +dnl in reverse direction >>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >>>> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) >>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \ >>>> + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) >>>> + >>>> + >>>> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>>> + strip_stats | strip_used | strip_recirc | dnl >>>> + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>>> + [0], [dnl >>>> +recirc_id(<recirc>),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:100,2 >>>> +recirc_id(<recirc>),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1 >>>> +]) >>>> + >>>> +ovs-appctl time/warp 100000 >>>> + >>>> +dnl Next we should clear the flows and install a complex case >>>> +AT_CHECK([ovs-ofctl del-flows br0]) >>>> + >>>> +AT_DATA([flows.txt], [dnl >>>> +table=0, arp actions=NORMAL >>>> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) >>>> +table=0, in_port=2 actions=output:1 >>>> +table=1, ip,ct_state=+trk+inv actions=drop >>>> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 >>>> +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >>>> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 >>>> +]) >>>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>>> + >>>> +ovs-appctl time/warp 100000 >>>> + >>>> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole >>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >>>> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) >>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ >>>> + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) >>>> + >>>> + >>>> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>>> + strip_stats | strip_used | strip_recirc | dnl >>>> + sed 's/pid=[[0-9]]*,// >>>> + s/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>>> + [0], [dnl >>>> +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 >>>> +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) >>>> +recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) >>>> +]) >>> >>> I tried to backport the patch to earlier branches, but this test fails >>> on 2.14 and 2.13 (works on 2.15+) with the following result: >>> >>> --- - 2022-01-26 21:33:15.713282778 +0000 >>> +++ >>> /home/runner/work/ovs/ovs/openvswitch-2.13.7/_build/sub/tests/testsuite.dir/at-groups/2302/stdout >>> 2022-01-26 21:33:15.708679313 +0000 >>> @@ -1,4 +1,4 @@ >>> +ct_state(+inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), >>> packets:0, bytes:0, used:never, actions:drop >>> ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), >>> packets:0, bytes:0, used:never, actions:2 >>> -ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), >>> packets:0, bytes:0, used:never, >>> actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) >>> recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), >>> packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) >>> >>> Packets seems to be marked +inv by conntrack. Is it some kind >>> of conntrack issue that got fixed in 2.15 that we should backport? >>> Can we modify the test somehow to make it work on 2.13 ? >> >> I'll look into it. Thanks. >> >>>> + >>>> +AT_CLEANUP >>>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >>>> index 736d9809cb..f906b5c3b5 100644 >>>> --- a/tests/ofproto-macros.at >>>> +++ b/tests/ofproto-macros.at >>>> @@ -134,6 +134,21 @@ strip_ufid () { >>>> sed 's/mega_ufid:[[-0-9a-f]]* // >>>> s/ufid:[[-0-9a-f]]* //' >>>> } >>>> + >>>> +# Strips packets: and bytes: from output >>>> +strip_stats () { >>>> + sed 's/packets:[[0-9]]*/packets:0/ >>>> + s/bytes:[[0-9]]*/bytes:0/' >>>> +} >>>> + >>>> +# Changes all 'recirc(...)' and 'recirc=...' to say 'recirc(<recirc_id>)' and >>>> +# 'recirc=<recirc_id>' respectively. This should make output easier to >>>> +# compare. >>>> +strip_recirc() { >>>> + sed 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/ >>>> + s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ >>>> + s/recirc([[x0-9]]*)/recirc(<recirc>)/' >>>> +} >>>> m4_divert_pop([PREPARE_TESTS]) >>>> >>>> m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) >>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>>> index d79753a99a..5d9449fada 100644 >>>> --- a/tests/system-traffic.at >>>> +++ b/tests/system-traffic.at >>>> @@ -6287,6 +6287,86 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE >>>> OVS_TRAFFIC_VSWITCHD_STOP >>>> AT_CLEANUP >>>> >>>> +AT_BANNER([IGMP]) >>>> + >>>> +AT_SETUP([IGMP - flood under normal action]) >>>> + >>>> +OVS_TRAFFIC_VSWITCHD_START() >>>> +ADD_NAMESPACES(at_ns0, at_ns1) >>>> + >>>> +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") >>>> +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") >>>> + >>>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"]) >>>> + >>>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 01 00 5e 01 01 03 dnl >>>> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >>>> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >>>> +00 00 00 00 > /dev/null]) >>>> + >>>> +AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl >>>> + br0: >>>> + br0 65534/1: (internal) >>> >>> This line is failing the system-userspace testsuite, because userspace >>> datapath reports 'tap' instead of 'internal'. Suggesting a following >>> change: >>> >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index 1ba28f7ee..7711f14b0 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -6681,18 +6681,11 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >>> 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >>> 00 00 00 00 > /dev/null]) >>> >>> -AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl >>> - br0: >>> - br0 65534/1: (internal) >>> - ovs-p1 1/2: (system) >>> - ovs-p2 2/3: (system) >>> -]) >>> - >>> -AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl >>> strip_stats | strip_used | strip_recirc | dnl >>> sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>> [0], [dnl >>> -recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1,3 >>> +recirc_id(<recirc>),in_port(ovs-p1),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:br0,ovs-p2 >>> ]) >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> --- >>> >>> What do you think? >> >> ACK - I'll pull that in. >> >>> >>>> + ovs-p1 1/2: (system) >>>> + ovs-p2 2/3: (system) >>>> +]) >>>> + >>>> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl >>>> + strip_stats | strip_used | strip_recirc | dnl >>>> + sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], >>>> + [0], [dnl >>>> +recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), >>>> packets:0, bytes:0, used:never, actions:1,3 >>>> +]) >>>> +OVS_TRAFFIC_VSWITCHD_STOP >>>> +AT_CLEANUP >>>> + >>>> +AT_SETUP([IGMP - forward with ICMP]) >>>> + >>>> +OVS_TRAFFIC_VSWITCHD_START() >>>> +ADD_NAMESPACES(at_ns0, at_ns1) >>>> + >>>> +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") >>>> +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") >>>> + >>>> +AT_DATA([flows.txt], [dnl >>>> +table=0, arp actions=NORMAL >>>> +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) >>>> +table=0, in_port=2 actions=output:1 >>>> +table=1, ip,ct_state=+trk+inv actions=drop >>>> +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 >>>> +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >>>> +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 >>>> +]) >>>> +AT_CHECK([ovs-ofctl del-flows br0]) >>>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>>> + >>>> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole >>>> + >>>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl >>>> +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >>>> +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >>>> +00 00 00 00 > /dev/null]) >>>> + >>>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl >>>> +f0 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a dnl >>>> +01 01 02 08 00 f7 ff ff ff ff ff > /dev/null]) >>>> + >>>> +sleep 1 >>>> + >>>> +AT_CHECK([ovs-ofctl dump-flows br0 | grep -v NXST | dnl >>>> + strip_duration | grep -v arp | grep -v n_packets=0 | dnl >>>> + grep -v 'in_port=2 actions=output:1' | dnl >>>> + sed 's/n_bytes=[[0-9]]*/n_bytes=0/ >>>> + s/idle_age=[[0-9]]*/idle_age=0/ >>>> + s/n_packets=[[1-9]]/n_packets=0/' | sort], [0], [dnl >>>> + cookie=0x0, table=0, n_packets=0, n_bytes=0, idle_age=0, >>>> ip,in_port=1 actions=ct(table=1,zone=64000) >>>> + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, >>>> ct_state=+new+trk,icmp,in_port=1 actions=output:2 >>>> + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, >>>> ct_state=+new+trk,ip,in_port=1 >>>> actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) >>>> +]) >>> >>> These are OpenFlow rules. Shouldn't we dump datapath flows, or >>> am I missing something? >> >> IIRC, we use the openflow rules because they are wildly different >> datapath rules between the two, so we rely on these rules being hit. >> Additionally, in the kernel DP case, if ipv6 is enabled then some ND >> will occur and that will tamper with n_packets/n_bytes counters (which >> is why I clear it before doing the comparison). >> >> I will look at the datapath flows again, but I believe the datapath >> action set is different because of the way controller() is implemented >> between the two (in the userspace DP, it can be a direct call, but in >> the kerneldp it needs to be a userspace call first). >> >> So I guess I will look at this, but expect to keep it as-is. > > OK. I guess, the main check here is n_packets=[[1-9]]. Is that correct? > Maybe just a comment would be enough in that case. It's not clear what > exactly we're testing here otherwise. Makes sense. Yes, that is the main check. Perhaps we should explicitly add a check like that as well. I'll see what I can do. > Best regards, Ilya Maximets.
diff --git a/lib/odp-util.c b/lib/odp-util.c index fbdfc7ad83..0f21ffe639 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -7132,11 +7132,6 @@ parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1], } } } - } else if (src_flow->nw_proto == IPPROTO_IGMP - && src_flow->dl_type == htons(ETH_TYPE_IP)) { - /* OVS userspace parses the IGMP type, code, and group, but its - * datapaths do not, so there is always missing information. */ - return ODP_FIT_TOO_LITTLE; } if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index cafcd014ad..29d26cf9ac 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3048,7 +3048,6 @@ xlate_normal(struct xlate_ctx *ctx) */ ctx->xout->slow |= SLOW_ACTION; - memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); if (mcast_snooping_is_membership(flow->tp_src) || mcast_snooping_is_query(flow->tp_src)) { if (ctx->xin->allow_side_effects && ctx->xin->packet) { diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at index 757cf7186e..e24f293a0c 100644 --- a/tests/mcast-snooping.at +++ b/tests/mcast-snooping.at @@ -216,3 +216,70 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl ]) AT_CLEANUP + + +AT_SETUP([mcast - igmp flood for non-snoop enabled]) +OVS_VSWITCHD_START([]) + +AT_CHECK([ + ovs-vsctl set bridge br0 \ + datapath_type=dummy], [0]) + +add_of_ports br0 1 2 + +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) + +ovs-appctl time/stop + +dnl Basic scenario - needs to flood for IGMP followed by unicast ICMP +dnl in reverse direction +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p2 \ + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) + + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl + strip_stats | strip_used | strip_recirc | dnl + sed -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], + [0], [dnl +recirc_id(<recirc>),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:100,2 +recirc_id(<recirc>),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1 +]) + +ovs-appctl time/warp 100000 + +dnl Next we should clear the flows and install a complex case +AT_CHECK([ovs-ofctl del-flows br0]) + +AT_DATA([flows.txt], [dnl +table=0, arp actions=NORMAL +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) +table=0, in_port=2 actions=output:1 +table=1, ip,ct_state=+trk+inv actions=drop +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +ovs-appctl time/warp 100000 + +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \ + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff']) + + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl + strip_stats | strip_used | strip_recirc | dnl + sed 's/pid=[[0-9]]*,// + s/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], + [0], [dnl +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), packets:0, bytes:0, used:never, actions:2 +ct_state(+new-inv+trk),recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), packets:0, bytes:0, used:never, actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=<recirc>,rule_cookie=0,controller_id=0,max_len=65535)) +recirc_id(<recirc>),in_port(1),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:0.0s, actions:ct(zone=64000),recirc(<recirc>) +]) + +AT_CLEANUP diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 736d9809cb..f906b5c3b5 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -134,6 +134,21 @@ strip_ufid () { sed 's/mega_ufid:[[-0-9a-f]]* // s/ufid:[[-0-9a-f]]* //' } + +# Strips packets: and bytes: from output +strip_stats () { + sed 's/packets:[[0-9]]*/packets:0/ + s/bytes:[[0-9]]*/bytes:0/' +} + +# Changes all 'recirc(...)' and 'recirc=...' to say 'recirc(<recirc_id>)' and +# 'recirc=<recirc_id>' respectively. This should make output easier to +# compare. +strip_recirc() { + sed 's/recirc_id([[x0-9]]*)/recirc_id(<recirc>)/ + s/recirc_id=[[x0-9]]*/recirc_id=<recirc>/ + s/recirc([[x0-9]]*)/recirc(<recirc>)/' +} m4_divert_pop([PREPARE_TESTS]) m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index d79753a99a..5d9449fada 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -6287,6 +6287,86 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | OFPROTO_CLEAR_DURATION_IDLE OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_BANNER([IGMP]) + +AT_SETUP([IGMP - flood under normal action]) + +OVS_TRAFFIC_VSWITCHD_START() +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") + +AT_CHECK([ovs-ofctl add-flow br0 "actions=NORMAL"]) + +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 01 00 5e 01 01 03 dnl +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl +00 00 00 00 > /dev/null]) + +AT_CHECK([ovs-appctl dpif/show | grep -v missed:], [0], [dnl + br0: + br0 65534/1: (internal) + ovs-p1 1/2: (system) + ovs-p2 2/3: (system) +]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | sort | dnl + strip_stats | strip_used | strip_recirc | dnl + sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], + [0], [dnl +recirc_id(<recirc>),in_port(2),eth(src=f0:00:00:01:01:01,dst=01:00:5e:01:01:03),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:1,3 +]) +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([IGMP - forward with ICMP]) + +OVS_TRAFFIC_VSWITCHD_START() +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p1, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01") +ADD_VETH(p2, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02") + +AT_DATA([flows.txt], [dnl +table=0, arp actions=NORMAL +table=0, ip,in_port=1 actions=ct(table=1,zone=64000) +table=0, in_port=2 actions=output:1 +table=1, ip,ct_state=+trk+inv actions=drop +table=1 ip,in_port=1,icmp,ct_state=+trk+new actions=output:2 +table=1, in_port=1,ip,ct_state=+trk+new actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) +table=1, in_port=1,ip,ct_state=+trk+est actions=output:2 +]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole + +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl +f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl +00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl +00 00 00 00 > /dev/null]) + +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p1 f0 00 00 01 01 02 dnl +f0 00 00 01 01 01 08 00 45 00 00 1c 00 01 00 00 40 01 64 dc 0a 01 01 01 0a dnl +01 01 02 08 00 f7 ff ff ff ff ff > /dev/null]) + +sleep 1 + +AT_CHECK([ovs-ofctl dump-flows br0 | grep -v NXST | dnl + strip_duration | grep -v arp | grep -v n_packets=0 | dnl + grep -v 'in_port=2 actions=output:1' | dnl + sed 's/n_bytes=[[0-9]]*/n_bytes=0/ + s/idle_age=[[0-9]]*/idle_age=0/ + s/n_packets=[[1-9]]/n_packets=0/' | sort], [0], [dnl + cookie=0x0, table=0, n_packets=0, n_bytes=0, idle_age=0, ip,in_port=1 actions=ct(table=1,zone=64000) + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,icmp,in_port=1 actions=output:2 + cookie=0x0, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,ip,in_port=1 actions=controller(userdata=00.de.ad.be.ef.ca.fe.01) +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_BANNER([802.1ad]) AT_SETUP([802.1ad - vlan_limit])
This reverts commit c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") Always forcing a slow path action can result in some over-broad flows which swallow all traffic and force them to userspace, as reported in the thread at https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html and at https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically. Additionally, remove the userspace wc mask to prevent revalidator from cycling flows. Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.") Signed-off-by: Aaron Conole <aconole@redhat.com> Acked-by: Eelco Chaudron <echaudro@redhat.com> --- v1->v2: Add unit test for coverage of mcast traffic when not doing snooping v2->v3: Add a unit test for the non-dummy datapaths as well to demonstrate the matching issue. v3->v4: Fix up the unit tests to be consistent when reporting dp flows. Split IGMP unit test into two different tests. Use a multicast eth addr for the FLOOD under NORMAL action case. lib/odp-util.c | 5 --- ofproto/ofproto-dpif-xlate.c | 1 - tests/mcast-snooping.at | 68 ++++++++++++++++++++++++++++++ tests/ofproto-macros.at | 15 +++++++ tests/system-traffic.at | 80 ++++++++++++++++++++++++++++++++++++ 5 files changed, 163 insertions(+), 6 deletions(-)