[PATCH net 0/3] Bonding: fix high prio not effect issue

[PATCH net 0/3] Bonding: fix high prio not effect issue

[Hangbin Liu]

When a high prio link up, if there has current link, it will not do
failover as we missed the check in link up event. Fix it in this patchset
and add a prio option test case.

Hangbin Liu (2):
  bonding: access curr_active_slave with rtnl_dereference
  bonding: do failover when high prio link up

Liang Li (1):
  selftests: bonding: add bonding prio option test

 drivers/net/bonding/bond_main.c               |   6 +-
 .../selftests/drivers/net/bonding/Makefile    |   3 +-
 .../drivers/net/bonding/option_prio.sh        | 246 ++++++++++++++++++
 3 files changed, 252 insertions(+), 3 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/option_prio.sh

-- 
2.38.1

[PATCH net 1/3] bonding: access curr_active_slave with rtnl_dereference

[Hangbin Liu]

Looks commit 4740d6382790 ("bonding: add proper __rcu annotation for
curr_active_slave") missed rtnl_dereference for curr_active_slave
in bond_miimon_commit().

Fixes: 4740d6382790 ("bonding: add proper __rcu annotation for curr_active_slave")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9a882f182d2..2b6cc4dbb70e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2689,7 +2689,7 @@ static void bond_miimon_commit(struct bonding *bond)
 
 			bond_miimon_link_change(bond, slave, BOND_LINK_UP);
 
-			if (!bond->curr_active_slave || slave == primary)
+			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)
 				goto do_failover;
 
 			continue;
-- 
2.38.1

Re: [PATCH net 1/3] bonding: access curr_active_slave with rtnl_dereference

[Saeed Mahameed]

