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

Aaron Conole aconole at redhat.com
Mon Nov 8 12:54:01 UTC 2021


Ilya Maximets <i.maximets at ovn.org> writes:

> On 11/1/21 15:47, 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
>
> Hey, Aaron.  Thanks for the patch!
>
> I think, we need to have unit tests for described issues included in
> this patch.  This over-board match seems to be a serious issue and
> we need to be sure that it's really fixed and will not happen again.

Thanks - yes you're right.  I will post a v2 with the tests.

>> 
>> Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically.
>> Additionally, remove the userspace wc mask to prevent revalidator from
>> cycling flows.
>
> This also looks like a good candidate for a unit test as we need to
> be sure that we're not re-introducing the issue that the original commit
> tried to fix.

Makes sense.

> Best regards, Ilya Maximets.
>
>> 
>> Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.")
>> Signed-off-by: Aaron Conole <aconole at redhat.com>
>> ---
>>  lib/odp-util.c               | 5 -----
>>  ofproto/ofproto-dpif-xlate.c | 1 -
>>  2 files changed, 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) {
>> 



More information about the dev mailing list