On Sun, Jun 14, 2020 at 1:31 PM Roi Dayan <[email protected]> wrote: > > > > On 2020-06-11 1:36 PM, [email protected] wrote: > > From: Tonghao Zhang <[email protected]> > > > > When ports were removed from bridge, we should flush the > > offload rules on the ports. The main reason is two factors: > > > > * The ports removed from bridge, will be managed by OvS, so > > flush the rules which installed by OvS. > > * If using the TC flower offload, for example, tc rules still > > are on the ports. And the information still are maintained by > > OvS, such as the mapping for tc and ufid. > > Then if adding the port to bridge and installing the rules > > to it again, *del_filter_and_ufid_mapping will be invoked, > > and delete the tc rule using tc handle which may not exist > > (offload init api flushed them.) on kernel or is used by other > > previous rules (if so, that rules will be deleted that is not > > we expected.). > > > > Cc: Simon Horman <[email protected]> > > Cc: Paul Blakey <[email protected]> > > Cc: Roi Dayan <[email protected]> > > Cc: Ben Pfaff <[email protected]> > > Cc: William Tu <[email protected]> > > Cc: Ilya Maximets <[email protected]> > > Tested-at: > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.com%2Fgithub%2Fovn-open-virtual-networks%2Fovs%2Fbuilds%2F170832624&data=02%7C01%7Croid%40mellanox.com%7Cf0bc38aa88074693105108d80df36979%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637274686378147436&sdata=Dlq4jnSclj0vXQoldO7Oq2Wwx%2BGEHjK%2FftPUDHnJcKM%3D&reserved=0 > > Co-authored-by: Wengang Hou <[email protected]> > > Signed-off-by: Tonghao Zhang <[email protected]> > > --- > > lib/netdev-offload.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > > index ab97a292ebac..964566caab1e 100644 > > --- a/lib/netdev-offload.c > > +++ b/lib/netdev-offload.c > > @@ -593,6 +593,7 @@ netdev_ports_remove(odp_port_t port_no, const struct > > dpif_class *dpif_class) > > data = netdev_ports_lookup(port_no, dpif_class); > > if (data) { > > dpif_port_destroy(&data->dpif_port); > > + netdev_flow_flush(data->netdev); /* flush offloaded rules. */ > > netdev_close(data->netdev); /* unref and possibly close */ > > hmap_remove(&port_to_netdev, &data->portno_node); > > hmap_remove(&ifindex_to_port, &data->ifindex_node); > > > Hi Roi Thanks for your review. The case is reproduced as below: # ovs-appctl dpctl/show system@ovs-system: lookups: hit:0 missed:0 lost:0 flows: 0 masks: hit:0 total:0 hit/pkt:0.00 port 0: ovs-system (internal) port 1: br-int (internal) port 2: enp130s0f0_0 port 3: enp130s0f0_1 port 4: vxlan_sys_4789 (vxlan: packet_type=ptap)
# cat /tmp/tmp-ovs-debug.sh set -x ovs-appctl dpctl/add-flow 'recirc_id(0),in_port(2),eth(dst=00:11:22:33:44:66),eth_type(0x0800),ipv4(dst=1.1.1.200)' 3 ovs-appctl dpctl/dump-flows ovs-vsctl del-port br-int enp130s0f0_0 tc filter show dev enp130s0f0_0 ingress ovs-appctl dpctl/dump-flows # sh /tmp/tmp-ovs-debug.sh + ovs-appctl dpctl/add-flow 'recirc_id(0),in_port(2),eth(dst=00:11:22:33:44:66),eth_type(0x0800),ipv4(dst=1.1.1.200)' 3 + ovs-appctl dpctl/dump-flows recirc_id(0),in_port(2),eth(dst=00:11:22:33:44:66),eth_type(0x0800),ipv4(dst=1.1.1.200), packets:0, bytes:0, used:0.020s, actions:3 + ovs-vsctl del-port br-int enp130s0f0_0 + tc filter show dev enp130s0f0_0 ingress filter protocol ip pref 2 flower chain 0 filter protocol ip pref 2 flower chain 0 handle 0x1 dst_mac 00:11:22:33:44:66 eth_type ipv4 dst_ip 1.1.1.200 in_hw in_hw_count 1 action order 1: mirred (Egress Redirect to device enp130s0f0_1) stolen index 1 ref 1 bind 1 cookie 40ca20750c4add5a9cc1c880ccb3d0e8 no_percpu used_hw_stats delayed + ovs-appctl dpctl/dump-flows The tc rules on port enp130s0f0_0 are not deleted, we can use the tc command to show them. > it looks ok but I wonder if it's redundant? > I tested and when I removed a port with tc rules on it, > all tc rules got deleted using the flow api flow del, > we get into netdev_tc_flow_del() for each flow. I didn't find the netdev_tc_flow_del was invoked while the port was deleting. The debug step is: 1. run the /tmp/tmp-ovs-debug.sh 2. and while gdb the ovs-vswitchd: gdb /root/local/openvswitch-master-dpdk/sbin/ovs-vswitchd -ex 'b netdev_tc_flow_del' -ex c --pid [ovs-vswitchd pid] the breakpoint was not triggered. > So you shouldn't have a tc rule left or ufid mapping. As I debug, the netdev_tc_flow_del was not invoked, and the ufids of flow still are in the mapping. When we add the port back and the install the rules again, the new rules(e.g. tc handle 10) may be deleted by the later new rules(which ufid mapping not deleted and the handle may be 10), via del_filter_and_ufid_mapping. > How did you test that you saw tc rules were still on the port? Yes, I test it using the tc command. -- Best regards, Tonghao _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
