[ovs-dev] [PATCH v2] Revert "odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP."

Aaron Conole aconole at redhat.com
Fri Nov 12 17:27:55 UTC 2021


Eelco Chaudron <echaudro at redhat.com> writes:

> 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.

It seems that dummy datapath doesn't exhibit the problem, or I broke
something when porting.  I'll make sure that's fixed in v3.  Thanks for
the heads up.

>>
>>  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