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

[Ilya Maximets]

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.

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.