[ovs-dev] [PATCH] ofproto-dpif-upcall: Don't put flows that datapath can't fully match.

Ilya Maximets i.maximets at ovn.org
Fri Apr 9 14:31:41 UTC 2021


On 12/3/20 1:24 PM, Hongzhi Guo wrote:
> As described in the two patches at the end of this message, the datapath
> cannot match all the fields that ovs supports.
> So some flows are processed only on the slow path and not on the datapath.
> Therefore, the flows whose parsing result is ODP_FIT_TOO_LITTLE don't
> need to be installed on the datapath.
> In handle_upcalls, flows of this type can be directly blocked to avoid
> a large number of "failed to put[create] (Invalid argument)" logs
> in some scenarios, for example, when a large number of IGMP control
> packets are received.
> 
> CC: Ben Pfaff <blp at ovn.org>
> Fixes: c645550bb249 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.")
> Fixes: 687bafbb8a79 ("ofproto-dpif-upcall: Slow path flows that datapath can't fully match.")
> 
> Signed-off-by: Hongzhi Guo <guohongzhi1 at huawei.com>
> ---
>  ofproto/ofproto-dpif-upcall.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index e022fde27..cadd1f059 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1581,6 +1581,11 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>          if (should_install_flow(udpif, upcall)) {
>              struct udpif_key *ukey = upcall->ukey;
>  
> +            /* OVS userspace solve it, no need to install this flow. */
> +            if (upcall->fitness == ODP_FIT_TOO_LITTLE) {
> +                continue;
> +            }
> +

ODP_FIT_TOO_LITTLE is already handled during flow translation by changing
actions to slow_path ones.  So, the flow should be installed to the datapath
without any problems and it will forward IGMP packets to userspace with the
SLOW_PATH_UPCALL.

However, there is a problem in the implementation of flow_put operation in
userspace datapath.  (I'm assuming that you're using userspace datapath,
because this should not affect kernel.)

dpif-netdev uses odp_flow_key_to_mask() to convert flow key back to flow
structure, but it fails the flow installation if fitness is not perfect,
but it should not do that.  Technically, ODP_FIT_ERROR indicates here a very
serious bug, because the flow key constructed by ofproto layer can not be
converted back to flow structure.  But ODP_FIT_TOO_LITTLE is not an issue
since it's already handled by modifying flow actions.  ODP_FIT_TOO_MUCH
should not be a problem the same way as it's not a problem for ofproto.

So, basically, we need to fix dpif_netdev_{flow,mask}_from_nlattrs() functions
by rejecting the flow only if fitness == ODP_FIT_ERROR.

There is one more place with the same bug, it's invocation of
parse_key_and_mask_to_match() inside dpif-netlink, that is used if hardware
offloading enabled.  It produces similar results, i.e. that IGMP flows can
not be installed to TC, but they could be successfully installed to the
usual kernel datapath on failover.
(basically, this part of the code was just copied from dpif-netdev, which
is not good.)

I agree that all this should be fixed, but it should be fixed by correcting
behavior of datapath interfaces (dpif-netdev, dpif-netlink).

What do you think?  Ben, do you have opinion on this?

Best regards, Ilya Maximets.


More information about the dev mailing list