[ovs-dev] [PATCH] netdev-offload-tc: Flush rules on ingress block when init tc flow api

Dmytro Linkin dmitrolin at mellanox.com
Tue Mar 24 09:42:13 UTC 2020


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 <dmitrolin at 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 <dmitrolin at mellanox.com>
> > >>>> Acked-by: Raed Salem <raeds at mellanox.com>
> > >>>> Acked-by: Roi Dayan <roid at 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.
> > 
> > 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


More information about the dev mailing list