[ovs-dev] netdev-linux: Remove ingress qdisc before trying to add shared block
diff mbox series

Message ID 1552308428-31985-1-git-send-email-roid@mellanox.com
State Accepted
Delegated to: Simon Horman
Headers show
Series
  • [ovs-dev] netdev-linux: Remove ingress qdisc before trying to add shared block
Related show

Commit Message

Roi Dayan March 11, 2019, 12:47 p.m. UTC
Adding shared ingress block with ingress qdisc already exists results
in a failure. So remove the ingress qdisc first.
Also while at it log the slave name.

Signed-off-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-linux.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

John Hurley March 11, 2019, 3:24 p.m. UTC | #1
On Mon, Mar 11, 2019 at 12:47 PM Roi Dayan <roid@mellanox.com> wrote:
>
> Adding shared ingress block with ingress qdisc already exists results
> in a failure. So remove the ingress qdisc first.
> Also while at it log the slave name.
>
> Signed-off-by: Roi Dayan <roid@mellanox.com>

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

> ---
>  lib/netdev-linux.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 92cfb229d445..0d0d045f7c70 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -742,11 +742,14 @@ netdev_linux_update_lag(struct rtnetlink_change *change)
>                  lag->block_id = block_id;
>                  lag->node = shash_add(&lag_shash, change->ifname, lag);
>
> +                /* delete ingress block in case it exists */
> +                tc_add_del_ingress_qdisc(change->if_index, false, 0);
>                  /* LAG master is linux netdev so add slave to same block. */
>                  error = tc_add_del_ingress_qdisc(change->if_index, true,
>                                                   block_id);
>                  if (error) {
> -                    VLOG_WARN("failed to bind LAG slave to master's block");
> +                    VLOG_WARN("failed to bind LAG slave %s to master's block",
> +                              change->ifname);
>                      shash_delete(&lag_shash, lag->node);
>                      free(lag);
>                  }
> --
> 2.7.0
>
Simon Horman March 12, 2019, 3:05 p.m. UTC | #2
On Mon, Mar 11, 2019 at 03:24:47PM +0000, John Hurley wrote:
> On Mon, Mar 11, 2019 at 12:47 PM Roi Dayan <roid@mellanox.com> wrote:
> >
> > Adding shared ingress block with ingress qdisc already exists results
> > in a failure. So remove the ingress qdisc first.
> > Also while at it log the slave name.
> >
> > 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-linux.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 92cfb229d445..0d0d045f7c70 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -742,11 +742,14 @@ netdev_linux_update_lag(struct rtnetlink_change *change)
> >                  lag->block_id = block_id;
> >                  lag->node = shash_add(&lag_shash, change->ifname, lag);
> >
> > +                /* delete ingress block in case it exists */
> > +                tc_add_del_ingress_qdisc(change->if_index, false, 0);
> >                  /* LAG master is linux netdev so add slave to same block. */
> >                  error = tc_add_del_ingress_qdisc(change->if_index, true,
> >                                                   block_id);
> >                  if (error) {
> > -                    VLOG_WARN("failed to bind LAG slave to master's block");
> > +                    VLOG_WARN("failed to bind LAG slave %s to master's block",
> > +                              change->ifname);
> >                      shash_delete(&lag_shash, lag->node);
> >                      free(lag);
> >                  }
> > --
> > 2.7.0
> >
Ilya Maximets March 12, 2019, 4:21 p.m. UTC | #3
> On Mon, Mar 11, 2019 at 03:24:47PM +0000, John Hurley wrote:
>> On Mon, Mar 11, 2019 at 12:47 PM Roi Dayan <roid at mellanox.com> wrote:
>> >
>> > Adding shared ingress block with ingress qdisc already exists results
>> > in a failure. So remove the ingress qdisc first.
>> > Also while at it log the slave name.
>> >
>> > Signed-off-by: Roi Dayan <roid at mellanox.com>
>> 
>> Acked-by: John Hurley <john.hurley at 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.

Hi Simon,

You may found these patches useful:
  https://patchwork.ozlabs.org/project/openvswitch/list/?series=94247

It's for 2.10 and all the branches down.

Bets regards, Ilya Maximets.
Simon Horman March 14, 2019, 8:49 a.m. UTC | #4
On Tue, Mar 12, 2019 at 07:21:05PM +0300, Ilya Maximets wrote:
> > On Mon, Mar 11, 2019 at 03:24:47PM +0000, John Hurley wrote:
> >> On Mon, Mar 11, 2019 at 12:47 PM Roi Dayan <roid at mellanox.com> wrote:
> >> >
> >> > Adding shared ingress block with ingress qdisc already exists results
> >> > in a failure. So remove the ingress qdisc first.
> >> > Also while at it log the slave name.
> >> >
> >> > Signed-off-by: Roi Dayan <roid at mellanox.com>
> >> 
> >> Acked-by: John Hurley <john.hurley at 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.
> 
> Hi Simon,
> 
> You may found these patches useful:
>   https://patchwork.ozlabs.org/project/openvswitch/list/?series=94247
> 
> It's for 2.10 and all the branches down.

Thanks, that helps a lot.

With that series applied I have gone ahead and applied
this patch to branch-2.10.

Patch
diff mbox series

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 92cfb229d445..0d0d045f7c70 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -742,11 +742,14 @@  netdev_linux_update_lag(struct rtnetlink_change *change)
                 lag->block_id = block_id;
                 lag->node = shash_add(&lag_shash, change->ifname, lag);
 
+                /* delete ingress block in case it exists */
+                tc_add_del_ingress_qdisc(change->if_index, false, 0);
                 /* LAG master is linux netdev so add slave to same block. */
                 error = tc_add_del_ingress_qdisc(change->if_index, true,
                                                  block_id);
                 if (error) {
-                    VLOG_WARN("failed to bind LAG slave to master's block");
+                    VLOG_WARN("failed to bind LAG slave %s to master's block",
+                              change->ifname);
                     shash_delete(&lag_shash, lag->node);
                     free(lag);
                 }