It's great! I should have read the comment. A new patch will be published soon.
From: Ilya Maximets <i.maxim...@ovn.org> Date: 2023-03-01 19:34:01 To: Faicker Mo <faicker...@ucloud.cn> Cc: d...@openvswitch.org,i.maxim...@ovn.org,Eelco Chaudron <echau...@redhat.com>,Simon Horman <simon.hor...@corigine.com> Subject: Re: [ovs-dev] [PATCH v7] netdev-offload-tc: del ufid mapping if device not exist>On 3/1/23 03:12, Faicker Mo wrote: >> The device may be deleted and added with ifindex changed. >> The tc rules on the device will be deleted if the device is deleted. >> The func tc_del_filter will fail when flow del. The mapping of >> ufid to tc will not be deleted. >> The traffic will trigger the same flow(with same ufid) to put to tc >> on the new device. Duplicated ufid mapping will be added. >> If the hashmap is expanded, the old mapping entry will be the first entry, >> and now the dp flow can't be deleted. >> >> >> Signed-off-by: Faicker Mo <faicker...@ucloud.cn> >> --- >> v2: >> - Add tc offload test case >> v3: >> - No change >> v4: >> - No change >> v5: >> - No change >> v6: >> - No change >> v7: >> - Minor fix for test case and rebased >> --- >> lib/netdev-offload-tc.c | 3 ++- >> tests/system-offloads-traffic.at | 46 ++++++++++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+), 1 deletion(-) >> >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 4fb9d9f21..dd2020cad 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -276,8 +276,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const >> ovs_u128 *ufid, >> } >> >> err = tc_del_flower_filter(id); >> - if (!err) { >> + if (!err || err == ENODEV) { >> del_ufid_tc_mapping(ufid); >> + return 0; >> } >> return err; >> } >> diff --git a/tests/system-offloads-traffic.at >> b/tests/system-offloads-traffic.at >> index 7558812eb..9b2f12ddb 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -690,3 +690,49 @@ >> OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(5),le(check_pkt_len(size=100,gt(5), >> >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> + >> +AT_SETUP([offloads - delete ufid mapping if device not exist - offloads >> enabled]) >> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . >> other_config:hw-offload=true]) >> + >> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> + >> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2) >> +# avoid unexpected ipv6 flow >> +AT_CHECK([sysctl -w net.ipv6.conf.br0.disable_ipv6=1], [0], [ignore]) >> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], >> [ignore]) >> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], >> [ignore]) >> +NS_CHECK_EXEC([at_ns2], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], >> [ignore]) >> + >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "aa:1a:54:e9:c5:56") >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> + >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], >> [dnl >> +2 packets transmitted, 2 received, 0% packet loss, time 0ms >> +]) >> +sleep 1 >> + >> +#delete and add interface ovs-p0/p0 >> +AT_CHECK([ip link del dev ovs-p0]) >> +AT_CHECK([ip link add p0 type veth peer name ovs-p0 || return 77]) >> +AT_CHECK([ip link set p0 netns at_ns0]) >> +AT_CHECK([ip link set dev ovs-p0 up]) >> +NS_CHECK_EXEC([at_ns0], [ip addr add dev p0 "10.1.1.1/24"]) >> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 up]) >> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address "aa:1a:54:e9:c5:56"]) >> + >> +sleep 12 > >Hi. AFAICT, this sleep is added to wait for flow expiration in the datapath. >But it's a very large seep time. We should be able to avoid it. > >I see the comment on the very first version of this patch from Eelco: > >" > I did not test the script, but the (long) sleeps are not really > desired. Why are they needed, and can they maybe be replaced with > “ovs-appctl revalidator/wait|purge” >" > >But I didn't find a reply to it. > >Did you try using 'revalidator/purge' appctl here instead? At least for >the first 12 second sleep. The second one seems to be more specific on >what it is trying to do, while the first one is just waiting for removal >of flows for the initial random traffic. > >If nothing can be done, then we should decrease the 'max-idle' parameter >from the default 10 seconds to something more reasonable, so revalidators >will remove flows faster. > >> +#flows to trigger the hmap expand once > >nit: 'dnl' is a better way of adding comments to the test (instead of '#'). > >> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24") >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], >> [dnl >> +2 packets transmitted, 2 received, 0% packet loss, time 0ms >> +]) >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.3 | FORMAT_PING], [0], >> [dnl >> +2 packets transmitted, 2 received, 0% packet loss, time 0ms >> +]) >> + >> +sleep 12 >> +AT_CHECK([test $(ovs-appctl dpctl/dump-flows | grep -c "eth_type(0x0800)") >> -eq 0], [0], [ignore]) >> + >> +OVS_TRAFFIC_VSWITCHD_STOP(["/could not open network device ovs-p0/d >> +/on nonexistent port/d"]) >> +AT_CLEANUP > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev