On 30 Jan 2023, at 9:29, Faicker Mo wrote:
> From b8ec4ac672e7ef3370d52bae66db7eef7b8d9da6 Mon Sep 17 00:00:00 2001From: > Faicker Mo <[email protected]> > 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 <[email protected]> > --- 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 <[email protected]> > Date: 2023-01-17 20:09:07 > To: Faicker Mo <[email protected]> > Cc: [email protected] > 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 <[email protected]> >>> --- >>> 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 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
