Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs

On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote:

On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov
[off-list ref] wrote:

quoted

On 08/09/17 17:17, Kosuke Tatsukawa wrote:

quoted

Hi,

quoted

On 08/09/17 13:10, Nikolay Aleksandrov wrote:

quoted

On 08/09/17 05:06, Kosuke Tatsukawa wrote:

quoted

Hi,

quoted

On  7.09.2017 01:47, Kosuke Tatsukawa wrote:

quoted

Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").

It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage).  In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.

This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.

I didn't add code to change tlb_balance_lb back to the default value for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.

Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface.  I didn't
change that behavior, because the value of tlb_balance_lb can be changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.

As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to.  However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.

Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
Reported-by: Reinis Rozitis <redacted>
Signed-off-by: Kosuke Tatsukawa <redacted>
Cc: stable@vger.kernel.org  # v4.12+
---
 drivers/net/bonding/bond_options.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

I don't believe this to be the right solution, hardcoding it like this
changes user-visible behaviour. The issue is that if someone configured
it to be 0 in tlb mode, suddenly it will become 1 and will silently
override their config if they switch the mode to alb. Also it robs users
from their choice.

If you think this should be settable in ALB mode, then IMO you should
edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
That would also be consistent with how it's handled in TLB mode.

No, I don't think tlb_dynamic_lb should be settable in balance-alb at
this point.  All the current commits regarding tlb_dynamic_lb are for
balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
to 0 is an intended usage.

quoted

Going back and looking at your previous fix I'd argue that it is also
wrong, you should've removed the mode check altogether to return the
original behaviour where the dynamic_lb is set to 1 (enabled) by
default and then ALB mode would've had it, of course that would've left
the case of setting it to 0 in TLB mode and switching to ALB, but that
is a different issue.

Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
tlb_dynamic_lb is referenced in the following functions.

 + bond_do_alb_xmit()  -- Used by both balance-tlb and balance-alb
 + bond_tlb_xmit()  -- Only used by balance-tlb
 + bond_open()  -- Used by both balance-tlb and balance-alb
 + bond_check_params()  -- Used during module initialization
 + bond_fill_info()  -- Used to get/set value
 + bond_option_tlb_dynamic_lb_set()  -- Used to get/set value
 + bonding_show_tlb_dynamic_lb()  -- Used to get/set value
 + bond_is_nondyn_tlb()  -- Only referenced if balance-tlb mode

The following untested patch adds code to make balance-alb work as if
tlb_dynamic_lb==1 for the functions which affect balance-alb mode.  It
also reverts my previous patch.

What do you think about this approach?
---
Kosuke TATSUKAWA  | 1st Platform Software Division
                  | NEC Solution Innovators
                  | tatsu@ab.jp.nec.com

Logically the approach looks good, that being said it adds unnecessary tests in
the fast path, why not just something like the patch below ? That only leaves the
problem if it is zeroed in TLB and switched to ALB mode, and that is a one line
fix to unsuppmodes just allow it to be set for that specific case. The below
returns the default behaviour before the commit in your Fixes tag.

Actually I'm fine with your approach, too. It will fix this regardless of the
value of tlb_dynamic_lb which sounds good to me for the price of a test in
the fast path.

If you're concerned about the additional test in the fast path, how
about the patch below.  I've added an arguemnt to bond_do_alb_xmit()
to handle both balance-tlb and balance-alb similary.

Even better, looks great! 1 question below though.

quoted

I'm not sure if this causes any problem if tlb_dynamic_lb is changed
while calling bond_do_alb_xmit() in balance-tlb mode.

The option has the ifdown flag, you shouldn't be able to change it while
the bond dev is up, but even if you could I don't think it will be an issue
for the xmit.

quoted

---
Kosuke TATSUKAWA  | 1st Platform Software Division
                  | NEC Solution Innovators
                  | tatsu@ab.jp.nec.com

------------------------------------------------------------------------
 drivers/net/bonding/bond_alb.c  |   11 ++++++-----
 drivers/net/bonding/bond_main.c |    5 +++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index c02cc81..7710f20 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c

@@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)
 }

 static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
-                         struct slave *tx_slave)
+                         struct slave *tx_slave, int tlb_dynamic_lb)
 {
      struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
      struct ethhdr *eth_data = eth_hdr(skb);

@@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
      if (!tx_slave) {
              /* unbalanced or unassigned, send through primary */
              tx_slave = rcu_dereference(bond->curr_active_slave);
-             if (bond->params.tlb_dynamic_lb)
+             if (tlb_dynamic_lb)
                      bond_info->unbalanced_load += skb->len;
      }

@@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
              goto out;
      }

-     if (tx_slave && bond->params.tlb_dynamic_lb) {
+     if (tx_slave && tlb_dynamic_lb) {
              spin_lock(&bond->mode_lock);
              __tlb_clear_slave(bond, tx_slave, 0);
              spin_unlock(&bond->mode_lock);

@@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
                      break;
              }
      }
-     return bond_do_alb_xmit(skb, bond, tx_slave);
+     return bond_do_alb_xmit(skb, bond, tx_slave,
+                             bond->params.tlb_dynamic_lb);
 }

 int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)

@@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
              tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
      }

-     return bond_do_alb_xmit(skb, bond, tx_slave);
+     return bond_do_alb_xmit(skb, bond, tx_slave, 1);
 }

 void bond_alb_monitor(struct work_struct *work)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index fc63992..bcb71e7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)
               */
              if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
                      return -ENOMEM;
-             if (bond->params.tlb_dynamic_lb)
+             if (bond->params.tlb_dynamic_lb ||
+                 (BOND_MODE(bond) == BOND_MODE_TLB))

mode == tlb ? shouldn't this check be for alb ?

Actually this is not needed since this is already inside if
(bond_is_lb()) condition.

quoted

quoted

                      queue_delayed_work(bond->wq, &bond->alb_work, 0);
      }

@@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)
      }
      ad_user_port_key = valptr->value;

-     if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
+     if (bond_mode == BOND_MODE_TLB) {
              bond_opt_initstr(&newval, "default");
              valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
                                      &newval);

I think the underlying issue is that tlb_dynamic_lb should be set to 1
for all modes which was not the case when it was getting initialized
only forTLB mode. So from that perspective I prefer Nik's patch with a
small variation that guards the case when mode transitions from TLB to
ALB. The reason why I like that patch is because it's simple and
avoids complications.

+1, I think this is the most straight-forward solution as well and
safest for -net

I will go ahead and submit it in a few minutes.

Thanks,
 Nik