On Tue, Mar 03, 2020 at 07:37:45PM +0200, Dmytro Linkin wrote: > On Tue, Mar 03, 2020 at 04:42:12PM +0100, Ilya Maximets wrote: > > 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. > > Flushing rules needed only for shared block, 'cause like in our case, > there are ifaces not attached to the bridge, so OVS don't ask kernel to > delete qdisc. In other known cases kernel flush rules by itself on qdisc > deletion, so flush inside tc_add_del_qdisc() mostly not needed. > Mb, better do it for shared blocks only. > > > BTW, one thing in this patch that doesn't look good is that you're > > using block_id before probing the support. > > Probably yes. What You suggest here? By case, it work. Probing triggers > on internal OVS ifaces at first, so later calls to get block_id return > valid id. > > Regards, Dmytro
Ping _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
