Date:   Sat, 9 Sep 2017 13:29:04 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Mahesh Bandewar (महेश बंडेवार) <maheshb@...gle.com>
Cc:     Kosuke Tatsukawa <tatsu@...jp.nec.com>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Reinis Rozitis <r@...e.lv>
Subject: 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
> <nikolay@...ulusnetworks.com> wrote:
>> On 08/09/17 17:17, Kosuke Tatsukawa wrote:
>>> Hi,
>>>
>>>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>>>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>>>>>> Hi,
>>>>>>
>>>>>>> On  7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>>>>>>> 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 <r@...e.lv>
>>>>>>>> Signed-off-by: Kosuke Tatsukawa <tatsu@...jp.nec.com>
>>>>>>>> Cc: stable@...r.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.
>>>>>>
>>>>>>
>>>>>>> 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@...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.
>>
>>> 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.
>>
>>> ---
>>> Kosuke TATSUKAWA  | 1st Platform Software Division
>>>                   | NEC Solution Innovators
>>>                   | tatsu@...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.
> 
>>>                       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

> 
> Here is what I meant -
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index fc63992ab0e0..c99dc59d729b 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)
>         int bond_mode   = BOND_MODE_ROUNDROBIN;
>         int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;
>         int lacp_fast = 0;
> -       int tlb_dynamic_lb = 0;
> +       int tlb_dynamic_lb;
> 
>         /* Convert string parameters. */
>         if (mode) {
> @@ -4601,16 +4601,13 @@ 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)) {
> -               bond_opt_initstr(&newval, "default");
> -               valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> -                                       &newval);
> -               if (!valptr) {
> -                       pr_err("Error: No tlb_dynamic_lb default value");
> -                       return -EINVAL;
> -               }
> -               tlb_dynamic_lb = valptr->value;
> +       bond_opt_initstr(&newval, "default");
> +       valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB), &newval);
> +       if (!valptr) {
> +               pr_err("Error: No tlb_dynamic_lb default value");
> +               return -EINVAL;
>         }
> +       tlb_dynamic_lb = valptr->value;
> 
>         if (lp_interval == 0) {
>                 pr_warn("Warning: ip_interval must be between 1 and
> %d, so it was reset to %d\n",
> diff --git a/drivers/net/bonding/bond_options.c
> b/drivers/net/bonding/bond_options.c
> index a12d603d41c6..7feacd262182 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -754,6 +754,12 @@ static int bond_option_mode_set(struct bonding *bond,
>                            bond->params.miimon);
>         }
> 
> +       /* Guard against transition TLB/tlb_dynamic_lb=0 -> ALB mode.
> +        * In ALB mode, tlb_dynamic_lb must be set to 1.
> +        */
> +       if (newval->value == BOND_MODE_ALB && bond->params.tlb_dynamic_lb != 1)
> +               bond->params.tlb_dynamic_lb = 1;
> +
>         /* don't cache arp_validate between modes */
>         bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
>         bond->params.mode = newval->value;
> 

Powered by blists - more mailing lists