On 3/3/20 4:29 PM, Dmytro Linkin wrote: > 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 <[email protected]> >>>> >>>> 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 <[email protected]> >>>> Acked-by: Raed Salem <[email protected]> >>>> Acked-by: Roi Dayan <[email protected]> >>>> --- >>>> 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.
Hmm. OK. Shouldn't we unconditionally flush rules from qdisk inside the tc_add_del_qdisc() if deletion is requested? From your explanation it seems like a prerequisite for that and qdisk will not be deleted if we will not flush rules. BTW, one thing in this patch that doesn't look good is that you're using block_id before probing the support. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
