Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
On Thu, Feb 23, 2023 at 10:24:54AM +0800, Faicker Mo wrote: > I can run the fail-test more easier. > There exists a flow in verbose log like this, > recirc_id(0),in_port(2),eth(src=aa:1a:54:e9:c5:56,dst=86:29:2a:05:94:90),eth_type(0x0800),ipv4(frag=no), > packets:1, bytes:84, used:12.240s, actions:3 > The env is strict. The hmap should be expanded exactly only once.The flow > number needs to be controlled. I'm not sure that I follow. But if you can improve the test that would be good. An update from my side: Since my previous email I've run the test, with this patch and -v (previously was without -v) 1333 times, and counting. So far, no failures. > By the way, should I updated the patch to v7 in here because I found it's > need to be changed when I rebased to the newest master? Yes, please do. I was going to mention this, but forgot to. That would be: * v7: rebased ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
I can run the fail-test more easier.
There exists a flow in verbose log like this,
recirc_id(0),in_port(2),eth(src=aa:1a:54:e9:c5:56,dst=86:29:2a:05:94:90),eth_type(0x0800),ipv4(frag=no),
packets:1, bytes:84, used:12.240s, actions:3
The env is strict. The hmap should be expanded exactly only once.The flow
number needs to be controlled.
By the way, should I updated the patch to v7 in here because I found it's need
to be changed when I rebased to the newest master?
From: Simon Horman
Date: 2023-02-23 01:12:46
To: Faicker Mo
Cc: d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device
not exist>On Wed, Feb 22, 2023 at 04:19:37PM +0100, Simon Horman wrote:
>> On Wed, Feb 22, 2023 at 06:33:50PM +0800, Faicker Mo wrote:
>> > It's not easy to add a fail test without the changed code.
>> > But I test it failed with the old code manually following these steps,
>> > 1. Apply this patch(with test in it)
>> > 2. Revert the changed code in netdev-offload-tc.c
>> > 3. Run the test
>> >
>> >
>> > Yes, the fail-test above sometimes may pass because of the env and the
>> > chance.Maybe run the fail-test several times.
>>
>> Thanks, I see this now.
>>
>> * Without the C-code changes in this patch I saw the test fail 3 times,
>> each time within 5 attempts.
>>
>> * With the C-code change I am yet to see the test fail,
>> so far I'm up to 80 attempts.
>
>It did eventually fail on the 257th attempt.
>
>I'll try again with '-v' and see if I can capture anything useful.
>
>> I do wonder if this warrants a Fixes tag.
>> And if so, if it should be:
>>
>> Fixes: 262a07956fab ("netdev-tc-offloads: Delete ufid tc mapping in the
>> right place")
>>
>> That notwithstanding, I am happy with this patch.
>>
>> Reviewed-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
On Wed, Feb 22, 2023 at 04:19:37PM +0100, Simon Horman wrote:
> On Wed, Feb 22, 2023 at 06:33:50PM +0800, Faicker Mo wrote:
> > It's not easy to add a fail test without the changed code.
> > But I test it failed with the old code manually following these steps,
> > 1. Apply this patch(with test in it)
> > 2. Revert the changed code in netdev-offload-tc.c
> > 3. Run the test
> >
> >
> > Yes, the fail-test above sometimes may pass because of the env and the
> > chance.Maybe run the fail-test several times.
>
> Thanks, I see this now.
>
> * Without the C-code changes in this patch I saw the test fail 3 times,
> each time within 5 attempts.
>
> * With the C-code change I am yet to see the test fail,
> so far I'm up to 80 attempts.
It did eventually fail on the 257th attempt.
I'll try again with '-v' and see if I can capture anything useful.
> I do wonder if this warrants a Fixes tag.
> And if so, if it should be:
>
> Fixes: 262a07956fab ("netdev-tc-offloads: Delete ufid tc mapping in the right
> place")
>
> That notwithstanding, I am happy with this patch.
>
> Reviewed-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
On Wed, Feb 22, 2023 at 06:33:50PM +0800, Faicker Mo wrote:
> It's not easy to add a fail test without the changed code.
> But I test it failed with the old code manually following these steps,
> 1. Apply this patch(with test in it)
> 2. Revert the changed code in netdev-offload-tc.c
> 3. Run the test
>
>
> Yes, the fail-test above sometimes may pass because of the env and the
> chance.Maybe run the fail-test several times.
Thanks, I see this now.
* Without the C-code changes in this patch I saw the test fail 3 times,
each time within 5 attempts.
* With the C-code change I am yet to see the test fail,
so far I'm up to 80 attempts.
I do wonder if this warrants a Fixes tag.
And if so, if it should be:
Fixes: 262a07956fab ("netdev-tc-offloads: Delete ufid tc mapping in the right
place")
That notwithstanding, I am happy with this patch.
Reviewed-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
It's not easy to add a fail test without the changed code. But I test it failed with the old code manually following these steps, 1. Apply this patch(with test in it) 2. Revert the changed code in netdev-offload-tc.c 3. Run the test Yes, the fail-test above sometimes may pass because of the env and the chance.Maybe run the fail-test several times. From: Simon Horman Date: 2023-02-22 17:07:18 To: Faicker Mo Cc: d...@openvswitch.org Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist>On Wed, Feb 22, 2023 at 10:03:07AM +0800, Faicker Mo wrote: >> Sorry. >> The commit message and code are not changed. >> Resended when I met a bug of intel-ovs-compilation test fail and add version >> descriptions. > >Thanks, I think I understand now. >But please don't top-post on this mailing list. > >And could you please look at my comment regarding >the test you have added in this patch. > >Thanks! > >> From: Simon Horman >> Date: 2023-02-21 23:09:05 >> To: Faicker Mo >> Cc: d...@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if >> device not exist>On Wed, Feb 01, 2023 at 10:49:22AM +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 >> >> --- >> >> v2: >> >> - Add tc offload test case >> >> v3: >> >> - No change >> >> v4: >> >> - No change >> >> v5: >> >> - No change >> >> v6: >> >> - No change >> > >> >I am confused. >> >Why are there 4 versions (v3 - v6) with no change? >> >What does that mean? > >... > >> >> diff --git a/tests/system-offloads-traffic.at >> >> b/tests/system-offloads-traffic.at >> >> index 16a4c1a00..15ea549a6 100644 >> >> --- a/tests/system-offloads-traffic.at >> >> +++ b/tests/system-offloads-traffic.at >> > >> >The test seems to pass both with and without the change to >> >del_filter_and_ufid_mapping(). I think it would be better to construct a >> >test that fails without the code change and succeeds with it. >> > >> >I ran: >> > >> >TESTSUITEFLAGS="-k ufid" make check-offloads > > >This comment, here. > >... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
On Wed, Feb 22, 2023 at 10:03:07AM +0800, Faicker Mo wrote: > Sorry. > The commit message and code are not changed. > Resended when I met a bug of intel-ovs-compilation test fail and add version > descriptions. Thanks, I think I understand now. But please don't top-post on this mailing list. And could you please look at my comment regarding the test you have added in this patch. Thanks! > From: Simon Horman > Date: 2023-02-21 23:09:05 > To: Faicker Mo > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if > device not exist>On Wed, Feb 01, 2023 at 10:49:22AM +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 > >> --- > >> v2: > >> - Add tc offload test case > >> v3: > >> - No change > >> v4: > >> - No change > >> v5: > >> - No change > >> v6: > >> - No change > > > >I am confused. > >Why are there 4 versions (v3 - v6) with no change? > >What does that mean? ... > >> diff --git a/tests/system-offloads-traffic.at > >> b/tests/system-offloads-traffic.at > >> index 16a4c1a00..15ea549a6 100644 > >> --- a/tests/system-offloads-traffic.at > >> +++ b/tests/system-offloads-traffic.at > > > >The test seems to pass both with and without the change to > >del_filter_and_ufid_mapping(). I think it would be better to construct a > >test that fails without the code change and succeeds with it. > > > >I ran: > > > >TESTSUITEFLAGS="-k ufid" make check-offloads This comment, here. ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
Sorry.
The commit message and code are not changed.
Resended when I met a bug of intel-ovs-compilation test fail and add version
descriptions.
From: Simon Horman
Date: 2023-02-21 23:09:05
To: Faicker Mo
Cc: d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device
not exist>On Wed, Feb 01, 2023 at 10:49:22AM +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
>> ---
>> v2:
>> - Add tc offload test case
>> v3:
>> - No change
>> v4:
>> - No change
>> v5:
>> - No change
>> v6:
>> - No change
>
>I am confused.
>Why are there 4 versions (v3 - v6) with no change?
>What does that mean?
>
>> ---
>> 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 6e1bbaa28..a4e8818ab 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_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 16a4c1a00..15ea549a6 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>
>The test seems to pass both with and without the change to
>del_filter_and_ufid_mapping(). I think it would be better to construct a
>test that fails without the code change and succeeds with it.
>
>I ran:
>
>TESTSUITEFLAGS="-k ufid" make check-offloads
>
>> @@ -680,3 +680,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
>> --
>> 2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device not exist
On Wed, Feb 01, 2023 at 10:49:22AM +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
> ---
> v2:
> - Add tc offload test case
> v3:
> - No change
> v4:
> - No change
> v5:
> - No change
> v6:
> - No change
I am confused.
Why are there 4 versions (v3 - v6) with no change?
What does that mean?
> ---
> 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 6e1bbaa28..a4e8818ab 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_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 16a4c1a00..15ea549a6 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
The test seems to pass both with and without the change to
del_filter_and_ufid_mapping(). I think it would be better to construct a
test that fails without the code change and succeeds with it.
I ran:
TESTSUITEFLAGS="-k ufid" make check-offloads
> @@ -680,3 +680,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
> --
> 2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
