Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754669AbdIHKNZ (ORCPT ); Fri, 8 Sep 2017 06:13:25 -0400 Received: from mail-wm0-f54.google.com ([74.125.82.54]:45527 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752387AbdIHKNY (ORCPT ); Fri, 8 Sep 2017 06:13:24 -0400 X-Google-Smtp-Source: AOwi7QBYRSfbTA/Xx8UZSAKscWnS5mH4t5BTUYpQu87oqagg1ex1tpVcTS72daB3jDiiOd0oIIX9FA== 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> <52a22af7-cc99-e441-79fa-e48da283b001@cumulusnetworks.com> Cc: Jay Vosburgh , Veaceslav Falico , Andy Gospodarek , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Reinis Rozitis From: Nikolay Aleksandrov Message-ID: <99818f9e-7ee0-4e53-b2be-b61b958f87e7@cumulusnetworks.com> Date: Fri, 8 Sep 2017 13:13:20 +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: <52a22af7-cc99-e441-79fa-e48da283b001@cumulusnetworks.com> 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: 4981 Lines: 106 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.