On 13 Mar 2023, at 8:15, Faicker Mo wrote:
> 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.
Guess it would be interesting to find out why the flows are not yet deleted
(maybe not added) before the purge call.
Maybe waiting for one revalidation round, before doing the purge might work,
i.e. ovs-appctl revalidator/wait? Ilya any other ideas?
I tried to replicate your issue, but after 500 runs I do not see it.
//Eelco
> From: Eelco Chaudron <[email protected]>
> Date: 2023-03-08 15:28:05
> To: Faicker Mo <[email protected]>
> Cc: [email protected],[email protected],[email protected]
> 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 <[email protected]>
>>> Date: 2023-03-06 12:22:48
>>> To:Eelco Chaudron <[email protected]>
>>> cc: [email protected],[email protected],[email protected]
>>> 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 <[email protected]>
>>> Date: 2023-03-03 19:11:25
>>> To: Faicker Mo <[email protected]>
>>> Cc: [email protected],[email protected],[email protected]
>>> 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 <[email protected]>
>>>>> ---
>>>>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