On 09 Dec 18:13, Hangbin Liu wrote:
>Looks commit 4740d6382790 ("bonding: add proper __rcu annotation for
>curr_active_slave") missed rtnl_dereference for curr_active_slave
>in bond_miimon_commit().
>
>Fixes: 4740d6382790 ("bonding: add proper __rcu annotation for curr_active_slave")
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Saeed Mahameed <saeed@kernel.org>

Re: [PATCH net 1/3] bonding: access curr_active_slave with rtnl_dereference

[Eric Dumazet]

On Fri, Dec 9, 2022 at 11:13 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Looks commit 4740d6382790 ("bonding: add proper __rcu annotation for
> curr_active_slave") missed rtnl_dereference for curr_active_slave
> in bond_miimon_commit().
>
> Fixes: 4740d6382790 ("bonding: add proper __rcu annotation for curr_active_slave")


> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b9a882f182d2..2b6cc4dbb70e 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2689,7 +2689,7 @@ static void bond_miimon_commit(struct bonding *bond)
>
>                         bond_miimon_link_change(bond, slave, BOND_LINK_UP);
>
> -                       if (!bond->curr_active_slave || slave == primary)
> +                       if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)

We do not dereference the pointer here.

If this is fixing a sparse issue, then use the correct RCU helper for this.

( rcu_access_pointer())

Re: [PATCH net 1/3] bonding: access curr_active_slave with rtnl_dereference

[Hangbin Liu]

On Sat, Dec 10, 2022 at 12:58:59AM +0100, Eric Dumazet wrote:
> On Fri, Dec 9, 2022 at 11:13 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> >
> > Looks commit 4740d6382790 ("bonding: add proper __rcu annotation for
> > curr_active_slave") missed rtnl_dereference for curr_active_slave
> > in bond_miimon_commit().
> >
> > Fixes: 4740d6382790 ("bonding: add proper __rcu annotation for curr_active_slave")
> 
> 
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  drivers/net/bonding/bond_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index b9a882f182d2..2b6cc4dbb70e 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -2689,7 +2689,7 @@ static void bond_miimon_commit(struct bonding *bond)
> >
> >                         bond_miimon_link_change(bond, slave, BOND_LINK_UP);
> >
> > -                       if (!bond->curr_active_slave || slave == primary)
> > +                       if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)
> 
> We do not dereference the pointer here.
> 
> If this is fixing a sparse issue, then use the correct RCU helper for this.
> 
> ( rcu_access_pointer())

Hmm... I saw in 4740d6382790 ("bonding: add proper __rcu annotation for
 curr_active_slave") there are also some dereference like that. Should I also
fix them at the same time? e.g.

@@ -2607,8 +2612,8 @@ static void bond_ab_arp_commit(struct bonding *bond)

                case BOND_LINK_UP:
                        trans_start = dev_trans_start(slave->dev);
-                       if (bond->curr_active_slave != slave ||
-                           (!bond->curr_active_slave &&
+                       if (rtnl_dereference(bond->curr_active_slave) != slave ||
+                           (!rtnl_dereference(bond->curr_active_slave) &&

Thanks
Hangbin

Re: [PATCH net 1/3] bonding: access curr_active_slave with rtnl_dereference

[Eric Dumazet]

On Sat, Dec 10, 2022 at 1:28 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Sat, Dec 10, 2022 at 12:58:59AM +0100, Eric Dumazet wrote:
> > On Fri, Dec 9, 2022 at 11:13 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > >
> > > Looks commit 4740d6382790 ("bonding: add proper __rcu annotation for
> > > curr_active_slave") missed rtnl_dereference for curr_active_slave
> > > in bond_miimon_commit().
> > >
> > > Fixes: 4740d6382790 ("bonding: add proper __rcu annotation for curr_active_slave")
> >
> >
> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > > ---
> > >  drivers/net/bonding/bond_main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index b9a882f182d2..2b6cc4dbb70e 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -2689,7 +2689,7 @@ static void bond_miimon_commit(struct bonding *bond)
> > >
> > >                         bond_miimon_link_change(bond, slave, BOND_LINK_UP);
> > >
> > > -                       if (!bond->curr_active_slave || slave == primary)
> > > +                       if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)
> >
> > We do not dereference the pointer here.
> >
> > If this is fixing a sparse issue, then use the correct RCU helper for this.
> >
> > ( rcu_access_pointer())
>
> Hmm... I saw in 4740d6382790 ("bonding: add proper __rcu annotation for
>  curr_active_slave") there are also some dereference like that. Should I also
> fix them at the same time? e.g.

There is no 'fix' really. I do not see any reason to change this part.

It is merely a matter of repeating or not the fact that RTNL (or
another lock) is held.

>
> @@ -2607,8 +2612,8 @@ static void bond_ab_arp_commit(struct bonding *bond)
>
>                 case BOND_LINK_UP:
>                         trans_start = dev_trans_start(slave->dev);
> -                       if (bond->curr_active_slave != slave ||
> -                           (!bond->curr_active_slave &&
> +                       if (rtnl_dereference(bond->curr_active_slave) != slave ||
> +                           (!rtnl_dereference(bond->curr_active_slave) &&
>

At the time of commit  4740d6382790 we wanted to make sure the
bond->curr_slave_lock
was taken, because it was the assertion at that time.

Then later, bond_deref_active_protected() has been removed, because
curr_slave_lock has been removed.

$ git log --oneline --reverse
b25bd2515ea32cf5ddd5fd5a2a93b8c9dd875e4f..8c0bc550288d81e9ad8a2ed9136a72140b9ef507
86e749866d7c6b0ee1f9377cf7142f2690596a05 bonding: 3ad: clean up
curr_slave_lock usage
62c5f5185397f4bd8e5defe6fcb86420deeb2b38 bonding: alb: remove curr_slave_lock
1c72cfdc96e63bf975cab514c4ca4d8a661ba0e6 bonding: clean curr_slave_lock use
b743562819bd97cc7c282e870896bae8016b64b5 bonding: convert
curr_slave_lock to a spinlock and rename it
4bab16d7c97498e91564231b922d49f52efaf7d4 bonding: alb: convert to
bond->mode_lock
e470259fa1bd7ce5a375b16c5ec97cc0e83b058d bonding: 3ad: convert to
bond->mode_lock
8c0bc550288d81e9ad8a2ed9136a72140b9ef507 bonding: adjust locking comments


Now, you post a patch for bond_miimon_commit() which already has :

if (slave == rcu_access_pointer(bond->curr_active_slave))
      goto do_failover;

So really it is a matter of consistency in _this_ function, which is
run under RTNL for sure.

It is also a patch for net-next tree, because it fixes no bug.

I would not add a Fixes: tag to avoid dealing with useless backports.

Re: [PATCH net 1/3] bonding: access curr_active_slave with rtnl_dereference

[Hangbin Liu]

On Sat, Dec 10, 2022 at 06:41:01PM +0100, Eric Dumazet wrote:
> Now, you post a patch for bond_miimon_commit() which already has :
> 
> if (slave == rcu_access_pointer(bond->curr_active_slave))
>       goto do_failover;
> 
> So really it is a matter of consistency in _this_ function, which is
> run under RTNL for sure.

Ah, thanks for the explanation.

> It is also a patch for net-next tree, because it fixes no bug.
> 
> I would not add a Fixes: tag to avoid dealing with useless backports.

OK, I will do as your suggest.

Cheers
Hangbin

[PATCH net 2/3] bonding: do failover when high prio link up

[Hangbin Liu]

Currently, when a high prio link enslaved, or when current link down,
the high prio port could be selected. But when high prio link up, the
new active slave reselection is not triggered. Fix it by checking link's
prio when getting up.

Reported-by: Liang Li <liali@redhat.com>
Fixes: 0a2ff7cc8ad4 ("Bonding: add per-port priority for failover re-selection")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2b6cc4dbb70e..dc6af790ff1e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2689,7 +2689,8 @@ static void bond_miimon_commit(struct bonding *bond)
 
 			bond_miimon_link_change(bond, slave, BOND_LINK_UP);
 
-			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)
+			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary ||
+			    slave->prio > rtnl_dereference(bond->curr_active_slave)->prio)
 				goto do_failover;
 
 			continue;
@@ -3550,7 +3551,8 @@ static void bond_ab_arp_commit(struct bonding *bond)
 				slave_info(bond->dev, slave->dev, "link status definitely up\n");
 
 				if (!rtnl_dereference(bond->curr_active_slave) ||
-				    slave == rtnl_dereference(bond->primary_slave))
+				    slave == rtnl_dereference(bond->primary_slave) ||
+				    slave->prio > rtnl_dereference(bond->curr_active_slave)->prio)
 					goto do_failover;
 
 			}
-- 
2.38.1

Re: [PATCH net 2/3] bonding: do failover when high prio link up

[Saeed Mahameed]

On 09 Dec 18:13, Hangbin Liu wrote:
>Currently, when a high prio link enslaved, or when current link down,
>the high prio port could be selected. But when high prio link up, the
>new active slave reselection is not triggered. Fix it by checking link's
>prio when getting up.
>
>Reported-by: Liang Li <liali@redhat.com>
>Fixes: 0a2ff7cc8ad4 ("Bonding: add per-port priority for failover re-selection")
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 2b6cc4dbb70e..dc6af790ff1e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2689,7 +2689,8 @@ static void bond_miimon_commit(struct bonding *bond)
>
> 			bond_miimon_link_change(bond, slave, BOND_LINK_UP);
>
>-			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)
>+			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary ||
>+			    slave->prio > rtnl_dereference(bond->curr_active_slave)->prio)
> 				goto do_failover;

I am not really familiar with this prio logic, seems to be new. 
Anyway, what if one of the next slaves has higher prio than this slave and the
current active ? 
I see that the loop over all the slaves continues even after the failover,
but why would you do all these failovers until you settle on the highest
prio one ? 

shouldn't you do something similar to bond_choose_primary_or_current()
outside the loop, once you've updated all the slaves link states 

Please let me know if I am wandering in the wrong directions
Anyway, LGTM:

Reviewed-by: Saeed Mahameed <saeed@kernel.org>

Re: [PATCH net 2/3] bonding: do failover when high prio link up

[Hangbin Liu]

On Fri, Dec 09, 2022 at 04:03:35PM -0800, Saeed Mahameed wrote:
> On 09 Dec 18:13, Hangbin Liu wrote:
> > Currently, when a high prio link enslaved, or when current link down,
> > the high prio port could be selected. But when high prio link up, the
> > new active slave reselection is not triggered. Fix it by checking link's
> > prio when getting up.
> > 
> > Reported-by: Liang Li <liali@redhat.com>
> > Fixes: 0a2ff7cc8ad4 ("Bonding: add per-port priority for failover re-selection")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> > drivers/net/bonding/bond_main.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 2b6cc4dbb70e..dc6af790ff1e 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -2689,7 +2689,8 @@ static void bond_miimon_commit(struct bonding *bond)
> > 
> > 			bond_miimon_link_change(bond, slave, BOND_LINK_UP);
> > 
> > -			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary)
> > +			if (!rtnl_dereference(bond->curr_active_slave) || slave == primary ||
> > +			    slave->prio > rtnl_dereference(bond->curr_active_slave)->prio)
> > 				goto do_failover;
> 
> I am not really familiar with this prio logic, seems to be new. Anyway, what
> if one of the next slaves has higher prio than this slave and the
> current active ? I see that the loop over all the slaves continues even
> after the failover,
> but why would you do all these failovers until you settle on the highest
> prio one ?

Thanks, this makes sense to me. I will fix it.

Hangbin
> 
> shouldn't you do something similar to bond_choose_primary_or_current()
> outside the loop, once you've updated all the slaves link states
> 
> Please let me know if I am wandering in the wrong directions
> Anyway, LGTM:
> 
> Reviewed-by: Saeed Mahameed <saeed@kernel.org>
> 
> 
> 
> 
> 
> 
> 

[PATCH net 3/3] selftests: bonding: add bonding prio option test

[Hangbin Liu]

From: Liang Li <liali@redhat.com>

Add a test for bonding prio option. Here is the test result:

]# ./option_prio.sh
TEST: prio_test (Test bonding option 'prio' with mode=1 monitor=arp_ip_target and primary_reselect=0)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=1 monitor=arp_ip_target and primary_reselect=1)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=1 monitor=arp_ip_target and primary_reselect=2)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=1 monitor=miimon and primary_reselect=0)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=1 monitor=miimon and primary_reselect=1)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=1 monitor=miimon and primary_reselect=2)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=5 monitor=miimon and primary_reselect=0)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=5 monitor=miimon and primary_reselect=1)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=5 monitor=miimon and primary_reselect=2)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=6 monitor=miimon and primary_reselect=0)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=6 monitor=miimon and primary_reselect=1)  [ OK ]
TEST: prio_test (Test bonding option 'prio' with mode=6 monitor=miimon and primary_reselect=2)  [ OK ]

