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

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>
---
 lib/netdev-offload-tc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
2.31.1

On 12 Jan 2023, at 7:55, 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>
> ---
>  lib/netdev-offload-tc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index ce7f8ad97..e731badc0 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -240,8 +240,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid)
>      int err;
>
>      err = tc_del_filter(id);
> -    if (!err) {
> +    if (!err || err == ENODEV) {
>          del_ufid_tc_mapping(ufid);
> +        return 0;
>      }
>      return err;
>  }
> --

The change by itself looks ok, but can you also add a test case for this in system-offloads-traffic.at?

//Eelco

From b8ec4ac672e7ef3370d52bae66db7eef7b8d9da6 Mon Sep 17 00:00:00 2001From: Faicker Mo <faicker.mo@ucloud.cn>
Date: Thu, 12 Jan 2023 11:55:37 +0800
Subject: [PATCH] netdev-offload-tc: del ufid mapping if device not exist.


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>
---
 lib/netdev-offload-tc.c          |  3 ++-
 tests/system-offloads-traffic.at | 45 ++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)


diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97..e731badc0 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -240,8 +240,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid)
     int err;
 
     err = tc_del_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 1a6057080..0e9d6ad15 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -678,3 +678,48 @@ 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
+#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
+])
+
+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"])
+AT_CLEANUP

On 30 Jan 2023, at 9:29, Faicker Mo wrote:

> From b8ec4ac672e7ef3370d52bae66db7eef7b8d9da6 Mon Sep 17 00:00:00 2001From: Faicker Mo <faicker.mo@ucloud.cn>
> Date: Thu, 12 Jan 2023 11:55:37 +0800
> Subject: [PATCH] netdev-offload-tc: del ufid mapping if device not exist.
>
>
> 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.

Thanks for adding the test case. Some comments below based on a visual review only.

Your subject should contain the version of the patch, i.e. [PATCH v2] “netdev-offload-tc: del ufid mapping if device not exist”

You should also not include any fragments of the previous thread in your email, and the “Re:Re” in your subject line.

Cheers,

Eelco


>
> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
> ---

Here you should mention what changes you have made from v1 to v2.

>  lib/netdev-offload-tc.c          |  3 ++-
>  tests/system-offloads-traffic.at | 45 ++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
>
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index ce7f8ad97..e731badc0 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -240,8 +240,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid)
>      int err;
>
>      err = tc_del_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 1a6057080..0e9d6ad15 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -678,3 +678,48 @@ 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])

offloads - delete ufid mapping if device does 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

Sart all comments on a new line and a Capital, also add a dot at the and.

> +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

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”

> +#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
> +#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
> +])
> +
> +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"])
> +AT_CLEANUP
> -- 
> 2.31.1

<Everything below this should not be in this email>

>
> From: Eelco Chaudron <echaudro@redhat.com>
> Date: 2023-01-17 20:09:07
> To:  Faicker Mo <faicker.mo@ucloud.cn>
> Cc:  dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] netdev-offload-tc: del ufid mapping if device not exist>
>>
>> On 12 Jan 2023, at 7:55, 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>
>>> ---
>>>  lib/netdev-offload-tc.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index ce7f8ad97..e731badc0 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -240,8 +240,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid)
>>>      int err;
>>>
>>>      err = tc_del_filter(id);
>>> -    if (!err) {
>>> +    if (!err || err == ENODEV) {
>>>          del_ufid_tc_mapping(ufid);
>>> +        return 0;
>>>      }
>>>      return err;
>>>  }
>>> --
>>
>> The change by itself looks ok, but can you also add a test case for this in system-offloads-traffic.at?
>>
>> //Eelco
>>