Message ID | 20201203122425.21640-1-guohongzhi1@huawei.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev] ofproto-dpif-upcall: Don't put flows that datapath can't fully match. | expand |
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@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@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.
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; + } + if (ukey_install(udpif, ukey)) { upcall->ukey_persists = true; put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE);
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@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@huawei.com> --- ofproto/ofproto-dpif-upcall.c | 5 +++++ 1 file changed, 5 insertions(+)