Signed-off-by: Liang Li <liali@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../selftests/drivers/net/bonding/Makefile    |   3 +-
 .../drivers/net/bonding/option_prio.sh        | 246 ++++++++++++++++++
 2 files changed, 248 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/option_prio.sh

diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile
index 6b8d2e2f23c2..82250dd7a25d 100644
--- a/tools/testing/selftests/drivers/net/bonding/Makefile
+++ b/tools/testing/selftests/drivers/net/bonding/Makefile
@@ -5,7 +5,8 @@ TEST_PROGS := \
 	bond-arp-interval-causes-panic.sh \
 	bond-break-lacpdu-tx.sh \
 	bond-lladdr-target.sh \
-	dev_addr_lists.sh
+	dev_addr_lists.sh \
+	option_prio.sh
 
 TEST_FILES := \
 	lag_lib.sh \
diff --git a/tools/testing/selftests/drivers/net/bonding/option_prio.sh b/tools/testing/selftests/drivers/net/bonding/option_prio.sh
new file mode 100755
index 000000000000..669a660a69b9
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/option_prio.sh
@@ -0,0 +1,246 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test bonding option prio
+#
+
+ALL_TESTS="
+	prio_arp_ip_target_test
+	prio_miimon_test
+"
+
+REQUIRE_MZ=no
+REQUIRE_JQ=no
+NUM_NETIFS=0
+lib_dir=$(dirname "$0")
+source "$lib_dir"/net_forwarding_lib.sh
+
+destroy()
+{
+	ip link del bond0 &>/dev/null
+	ip link del br0 &>/dev/null
+	ip link del veth0 &>/dev/null
+	ip link del veth1 &>/dev/null
+	ip link del veth2 &>/dev/null
+	ip netns del ns1 &>/dev/null
+	ip link del veth3 &>/dev/null
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	destroy
+}
+
+skip()
+{
+        local skip=1
+	ip link add name bond0 type bond mode 1 miimon 100 &>/dev/null
+	ip link add name veth0 type veth peer name veth0_p
+	ip link set veth0 master bond0
+
+	# check if iproute support prio option
+	ip link set dev veth0 type bond_slave prio 10
+	[[ $? -ne 0 ]] && skip=0
+
+	# check if bonding support prio option
+	ip -d link show veth0 | grep -q "prio 10"
+	[[ $? -ne 0 ]] && skip=0
+
+	ip link del bond0 &>/dev/null
+	ip link del veth0
+
+	return $skip
+}
+
+active_slave=""
+check_active_slave()
+{
+	local target_active_slave=$1
+	active_slave="$(cat /sys/class/net/bond0/bonding/active_slave)"
+	test "$active_slave" = "$target_active_slave"
+	check_err $? "Current active slave is $active_slave but not $target_active_slave"
+}
+
+
+# Test bonding prio option with mode=$mode monitor=$monitor
+# and primary_reselect=$primary_reselect
+prio_test()
+{
+	RET=0
+
+	local monitor=$1
+	local mode=$2
+	local primary_reselect=$3
+
+	local bond_ip4="192.169.1.2"
+	local peer_ip4="192.169.1.1"
+	local bond_ip6="2009:0a:0b::02"
+	local peer_ip6="2009:0a:0b::01"
+
+
+	# create veths
+	ip link add name veth0 type veth peer name veth0_p
+	ip link add name veth1 type veth peer name veth1_p
+	ip link add name veth2 type veth peer name veth2_p
+
+	# create bond
+	if [[ "$monitor" == "miimon" ]];then
+		ip link add name bond0 type bond mode $mode miimon 100 primary veth1 primary_reselect $primary_reselect
+	elif [[ "$monitor" == "arp_ip_target" ]];then
+		ip link add name bond0 type bond mode $mode arp_interval 1000 arp_ip_target $peer_ip4 primary veth1 primary_reselect $primary_reselect
+	elif [[ "$monitor" == "ns_ip6_target" ]];then
+		ip link add name bond0 type bond mode $mode arp_interval 1000 ns_ip6_target $peer_ip6 primary veth1 primary_reselect $primary_reselect
+	fi
+	ip link set bond0 up
+	ip link set veth0 master bond0
+	ip link set veth1 master bond0
+	ip link set veth2 master bond0
+	# check bonding member prio value
+	ip link set dev veth0 type bond_slave prio 0
+	ip link set dev veth1 type bond_slave prio 10
+	ip link set dev veth2 type bond_slave prio 11
+	ip -d link show veth0 | grep -q 'prio 0'
+	check_err $? "veth0 prio is not 0"
+	ip -d link show veth1 | grep -q 'prio 10'
+	check_err $? "veth0 prio is not 10"
+	ip -d link show veth2 | grep -q 'prio 11'
+	check_err $? "veth0 prio is not 11"
+
+	ip link set veth0 up
+	ip link set veth1 up
+	ip link set veth2 up
+	ip link set veth0_p up
+	ip link set veth1_p up
+	ip link set veth2_p up
+
+	# prepare ping target
+	ip link add name br0 type bridge
+	ip link set br0 up
+	ip link set veth0_p master br0
+	ip link set veth1_p master br0
+	ip link set veth2_p master br0
+	ip link add name veth3 type veth peer name veth3_p
+	ip netns add ns1
+	ip link set veth3_p master br0 up
+	ip link set veth3 netns ns1 up
+	ip netns exec ns1 ip addr add $peer_ip4/24 dev veth3
+	ip netns exec ns1 ip addr add $peer_ip6/64 dev veth3
+	ip addr add $bond_ip4/24 dev bond0
+	ip addr add $bond_ip6/64 dev bond0
+	sleep 5
+
+	ping $peer_ip4 -c5 -I bond0 &>/dev/null
+	check_err $? "ping failed 1."
+	ping6 $peer_ip6 -c5 -I bond0 &>/dev/null
+	check_err $? "ping6 failed 1."
+
+	# active salve should be the primary slave
+	check_active_slave veth1
+
+	# active slave should be the higher prio slave
+	ip link set $active_slave down
+	ping $peer_ip4 -c5 -I bond0 &>/dev/null
+	check_err $? "ping failed 2."
+	check_active_slave veth2
+
+	# when only 1 slave is up
+	ip link set $active_slave down
+	ping $peer_ip4 -c5 -I bond0 &>/dev/null
+	check_err $? "ping failed 3."
+	check_active_slave veth0
+
+	# when a higher prio slave change to up
+	ip link set veth2 up
+	ping $peer_ip4 -c5 -I bond0 &>/dev/null
+	check_err $? "ping failed 4."
+	case $primary_reselect in
+		"0")
+			check_active_slave "veth2"
+			;;
+		"1")
+			check_active_slave "veth0"
+			;;
+		"2")
+			check_active_slave "veth0"
+			;;
+	esac
+	local pre_active_slave=$active_slave
+
+	# when the primary slave change to up
+	ip link set veth1 up
+	ping $peer_ip4 -c5 -I bond0 &>/dev/null
+	check_err $? "ping failed 5."
+	case $primary_reselect in
+		"0")
+			check_active_slave "veth1"
+			;;
+		"1")
+			check_active_slave "$pre_active_slave"
+			;;
+		"2")
+			check_active_slave "$pre_active_slave"
+			ip link set $active_slave down
+			ping $peer_ip4 -c5 -I bond0 &>/dev/null
+			check_err $? "ping failed 6."
+			check_active_slave "veth1"
+			;;
+	esac
+
+	# Test changing bond salve prio
+	if [[ "$primary_reselect" == "0" ]];then
+		ip link set dev veth0 type bond_slave prio 1000000
+		ip link set dev veth1 type bond_slave prio 0
+		ip link set dev veth2 type bond_slave prio 50
+		ip -d link show veth0 | grep -q 'prio 1000000'
+		check_err $? "veth0 prio is not 1000000"
+		ip -d link show veth1 | grep -q 'prio 0'
+		check_err $? "veth1 prio is not 0"
+		ip -d link show veth2 | grep -q 'prio 50'
+		check_err $? "veth3 prio is not 50"
+		check_active_slave "veth1"
+
+		ip link set $active_slave down
+		ping $peer_ip4 -c5 -I bond0 &>/dev/null
+		check_err $? "ping failed 7."
+		check_active_slave "veth0"
+	fi
+
+	cleanup
+
+	log_test "prio_test" "Test bonding option 'prio' with mode=$mode monitor=$monitor and primary_reselect=$primary_reselect"
+}
+
+prio_miimon_test()
+{
+	local mode
+	local primary_reselect
+
+	for mode in 1 5 6; do
+		for primary_reselect in 0 1 2; do
+			prio_test "miimon" $mode $primary_reselect
+		done
+	done
+}
+
+prio_arp_ip_target_test()
+{
+	local primary_reselect
+
+	for primary_reselect in 0 1 2; do
+		prio_test "arp_ip_target" 1 $primary_reselect
+	done
+}
+
+if skip;then
+	log_test_skip "option_prio.sh" "Current iproute doesn't support 'prio'."
+	exit 0
+fi
+
+trap cleanup EXIT
+
+tests_run
+
+exit "$EXIT_STATUS"
+
-- 
2.38.1

Re: [PATCH net 0/3] Bonding: fix high prio not effect issue

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>When a high prio link up, if there has current link, it will not do
>failover as we missed the check in link up event. Fix it in this patchset
>and add a prio option test case.
>
>Hangbin Liu (2):
>  bonding: access curr_active_slave with rtnl_dereference
>  bonding: do failover when high prio link up
>
>Liang Li (1):
>  selftests: bonding: add bonding prio option test

	For the series:

Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

	The only comment I have is that since prio is a signed value, it
would be nice if the selftest tested negative prio values.

	-J


> drivers/net/bonding/bond_main.c               |   6 +-
> .../selftests/drivers/net/bonding/Makefile    |   3 +-
> .../drivers/net/bonding/option_prio.sh        | 246 ++++++++++++++++++
> 3 files changed, 252 insertions(+), 3 deletions(-)
> create mode 100755 tools/testing/selftests/drivers/net/bonding/option_prio.sh
>
>-- 
>2.38.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com