[ovs-dev] netdev-tc-offloads: Remove ingress qdisc on tc init flow api
diff mbox series

Message ID 1552314845-23758-1-git-send-email-roid@mellanox.com
State Accepted
Delegated to: Simon Horman
Headers show
Series
  • [ovs-dev] netdev-tc-offloads: Remove ingress qdisc on tc init flow api
Related show

Commit Message

Roi Dayan March 11, 2019, 2:34 p.m. UTC
It could be a port added to ovs bridge already has ingress qdisc
which will make the block probe fail.
The probes should start clean and ingress is being added later
so just remove ingress in case it exists.

Signed-off-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-tc-offloads.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

John Hurley March 11, 2019, 3:25 p.m. UTC | #1
On Mon, Mar 11, 2019 at 2:34 PM Roi Dayan <roid@mellanox.com> wrote:
>
> It could be a port added to ovs bridge already has ingress qdisc
> which will make the block probe fail.
> The probes should start clean and ingress is being added later
> so just remove ingress in case it exists.
>
> Signed-off-by: Roi Dayan <roid@mellanox.com>

Acked-by: John Hurley <john.hurley@netronome.com>

> ---
>  lib/netdev-tc-offloads.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index da43549f2f22..b33a79bb14a2 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -1527,6 +1527,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>          return -ifindex;
>      }
>
> +    /* make sure there is no ingress qdisc */
> +    tc_add_del_ingress_qdisc(ifindex, false, 0);
> +
>      if (ovsthread_once_start(&block_once)) {
>          probe_tc_block_support(ifindex);
>          ovsthread_once_done(&block_once);
> --
> 2.7.0
>
Simon Horman March 12, 2019, 3:05 p.m. UTC | #2
On Mon, Mar 11, 2019 at 03:25:09PM +0000, John Hurley wrote:
> On Mon, Mar 11, 2019 at 2:34 PM Roi Dayan <roid@mellanox.com> wrote:
> >
> > It could be a port added to ovs bridge already has ingress qdisc
> > which will make the block probe fail.
> > The probes should start clean and ingress is being added later
> > so just remove ingress in case it exists.
> >
> > Signed-off-by: Roi Dayan <roid@mellanox.com>
> 
> Acked-by: John Hurley <john.hurley@netronome.com>

Thanks,

I have applied this to master and branch-2.11.

It also appears to apply cleanly to branch-2.10, but currently travis-ci
fails on that branch so I'm seeing if I can get to the bottom of that.

If the patch is also appropriate for older branches please consider
supplying a backport.

> 
> > ---
> >  lib/netdev-tc-offloads.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> > index da43549f2f22..b33a79bb14a2 100644
> > --- a/lib/netdev-tc-offloads.c
> > +++ b/lib/netdev-tc-offloads.c
> > @@ -1527,6 +1527,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >          return -ifindex;
> >      }
> >
> > +    /* make sure there is no ingress qdisc */
> > +    tc_add_del_ingress_qdisc(ifindex, false, 0);
> > +
> >      if (ovsthread_once_start(&block_once)) {
> >          probe_tc_block_support(ifindex);
> >          ovsthread_once_done(&block_once);
> > --
> > 2.7.0
> >
Roi Dayan March 14, 2019, 8:50 a.m. UTC | #3
On 12/03/2019 17:05, Simon Horman wrote:
> On Mon, Mar 11, 2019 at 03:25:09PM +0000, John Hurley wrote:
>> On Mon, Mar 11, 2019 at 2:34 PM Roi Dayan <roid@mellanox.com> wrote:
>>>
>>> It could be a port added to ovs bridge already has ingress qdisc
>>> which will make the block probe fail.
>>> The probes should start clean and ingress is being added later
>>> so just remove ingress in case it exists.
>>>
>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>
>> Acked-by: John Hurley <john.hurley@netronome.com>
> 
> Thanks,
> 
> I have applied this to master and branch-2.11.
> 
> It also appears to apply cleanly to branch-2.10, but currently travis-ci
> fails on that branch so I'm seeing if I can get to the bottom of that.
> 
> If the patch is also appropriate for older branches please consider
> supplying a backport.
> 

The patches only needed because we might need ingress or shared block.
so we must first clean ingress in case it's available.

The shared block patches are merged in v2.10 but not v2.9.
In v2.9 we don't care if ingress already added to a port or not.
so should be good there.

Now thinking it, the issue might repeat itself if a port added
to a bridge might had shared block (any number).
Maybe we should clean also clean any ingress_block that might be
on that port.
or is it too much?


>>
>>> ---
>>>  lib/netdev-tc-offloads.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>>> index da43549f2f22..b33a79bb14a2 100644
>>> --- a/lib/netdev-tc-offloads.c
>>> +++ b/lib/netdev-tc-offloads.c
>>> @@ -1527,6 +1527,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>>>          return -ifindex;
>>>      }
>>>
>>> +    /* make sure there is no ingress qdisc */
>>> +    tc_add_del_ingress_qdisc(ifindex, false, 0);
>>> +
>>>      if (ovsthread_once_start(&block_once)) {
>>>          probe_tc_block_support(ifindex);
>>>          ovsthread_once_done(&block_once);
>>> --
>>> 2.7.0
>>>
Simon Horman March 14, 2019, 8:50 a.m. UTC | #4
On Tue, Mar 12, 2019 at 04:05:16PM +0100, Simon Horman wrote:
> On Mon, Mar 11, 2019 at 03:25:09PM +0000, John Hurley wrote:
> > On Mon, Mar 11, 2019 at 2:34 PM Roi Dayan <roid@mellanox.com> wrote:
> > >
> > > It could be a port added to ovs bridge already has ingress qdisc
> > > which will make the block probe fail.
> > > The probes should start clean and ingress is being added later
> > > so just remove ingress in case it exists.
> > >
> > > Signed-off-by: Roi Dayan <roid@mellanox.com>
> > 
> > Acked-by: John Hurley <john.hurley@netronome.com>
> 
> Thanks,
> 
> I have applied this to master and branch-2.11.
> 
> It also appears to apply cleanly to branch-2.10, but currently travis-ci
> fails on that branch so I'm seeing if I can get to the bottom of that.

branch-2.10 passes travis-ci testing once again and
I have gone ahead and applied this patch to that branch.

> If the patch is also appropriate for older branches please consider
> supplying a backport.

Any comment on this?

> 
> > 
> > > ---
> > >  lib/netdev-tc-offloads.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> > > index da43549f2f22..b33a79bb14a2 100644
> > > --- a/lib/netdev-tc-offloads.c
> > > +++ b/lib/netdev-tc-offloads.c
> > > @@ -1527,6 +1527,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> > >          return -ifindex;
> > >      }
> > >
> > > +    /* make sure there is no ingress qdisc */
> > > +    tc_add_del_ingress_qdisc(ifindex, false, 0);
> > > +
> > >      if (ovsthread_once_start(&block_once)) {
> > >          probe_tc_block_support(ifindex);
> > >          ovsthread_once_done(&block_once);
> > > --
> > > 2.7.0
> > >
Roi Dayan March 17, 2019, 5:58 a.m. UTC | #5
On 14/03/2019 10:50, Simon Horman wrote:
> On Tue, Mar 12, 2019 at 04:05:16PM +0100, Simon Horman wrote:
>> On Mon, Mar 11, 2019 at 03:25:09PM +0000, John Hurley wrote:
>>> On Mon, Mar 11, 2019 at 2:34 PM Roi Dayan <roid@mellanox.com> wrote:
>>>>
>>>> It could be a port added to ovs bridge already has ingress qdisc
>>>> which will make the block probe fail.
>>>> The probes should start clean and ingress is being added later
>>>> so just remove ingress in case it exists.
>>>>
>>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
>>>
>>> Acked-by: John Hurley <john.hurley@netronome.com>
>>
>> Thanks,
>>
>> I have applied this to master and branch-2.11.
>>
>> It also appears to apply cleanly to branch-2.10, but currently travis-ci
>> fails on that branch so I'm seeing if I can get to the bottom of that.
> 
> branch-2.10 passes travis-ci testing once again and
> I have gone ahead and applied this patch to that branch.
> 
>> If the patch is also appropriate for older branches please consider
>> supplying a backport.
> 
> Any comment on this?

I replied on this. did u get my reply?
I don't think older branches need this.

> 
>>
>>>
>>>> ---
>>>>  lib/netdev-tc-offloads.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>>>> index da43549f2f22..b33a79bb14a2 100644
>>>> --- a/lib/netdev-tc-offloads.c
>>>> +++ b/lib/netdev-tc-offloads.c
>>>> @@ -1527,6 +1527,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>>>>          return -ifindex;
>>>>      }
>>>>
>>>> +    /* make sure there is no ingress qdisc */
>>>> +    tc_add_del_ingress_qdisc(ifindex, false, 0);
>>>> +
>>>>      if (ovsthread_once_start(&block_once)) {
>>>>          probe_tc_block_support(ifindex);
>>>>          ovsthread_once_done(&block_once);
>>>> --
>>>> 2.7.0
>>>>
Simon Horman March 18, 2019, 9 a.m. UTC | #6
On Sun, Mar 17, 2019 at 05:58:47AM +0000, Roi Dayan wrote:
> 
> 
> On 14/03/2019 10:50, Simon Horman wrote:
> > On Tue, Mar 12, 2019 at 04:05:16PM +0100, Simon Horman wrote:
> >> On Mon, Mar 11, 2019 at 03:25:09PM +0000, John Hurley wrote:
> >>> On Mon, Mar 11, 2019 at 2:34 PM Roi Dayan <roid@mellanox.com> wrote:
> >>>>
> >>>> It could be a port added to ovs bridge already has ingress qdisc
> >>>> which will make the block probe fail.
> >>>> The probes should start clean and ingress is being added later
> >>>> so just remove ingress in case it exists.
> >>>>
> >>>> Signed-off-by: Roi Dayan <roid@mellanox.com>
> >>>
> >>> Acked-by: John Hurley <john.hurley@netronome.com>
> >>
> >> Thanks,
> >>
> >> I have applied this to master and branch-2.11.
> >>
> >> It also appears to apply cleanly to branch-2.10, but currently travis-ci
> >> fails on that branch so I'm seeing if I can get to the bottom of that.
> > 
> > branch-2.10 passes travis-ci testing once again and
> > I have gone ahead and applied this patch to that branch.
> > 
> >> If the patch is also appropriate for older branches please consider
> >> supplying a backport.
> > 
> > Any comment on this?
> 
> I replied on this. did u get my reply?
> I don't think older branches need this.

Thanks and sorry for not responding to your earlier reply.
I don't think any further action is required on this patch.

> >>>> ---
> >>>>  lib/netdev-tc-offloads.c | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> >>>> index da43549f2f22..b33a79bb14a2 100644
> >>>> --- a/lib/netdev-tc-offloads.c
> >>>> +++ b/lib/netdev-tc-offloads.c
> >>>> @@ -1527,6 +1527,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >>>>          return -ifindex;
> >>>>      }
> >>>>
> >>>> +    /* make sure there is no ingress qdisc */
> >>>> +    tc_add_del_ingress_qdisc(ifindex, false, 0);
> >>>> +
> >>>>      if (ovsthread_once_start(&block_once)) {
> >>>>          probe_tc_block_support(ifindex);
> >>>>          ovsthread_once_done(&block_once);
> >>>> --
> >>>> 2.7.0
> >>>>

Patch
diff mbox series

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index da43549f2f22..b33a79bb14a2 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -1527,6 +1527,9 @@  netdev_tc_init_flow_api(struct netdev *netdev)
         return -ifindex;
     }
 
+    /* make sure there is no ingress qdisc */
+    tc_add_del_ingress_qdisc(ifindex, false, 0);
+
     if (ovsthread_once_start(&block_once)) {
         probe_tc_block_support(ifindex);
         ovsthread_once_done(&block_once);