[ovs-dev] [PATCH v2] Revert "odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP."
Eelco Chaudron
echaudro at redhat.com
Fri Nov 12 12:11:33 UTC 2021
On 9 Nov 2021, at 15:42, 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 at redhat.com>
> Acked-by: Eelco Chaudron <echaudro at redhat.com>
> ---
> v1: Add test case as requested
Aaron, are you sure you included all the tests, as this test does not fail without your patch applied. The problem occurred with snooping enabled, and I can remember you had a test for that as well.
>
> lib/odp-util.c | 5 ---
> ofproto/ofproto-dpif-xlate.c | 1 -
> tests/mcast-snooping.at | 72 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 72 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 9d336bc6a6..3a0c610276 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..478aced91d 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -216,3 +216,75 @@ 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])
> +
> +AT_CHECK([
> + ovs-vsctl add-port br0 p0 -- set Interface p0 type=dummy \
> + other-config:hwaddr=aa:55:aa:55:00:01 ofport_request=1 \
> + -- add-port br0 p1 \
> + -- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 ofport_request=2 \
> +], [0])
> +
> +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 p0 \
> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> + 'aa55aa5500010101000c29a008004500001c00010000400164dc0a0101010a0101020800f7ffffffffff'])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
> + sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
> + -e 's/ used:[[a-zA-Z0-9\.]]*,//' -e 's/pid=[[0-9]]*,//' dnl
> + -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
> + [0], [dnl
> +recirc_id(0),in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:01:00:0c:29:a0),eth_type(0x0800),ipv4(frag=no), actions:100,2
> +recirc_id(0),in_port(2),eth(src=01:01:00:0c:29:a0,dst=aa:55:aa:55:00:01),eth_type(0x0800),ipv4(frag=no), 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/stop
> +
> +dnl Send the IGMP, followed by a unicast ICMP - ensure we won't black hole
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 \
> + '0101000c29a0aa55aa550001080046c00028000040000102d3494565eb4ae0000016940400002200f9020000000104000000e00000fb000000000000'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 \
> + 'aa55aa550001aa55aa55000208004500001c00010000400164dc0a0101010a0101020800f7ffffffffff'])
> +
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -e .*ipv4 | dnl
> + sed -e 's/ packets:[[0-9]]*,//' -e 's/ bytes:[[0-9]]*,//' dnl
> + -e 's/ used:[[a-zA-Z0-9\.]]*,//' -e 's/pid=[[0-9]]*,//' dnl
> + -e 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
> + [0], [dnl
> +ct_state(+new-inv+trk),recirc_id(0x1),in_port(1),eth_type(0x0800),ipv4(proto=2,frag=no), actions:userspace(controller(reason=1,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535))
> +recirc_id(0),in_port(1),eth_type(0x0800),ipv4(frag=no), actions:ct(zone=64000),recirc(0x1)
> +ct_state(+new-inv+trk),recirc_id(0x1),in_port(1),eth_type(0x0800),ipv4(proto=1,frag=no), actions:2
> +])
> +
> +AT_CLEANUP
> --
> 2.31.1
More information about the dev
mailing list