Hi,
The last sleep can't be removed in my env. There are errors occasionlly.
The test procedure and the fail result are in my previous emails.
But if I add revalidator/purge twice, the test is passed. No error complaints.
From: Eelco Chaudron <echau...@redhat.com>
Date: 2023-03-08 15:28:05
To: Faicker Mo <faicker...@ucloud.cn>
Cc: d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
Subject: Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not exist>
>
>On 8 Mar 2023, at 3:34, Faicker Mo wrote:
>
>> Updated,
>> Under my env, the sleep time before revalidator/purge can't be shorten to
>> 0.2s or 0.5s.
>> It's not reasonable there exists a dp flow after revalidator/purge.
>
>What about my suggested change of removing the sleeps completely? Does it work
>for you, or is it not testing correctly?
>
>>
>> From: Faicker Mo <faicker...@ucloud.cn>
>> Date: 2023-03-06 12:22:48
>> To:Eelco Chaudron <echau...@redhat.com>
>> cc: d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
>> Subject: Re:Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not
>> exist
>> It failed at system-offloads-traffic.at:737 without the sleep before
>> revalidator/purge. There was a flow,
>>> 2023-03-06T02:17:13.245Z|00079|unixctl|DBG|received request
>>> dpctl/dump-flows[], id=0
>>> 2023-03-06T02:17:13.245Z|00080|unixctl|DBG|replying with success, id=0:
>>> "recirc_id(0),in_port(4),eth(src=f6:2d:a8:f7:d2:f7,dst=aa:1a:54:e9:c5:56),eth_type(0x0800),ipv4(frag=no),
>>> packe
>> ts:1, bytes:84, used:0.040s, actions:2
>>> "
>>
>> Tested,
>> The first sleep can be removed with msg filters.
>> The sleep before revalidator/purge can be shorten to 0.2s.
>>
>>
>>
>>
>>
>>
>> From: Eelco Chaudron <echau...@redhat.com>
>> Date: 2023-03-03 19:11:25
>> To: Faicker Mo <faicker...@ucloud.cn>
>> Cc: d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
>> Subject: Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not
>> exist>
>>>
>>> On 2 Mar 2023, at 8:57, 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
>>>> v8:
>>>> - Shorten the time of 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 7558812eb..ba18711cb 100644
>>>> --- a/tests/system-offloads-traffic.at
>>>> +++ b/tests/system-offloads-traffic.at
>>>> @@ -690,3 +690,57 @@
>>>> 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)
>>>> +
>>>> +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
>>>> +])
>>>> +sleep 1
>>>> +
>>>> +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"])
>>>> +
>>>> +sleep 1
>>>> +AT_CHECK([ovs-appctl revalidator/purge])
>>>> +
>>>> +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
>>>> +])
>>>> +
>>>> +sleep 1
>>>> +AT_CHECK([ovs-appctl revalidator/purge])
>>>> +
>>>> +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
>>>> +"])
>>>> +AT_CLEANUP
>>>
>>> The changes look fine, however, you still have a 3-second delay in your
>>> tests. Any specific reason for this?
>>>
>>> I tried removing them and I found that if I did I needed to add another log
>>> messages exclusion. With this I was able to still make it fail without your
>>> change, and get it to pass for 400 runs.
>>>
>>> Thought?
>>>
>>> //Eelco
>>>
>>> diff --git a/tests/system-offloads-traffic.at
>>> b/tests/system-offloads-traffic.at
>>> index ba18711cb..b2a402dec 100644
>>> --- a/tests/system-offloads-traffic.at
>>> +++ b/tests/system-offloads-traffic.at
>>> @@ -710,7 +710,7 @@ 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
>>> +#sleep 1
>>>
>>> dnl Delete and add interface ovs-p0/p0
>>> AT_CHECK([ip link del dev ovs-p0])
>>> @@ -721,7 +721,7 @@ 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 1
>>> +#sleep 1
>>> AT_CHECK([ovs-appctl revalidator/purge])
>>>
>>> dnl Generate flows to trigger the hmap expand once
>>> @@ -733,7 +733,7 @@ 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 1
>>> +#sleep 1
>>> AT_CHECK([ovs-appctl revalidator/purge])
>>>
>>> AT_CHECK([test $(ovs-appctl dpctl/dump-flows | grep -c "eth_type(0x0800)")
>>> -eq 0], [0], [ignore])
>>> @@ -742,5 +742,6 @@ 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
>>> +/failed to get ifindex for ovs-p0: No such device/d
>>> "])
>>> AT_CLEANUP
>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
