Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757295AbdIHXzM (ORCPT ); Fri, 8 Sep 2017 19:55:12 -0400 Received: from mail-yw0-f175.google.com ([209.85.161.175]:36506 "EHLO mail-yw0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757173AbdIHXzK (ORCPT ); Fri, 8 Sep 2017 19:55:10 -0400 X-Google-Smtp-Source: ADKCNb7pPJ40J0dWbBP6Oz0bmeVwOW+A7TB/D37jG72DUGIA20oECGlzUtPDRGFXBFZTZVpcTSsrxC8EZmRgxxtgmiI= MIME-Version: 1.0 In-Reply-To: <03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com> References: <17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp> <03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com> From: =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= Date: Fri, 8 Sep 2017 16:54:48 -0700 Message-ID: Subject: Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs To: Nikolay Aleksandrov Cc: Kosuke Tatsukawa , Jay Vosburgh , Veaceslav Falico , Andy Gospodarek , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Reinis Rozitis Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12539 Lines: 278 On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov 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 >>>>>>> Signed-off-by: Kosuke Tatsukawa >>>>>>> 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. >>>>> >>>>> >>>>>> 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. > >> 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@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. >> 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. 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;