| Message ID | AC2AzADpI-4p-yFCovMSbqrt.1.1680168443047.Hmail.mocan@ucloud.cn |
|---|---|
| State | Accepted |
| Commit | f9507c1ea4343efddbc5c5425c8938b74d3a6260 |
| Headers | show |
| Series | [ovs-dev,v10] netdev-offload-tc: del ufid mapping if device not exist. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Thu, Mar 30, 2023 at 05:27:23PM +0800, 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.mo@ucloud.cn> Thanks, I am running this through the test I described in [1] - a tight loop on a low-end VM. So far things look good, but the loop has not run many times yet. I plan to let it run over the weekend.
On 30 Mar 2023, at 11:27, 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.mo@ucloud.cn> Changes look good to me, so if Simon’s tests pass over the weekend: Acked-by: Eelco Chaudron <echaudro@redhat.com> <SNIP> > +]) > +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 > +]) > + > +AT_CHECK([ovs-appctl revalidator/purge], [0]) > +dnl Fix purge fail occasionally FYI, I noticed when debugging some other TC-related tests that the reason could be that the purge is called before the TC flows get installed in the kernel. > +AT_CHECK([ovs-appctl revalidator/purge], [0]) <SNIP>
On Fri, Mar 31, 2023 at 03:22:44PM +0200, Eelco Chaudron wrote: > > > On 30 Mar 2023, at 11:27, 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.mo@ucloud.cn> > > Changes look good to me, so if Simon’s tests pass over the weekend: > > Acked-by: Eelco Chaudron <echaudro@redhat.com> The weekend came and the weekend went. I checked just now and the loop has run the test 73014 times without recording any failures. I think we are good here :) Reviewed-by: Simon Horman <simon.horman@corigine.com> Tested-by: Simon Horman <simon.horman@corigine.com> > <SNIP> > > > +]) > > +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 > > +]) > > + > > +AT_CHECK([ovs-appctl revalidator/purge], [0]) > > +dnl Fix purge fail occasionally > > FYI, I noticed when debugging some other TC-related tests that the reason could be that the purge is called before the TC flows get installed in the kernel. > > > +AT_CHECK([ovs-appctl revalidator/purge], [0]) > > <SNIP> >
On 4/3/23 13:00, Simon Horman wrote: > On Fri, Mar 31, 2023 at 03:22:44PM +0200, Eelco Chaudron wrote: >> >> >> On 30 Mar 2023, at 11:27, 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.mo@ucloud.cn> >> >> Changes look good to me, so if Simon’s tests pass over the weekend: >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > > The weekend came and the weekend went. > I checked just now and the loop has run the test 73014 times > without recording any failures. > > I think we are good here :) > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Tested-by: Simon Horman <simon.horman@corigine.com> Thanks! Applied. Backported down to 2.17. Best regards, Ilya Maximets.
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 4721f0160..c9662081f 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 eb331d6ce..da18597cd 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -750,3 +750,58 @@ AT_CHECK([ovs-appctl coverage/read-counter ukey_invalid_stat_reset], [0], [dnl 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) + +dnl Disable IPv6 to skip unexpected 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 +]) + +dnl 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"]) + +AT_CHECK([ovs-appctl revalidator/purge], [0]) + +dnl Generate flows to trigger the hmap expand once +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 +]) + +AT_CHECK([ovs-appctl revalidator/purge], [0]) +dnl Fix purge fail occasionally +AT_CHECK([ovs-appctl revalidator/purge], [0]) + +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 +/failed to flow_get/d +/Failed to acquire udpif_key/d +/No such device/d +/failed to offload flow/d +"]) +AT_CLEANUP
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.mo@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 v8: - Shorten the time of the test case v9: - Remove sleep in the test case v10: - Improve the success rate of the test case --- lib/netdev-offload-tc.c | 3 +- tests/system-offloads-traffic.at | 55 ++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-)