On Tue, Mar 03, 2020 at 02:15:21PM +0100, Ilya Maximets wrote: > On 3/3/20 8:58 AM, Roi Dayan wrote: > > > > > > On 2020-02-27 5:22 PM, Roi Dayan wrote: > >> From: Dmytro Linkin <dmitro...@mellanox.com> > >> > >> OVS can fail to attach ingress block on iface when init tc flow api, > >> if block already exist with rules on it and is shared with other iface. > >> Fix by flush all existing rules on the ingress block prior to deleting > >> it. > >> > >> Fixes: 093c9458fb02 ("tc: allow offloading of block ids") > >> Signed-off-by: Dmytro Linkin <dmitro...@mellanox.com> > >> Acked-by: Raed Salem <ra...@mellanox.com> > >> Acked-by: Roi Dayan <r...@mellanox.com> > >> --- > >> lib/netdev-offload-tc.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > >> index 550e440b3a45..b5ff6ccca55e 100644 > >> --- a/lib/netdev-offload-tc.c > >> +++ b/lib/netdev-offload-tc.c > >> @@ -1907,6 +1907,7 @@ netdev_tc_init_flow_api(struct netdev *netdev) > >> static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER; > >> enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev); > >> uint32_t block_id = 0; > >> + struct tcf_id id; > >> int ifindex; > >> int error; > >> > >> @@ -1917,6 +1918,14 @@ netdev_tc_init_flow_api(struct netdev *netdev) > >> return -ifindex; > >> } > >> > >> + block_id = get_block_id_from_netdev(netdev); > >> + > >> + /* Flush rules explicitly needed when we work with ingress_block, > >> + * so we will not fail with reattaching block to bond iface, for ex. > >> + */ > >> + id = tc_make_tcf_id(ifindex, block_id, 0, hook); > >> + tc_del_filter(&id); > >> + > >> /* make sure there is no ingress/egress qdisc */ > >> tc_add_del_qdisc(ifindex, false, 0, hook); > >> > >> @@ -1930,7 +1939,6 @@ netdev_tc_init_flow_api(struct netdev *netdev) > >> ovsthread_once_done(&multi_mask_once); > >> } > >> > >> - block_id = get_block_id_from_netdev(netdev); > >> error = tc_add_del_qdisc(ifindex, true, block_id, hook); > >> > >> if (error && error != EEXIST) { > >> > > > > +ilya > > > > Hi Ilya, > > > > can you help review/ack this? > > Hi. I'm not an expert in linux tc, but since you're asking... > > IIUC, the issue is that tc_add_del_qdisc(ifindex, true, block_id, hook) > fails on bond iface. Is it correct?
At first tc_add_del_qdisc() fails on deletion, because qdisc, which is added to block, is in use (rules are exist). We just don't care about any error returned. Then tc_add_del_qdisc() fail to add it, because it wasn't deleted and in use. > In this case I see one strange thing. We're clearing ingress qdisk > for block_id == 0, but after that trying to create new one with > block_id == ifindex (for LAG interface). Will it help if we delete > ingress qdisk providing correct block_id? This sounds like something > sane to do. Deleting block_id != 0 will fail, due to existing rules. But actually, deleting qdisc with block_id == 0 still delete correct block. Anyway, the point here is to flush rules on specified ingress block. Regards, Dmytro. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev