Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753920AbdIHKKT (ORCPT ); Fri, 8 Sep 2017 06:10:19 -0400 Received: from mail-wr0-f179.google.com ([209.85.128.179]:35944 "EHLO mail-wr0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751556AbdIHKKR (ORCPT ); Fri, 8 Sep 2017 06:10:17 -0400 X-Google-Smtp-Source: ADKCNb402GKWKpPdH4ZbucwUzO3+NbhbgNsGDTcoHPX1U9b/xU5CX9yvdgwAlHFw4WJsI7yvEQi6RA== Subject: Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs To: Kosuke Tatsukawa References: <17EC94B0A072C34B8DCF0D30AD16044A029864AC@BPXM09GP.gisp.nec.co.jp> Cc: Jay Vosburgh , Veaceslav Falico , Andy Gospodarek , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Reinis Rozitis From: Nikolay Aleksandrov X-Enigmail-Draft-Status: N1110 Message-ID: <52a22af7-cc99-e441-79fa-e48da283b001@cumulusnetworks.com> Date: Fri, 8 Sep 2017 13:10:12 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.6.0 MIME-Version: 1.0 In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A029864AC@BPXM09GP.gisp.nec.co.jp> Content-Type: text/plain; charset=iso-2022-jp Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5949 Lines: 134 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. 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",