diff mbox series

[ovs-dev,v9] netdev-offload-tc: del ufid mapping if device not exist

Message ID AK2AvACPIyuVZQXpsNFcg4qb.1.1678850815028.Hmail.mocan@ucloud.cn
State Superseded
Headers show
Series [ovs-dev,v9] netdev-offload-tc: del ufid mapping if device not exist | expand

Checks

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

Commit Message

Faicker Mo March 15, 2023, 3:26 a.m. UTC 
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
---
 lib/netdev-offload-tc.c          |  3 +-
 tests/system-offloads-traffic.at | 54 ++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

Comments

Simon Horman March 29, 2023, 3:22 p.m. UTC | #1 
On Wed, Mar 15, 2023 at 11:26:55AM +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>

Hi,

I have run the test added by this patch without the code changes of this
patch, and verified that it fails reliably.

I have also run the test with the code changes in a for loop on on a low-end
Ubuntu 22.04 VM 34,500k times (for about 2 days).

Of those test runs, 17 failed, and the rest succeeded:
a success rate of 99.95% :)

As it's not entirely clear how reliable such a setup is
(or our expectations for it) [1], I think I am ok with this patch.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-March/403164.html

For the record, of the failures, 8 looked like this:

./system-offloads-traffic.at:803: check_logs "/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
"
--- /dev/null   2023-03-27 12:47:20.572000000 +0000
+++ /home/horms/ovs/tests/system-offloads-testsuite.dir/at-groups/12/stdout     2023-03-28 06:23:59.071853684 +0000
@@ -0,0 +1,8 @@
+2023-03-28T06:23:58.360Z|00001|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.360Z|00002|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.388Z|00003|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.388Z|00004|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.603Z|00001|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.611Z|00002|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.819Z|00003|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T06:23:58.823Z|00004|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
ovsdb-server.log:

Another 8 looked like this:

./system-offloads-traffic.at:803: check_logs "/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
"
--- /dev/null   2023-03-27 12:47:20.572000000 +0000
+++ /home/horms/ovs/tests/system-offloads-testsuite.dir/at-groups/12/stdout     2023-03-28 07:38:19.457642442 +0000
@@ -0,0 +1 @@
+2023-03-28T07:38:18.587Z|00001|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
ovsdb-server.log:
> 2023-03-28T07:38:17.713Z|00001|vlog|INFO|opened log file /home/horms/ovs/tests/system-offloads-testsuite.dir/012/ovsdb-server.log
> 2023-03-28T07:38:17.736Z|00002|ovsdb_server|INFO|ovsdb-server (Open vSwitch) 3.1.90
ovs-vswitchd.log:

And 1 looked like this:

./system-offloads-traffic.at:803: check_logs "/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
"
--- /dev/null   2023-03-27 12:47:20.572000000 +0000
+++ /home/horms/ovs/tests/system-offloads-testsuite.dir/at-groups/12/stdout     2023-03-28 01:59:56.526587195 +0000
@@ -0,0 +1,4 @@
+2023-03-28T01:59:56.050Z|00001|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T01:59:56.055Z|00002|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T01:59:56.262Z|00003|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
+2023-03-28T01:59:56.267Z|00004|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
ovsdb-server.log:
> 2023-03-28T01:59:54.878Z|00001|vlog|INFO|opened log file /home/horms/ovs/tests/system-offloads-testsuite.dir/012/ovsdb-server.log
> 2023-03-28T01:59:54.895Z|00002|ovsdb_server|INFO|ovsdb-server (Open vSwitch) 3.1.90
ovs-vswitchd.log:


> ---
> 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
> ---
>  lib/netdev-offload-tc.c          |  3 +-
>  tests/system-offloads-traffic.at | 54 ++++++++++++++++++++++++++++++++
>  2 files changed, 56 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 eb331d6ce..19dd7a966 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -750,3 +750,57 @@ 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
> +"])
> +AT_CLEANUP
> -- 
> 2.31.1
> 
> 
> 
> 
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Faicker Mo March 30, 2023, 3:13 a.m. UTC | #2 
Thanks for your testing.
Add the ignore msg "failed to offload flow" to OVS_TRAFFIC_VSWITCHD_STOP can pass the fail test.
I'll post an update later.

