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
