diff mbox series

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

Message ID 20200227152232.187518-1-roid@mellanox.com
State Accepted
Commit edc2055a2bf73258d5731a8f8853397190348b04
Headers show
Series [ovs-dev] netdev-offload-tc: Flush rules on ingress block when init tc flow api | expand

Commit Message

Roi Dayan Feb. 27, 2020, 3:22 p.m. UTC 
From: Dmytro Linkin <dmitrolin@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@mellanox.com>
Acked-by: Raed Salem <raeds@mellanox.com>
Acked-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-offload-tc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Roi Dayan March 3, 2020, 7:58 a.m. UTC | #1 
On 2020-02-27 5:22 PM, Roi Dayan wrote:
> From: Dmytro Linkin <dmitrolin@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@mellanox.com>
> Acked-by: Raed Salem <raeds@mellanox.com>
> Acked-by: Roi Dayan <roid@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?

Thanks,
Roi
Ilya Maximets March 3, 2020, 1:15 p.m. UTC | #2 
On 3/3/20 8:58 AM, Roi Dayan wrote:
> 
> 
> On 2020-02-27 5:22 PM, Roi Dayan wrote:
>> From: Dmytro Linkin <dmitrolin@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@mellanox.com>
>> Acked-by: Raed Salem <raeds@mellanox.com>
>> Acked-by: Roi Dayan <roid@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?

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.

What do you think?

Best regards, Ilya Maximets.
Dmytro Linkin March 3, 2020, 3:29 p.m. UTC | #3 
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@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@mellanox.com>
> >> Acked-by: Raed Salem <raeds@mellanox.com>
> >> Acked-by: Roi Dayan <roid@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.
Ilya Maximets March 3, 2020, 3:42 p.m. UTC | #4 
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@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@mellanox.com>
>>>> Acked-by: Raed Salem <raeds@mellanox.com>
>>>> Acked-by: Roi Dayan <roid@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.
Dmytro Linkin March 3, 2020, 5:37 p.m. UTC | #5 
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@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@mellanox.com>
> >>>> Acked-by: Raed Salem <raeds@mellanox.com>
> >>>> Acked-by: Roi Dayan <roid@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
Dmytro Linkin March 24, 2020, 9:42 a.m. UTC | #6 
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@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@mellanox.com>
> > >>>> Acked-by: Raed Salem <raeds@mellanox.com>
> > >>>> Acked-by: Roi Dayan <roid@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
Simon Horman March 24, 2020, 12:49 p.m. UTC | #7 
On Tue, Mar 24, 2020 at 11:42:13AM +0200, Dmytro Linkin wrote:
> 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@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@mellanox.com>
> > > >>>> Acked-by: Raed Salem <raeds@mellanox.com>
> > > >>>> Acked-by: Roi Dayan <roid@mellanox.com>

...

> Ping

Sorry for the delay.

This seems reasonable. I am running it through Travis CI and if
that succeeds I plan to push it to master, branch-2.13 and branch-2.12.

It did not seem to apply cleanly to branches for older releases
so please consider submitting backports if appropriate.

https://travis-ci.org/github/horms2/ovs/builds/666306585
https://travis-ci.org/github/horms2/ovs/builds/666306504
https://travis-ci.org/github/horms2/ovs/builds/666305789
Simon Horman March 25, 2020, 5:48 p.m. UTC | #8 
On Tue, Mar 24, 2020 at 01:49:37PM +0100, Simon Horman wrote:
> On Tue, Mar 24, 2020 at 11:42:13AM +0200, Dmytro Linkin wrote:
> > 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@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@mellanox.com>
> > > > >>>> Acked-by: Raed Salem <raeds@mellanox.com>
> > > > >>>> Acked-by: Roi Dayan <roid@mellanox.com>
> 
> ...
> 
> > Ping
> 
> Sorry for the delay.
> 
> This seems reasonable. I am running it through Travis CI and if
> that succeeds I plan to push it to master, branch-2.13 and branch-2.12.
> 
> It did not seem to apply cleanly to branches for older releases
> so please consider submitting backports if appropriate.
> 
> https://travis-ci.org/github/horms2/ovs/builds/666306585
> https://travis-ci.org/github/horms2/ovs/builds/666306504
> https://travis-ci.org/github/horms2/ovs/builds/666305789

Thanks again,

I have pushed this patch to master and branch-2.13.

Travis tells me that the patch does not compile cleanly when
applied to branch-2.12. Please consider supplying a backport if
appropriate.
diff mbox series

Patch

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) {