From: Simon Horman <simon.horman@corigine.com>
Date: 2023-03-29 23:22:17
To:  Faicker Mo <faicker.mo@ucloud.cn>
Cc:  dev@openvswitch.org,Eelco Chaudron <echaudro@redhat.com>,Ilya Maximets <i.maximets@ovn.org>
Subject: Re: [ovs-dev] [PATCH v9] netdev-offload-tc: del ufid mapping if device not exist>On Wed, Mar 15, 2023 at 11:26:55AM +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>
>
>Hi,
>
>I have run the test added by this patch without the code changes of this
>patch, and verified that it fails reliably.
>
>I have also run the test with the code changes in a for loop on on a low-end
>Ubuntu 22.04 VM 34,500k times (for about 2 days).
>
>Of those test runs, 17 failed, and the rest succeeded:
>a success rate of 99.95% :)
>
>As it's not entirely clear how reliable such a setup is
>(or our expectations for it) [1], I think I am ok with this patch.
>
>Reviewed-by: Simon Horman <simon.horman@corigine.com>
>
>[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-March/403164.html
>
>For the record, of the failures, 8 looked like this:
>
>./system-offloads-traffic.at:803: check_logs "/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
>"
>--- /dev/null   2023-03-27 12:47:20.572000000 +0000
>+++ /home/horms/ovs/tests/system-offloads-testsuite.dir/at-groups/12/stdout     2023-03-28 06:23:59.071853684 +0000
>@@ -0,0 +1,8 @@
>+2023-03-28T06:23:58.360Z|00001|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.360Z|00002|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.388Z|00003|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.388Z|00004|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.603Z|00001|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.611Z|00002|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.819Z|00003|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T06:23:58.823Z|00004|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>ovsdb-server.log:
>
>Another 8 looked like this:
>
>./system-offloads-traffic.at:803: check_logs "/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
>"
>--- /dev/null   2023-03-27 12:47:20.572000000 +0000
>+++ /home/horms/ovs/tests/system-offloads-testsuite.dir/at-groups/12/stdout     2023-03-28 07:38:19.457642442 +0000
>@@ -0,0 +1 @@
>+2023-03-28T07:38:18.587Z|00001|dpif_netlink(revalidator1)|ERR|failed to offload flow: Invalid argument: ovs-p0
>ovsdb-server.log:
>> 2023-03-28T07:38:17.713Z|00001|vlog|INFO|opened log file /home/horms/ovs/tests/system-offloads-testsuite.dir/012/ovsdb-server.log
>> 2023-03-28T07:38:17.736Z|00002|ovsdb_server|INFO|ovsdb-server (Open vSwitch) 3.1.90
>ovs-vswitchd.log:
>
>And 1 looked like this:
>
>./system-offloads-traffic.at:803: check_logs "/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
>"
>--- /dev/null   2023-03-27 12:47:20.572000000 +0000
>+++ /home/horms/ovs/tests/system-offloads-testsuite.dir/at-groups/12/stdout     2023-03-28 01:59:56.526587195 +0000
>@@ -0,0 +1,4 @@
>+2023-03-28T01:59:56.050Z|00001|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T01:59:56.055Z|00002|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T01:59:56.262Z|00003|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>+2023-03-28T01:59:56.267Z|00004|dpif_netlink(handler2)|ERR|failed to offload flow: Invalid argument: ovs-p0
>ovsdb-server.log:
>> 2023-03-28T01:59:54.878Z|00001|vlog|INFO|opened log file /home/horms/ovs/tests/system-offloads-testsuite.dir/012/ovsdb-server.log
>> 2023-03-28T01:59:54.895Z|00002|ovsdb_server|INFO|ovsdb-server (Open vSwitch) 3.1.90
>ovs-vswitchd.log:
>
>
>> ---
>> 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
>> ---
>>  lib/netdev-offload-tc.c          |  3 +-
>>  tests/system-offloads-traffic.at | 54 ++++++++++++++++++++++++++++++++
>>  2 files changed, 56 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 eb331d6ce..19dd7a966 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -750,3 +750,57 @@ 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
>> +"])
>> +AT_CLEANUP
>> -- 
>> 2.31.1
>> 
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Simon Horman March 30, 2023, 5:56 a.m. UTC | #3 
On Thu, Mar 30, 2023 at 11:13:32AM +0800, Faicker Mo wrote:
> Thanks for your testing.
> Add the ignore msg "failed to offload flow" to OVS_TRAFFIC_VSWITCHD_STOP can pass the fail test.
> I'll post an update later.

Thanks. I'll look out for it.

> From: Simon Horman <simon.horman@corigine.com>
> Date: 2023-03-29 23:22:17
> To:  Faicker Mo <faicker.mo@ucloud.cn>
> Cc:  dev@openvswitch.org,Eelco Chaudron <echaudro@redhat.com>,Ilya Maximets <i.maximets@ovn.org>
> Subject: Re: [ovs-dev] [PATCH v9] netdev-offload-tc: del ufid mapping if device not exist>On Wed, Mar 15, 2023 at 11:26:55AM +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>
> >
> >Hi,
> >
> >I have run the test added by this patch without the code changes of this
> >patch, and verified that it fails reliably.
> >
> >I have also run the test with the code changes in a for loop on on a low-end
> >Ubuntu 22.04 VM 34,500k times (for about 2 days).

Sorry there is a typo here. It should be 34,500 (no k). I.e. just under 35k.

...
diff mbox series

Patch

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 eb331d6ce..19dd7a966 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -750,3 +750,57 @@  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
+"])
+AT_CLEANUP