[{"id":1764998,"web_url":"http://patchwork.ozlabs.org/comment/1764998/","msgid":"","list_archive_url":null,"date":"2017-09-07T23:09:26","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n> balance-alb mode\") tried to fix transmit dynamic load balancing in\n> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n> (\"bonding: remove hardcoded value\").\n> \n> It turned out that my previous patch only fixed the case when\n> balance-alb was specified as bonding module parameter, and not when\n> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n> common usage). In the latter case, tlb_dynamic_lb was set up according\n> to the default mode of the bonding interface, which happens to be\n> balance-rr.\n> \n> This additional patch addresses this issue by setting up tlb_dynamic_lb\n> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n> \n> I didn't add code to change tlb_balance_lb back to the default value for\n> other modes, because \"mode\" is usually set up only once during\n> initialization, and it's not worthwhile to change the static variable\n> bonding_defaults in bond_main.c to a global variable just for this\n> purpose.\n> \n> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n> balance-tlb mode if it is set up using the sysfs interface. I didn't\n> change that behavior, because the value of tlb_balance_lb can be changed\n> using the sysfs interface for balance-tlb, and I didn't like changing\n> the default value back and forth for balance-tlb.\n> \n> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n> is not an intended usage, so there is little use making it writable at\n> this moment.\n> \n> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n> Reported-by: Reinis Rozitis \n> Signed-off-by: Kosuke Tatsukawa \n> Cc: stable@vger.kernel.org # v4.12+\n> ---\n> drivers/net/bonding/bond_options.c | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n> \n\nI don't believe this to be the right solution, hardcoding it like this\nchanges user-visible behaviour. The issue is that if someone configured\nit to be 0 in tlb mode, suddenly it will become 1 and will silently\noverride their config if they switch the mode to alb. Also it robs users\nfrom their choice.\n\nIf you think this should be settable in ALB mode, then IMO you should\nedit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\nThat would also be consistent with how it's handled in TLB mode.\n\nGoing back and looking at your previous fix I'd argue that it is also\nwrong, you should've removed the mode check altogether to return the\noriginal behaviour where the dynamic_lb is set to 1 (enabled) by\ndefault and then ALB mode would've had it, of course that would've left\nthe case of setting it to 0 in TLB mode and switching to ALB, but that\nis a different issue.\n\nCheers,\n Nik","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"HEiwf9Vp\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpGPP6HMmz9sBZ\n\tfor ;\n\tFri, 8 Sep 2017 09:09:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752809AbdIGXJc (ORCPT );\n\tThu, 7 Sep 2017 19:09:32 -0400","from mail-wm0-f52.google.com ([74.125.82.52]:44254 \"EHLO\n\tmail-wm0-f52.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750994AbdIGXJa (ORCPT\n\t); Thu, 7 Sep 2017 19:09:30 -0400","by mail-wm0-f52.google.com with SMTP id 137so2151753wmj.1\n\tfor ; Thu, 07 Sep 2017 16:09:29 -0700 (PDT)","from [192.168.0.107] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.gmail.com with ESMTPSA id\n\tl15sm371393wmd.23.2017.09.07.16.09.27\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 07 Sep 2017 16:09:27 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=xNuiFN9dJbpDO8dr0xDNwlkAO5ZlYauysgOHREK0Hgg=;\n\tb=HEiwf9Vpf7CIvBlG4HZQ090rYBSjdPLgmFCh9eLCtl0to7siwrvmZbwWX90NZPm7I6\n\tdqRUpYUooZgUVEQXEX25Oj5NuXSnVwCfv+q3/0zS/9MpLVTsArx8nvfglTjXXEDdkJzA\n\txSPD7y9hO+Oo0VFX5CfGOWIRVmz0s16MnjW+k=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=xNuiFN9dJbpDO8dr0xDNwlkAO5ZlYauysgOHREK0Hgg=;\n\tb=B0BrNNxo8C+ESgZxb4J0bN99caz4LkWPTjVnLF+Expxp3nNP4oEJhuLDhEY/ScTLA2\n\tTAhiiqPbaUNGiWWwpnkTg51iC+FAvjft/8lC+fHdTipdTii/yu6k9IoIjeEGiR7yqWJ5\n\t//hoXQKQ7EX/AYIbdEkUL6stZSU2rZuC2wt0Rbsml14D+8g82i1LPH8STN+ZuWdZ/U9Y\n\tV7DOPc7CmuqrrvE9GXi8TQne3+MKWjf5+Oimi38kfAC9Jsp+UUxKACJKtHypH7MPsTQC\n\t8Vgf7XmPudNv3zEyyZj7/XDf9BkkdGdEuGlx2HT5l9At+ybr9rrTUY2jcj4UMyjhht5N\n\tUMFA==","X-Gm-Message-State":"AHPjjUife/+uCBl17rgBiZl7JAlfOyJtabj6BU/t38cczvd2JiDPtCLj\n\tvFQEtZFqiIWFA3uEF8DREP6clw==","X-Google-Smtp-Source":"ADKCNb6ClbHTGgmBaUpLJN1A0d7N74jL4/qdM6sjwD0dB78WGGvWbMj6n6OIPx/oLiEadkh2J0nyBQ==","X-Received":"by 10.28.229.129 with SMTP id c123mr239648wmh.153.1504825769187; \n\tThu, 07 Sep 2017 16:09:29 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>","From":"Nikolay Aleksandrov ","Message-ID":"","Date":"Fri, 8 Sep 2017 02:09:26 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>","Content-Type":"text/plain; charset=iso-2022-jp","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765019,"web_url":"http://patchwork.ozlabs.org/comment/1765019/","msgid":"","list_archive_url":null,"date":"2017-09-08T00:39:36","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":6061,"url":"http://patchwork.ozlabs.org/api/people/6061/","name":"Mahesh Bandewar (महेश बंडेवार)","email":"maheshb@google.com"},"content":"On Thu, Sep 7, 2017 at 4:09 PM, Nikolay Aleksandrov\n wrote:\n> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>> (\"bonding: remove hardcoded value\").\n>>\n>> It turned out that my previous patch only fixed the case when\n>> balance-alb was specified as bonding module parameter, and not when\n>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>> to the default mode of the bonding interface, which happens to be\n>> balance-rr.\n>>\n>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>\n>> I didn't add code to change tlb_balance_lb back to the default value for\n>> other modes, because \"mode\" is usually set up only once during\n>> initialization, and it's not worthwhile to change the static variable\n>> bonding_defaults in bond_main.c to a global variable just for this\n>> purpose.\n>>\n>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>> change that behavior, because the value of tlb_balance_lb can be changed\n>> using the sysfs interface for balance-tlb, and I didn't like changing\n>> the default value back and forth for balance-tlb.\n>>\n>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>> is not an intended usage, so there is little use making it writable at\n>> this moment.\n>>\n>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n\nThis is wrong! I think you are confusing various things here. ALB is\ndifferent mode from TLB and TLB-dynamic-lb is *only* a special case of\nTLB. Your earlier patch is also wrong for the same reasons. However,\nsince the default value of tlb_dynamic_lb is set to 0 it's not\ncausing issues for ALB mode otherwise it would break ALB mode.\ntlb_dynamic_lb has absolutely nothing to do with ALB mode. Please\nrevert the earlier change (cbf5ecb30560).\n\nIt's not clear to me what you saw as broken, so can't really suggest\nwhat really need to be done.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"SbJszVY9\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpJQ52r5Mz9sBZ\n\tfor ;\n\tFri, 8 Sep 2017 10:40:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752848AbdIHAj7 (ORCPT );\n\tThu, 7 Sep 2017 20:39:59 -0400","from mail-yw0-f178.google.com ([209.85.161.178]:35149 \"EHLO\n\tmail-yw0-f178.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751774AbdIHAj5 (ORCPT\n\t); Thu, 7 Sep 2017 20:39:57 -0400","by mail-yw0-f178.google.com with SMTP id q80so3607424ywg.2\n\tfor ; Thu, 07 Sep 2017 17:39:57 -0700 (PDT)","by 10.37.161.71 with HTTP; Thu, 7 Sep 2017 17:39:36 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=NAuPUkBLxOYV8SbIEFvpkz/rbm/AtwoUrymcJDbdgBs=;\n\tb=SbJszVY99Z8SRcAGddfLw5PcmmF9mAOLCw00Y5ZJhHDdXr1CMpLrgICKM5I30BCurk\n\tj/KfrClflB0iR8plhwat19/OnlXGAvR4zTdO/4zTuA2G03PXPZxYyMee7Ak6QyIJF02z\n\tY9na0yKhZ1087vcNzhlN87mSCgKT9+QKVY8iugJNnVuMGAFb2lrAEfsTSslwEv76onVH\n\tjaGbKLRuyTxcsFWUbqi99w04qHKB8mB8I0DMAQijUpHvi8NVeoYq5djYRJT2DZeSmDz9\n\tOUwlOZ/8GXeyeNu224lhI/6b2jO73NYriopLRE3qrs9KwFhh5PuSC+HQNWoqfPVVSB65\n\t4n/g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=NAuPUkBLxOYV8SbIEFvpkz/rbm/AtwoUrymcJDbdgBs=;\n\tb=ahIexA0AoSMSqqHkrD1/UsVFORo2AXqzCpSadMC5eD7t7TPz7qhl5mHlfLSpWLu6MR\n\tMwaJF9f6jGKKBCD6/wNbFeGytl9MiflidoeZ2LtGvbSArQRc+bqwWrc+VWMCinXcm0Qh\n\tygr5mv9eW35NWlPpa/u5Aplbrla2ONxzl0B3Xj/H88ONuthIHKHqWAqlB6QzDY5HqoYS\n\tElBHJixjn8hhjxe48fCnZC2k0OVmcfd3MNBWXyTvR9Dqs0o3jRACSmHjZcJ5DrWedBlV\n\tMGeuWYo8nP1VD5hX5XXDnDDn+xGYmXRhau3uflPaF3dtr1XC/im1O00Hg455wBqZwV3g\n\tldqA==","X-Gm-Message-State":"AHPjjUj7GEw8Jlm/qwBFZMl50morhBRa1mS8LmutIeivqfMBbZzl1VH6\n\tutpgHAYJnfg3uEwTZQbip4NR7YiaF42g","X-Google-Smtp-Source":"ADKCNb4qxxVLnyUqlJqEUT2Ut+KGHeYkZqsa0j13f78r9hdzWmy1SdOxWevAWsh7lu1INHCH7n2HEZyN0zyBd1KVTYw=","X-Received":"by 10.37.117.10 with SMTP id q10mr1010289ybc.64.1504831196848;\n\tThu, 07 Sep 2017 17:39:56 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>\n\t","From":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","Date":"Thu, 7 Sep 2017 17:39:36 -0700","Message-ID":"","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Nikolay Aleksandrov ","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765021,"web_url":"http://patchwork.ozlabs.org/comment/1765021/","msgid":"","list_archive_url":null,"date":"2017-09-08T00:47:32","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":6061,"url":"http://patchwork.ozlabs.org/api/people/6061/","name":"Mahesh Bandewar (महेश बंडेवार)","email":"maheshb@google.com"},"content":"On Thu, Sep 7, 2017 at 5:39 PM, Mahesh Bandewar (महेश बंडेवार)\n wrote:\n> On Thu, Sep 7, 2017 at 4:09 PM, Nikolay Aleksandrov\n> wrote:\n>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>> (\"bonding: remove hardcoded value\").\n>>>\n>>> It turned out that my previous patch only fixed the case when\n>>> balance-alb was specified as bonding module parameter, and not when\n>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>> to the default mode of the bonding interface, which happens to be\n>>> balance-rr.\n>>>\n>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>\n>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>> other modes, because \"mode\" is usually set up only once during\n>>> initialization, and it's not worthwhile to change the static variable\n>>> bonding_defaults in bond_main.c to a global variable just for this\n>>> purpose.\n>>>\n>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>> the default value back and forth for balance-tlb.\n>>>\n>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>> is not an intended usage, so there is little use making it writable at\n>>> this moment.\n>>>\n>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>\n> This is wrong! I think you are confusing various things here. ALB is\n> different mode from TLB and TLB-dynamic-lb is *only* a special case of\n> TLB. Your earlier patch is also wrong for the same reasons. However,\n> since the default value of tlb_dynamic_lb is set to 0 it's not\n> causing issues for ALB mode otherwise it would break ALB mode.\nI take this back. The default value is 1 so ALB is broken because of\nthe referenced change.\n\n> tlb_dynamic_lb has absolutely nothing to do with ALB mode. Please\n> revert the earlier change (cbf5ecb30560).\n>\n> It's not clear to me what you saw as broken, so can't really suggest\n> what really need to be done.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"nCZeTvGt\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpJZs6TsBz9sBZ\n\tfor ;\n\tFri, 8 Sep 2017 10:48:05 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753362AbdIHAr5 (ORCPT );\n\tThu, 7 Sep 2017 20:47:57 -0400","from mail-yw0-f175.google.com ([209.85.161.175]:32941 \"EHLO\n\tmail-yw0-f175.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752643AbdIHAry (ORCPT\n\t); Thu, 7 Sep 2017 20:47:54 -0400","by mail-yw0-f175.google.com with SMTP id s62so3674328ywg.0\n\tfor ; Thu, 07 Sep 2017 17:47:53 -0700 (PDT)","by 10.37.161.71 with HTTP; Thu, 7 Sep 2017 17:47:32 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=3rXZe06y+FeKhoHoZsnqorzIbc0+07vjQlz8Cqkm7WQ=;\n\tb=nCZeTvGtsqGn0OIREPqO0VaMCGvRiOJKOVQVXh7Ig1ZsgNuGYBmsyQurEDHe9/a9Us\n\t7pBOTbZKPz6jd8xjUQyM5/SQ9pGfVB2a4a51JUByp4Uow3aP+qTaXz1e02A2FbBub+/P\n\tyX8S2fCvltEhM7RC/OPLdUuNCIkNXlUOW8kLo8QoYwIx8sdYR4ig3UMZdv6nX5bNjIza\n\tzg+ocIjL/moISWHUG8SMQlcVTpHUfVPAwCOdaigd5jnFbvOdGteDVLYgP/zSylJ7cyba\n\tKUIYknp64+8O/SNWeVSSgXO/CS0hTKLemkkjnSYBAFEhm/fDTQ7gKW66XxbOGl49UGBl\n\toKcA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=3rXZe06y+FeKhoHoZsnqorzIbc0+07vjQlz8Cqkm7WQ=;\n\tb=pcC1dXzZwGFdkvyv5bwjL0+QVEhx9gcOrjuUdDjj5dLU24DqqqA63pcacIAvbW9MMQ\n\t9dHwWWhIB5Ls+1TMssk6sagxGmM5B7YHe0KtdVLmViWoI65I7MFE9F7o6yvkxU5ZseIh\n\t1Q0W6KfCtj3vI2CQLnH3cFaNLtdhk/uWRtW1kfOcXRZDo++UuIpyA2d9nPP331TAUis0\n\ti02a7Q/Zvk7WkWdK6/kc46WxcStCmDuw5ZHY6lm3c6aOzbNM+uLmhMONWH/6gC2X/KhL\n\tAS2aM9WO/1kG9+JnnUhmwJ9atVLwHKeWLgWxe0lgiNFGpzaaxVbWN3C4Em9GuDuerocY\n\tlMmw==","X-Gm-Message-State":"AHPjjUhhD6tnC7xbWW2cx5SasIn7lYKOXQ0qu+VJIPHijafxiCkvARKJ\n\tWqXuUYh/O4YqlZ+vW62eeU6StsIHZD0k","X-Google-Smtp-Source":"ADKCNb5wa941PfBdsyqfqbOfvJUdWdC+X/ywpTHHvWOZlvYp8qaUOYL4YOKpsQ6rwdO67VDb+V2U65CYj/ZiObvubPE=","X-Received":"by 10.129.189.16 with SMTP id b16mr1060253ywi.336.1504831672919; \n\tThu, 07 Sep 2017 17:47:52 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>\n\t\n\t","From":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","Date":"Thu, 7 Sep 2017 17:47:32 -0700","Message-ID":"","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Nikolay Aleksandrov ","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765035,"web_url":"http://patchwork.ozlabs.org/comment/1765035/","msgid":"","list_archive_url":null,"date":"2017-09-08T01:54:54","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":6061,"url":"http://patchwork.ozlabs.org/api/people/6061/","name":"Mahesh Bandewar (महेश बंडेवार)","email":"maheshb@google.com"},"content":"On Thu, Sep 7, 2017 at 5:47 PM, Mahesh Bandewar (महेश बंडेवार)\n wrote:\n> On Thu, Sep 7, 2017 at 5:39 PM, Mahesh Bandewar (महेश बंडेवार)\n> wrote:\n>> On Thu, Sep 7, 2017 at 4:09 PM, Nikolay Aleksandrov\n>> wrote:\n>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>>> (\"bonding: remove hardcoded value\").\n>>>>\n>>>> It turned out that my previous patch only fixed the case when\n>>>> balance-alb was specified as bonding module parameter, and not when\n>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>>> to the default mode of the bonding interface, which happens to be\n>>>> balance-rr.\n>>>>\n>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>>\n>>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>>> other modes, because \"mode\" is usually set up only once during\n>>>> initialization, and it's not worthwhile to change the static variable\n>>>> bonding_defaults in bond_main.c to a global variable just for this\n>>>> purpose.\n>>>>\n>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>>> the default value back and forth for balance-tlb.\n>>>>\n>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>>> is not an intended usage, so there is little use making it writable at\n>>>> this moment.\n>>>>\n>>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>\n>> This is wrong! I think you are confusing various things here. ALB is\n>> different mode from TLB and TLB-dynamic-lb is *only* a special case of\n>> TLB. Your earlier patch is also wrong for the same reasons. However,\n>> since the default value of tlb_dynamic_lb is set to 0 it's not\n>> causing issues for ALB mode otherwise it would break ALB mode.\n> I take this back. The default value is 1 so ALB is broken because of\n> the referenced change.\n>\n>> tlb_dynamic_lb has absolutely nothing to do with ALB mode. Please\n>> revert the earlier change (cbf5ecb30560).\n>>\n>> It's not clear to me what you saw as broken, so can't really suggest\n>> what really need to be done.\nPlease scratch all I said.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"cS7P7lCX\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpL4h3GF6z9sPk\n\tfor ;\n\tFri, 8 Sep 2017 11:55:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752759AbdIHBzS (ORCPT );\n\tThu, 7 Sep 2017 21:55:18 -0400","from mail-yw0-f173.google.com ([209.85.161.173]:34620 \"EHLO\n\tmail-yw0-f173.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750978AbdIHBzQ (ORCPT\n\t); Thu, 7 Sep 2017 21:55:16 -0400","by mail-yw0-f173.google.com with SMTP id r85so3938172ywg.1\n\tfor ; Thu, 07 Sep 2017 18:55:16 -0700 (PDT)","by 10.37.161.71 with HTTP; Thu, 7 Sep 2017 18:54:54 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=dc9y6vHToUCAvWxUzvx98q/A5UW80Njj8JU6OV2ZsEI=;\n\tb=cS7P7lCXzG1eD+pxPmBjGYc9vdpmNnr4Y9Pvbi4dJaMfBYOgaHatDSzw1Qkrh8u2sr\n\tX+6fph0EEgLfcl/L4R7U38r1mcYg7xvYpLgvnuWP90HwsRPw6uRiXtLR63kpEJoem73G\n\tBG71HsGxW7NSfLz1VE9knY/vVdOcT6H7jYEeqCQ471Qd1ooV4XkbC6XiBohXr7+XuB0b\n\t2mCekQyDV35w21coD6tnfpDDgLYfr6AFGwY4uRAfVEULd5tEZfB/gOJRqzH2yR35sAXc\n\tjfhUm0CsXh1bnxspPGKeEjDR1Gk22uBwLcwQuIznNCMbur+jiris8Ezy4Q7hx9VEyWk0\n\tt6JA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=dc9y6vHToUCAvWxUzvx98q/A5UW80Njj8JU6OV2ZsEI=;\n\tb=lrqyTP91LzSUyhBzZXgjs2mi2yPLtZswZFY3HmEO+JmwgaFIQQDk1MT2ceEMlC02GQ\n\t+UVUfACoDVn0jjeGTo94xXwRhGI1vFhDDrXjp6svVnbSzmRUKpibMlF728P+PrA4kaYa\n\t+hS5khDLAXB4OLBXDKWgX7QPqj/qtCVcbbbyAjkUhQc0h9oeTAm5VixKVKcdoempxBKT\n\tYq3gG3oKGX8kJdfnSuW8vtIRY3uoTQ76frqEtiGUZXJ4JmE9A4x9W1Ha8dWxqsL/PvwU\n\tSjuASM8KAGMG4zn0SnYg1ihLxXvJ28jNTIZ20h+1pJb1biJMREbDciheOZdh74/X+YKx\n\tRIDw==","X-Gm-Message-State":"AHPjjUhvMKxN+Z9RwxcaA1Mjo2Kbi2KRO6JWJhNsVEdDj3nHPHPt4HI7\n\t63vtK8kCmiNWGnYtMBaxCpJuSIpFaPqF","X-Google-Smtp-Source":"ADKCNb5iA5pN7vhmfZA8iKbj4nwYkMDXVCCLhv5X3/laXeUwyaMNECo+0VdYZ6E6csqSQcBUqA8ESLeWr5WHn9g6UGk=","X-Received":"by 10.129.153.81 with SMTP id q78mr1143521ywg.133.1504835715473; \n\tThu, 07 Sep 2017 18:55:15 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>\n\t\n\t\n\t","From":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","Date":"Thu, 7 Sep 2017 18:54:54 -0700","Message-ID":"","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Nikolay Aleksandrov ","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765040,"web_url":"http://patchwork.ozlabs.org/comment/1765040/","msgid":"<17EC94B0A072C34B8DCF0D30AD16044A029864AC@BPXM09GP.gisp.nec.co.jp>","list_archive_url":null,"date":"2017-09-08T02:06:16","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs ","submitter":{"id":67390,"url":"http://patchwork.ozlabs.org/api/people/67390/","name":"Kosuke Tatsukawa","email":"tatsu@ab.jp.nec.com"},"content":"Hi,\n\n> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>> (\"bonding: remove hardcoded value\").\n>> \n>> It turned out that my previous patch only fixed the case when\n>> balance-alb was specified as bonding module parameter, and not when\n>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>> to the default mode of the bonding interface, which happens to be\n>> balance-rr.\n>> \n>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>> \n>> I didn't add code to change tlb_balance_lb back to the default value for\n>> other modes, because \"mode\" is usually set up only once during\n>> initialization, and it's not worthwhile to change the static variable\n>> bonding_defaults in bond_main.c to a global variable just for this\n>> purpose.\n>> \n>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>> change that behavior, because the value of tlb_balance_lb can be changed\n>> using the sysfs interface for balance-tlb, and I didn't like changing\n>> the default value back and forth for balance-tlb.\n>> \n>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>> is not an intended usage, so there is little use making it writable at\n>> this moment.\n>> \n>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>> Reported-by: Reinis Rozitis \n>> Signed-off-by: Kosuke Tatsukawa \n>> Cc: stable@vger.kernel.org # v4.12+\n>> ---\n>> drivers/net/bonding/bond_options.c | 3 +++\n>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>> \n> \n> I don't believe this to be the right solution, hardcoding it like this\n> changes user-visible behaviour. The issue is that if someone configured\n> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n> override their config if they switch the mode to alb. Also it robs users\n> from their choice.\n> \n> If you think this should be settable in ALB mode, then IMO you should\n> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n> That would also be consistent with how it's handled in TLB mode.\n\nNo, I don't think tlb_dynamic_lb should be settable in balance-alb at\nthis point. All the current commits regarding tlb_dynamic_lb are for\nbalance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\nto 0 is an intended usage.\n\n\n> Going back and looking at your previous fix I'd argue that it is also\n> wrong, you should've removed the mode check altogether to return the\n> original behaviour where the dynamic_lb is set to 1 (enabled) by\n> default and then ALB mode would've had it, of course that would've left\n> the case of setting it to 0 in TLB mode and switching to ALB, but that\n> is a different issue.\n\nMaybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\ntlb_dynamic_lb is referenced in the following functions.\n\n + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n + bond_tlb_xmit() -- Only used by balance-tlb\n + bond_open() -- Used by both balance-tlb and balance-alb\n + bond_check_params() -- Used during module initialization\n + bond_fill_info() -- Used to get/set value\n + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n\nThe following untested patch adds code to make balance-alb work as if\ntlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\nalso reverts my previous patch.\n\nWhat do you think about this approach?\n---\nKosuke TATSUKAWA | 1st Platform Software Division\n | NEC Solution Innovators\n | tatsu@ab.jp.nec.com\n\n------------------------------------------------------------------------\n drivers/net/bonding/bond_alb.c | 6 ++++--\n drivers/net/bonding/bond_main.c | 5 +++--\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c\nindex c02cc81..a9229b5 100644\n--- a/drivers/net/bonding/bond_alb.c\n+++ b/drivers/net/bonding/bond_alb.c\n@@ -1325,7 +1325,8 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n \tif (!tx_slave) {\n \t\t/* unbalanced or unassigned, send through primary */\n \t\ttx_slave = rcu_dereference(bond->curr_active_slave);\n-\t\tif (bond->params.tlb_dynamic_lb)\n+\t\tif (bond->params.tlb_dynamic_lb ||\n+\t\t (BOND_MODE(bond) == BOND_MODE_ALB))\n \t\t\tbond_info->unbalanced_load += skb->len;\n \t}\n \n@@ -1339,7 +1340,8 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n \t\tgoto out;\n \t}\n \n-\tif (tx_slave && bond->params.tlb_dynamic_lb) {\n+\tif (tx_slave && (bond->params.tlb_dynamic_lb ||\n+\t\t\t BOND_MODE(bond) == BOND_MODE_ALB)) {\n \t\tspin_lock(&bond->mode_lock);\n \t\t__tlb_clear_slave(bond, tx_slave, 0);\n \t\tspin_unlock(&bond->mode_lock);\ndiff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\nindex fc63992..bcb71e7 100644\n--- a/drivers/net/bonding/bond_main.c\n+++ b/drivers/net/bonding/bond_main.c\n@@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)\n \t\t */\n \t\tif (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))\n \t\t\treturn -ENOMEM;\n-\t\tif (bond->params.tlb_dynamic_lb)\n+\t\tif (bond->params.tlb_dynamic_lb ||\n+\t\t (BOND_MODE(bond) == BOND_MODE_ALB))\n \t\t\tqueue_delayed_work(bond->wq, &bond->alb_work, 0);\n \t}\n \n@@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)\n \t}\n \tad_user_port_key = valptr->value;\n \n-\tif ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n+\tif (bond_mode == BOND_MODE_TLB) {\n \t\tbond_opt_initstr(&newval, \"default\");\n \t\tvalptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n \t\t\t\t\t&newval);","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpLL82XHjz9sPk\n\tfor ;\n\tFri, 8 Sep 2017 12:07:12 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753515AbdIHCG7 convert rfc822-to-8bit (ORCPT\n\t);\n\tThu, 7 Sep 2017 22:06:59 -0400","from tyo162.gate.nec.co.jp ([114.179.232.162]:47066 \"EHLO\n\ttyo162.gate.nec.co.jp\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753414AbdIHCG5 (ORCPT\n\t); Thu, 7 Sep 2017 22:06:57 -0400","from mailgate01.nec.co.jp ([114.179.233.122])\n\tby tyo162.gate.nec.co.jp (8.15.1/8.15.1) with ESMTPS id\n\tv8826jA9015813\n\t(version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO);\n\tFri, 8 Sep 2017 11:06:45 +0900","from mailsv02.nec.co.jp (mailgate-v.nec.co.jp [10.204.236.94])\n\tby mailgate01.nec.co.jp (8.15.1/8.15.1) with ESMTP id v8826jaN005534; \n\tFri, 8 Sep 2017 11:06:45 +0900","from mail01b.kamome.nec.co.jp (mail01b.kamome.nec.co.jp\n\t[10.25.43.2])\n\tby mailsv02.nec.co.jp (8.15.1/8.15.1) with ESMTP id v8826jUh003371;\n\tFri, 8 Sep 2017 11:06:45 +0900","from bpxc99gp.gisp.nec.co.jp ([10.38.151.140] [10.38.151.140]) by\n\tmail01b.kamome.nec.co.jp with ESMTP id BT-MMP-2937060;\n\tFri, 8 Sep 2017 11:06:17 +0900","from BPXM09GP.gisp.nec.co.jp ([10.38.151.201]) by\n\tBPXC12GP.gisp.nec.co.jp ([10.38.151.140]) with mapi id 14.03.0319.002;\n\tFri, 8 Sep 2017 11:06:16 +0900"],"From":"Kosuke Tatsukawa ","To":"Nikolay Aleksandrov ","CC":"Jay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs ","Thread-Topic":"[PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs ","Thread-Index":"AdMoRw2o0sBvF5aHTv+qn4ITKRSxcg==","Date":"Fri, 8 Sep 2017 02:06:16 +0000","Message-ID":"<17EC94B0A072C34B8DCF0D30AD16044A029864AC@BPXM09GP.gisp.nec.co.jp>","In-Reply-To":"","Accept-Language":"ja-JP, en-US","Content-Language":"ja-JP","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.34.125.78]","Content-Type":"text/plain; charset=\"iso-2022-jp\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-TM-AS-MML":"disable","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765218,"web_url":"http://patchwork.ozlabs.org/comment/1765218/","msgid":"<52a22af7-cc99-e441-79fa-e48da283b001@cumulusnetworks.com>","list_archive_url":null,"date":"2017-09-08T10:10:12","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 08/09/17 05:06, Kosuke Tatsukawa wrote:\n> Hi,\n> \n>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>> (\"bonding: remove hardcoded value\").\n>>>\n>>> It turned out that my previous patch only fixed the case when\n>>> balance-alb was specified as bonding module parameter, and not when\n>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>> to the default mode of the bonding interface, which happens to be\n>>> balance-rr.\n>>>\n>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>\n>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>> other modes, because \"mode\" is usually set up only once during\n>>> initialization, and it's not worthwhile to change the static variable\n>>> bonding_defaults in bond_main.c to a global variable just for this\n>>> purpose.\n>>>\n>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>> the default value back and forth for balance-tlb.\n>>>\n>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>> is not an intended usage, so there is little use making it writable at\n>>> this moment.\n>>>\n>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>> Reported-by: Reinis Rozitis \n>>> Signed-off-by: Kosuke Tatsukawa \n>>> Cc: stable@vger.kernel.org # v4.12+\n>>> ---\n>>> drivers/net/bonding/bond_options.c | 3 +++\n>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>\n>>\n>> I don't believe this to be the right solution, hardcoding it like this\n>> changes user-visible behaviour. The issue is that if someone configured\n>> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n>> override their config if they switch the mode to alb. Also it robs users\n>> from their choice.\n>>\n>> If you think this should be settable in ALB mode, then IMO you should\n>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n>> That would also be consistent with how it's handled in TLB mode.\n> \n> No, I don't think tlb_dynamic_lb should be settable in balance-alb at\n> this point. All the current commits regarding tlb_dynamic_lb are for\n> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\n> to 0 is an intended usage.\n> \n> \n>> Going back and looking at your previous fix I'd argue that it is also\n>> wrong, you should've removed the mode check altogether to return the\n>> original behaviour where the dynamic_lb is set to 1 (enabled) by\n>> default and then ALB mode would've had it, of course that would've left\n>> the case of setting it to 0 in TLB mode and switching to ALB, but that\n>> is a different issue.\n> \n> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\n> tlb_dynamic_lb is referenced in the following functions.\n> \n> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n> + bond_tlb_xmit() -- Only used by balance-tlb\n> + bond_open() -- Used by both balance-tlb and balance-alb\n> + bond_check_params() -- Used during module initialization\n> + bond_fill_info() -- Used to get/set value\n> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n> + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n> \n> The following untested patch adds code to make balance-alb work as if\n> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\n> also reverts my previous patch.\n> \n> What do you think about this approach?\n> ---\n> Kosuke TATSUKAWA | 1st Platform Software Division\n> | NEC Solution Innovators\n> | tatsu@ab.jp.nec.com\n> \n\nLogically the approach looks good, that being said it adds unnecessary tests in\nthe fast path, why not just something like the patch below ? That only leaves the\nproblem if it is zeroed in TLB and switched to ALB mode, and that is a one line\nfix to unsuppmodes just allow it to be set for that specific case. The below\nreturns the default behaviour before the commit in your Fixes tag.\n\n\ndiff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\nindex fc63992ab0e0..c99dc59d729b 100644\n--- a/drivers/net/bonding/bond_main.c\n+++ b/drivers/net/bonding/bond_main.c\n@@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)\n \tint bond_mode\t= BOND_MODE_ROUNDROBIN;\n \tint xmit_hashtype = BOND_XMIT_POLICY_LAYER2;\n \tint lacp_fast = 0;\n-\tint tlb_dynamic_lb = 0;\n+\tint tlb_dynamic_lb;\n \n \t/* Convert string parameters. */\n \tif (mode) {\n@@ -4601,16 +4601,13 @@ static int bond_check_params(struct bond_params *params)\n \t}\n \tad_user_port_key = valptr->value;\n \n-\tif ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n-\t\tbond_opt_initstr(&newval, \"default\");\n-\t\tvalptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n-\t\t\t\t\t&newval);\n-\t\tif (!valptr) {\n-\t\t\tpr_err(\"Error: No tlb_dynamic_lb default value\");\n-\t\t\treturn -EINVAL;\n-\t\t}\n-\t\ttlb_dynamic_lb = valptr->value;\n+\tbond_opt_initstr(&newval, \"default\");\n+\tvalptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB), &newval);\n+\tif (!valptr) {\n+\t\tpr_err(\"Error: No tlb_dynamic_lb default value\");\n+\t\treturn -EINVAL;\n \t}\n+\ttlb_dynamic_lb = valptr->value;\n \n \tif (lp_interval == 0) {\n \t\tpr_warn(\"Warning: ip_interval must be between 1 and %d, so it was reset to %d\\n\",","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"Ow9s/3gy\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpY3n2Sx1z9sDB\n\tfor ;\n\tFri, 8 Sep 2017 20:10:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753740AbdIHKKT (ORCPT );\n\tFri, 8 Sep 2017 06:10:19 -0400","from mail-wr0-f180.google.com ([209.85.128.180]:34920 \"EHLO\n\tmail-wr0-f180.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751577AbdIHKKR (ORCPT\n\t); Fri, 8 Sep 2017 06:10:17 -0400","by mail-wr0-f180.google.com with SMTP id m18so3789269wrm.2\n\tfor ; Fri, 08 Sep 2017 03:10:16 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\tz14sm803060wre.35.2017.09.08.03.10.13\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 03:10:14 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=2+2hYZB3QwK8iHHlbCh9PZkh3OjPhW33g2H/mB7kAZY=;\n\tb=Ow9s/3gyth23rOm4CJH2zkLQUF83FnyB59GdCV1JxzIbN3W+dBdqk0LEPq8p4Ka5vy\n\tDMe2L1pUHjSwCgdr14QFIa0HLB4iSkaFg7qBRbau2RYWa2DDtqA6Mdf3o3OxAySky3sF\n\tWUYG/luT9u8ApwW14VkjQff94B5XXtfa+L+l0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=2+2hYZB3QwK8iHHlbCh9PZkh3OjPhW33g2H/mB7kAZY=;\n\tb=XGeuXCvcuF39qf3Z8cQB6QasyHEGOhtDsAZcNDh1wmkNbaQQHTFE0EdmzsiZotr/hu\n\tEPRm7GTnKfhcGgZ7jWOCcOqGjY7r/nE7ry2/oWu2K+F5wMsxwWQ7OkuOOthVZACOoTAa\n\t33Sx1YpPrxCHhUqNMBM3cWvSgyNTuXkV0L3WwvboPFgxxSDPX+UyorkvobFlGSzCK2MI\n\tUxjiSVJjwmh869zvmjRz9uqLBmkx2VfqULTcIYqWhV8oRdXZG8Y+GZdRxFJgwtr6m7ME\n\tNKLu7NMOZ6++/O6kskCKKJyYlRrw4yBiMgeqRyVXHSv6rr6kDusay76fxJYQIINVGaah\n\tNEoQ==","X-Gm-Message-State":"AHPjjUjACkF2GpCBeqXC+OsgbCLJ4QgRDlWAeDKzJnktm3/rKeKwEVRo\n\t6ABbhtEDWt+STSFa","X-Google-Smtp-Source":"ADKCNb402GKWKpPdH4ZbucwUzO3+NbhbgNsGDTcoHPX1U9b/xU5CX9yvdgwAlHFw4WJsI7yvEQi6RA==","X-Received":"by 10.223.196.161 with SMTP id m30mr1531994wrf.187.1504865415511;\n\tFri, 08 Sep 2017 03:10:15 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Kosuke Tatsukawa ","References":"<17EC94B0A072C34B8DCF0D30AD16044A029864AC@BPXM09GP.gisp.nec.co.jp>","Cc":"Jay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis 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\n\tIcedove/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":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765220,"web_url":"http://patchwork.ozlabs.org/comment/1765220/","msgid":"<99818f9e-7ee0-4e53-b2be-b61b958f87e7@cumulusnetworks.com>","list_archive_url":null,"date":"2017-09-08T10:13:20","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 08/09/17 13:10, Nikolay Aleksandrov wrote:\n> On 08/09/17 05:06, Kosuke Tatsukawa wrote:\n>> Hi,\n>>\n>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>>> (\"bonding: remove hardcoded value\").\n>>>>\n>>>> It turned out that my previous patch only fixed the case when\n>>>> balance-alb was specified as bonding module parameter, and not when\n>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>>> to the default mode of the bonding interface, which happens to be\n>>>> balance-rr.\n>>>>\n>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>>\n>>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>>> other modes, because \"mode\" is usually set up only once during\n>>>> initialization, and it's not worthwhile to change the static variable\n>>>> bonding_defaults in bond_main.c to a global variable just for this\n>>>> purpose.\n>>>>\n>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>>> the default value back and forth for balance-tlb.\n>>>>\n>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>>> is not an intended usage, so there is little use making it writable at\n>>>> this moment.\n>>>>\n>>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>>> Reported-by: Reinis Rozitis \n>>>> Signed-off-by: Kosuke Tatsukawa \n>>>> Cc: stable@vger.kernel.org # v4.12+\n>>>> ---\n>>>> drivers/net/bonding/bond_options.c | 3 +++\n>>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>>\n>>>\n>>> I don't believe this to be the right solution, hardcoding it like this\n>>> changes user-visible behaviour. The issue is that if someone configured\n>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n>>> override their config if they switch the mode to alb. Also it robs users\n>>> from their choice.\n>>>\n>>> If you think this should be settable in ALB mode, then IMO you should\n>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n>>> That would also be consistent with how it's handled in TLB mode.\n>>\n>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at\n>> this point. All the current commits regarding tlb_dynamic_lb are for\n>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\n>> to 0 is an intended usage.\n>>\n>>\n>>> Going back and looking at your previous fix I'd argue that it is also\n>>> wrong, you should've removed the mode check altogether to return the\n>>> original behaviour where the dynamic_lb is set to 1 (enabled) by\n>>> default and then ALB mode would've had it, of course that would've left\n>>> the case of setting it to 0 in TLB mode and switching to ALB, but that\n>>> is a different issue.\n>>\n>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\n>> tlb_dynamic_lb is referenced in the following functions.\n>>\n>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n>> + bond_tlb_xmit() -- Only used by balance-tlb\n>> + bond_open() -- Used by both balance-tlb and balance-alb\n>> + bond_check_params() -- Used during module initialization\n>> + bond_fill_info() -- Used to get/set value\n>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n>>\n>> The following untested patch adds code to make balance-alb work as if\n>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\n>> also reverts my previous patch.\n>>\n>> What do you think about this approach?\n>> ---\n>> Kosuke TATSUKAWA | 1st Platform Software Division\n>> | NEC Solution Innovators\n>> | tatsu@ab.jp.nec.com\n>>\n> \n> Logically the approach looks good, that being said it adds unnecessary tests in\n> the fast path, why not just something like the patch below ? That only leaves the\n> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line\n> fix to unsuppmodes just allow it to be set for that specific case. The below\n> returns the default behaviour before the commit in your Fixes tag.\n> \n> \n\nActually I'm fine with your approach, too. It will fix this regardless of the\nvalue of tlb_dynamic_lb which sounds good to me for the price of a test in\nthe fast path.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"QZ7V8q06\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpY7R25wfz9s3T\n\tfor ;\n\tFri, 8 Sep 2017 20:13:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754599AbdIHKNZ (ORCPT );\n\tFri, 8 Sep 2017 06:13:25 -0400","from mail-wm0-f43.google.com ([74.125.82.43]:46469 \"EHLO\n\tmail-wm0-f43.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751556AbdIHKNY (ORCPT\n\t); Fri, 8 Sep 2017 06:13:24 -0400","by mail-wm0-f43.google.com with SMTP id i189so1931386wmf.1\n\tfor ; Fri, 08 Sep 2017 03:13:23 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\tc83sm1071013wmd.2.2017.09.08.03.13.20\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 03:13:21 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=xOw9RqabCuAjXr4a/W7cpjp0bOm8hf9meRAJlrU5SZo=;\n\tb=QZ7V8q067ho9I5S50h+BgydodEYAuodoalnNuXebg7RuP7NyGCPqQonbwND2he7SBW\n\tMjRhwED5AJvvfI+kYZ5EwCX5ybEbM5Xj6a63IB9663pDmwLkyjKLBPq1Tr1ZcZvEQrQD\n\tkeECCKjnKqBsVuprBdCcMmBq2ffuklyA1YnqQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=xOw9RqabCuAjXr4a/W7cpjp0bOm8hf9meRAJlrU5SZo=;\n\tb=BtfVqXaqrz3eIYRxr9xznkIwURufBm4LAkzxnBUBpVHTaAA2Ruy3fgFnQCF4el1rVT\n\tfgbu7RyYTMlkikXFWEkEaKj+dzrU2+VCKz5FuYeQIFdqsj25ODC5pBEIdc4QaxsKRxWw\n\t0c8lahskQ1ji5Dn/WOXmL7u4KNzqYJkMR8kbCeq8vKPXpve8lHduJYQImlkdpmh9vzfn\n\t+im+9zI8beMCl3DWlbpNTVX+pLO8lZRL2y2OUv+buKosa1cMK9Y2c04msA+v+Xdond+T\n\tcxS2F2B4zxyyMAWsgjyaamW3Js+M4O8zs3dHAfB5xMTIsnBCK19khKssV93Qs35ossV2\n\tDopQ==","X-Gm-Message-State":"AHPjjUgVO2+Skif69ZZ5wQNlrQHhMhgjgTPLERtvo+3xXwL0+oRsXHkO\n\t11WyWkR1xGV7SaaJssQYWblnjw==","X-Google-Smtp-Source":"AOwi7QBYRSfbTA/Xx8UZSAKscWnS5mH4t5BTUYpQu87oqagg1ex1tpVcTS72daB3jDiiOd0oIIX9FA==","X-Received":"by 10.28.159.6 with SMTP id i6mr1414829wme.119.1504865602469;\n\tFri, 08 Sep 2017 03:13:22 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Kosuke Tatsukawa ","References":"<17EC94B0A072C34B8DCF0D30AD16044A029864AC@BPXM09GP.gisp.nec.co.jp>\n\t<52a22af7-cc99-e441-79fa-e48da283b001@cumulusnetworks.com>","Cc":"Jay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis 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\n\tIcedove/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":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765411,"web_url":"http://patchwork.ozlabs.org/comment/1765411/","msgid":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>","list_archive_url":null,"date":"2017-09-08T14:17:03","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs ","submitter":{"id":67390,"url":"http://patchwork.ozlabs.org/api/people/67390/","name":"Kosuke Tatsukawa","email":"tatsu@ab.jp.nec.com"},"content":"Hi,\n\n> On 08/09/17 13:10, Nikolay Aleksandrov wrote:\n>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:\n>>> Hi,\n>>>\n>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>>>> (\"bonding: remove hardcoded value\").\n>>>>>\n>>>>> It turned out that my previous patch only fixed the case when\n>>>>> balance-alb was specified as bonding module parameter, and not when\n>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>>>> to the default mode of the bonding interface, which happens to be\n>>>>> balance-rr.\n>>>>>\n>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>>>\n>>>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>>>> other modes, because \"mode\" is usually set up only once during\n>>>>> initialization, and it's not worthwhile to change the static variable\n>>>>> bonding_defaults in bond_main.c to a global variable just for this\n>>>>> purpose.\n>>>>>\n>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>>>> the default value back and forth for balance-tlb.\n>>>>>\n>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>>>> is not an intended usage, so there is little use making it writable at\n>>>>> this moment.\n>>>>>\n>>>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>>>> Reported-by: Reinis Rozitis \n>>>>> Signed-off-by: Kosuke Tatsukawa \n>>>>> Cc: stable@vger.kernel.org # v4.12+\n>>>>> ---\n>>>>> drivers/net/bonding/bond_options.c | 3 +++\n>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>>>\n>>>>\n>>>> I don't believe this to be the right solution, hardcoding it like this\n>>>> changes user-visible behaviour. The issue is that if someone configured\n>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n>>>> override their config if they switch the mode to alb. Also it robs users\n>>>> from their choice.\n>>>>\n>>>> If you think this should be settable in ALB mode, then IMO you should\n>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n>>>> That would also be consistent with how it's handled in TLB mode.\n>>>\n>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at\n>>> this point. All the current commits regarding tlb_dynamic_lb are for\n>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\n>>> to 0 is an intended usage.\n>>>\n>>>\n>>>> Going back and looking at your previous fix I'd argue that it is also\n>>>> wrong, you should've removed the mode check altogether to return the\n>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by\n>>>> default and then ALB mode would've had it, of course that would've left\n>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that\n>>>> is a different issue.\n>>>\n>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\n>>> tlb_dynamic_lb is referenced in the following functions.\n>>>\n>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n>>> + bond_tlb_xmit() -- Only used by balance-tlb\n>>> + bond_open() -- Used by both balance-tlb and balance-alb\n>>> + bond_check_params() -- Used during module initialization\n>>> + bond_fill_info() -- Used to get/set value\n>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n>>>\n>>> The following untested patch adds code to make balance-alb work as if\n>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\n>>> also reverts my previous patch.\n>>>\n>>> What do you think about this approach?\n>>> ---\n>>> Kosuke TATSUKAWA | 1st Platform Software Division\n>>> | NEC Solution Innovators\n>>> | tatsu@ab.jp.nec.com\n>>>\n>> \n>> Logically the approach looks good, that being said it adds unnecessary tests in\n>> the fast path, why not just something like the patch below ? That only leaves the\n>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line\n>> fix to unsuppmodes just allow it to be set for that specific case. The below\n>> returns the default behaviour before the commit in your Fixes tag.\n>> \n>> \n> \n> Actually I'm fine with your approach, too. It will fix this regardless of the\n> value of tlb_dynamic_lb which sounds good to me for the price of a test in\n> the fast path.\n\nIf you're concerned about the additional test in the fast path, how\nabout the patch below. I've added an arguemnt to bond_do_alb_xmit()\nto handle both balance-tlb and balance-alb similary.\n\nI'm not sure if this causes any problem if tlb_dynamic_lb is changed\nwhile calling bond_do_alb_xmit() in balance-tlb mode.\n---\nKosuke TATSUKAWA | 1st Platform Software Division\n | NEC Solution Innovators\n | tatsu@ab.jp.nec.com\n\n------------------------------------------------------------------------\n drivers/net/bonding/bond_alb.c | 11 ++++++-----\n drivers/net/bonding/bond_main.c | 5 +++--\n 2 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c\nindex c02cc81..7710f20 100644\n--- a/drivers/net/bonding/bond_alb.c\n+++ b/drivers/net/bonding/bond_alb.c\n@@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)\n }\n \n static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n-\t\t\t struct slave *tx_slave)\n+\t\t\t struct slave *tx_slave, int tlb_dynamic_lb)\n {\n \tstruct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));\n \tstruct ethhdr *eth_data = eth_hdr(skb);\n@@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n \tif (!tx_slave) {\n \t\t/* unbalanced or unassigned, send through primary */\n \t\ttx_slave = rcu_dereference(bond->curr_active_slave);\n-\t\tif (bond->params.tlb_dynamic_lb)\n+\t\tif (tlb_dynamic_lb)\n \t\t\tbond_info->unbalanced_load += skb->len;\n \t}\n \n@@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n \t\tgoto out;\n \t}\n \n-\tif (tx_slave && bond->params.tlb_dynamic_lb) {\n+\tif (tx_slave && tlb_dynamic_lb) {\n \t\tspin_lock(&bond->mode_lock);\n \t\t__tlb_clear_slave(bond, tx_slave, 0);\n \t\tspin_unlock(&bond->mode_lock);\n@@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n \t\t\tbreak;\n \t\t}\n \t}\n-\treturn bond_do_alb_xmit(skb, bond, tx_slave);\n+\treturn bond_do_alb_xmit(skb, bond, tx_slave,\n+\t\t\t\tbond->params.tlb_dynamic_lb);\n }\n \n int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n@@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n \t\ttx_slave = tlb_choose_channel(bond, hash_index, skb->len);\n \t}\n \n-\treturn bond_do_alb_xmit(skb, bond, tx_slave);\n+\treturn bond_do_alb_xmit(skb, bond, tx_slave, 1);\n }\n \n void bond_alb_monitor(struct work_struct *work)\ndiff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\nindex fc63992..bcb71e7 100644\n--- a/drivers/net/bonding/bond_main.c\n+++ b/drivers/net/bonding/bond_main.c\n@@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)\n \t\t */\n \t\tif (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))\n \t\t\treturn -ENOMEM;\n-\t\tif (bond->params.tlb_dynamic_lb)\n+\t\tif (bond->params.tlb_dynamic_lb ||\n+\t\t (BOND_MODE(bond) == BOND_MODE_TLB))\n \t\t\tqueue_delayed_work(bond->wq, &bond->alb_work, 0);\n \t}\n \n@@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)\n \t}\n \tad_user_port_key = valptr->value;\n \n-\tif ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n+\tif (bond_mode == BOND_MODE_TLB) {\n \t\tbond_opt_initstr(&newval, \"default\");\n \t\tvalptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n \t\t\t\t\t&newval);","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpfYv6WdLz9sCZ\n\tfor ;\n\tSat, 9 Sep 2017 00:18:27 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755765AbdIHOSP convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 8 Sep 2017 10:18:15 -0400","from tyo162.gate.nec.co.jp ([114.179.232.162]:37847 \"EHLO\n\ttyo162.gate.nec.co.jp\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754599AbdIHOSN (ORCPT\n\t); Fri, 8 Sep 2017 10:18:13 -0400","from mailgate02.nec.co.jp ([114.179.233.122])\n\tby tyo162.gate.nec.co.jp (8.15.1/8.15.1) with ESMTPS id\n\tv88EI2oh010733\n\t(version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO);\n\tFri, 8 Sep 2017 23:18:02 +0900","from mailsv01.nec.co.jp (mailgate-v.nec.co.jp [10.204.236.94])\n\tby mailgate02.nec.co.jp (8.15.1/8.15.1) with ESMTP id v88EI2Lw005271; \n\tFri, 8 Sep 2017 23:18:02 +0900","from mail03.kamome.nec.co.jp (mail03.kamome.nec.co.jp [10.25.43.7])\n\tby mailsv01.nec.co.jp (8.15.1/8.15.1) with ESMTP id v88EHWvQ016868;\n\tFri, 8 Sep 2017 23:18:02 +0900","from bpxc99gp.gisp.nec.co.jp ([10.38.151.140] [10.38.151.140]) by\n\tmail01b.kamome.nec.co.jp with ESMTP id BT-MMP-2958880;\n\tFri, 8 Sep 2017 23:17:05 +0900","from BPXM09GP.gisp.nec.co.jp ([10.38.151.201]) by\n\tBPXC12GP.gisp.nec.co.jp ([10.38.151.140]) with mapi id 14.03.0319.002;\n\tFri, 8 Sep 2017 23:17:05 +0900"],"From":"Kosuke Tatsukawa ","To":"Nikolay Aleksandrov ","CC":"Jay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs ","Thread-Topic":"[PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs ","Thread-Index":"AdMorSTk0sBvF5aHTv+qn4ITKRSxcg==","Date":"Fri, 8 Sep 2017 14:17:03 +0000","Message-ID":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>","In-Reply-To":"<99818f9e-7ee0-4e53-b2be-b61b958f87e7@cumulusnetworks.com>","Accept-Language":"ja-JP, en-US","Content-Language":"ja-JP","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.34.125.78]","Content-Type":"text/plain; charset=\"iso-2022-jp\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-TM-AS-MML":"disable","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765419,"web_url":"http://patchwork.ozlabs.org/comment/1765419/","msgid":"<03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>","list_archive_url":null,"date":"2017-09-08T14:30:38","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 08/09/17 17:17, Kosuke Tatsukawa wrote:\n> Hi,\n> \n>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:\n>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:\n>>>> Hi,\n>>>>\n>>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>>>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>>>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>>>>> (\"bonding: remove hardcoded value\").\n>>>>>>\n>>>>>> It turned out that my previous patch only fixed the case when\n>>>>>> balance-alb was specified as bonding module parameter, and not when\n>>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>>>>> to the default mode of the bonding interface, which happens to be\n>>>>>> balance-rr.\n>>>>>>\n>>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>>>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>>>>\n>>>>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>>>>> other modes, because \"mode\" is usually set up only once during\n>>>>>> initialization, and it's not worthwhile to change the static variable\n>>>>>> bonding_defaults in bond_main.c to a global variable just for this\n>>>>>> purpose.\n>>>>>>\n>>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>>>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>>>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>>>>> the default value back and forth for balance-tlb.\n>>>>>>\n>>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>>>>> is not an intended usage, so there is little use making it writable at\n>>>>>> this moment.\n>>>>>>\n>>>>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>>>>> Reported-by: Reinis Rozitis \n>>>>>> Signed-off-by: Kosuke Tatsukawa \n>>>>>> Cc: stable@vger.kernel.org # v4.12+\n>>>>>> ---\n>>>>>> drivers/net/bonding/bond_options.c | 3 +++\n>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>>>>\n>>>>>\n>>>>> I don't believe this to be the right solution, hardcoding it like this\n>>>>> changes user-visible behaviour. The issue is that if someone configured\n>>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n>>>>> override their config if they switch the mode to alb. Also it robs users\n>>>>> from their choice.\n>>>>>\n>>>>> If you think this should be settable in ALB mode, then IMO you should\n>>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n>>>>> That would also be consistent with how it's handled in TLB mode.\n>>>>\n>>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at\n>>>> this point. All the current commits regarding tlb_dynamic_lb are for\n>>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\n>>>> to 0 is an intended usage.\n>>>>\n>>>>\n>>>>> Going back and looking at your previous fix I'd argue that it is also\n>>>>> wrong, you should've removed the mode check altogether to return the\n>>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by\n>>>>> default and then ALB mode would've had it, of course that would've left\n>>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that\n>>>>> is a different issue.\n>>>>\n>>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\n>>>> tlb_dynamic_lb is referenced in the following functions.\n>>>>\n>>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n>>>> + bond_tlb_xmit() -- Only used by balance-tlb\n>>>> + bond_open() -- Used by both balance-tlb and balance-alb\n>>>> + bond_check_params() -- Used during module initialization\n>>>> + bond_fill_info() -- Used to get/set value\n>>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n>>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n>>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n>>>>\n>>>> The following untested patch adds code to make balance-alb work as if\n>>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\n>>>> also reverts my previous patch.\n>>>>\n>>>> What do you think about this approach?\n>>>> ---\n>>>> Kosuke TATSUKAWA | 1st Platform Software Division\n>>>> | NEC Solution Innovators\n>>>> | tatsu@ab.jp.nec.com\n>>>>\n>>>\n>>> Logically the approach looks good, that being said it adds unnecessary tests in\n>>> the fast path, why not just something like the patch below ? That only leaves the\n>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line\n>>> fix to unsuppmodes just allow it to be set for that specific case. The below\n>>> returns the default behaviour before the commit in your Fixes tag.\n>>>\n>>>\n>>\n>> Actually I'm fine with your approach, too. It will fix this regardless of the\n>> value of tlb_dynamic_lb which sounds good to me for the price of a test in\n>> the fast path.\n> \n> If you're concerned about the additional test in the fast path, how\n> about the patch below. I've added an arguemnt to bond_do_alb_xmit()\n> to handle both balance-tlb and balance-alb similary.\n> \n\nEven better, looks great! 1 question below though.\n\n> I'm not sure if this causes any problem if tlb_dynamic_lb is changed\n> while calling bond_do_alb_xmit() in balance-tlb mode.\n\nThe option has the ifdown flag, you shouldn't be able to change it while\nthe bond dev is up, but even if you could I don't think it will be an issue\nfor the xmit.\n\n> ---\n> Kosuke TATSUKAWA | 1st Platform Software Division\n> | NEC Solution Innovators\n> | tatsu@ab.jp.nec.com\n> \n> ------------------------------------------------------------------------\n> drivers/net/bonding/bond_alb.c | 11 ++++++-----\n> drivers/net/bonding/bond_main.c | 5 +++--\n> 2 files changed, 9 insertions(+), 7 deletions(-)\n> \n> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c\n> index c02cc81..7710f20 100644\n> --- a/drivers/net/bonding/bond_alb.c\n> +++ b/drivers/net/bonding/bond_alb.c\n> @@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)\n> }\n> \n> static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n> -\t\t\t struct slave *tx_slave)\n> +\t\t\t struct slave *tx_slave, int tlb_dynamic_lb)\n> {\n> \tstruct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));\n> \tstruct ethhdr *eth_data = eth_hdr(skb);\n> @@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n> \tif (!tx_slave) {\n> \t\t/* unbalanced or unassigned, send through primary */\n> \t\ttx_slave = rcu_dereference(bond->curr_active_slave);\n> -\t\tif (bond->params.tlb_dynamic_lb)\n> +\t\tif (tlb_dynamic_lb)\n> \t\t\tbond_info->unbalanced_load += skb->len;\n> \t}\n> \n> @@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n> \t\tgoto out;\n> \t}\n> \n> -\tif (tx_slave && bond->params.tlb_dynamic_lb) {\n> +\tif (tx_slave && tlb_dynamic_lb) {\n> \t\tspin_lock(&bond->mode_lock);\n> \t\t__tlb_clear_slave(bond, tx_slave, 0);\n> \t\tspin_unlock(&bond->mode_lock);\n> @@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> -\treturn bond_do_alb_xmit(skb, bond, tx_slave);\n> +\treturn bond_do_alb_xmit(skb, bond, tx_slave,\n> +\t\t\t\tbond->params.tlb_dynamic_lb);\n> }\n> \n> int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n> @@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n> \t\ttx_slave = tlb_choose_channel(bond, hash_index, skb->len);\n> \t}\n> \n> -\treturn bond_do_alb_xmit(skb, bond, tx_slave);\n> +\treturn bond_do_alb_xmit(skb, bond, tx_slave, 1);\n> }\n> \n> void bond_alb_monitor(struct work_struct *work)\n> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\n> index fc63992..bcb71e7 100644\n> --- a/drivers/net/bonding/bond_main.c\n> +++ b/drivers/net/bonding/bond_main.c\n> @@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)\n> \t\t */\n> \t\tif (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))\n> \t\t\treturn -ENOMEM;\n> -\t\tif (bond->params.tlb_dynamic_lb)\n> +\t\tif (bond->params.tlb_dynamic_lb ||\n> +\t\t (BOND_MODE(bond) == BOND_MODE_TLB))\n\nmode == tlb ? shouldn't this check be for alb ?\n\n> \t\t\tqueue_delayed_work(bond->wq, &bond->alb_work, 0);\n> \t}\n> \n> @@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)\n> \t}\n> \tad_user_port_key = valptr->value;\n> \n> -\tif ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n> +\tif (bond_mode == BOND_MODE_TLB) {\n> \t\tbond_opt_initstr(&newval, \"default\");\n> \t\tvalptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n> \t\t\t\t\t&newval);\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"dM1v2D+4\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpfrs6BCBz9s7G\n\tfor ;\n\tSat, 9 Sep 2017 00:31:25 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755960AbdIHOan (ORCPT );\n\tFri, 8 Sep 2017 10:30:43 -0400","from mail-wr0-f177.google.com ([209.85.128.177]:33390 \"EHLO\n\tmail-wr0-f177.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1755880AbdIHOal (ORCPT\n\t); Fri, 8 Sep 2017 10:30:41 -0400","by mail-wr0-f177.google.com with SMTP id a43so5055683wrc.0\n\tfor ; Fri, 08 Sep 2017 07:30:40 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\tc46sm2429047wrg.92.2017.09.08.07.30.38\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 07:30:39 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=KyhRzdfFLOm1MyizVL+INYptmQJJNCx0adzkn+XxDCw=;\n\tb=dM1v2D+4JzIBVSAvs3anZreIhxgbAVHjWlARHs5nknJEnEwf7UdR3ZTXUJKzs3wLKT\n\trF9v7kckkYIk4Qpq7r1B/vOSFajH8O3XivPf5BKxMQvUjhAVF29WimuiznOXjYz81E+1\n\tbt6Iw6j6kZTACvZWTQ72+rwetherDaaXpnxFI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=KyhRzdfFLOm1MyizVL+INYptmQJJNCx0adzkn+XxDCw=;\n\tb=A5KuHDdD+6gTb9eEPm3euloomYHHGHktKyazWUGgUQNL2hNHhrCZsxGwiDEWkmwzpb\n\t+HmF9QpRo+yaSSxlBiPNde4gd6RDvH06qtWVy7SOLhNN9QBkMa/EKvzO5qlN/PKWizfy\n\tuyYq/EfRRRgxKAt7S3B+na+vh+40TH/IpeebUzFTJFQXl4SbsxLCOrAV3wCeRZ90FYnu\n\tVfL1chAiv4hb3a1LFX/odO5X9vppGE1MM63FI5v49YROpGjrdwOdwCOWdJxOFnEJMGdA\n\tG3vaXR3D4ShXisN3/dHkHKWznGYlxiq9G4WAohWR+F2CtFD/po5ibBvkmjeZJbxlioCp\n\teUYA==","X-Gm-Message-State":"AHPjjUjJjn/c6AiOBKnw9xFtrnYjXDBWnEq633La7hiJbTh56CMmsQIu\n\tjxPggRHZ7CUzUE6O","X-Google-Smtp-Source":"ADKCNb7JB8KBuxcWvbk559a5uo6bf1uItUfHWjVspJIl5wbWMrK5ygHCGfNZyrSvqh/+QJlNDxMZ1w==","X-Received":"by 10.223.168.105 with SMTP id l96mr2302195wrc.248.1504881039932;\n\tFri, 08 Sep 2017 07:30:39 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Kosuke Tatsukawa ","References":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>","Cc":"Jay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","From":"Nikolay Aleksandrov ","X-Enigmail-Draft-Status":"N1110","Message-ID":"<03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>","Date":"Fri, 8 Sep 2017 17:30:38 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tIcedove/45.6.0","MIME-Version":"1.0","In-Reply-To":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>","Content-Type":"text/plain; charset=iso-2022-jp","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765664,"web_url":"http://patchwork.ozlabs.org/comment/1765664/","msgid":"","list_archive_url":null,"date":"2017-09-08T23:54:48","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":6061,"url":"http://patchwork.ozlabs.org/api/people/6061/","name":"Mahesh Bandewar (महेश बंडेवार)","email":"maheshb@google.com"},"content":"On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov\n wrote:\n> On 08/09/17 17:17, Kosuke Tatsukawa wrote:\n>> Hi,\n>>\n>>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:\n>>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:\n>>>>> Hi,\n>>>>>\n>>>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>>>>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>>>>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>>>>>> (\"bonding: remove hardcoded value\").\n>>>>>>>\n>>>>>>> It turned out that my previous patch only fixed the case when\n>>>>>>> balance-alb was specified as bonding module parameter, and not when\n>>>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>>>>>> to the default mode of the bonding interface, which happens to be\n>>>>>>> balance-rr.\n>>>>>>>\n>>>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>>>>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>>>>>\n>>>>>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>>>>>> other modes, because \"mode\" is usually set up only once during\n>>>>>>> initialization, and it's not worthwhile to change the static variable\n>>>>>>> bonding_defaults in bond_main.c to a global variable just for this\n>>>>>>> purpose.\n>>>>>>>\n>>>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>>>>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>>>>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>>>>>> the default value back and forth for balance-tlb.\n>>>>>>>\n>>>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>>>>>> is not an intended usage, so there is little use making it writable at\n>>>>>>> this moment.\n>>>>>>>\n>>>>>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>>>>>> Reported-by: Reinis Rozitis \n>>>>>>> Signed-off-by: Kosuke Tatsukawa \n>>>>>>> Cc: stable@vger.kernel.org # v4.12+\n>>>>>>> ---\n>>>>>>> drivers/net/bonding/bond_options.c | 3 +++\n>>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>>>>>\n>>>>>>\n>>>>>> I don't believe this to be the right solution, hardcoding it like this\n>>>>>> changes user-visible behaviour. The issue is that if someone configured\n>>>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n>>>>>> override their config if they switch the mode to alb. Also it robs users\n>>>>>> from their choice.\n>>>>>>\n>>>>>> If you think this should be settable in ALB mode, then IMO you should\n>>>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n>>>>>> That would also be consistent with how it's handled in TLB mode.\n>>>>>\n>>>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at\n>>>>> this point. All the current commits regarding tlb_dynamic_lb are for\n>>>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\n>>>>> to 0 is an intended usage.\n>>>>>\n>>>>>\n>>>>>> Going back and looking at your previous fix I'd argue that it is also\n>>>>>> wrong, you should've removed the mode check altogether to return the\n>>>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by\n>>>>>> default and then ALB mode would've had it, of course that would've left\n>>>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that\n>>>>>> is a different issue.\n>>>>>\n>>>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\n>>>>> tlb_dynamic_lb is referenced in the following functions.\n>>>>>\n>>>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n>>>>> + bond_tlb_xmit() -- Only used by balance-tlb\n>>>>> + bond_open() -- Used by both balance-tlb and balance-alb\n>>>>> + bond_check_params() -- Used during module initialization\n>>>>> + bond_fill_info() -- Used to get/set value\n>>>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n>>>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n>>>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n>>>>>\n>>>>> The following untested patch adds code to make balance-alb work as if\n>>>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\n>>>>> also reverts my previous patch.\n>>>>>\n>>>>> What do you think about this approach?\n>>>>> ---\n>>>>> Kosuke TATSUKAWA | 1st Platform Software Division\n>>>>> | NEC Solution Innovators\n>>>>> | tatsu@ab.jp.nec.com\n>>>>>\n>>>>\n>>>> Logically the approach looks good, that being said it adds unnecessary tests in\n>>>> the fast path, why not just something like the patch below ? That only leaves the\n>>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line\n>>>> fix to unsuppmodes just allow it to be set for that specific case. The below\n>>>> returns the default behaviour before the commit in your Fixes tag.\n>>>>\n>>>>\n>>>\n>>> Actually I'm fine with your approach, too. It will fix this regardless of the\n>>> value of tlb_dynamic_lb which sounds good to me for the price of a test in\n>>> the fast path.\n>>\n>> If you're concerned about the additional test in the fast path, how\n>> about the patch below. I've added an arguemnt to bond_do_alb_xmit()\n>> to handle both balance-tlb and balance-alb similary.\n>>\n>\n> Even better, looks great! 1 question below though.\n>\n>> I'm not sure if this causes any problem if tlb_dynamic_lb is changed\n>> while calling bond_do_alb_xmit() in balance-tlb mode.\n>\n> The option has the ifdown flag, you shouldn't be able to change it while\n> the bond dev is up, but even if you could I don't think it will be an issue\n> for the xmit.\n>\n>> ---\n>> Kosuke TATSUKAWA | 1st Platform Software Division\n>> | NEC Solution Innovators\n>> | tatsu@ab.jp.nec.com\n>>\n>> ------------------------------------------------------------------------\n>> drivers/net/bonding/bond_alb.c | 11 ++++++-----\n>> drivers/net/bonding/bond_main.c | 5 +++--\n>> 2 files changed, 9 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c\n>> index c02cc81..7710f20 100644\n>> --- a/drivers/net/bonding/bond_alb.c\n>> +++ b/drivers/net/bonding/bond_alb.c\n>> @@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)\n>> }\n>>\n>> static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n>> - struct slave *tx_slave)\n>> + struct slave *tx_slave, int tlb_dynamic_lb)\n>> {\n>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));\n>> struct ethhdr *eth_data = eth_hdr(skb);\n>> @@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n>> if (!tx_slave) {\n>> /* unbalanced or unassigned, send through primary */\n>> tx_slave = rcu_dereference(bond->curr_active_slave);\n>> - if (bond->params.tlb_dynamic_lb)\n>> + if (tlb_dynamic_lb)\n>> bond_info->unbalanced_load += skb->len;\n>> }\n>>\n>> @@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n>> goto out;\n>> }\n>>\n>> - if (tx_slave && bond->params.tlb_dynamic_lb) {\n>> + if (tx_slave && tlb_dynamic_lb) {\n>> spin_lock(&bond->mode_lock);\n>> __tlb_clear_slave(bond, tx_slave, 0);\n>> spin_unlock(&bond->mode_lock);\n>> @@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n>> break;\n>> }\n>> }\n>> - return bond_do_alb_xmit(skb, bond, tx_slave);\n>> + return bond_do_alb_xmit(skb, bond, tx_slave,\n>> + bond->params.tlb_dynamic_lb);\n>> }\n>>\n>> int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n>> @@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n>> tx_slave = tlb_choose_channel(bond, hash_index, skb->len);\n>> }\n>>\n>> - return bond_do_alb_xmit(skb, bond, tx_slave);\n>> + return bond_do_alb_xmit(skb, bond, tx_slave, 1);\n>> }\n>>\n>> void bond_alb_monitor(struct work_struct *work)\n>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\n>> index fc63992..bcb71e7 100644\n>> --- a/drivers/net/bonding/bond_main.c\n>> +++ b/drivers/net/bonding/bond_main.c\n>> @@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)\n>> */\n>> if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))\n>> return -ENOMEM;\n>> - if (bond->params.tlb_dynamic_lb)\n>> + if (bond->params.tlb_dynamic_lb ||\n>> + (BOND_MODE(bond) == BOND_MODE_TLB))\n>\n> mode == tlb ? shouldn't this check be for alb ?\n>\nActually this is not needed since this is already inside if\n(bond_is_lb()) condition.\n\n>> queue_delayed_work(bond->wq, &bond->alb_work, 0);\n>> }\n>>\n>> @@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)\n>> }\n>> ad_user_port_key = valptr->value;\n>>\n>> - if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n>> + if (bond_mode == BOND_MODE_TLB) {\n>> bond_opt_initstr(&newval, \"default\");\n>> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n>> &newval);\n>>\n>\nI think the underlying issue is that tlb_dynamic_lb should be set to 1\nfor all modes which was not the case when it was getting initialized\nonly forTLB mode. So from that perspective I prefer Nik's patch with a\nsmall variation that guards the case when mode transitions from TLB to\nALB. The reason why I like that patch is because it's simple and\navoids complications.\n\nHere is what I meant -\n\ndiff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\nindex fc63992ab0e0..c99dc59d729b 100644\n--- a/drivers/net/bonding/bond_main.c\n+++ b/drivers/net/bonding/bond_main.c\n@@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)\n int bond_mode = BOND_MODE_ROUNDROBIN;\n int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;\n int lacp_fast = 0;\n- int tlb_dynamic_lb = 0;\n+ int tlb_dynamic_lb;\n\n /* Convert string parameters. */\n if (mode) {\n@@ -4601,16 +4601,13 @@ static int bond_check_params(struct bond_params *params)\n }\n ad_user_port_key = valptr->value;\n\n- if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n- bond_opt_initstr(&newval, \"default\");\n- valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n- &newval);\n- if (!valptr) {\n- pr_err(\"Error: No tlb_dynamic_lb default value\");\n- return -EINVAL;\n- }\n- tlb_dynamic_lb = valptr->value;\n+ bond_opt_initstr(&newval, \"default\");\n+ valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB), &newval);\n+ if (!valptr) {\n+ pr_err(\"Error: No tlb_dynamic_lb default value\");\n+ return -EINVAL;\n }\n+ tlb_dynamic_lb = valptr->value;\n\n if (lp_interval == 0) {\n pr_warn(\"Warning: ip_interval must be between 1 and\n%d, so it was reset to %d\\n\",\ndiff --git a/drivers/net/bonding/bond_options.c\nb/drivers/net/bonding/bond_options.c\nindex a12d603d41c6..7feacd262182 100644\n--- a/drivers/net/bonding/bond_options.c\n+++ b/drivers/net/bonding/bond_options.c\n@@ -754,6 +754,12 @@ static int bond_option_mode_set(struct bonding *bond,\n bond->params.miimon);\n }\n\n+ /* Guard against transition TLB/tlb_dynamic_lb=0 -> ALB mode.\n+ * In ALB mode, tlb_dynamic_lb must be set to 1.\n+ */\n+ if (newval->value == BOND_MODE_ALB && bond->params.tlb_dynamic_lb != 1)\n+ bond->params.tlb_dynamic_lb = 1;\n+\n /* don't cache arp_validate between modes */\n bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;\n bond->params.mode = newval->value;","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"TP/mFi7/\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpvMZ1nJnz9t16\n\tfor ;\n\tSat, 9 Sep 2017 09:55:22 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757287AbdIHXzM (ORCPT );\n\tFri, 8 Sep 2017 19:55:12 -0400","from mail-yw0-f171.google.com ([209.85.161.171]:35633 \"EHLO\n\tmail-yw0-f171.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1757178AbdIHXzK (ORCPT\n\t); Fri, 8 Sep 2017 19:55:10 -0400","by mail-yw0-f171.google.com with SMTP id q80so11372607ywg.2\n\tfor ; Fri, 08 Sep 2017 16:55:09 -0700 (PDT)","by 10.37.161.71 with HTTP; Fri, 8 Sep 2017 16:54:48 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=TQa9b9zYoRN1PhkfMuQI3fnITY0RoZZMZIEL7njOc5g=;\n\tb=TP/mFi7/LOfim1KrhQTnqhl1mBAMmX9irUqwmGttTZLezmXIfDrcqxBofT+AnU4BJ4\n\tKzODzN5kHk5GK/eDHAoti9PpUveyGmFKELInpaFpQUWfWrBG+ubbWbAWDqdnzS5NYe/v\n\te8DoRmprUhj0/6Ugg8a4mB+wB0gko0HmhWfCEqu8mMBs2aSVH97kcLZc7dv6cePBbBga\n\tGBwFfAZlD8lA9s9CZO8roATqvqIqNHe/XErju/jPfxrhxYbTrswOsUbDzKr6t7feNSJ0\n\tR4CBXYlO36FUANADQEt9CifgjFyjpOuGVZxYE02J2VHAlfc8KJuhT/7ALf4FL3rdalq+\n\tVIKw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=TQa9b9zYoRN1PhkfMuQI3fnITY0RoZZMZIEL7njOc5g=;\n\tb=hefYNOLe6iJ0Nubcl3QNI5QEalwfkYMOXrvdDnZhpXTX1hZ1fBRkKp9EBaR0i8XLvo\n\t4FO6ToBFo0Pq8vTIWweg+ooNErQfDN3xrATXlFw7cy/BaCXrPj5WIrCjqTIUQPg/1u7i\n\tRQlWPzvxPAN4uh993ndFTrrrA3UICCNFZcDe0ro6krRMVdiyUTu2IoCQlvb552DROdNc\n\tNqsoaTWgNtVtSe1i1yVvdpQzvXqLDzaz5hciBzfZynS7l7SZzmhAn6kMaJC5/za7qNqz\n\tXu7iMkvrlyIyqJuwecT5FzjxEnLU8qPertyzoCy7sXF5dzRzEYGbJjG6M0brUG6aS2n1\n\tBWlg==","X-Gm-Message-State":"AHPjjUhMFNXT+k5ljDXqw7oaxsdSDEAjwexgUX27WJxK5UyfdPYNDWJ1\n\t9DFO31hVAfGS/Y51VC2A1LmOeVi50Ru0BaKTGQ==","X-Google-Smtp-Source":"ADKCNb7pPJ40J0dWbBP6Oz0bmeVwOW+A7TB/D37jG72DUGIA20oECGlzUtPDRGFXBFZTZVpcTSsrxC8EZmRgxxtgmiI=","X-Received":"by 10.129.46.196 with SMTP id u187mr3770349ywu.268.1504914908713;\n\tFri, 08 Sep 2017 16:55:08 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>","References":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>\n\t<03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>","From":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","Date":"Fri, 8 Sep 2017 16:54:48 -0700","Message-ID":"","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Nikolay Aleksandrov ","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765781,"web_url":"http://patchwork.ozlabs.org/comment/1765781/","msgid":"<9ae82441-78da-0056-88c1-2f9d863fb81f@cumulusnetworks.com>","list_archive_url":null,"date":"2017-09-09T10:29:04","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote:\n> On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov\n> wrote:\n>> On 08/09/17 17:17, Kosuke Tatsukawa wrote:\n>>> Hi,\n>>>\n>>>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:\n>>>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:\n>>>>>> Hi,\n>>>>>>\n>>>>>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:\n>>>>>>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>>>>>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>>>>>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>>>>>>> (\"bonding: remove hardcoded value\").\n>>>>>>>>\n>>>>>>>> It turned out that my previous patch only fixed the case when\n>>>>>>>> balance-alb was specified as bonding module parameter, and not when\n>>>>>>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>>>>>>>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>>>>>>>> to the default mode of the bonding interface, which happens to be\n>>>>>>>> balance-rr.\n>>>>>>>>\n>>>>>>>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>>>>>>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>>>>>>\n>>>>>>>> I didn't add code to change tlb_balance_lb back to the default value for\n>>>>>>>> other modes, because \"mode\" is usually set up only once during\n>>>>>>>> initialization, and it's not worthwhile to change the static variable\n>>>>>>>> bonding_defaults in bond_main.c to a global variable just for this\n>>>>>>>> purpose.\n>>>>>>>>\n>>>>>>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>>>>>>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>>>>>>>> change that behavior, because the value of tlb_balance_lb can be changed\n>>>>>>>> using the sysfs interface for balance-tlb, and I didn't like changing\n>>>>>>>> the default value back and forth for balance-tlb.\n>>>>>>>>\n>>>>>>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>>>>>>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>>>>>>>> is not an intended usage, so there is little use making it writable at\n>>>>>>>> this moment.\n>>>>>>>>\n>>>>>>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>>>>>>> Reported-by: Reinis Rozitis \n>>>>>>>> Signed-off-by: Kosuke Tatsukawa \n>>>>>>>> Cc: stable@vger.kernel.org # v4.12+\n>>>>>>>> ---\n>>>>>>>> drivers/net/bonding/bond_options.c | 3 +++\n>>>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>>>>>>\n>>>>>>>\n>>>>>>> I don't believe this to be the right solution, hardcoding it like this\n>>>>>>> changes user-visible behaviour. The issue is that if someone configured\n>>>>>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently\n>>>>>>> override their config if they switch the mode to alb. Also it robs users\n>>>>>>> from their choice.\n>>>>>>>\n>>>>>>> If you think this should be settable in ALB mode, then IMO you should\n>>>>>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.\n>>>>>>> That would also be consistent with how it's handled in TLB mode.\n>>>>>>\n>>>>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at\n>>>>>> this point. All the current commits regarding tlb_dynamic_lb are for\n>>>>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set\n>>>>>> to 0 is an intended usage.\n>>>>>>\n>>>>>>\n>>>>>>> Going back and looking at your previous fix I'd argue that it is also\n>>>>>>> wrong, you should've removed the mode check altogether to return the\n>>>>>>> original behaviour where the dynamic_lb is set to 1 (enabled) by\n>>>>>>> default and then ALB mode would've had it, of course that would've left\n>>>>>>> the case of setting it to 0 in TLB mode and switching to ALB, but that\n>>>>>>> is a different issue.\n>>>>>>\n>>>>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.\n>>>>>> tlb_dynamic_lb is referenced in the following functions.\n>>>>>>\n>>>>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb\n>>>>>> + bond_tlb_xmit() -- Only used by balance-tlb\n>>>>>> + bond_open() -- Used by both balance-tlb and balance-alb\n>>>>>> + bond_check_params() -- Used during module initialization\n>>>>>> + bond_fill_info() -- Used to get/set value\n>>>>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value\n>>>>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value\n>>>>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode\n>>>>>>\n>>>>>> The following untested patch adds code to make balance-alb work as if\n>>>>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It\n>>>>>> also reverts my previous patch.\n>>>>>>\n>>>>>> What do you think about this approach?\n>>>>>> ---\n>>>>>> Kosuke TATSUKAWA | 1st Platform Software Division\n>>>>>> | NEC Solution Innovators\n>>>>>> | tatsu@ab.jp.nec.com\n>>>>>>\n>>>>>\n>>>>> Logically the approach looks good, that being said it adds unnecessary tests in\n>>>>> the fast path, why not just something like the patch below ? That only leaves the\n>>>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one line\n>>>>> fix to unsuppmodes just allow it to be set for that specific case. The below\n>>>>> returns the default behaviour before the commit in your Fixes tag.\n>>>>>\n>>>>>\n>>>>\n>>>> Actually I'm fine with your approach, too. It will fix this regardless of the\n>>>> value of tlb_dynamic_lb which sounds good to me for the price of a test in\n>>>> the fast path.\n>>>\n>>> If you're concerned about the additional test in the fast path, how\n>>> about the patch below. I've added an arguemnt to bond_do_alb_xmit()\n>>> to handle both balance-tlb and balance-alb similary.\n>>>\n>>\n>> Even better, looks great! 1 question below though.\n>>\n>>> I'm not sure if this causes any problem if tlb_dynamic_lb is changed\n>>> while calling bond_do_alb_xmit() in balance-tlb mode.\n>>\n>> The option has the ifdown flag, you shouldn't be able to change it while\n>> the bond dev is up, but even if you could I don't think it will be an issue\n>> for the xmit.\n>>\n>>> ---\n>>> Kosuke TATSUKAWA | 1st Platform Software Division\n>>> | NEC Solution Innovators\n>>> | tatsu@ab.jp.nec.com\n>>>\n>>> ------------------------------------------------------------------------\n>>> drivers/net/bonding/bond_alb.c | 11 ++++++-----\n>>> drivers/net/bonding/bond_main.c | 5 +++--\n>>> 2 files changed, 9 insertions(+), 7 deletions(-)\n>>>\n>>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c\n>>> index c02cc81..7710f20 100644\n>>> --- a/drivers/net/bonding/bond_alb.c\n>>> +++ b/drivers/net/bonding/bond_alb.c\n>>> @@ -1317,7 +1317,7 @@ void bond_alb_deinitialize(struct bonding *bond)\n>>> }\n>>>\n>>> static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n>>> - struct slave *tx_slave)\n>>> + struct slave *tx_slave, int tlb_dynamic_lb)\n>>> {\n>>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));\n>>> struct ethhdr *eth_data = eth_hdr(skb);\n>>> @@ -1325,7 +1325,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n>>> if (!tx_slave) {\n>>> /* unbalanced or unassigned, send through primary */\n>>> tx_slave = rcu_dereference(bond->curr_active_slave);\n>>> - if (bond->params.tlb_dynamic_lb)\n>>> + if (tlb_dynamic_lb)\n>>> bond_info->unbalanced_load += skb->len;\n>>> }\n>>>\n>>> @@ -1339,7 +1339,7 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,\n>>> goto out;\n>>> }\n>>>\n>>> - if (tx_slave && bond->params.tlb_dynamic_lb) {\n>>> + if (tx_slave && tlb_dynamic_lb) {\n>>> spin_lock(&bond->mode_lock);\n>>> __tlb_clear_slave(bond, tx_slave, 0);\n>>> spin_unlock(&bond->mode_lock);\n>>> @@ -1386,7 +1386,8 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n>>> break;\n>>> }\n>>> }\n>>> - return bond_do_alb_xmit(skb, bond, tx_slave);\n>>> + return bond_do_alb_xmit(skb, bond, tx_slave,\n>>> + bond->params.tlb_dynamic_lb);\n>>> }\n>>>\n>>> int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n>>> @@ -1483,7 +1484,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)\n>>> tx_slave = tlb_choose_channel(bond, hash_index, skb->len);\n>>> }\n>>>\n>>> - return bond_do_alb_xmit(skb, bond, tx_slave);\n>>> + return bond_do_alb_xmit(skb, bond, tx_slave, 1);\n>>> }\n>>>\n>>> void bond_alb_monitor(struct work_struct *work)\n>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\n>>> index fc63992..bcb71e7 100644\n>>> --- a/drivers/net/bonding/bond_main.c\n>>> +++ b/drivers/net/bonding/bond_main.c\n>>> @@ -3305,7 +3305,8 @@ static int bond_open(struct net_device *bond_dev)\n>>> */\n>>> if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))\n>>> return -ENOMEM;\n>>> - if (bond->params.tlb_dynamic_lb)\n>>> + if (bond->params.tlb_dynamic_lb ||\n>>> + (BOND_MODE(bond) == BOND_MODE_TLB))\n>>\n>> mode == tlb ? shouldn't this check be for alb ?\n>>\n> Actually this is not needed since this is already inside if\n> (bond_is_lb()) condition.\n> \n>>> queue_delayed_work(bond->wq, &bond->alb_work, 0);\n>>> }\n>>>\n>>> @@ -4601,7 +4602,7 @@ static int bond_check_params(struct bond_params *params)\n>>> }\n>>> ad_user_port_key = valptr->value;\n>>>\n>>> - if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n>>> + if (bond_mode == BOND_MODE_TLB) {\n>>> bond_opt_initstr(&newval, \"default\");\n>>> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n>>> &newval);\n>>>\n>>\n> I think the underlying issue is that tlb_dynamic_lb should be set to 1\n> for all modes which was not the case when it was getting initialized\n> only forTLB mode. So from that perspective I prefer Nik's patch with a\n> small variation that guards the case when mode transitions from TLB to\n> ALB. The reason why I like that patch is because it's simple and\n> avoids complications.\n\n+1, I think this is the most straight-forward solution as well and\nsafest for -net\n\nI will go ahead and submit it in a few minutes.\n\nThanks,\n Nik\n\n> \n> Here is what I meant -\n> \n> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c\n> index fc63992ab0e0..c99dc59d729b 100644\n> --- a/drivers/net/bonding/bond_main.c\n> +++ b/drivers/net/bonding/bond_main.c\n> @@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)\n> int bond_mode = BOND_MODE_ROUNDROBIN;\n> int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;\n> int lacp_fast = 0;\n> - int tlb_dynamic_lb = 0;\n> + int tlb_dynamic_lb;\n> \n> /* Convert string parameters. */\n> if (mode) {\n> @@ -4601,16 +4601,13 @@ static int bond_check_params(struct bond_params *params)\n> }\n> ad_user_port_key = valptr->value;\n> \n> - if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {\n> - bond_opt_initstr(&newval, \"default\");\n> - valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),\n> - &newval);\n> - if (!valptr) {\n> - pr_err(\"Error: No tlb_dynamic_lb default value\");\n> - return -EINVAL;\n> - }\n> - tlb_dynamic_lb = valptr->value;\n> + bond_opt_initstr(&newval, \"default\");\n> + valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB), &newval);\n> + if (!valptr) {\n> + pr_err(\"Error: No tlb_dynamic_lb default value\");\n> + return -EINVAL;\n> }\n> + tlb_dynamic_lb = valptr->value;\n> \n> if (lp_interval == 0) {\n> pr_warn(\"Warning: ip_interval must be between 1 and\n> %d, so it was reset to %d\\n\",\n> diff --git a/drivers/net/bonding/bond_options.c\n> b/drivers/net/bonding/bond_options.c\n> index a12d603d41c6..7feacd262182 100644\n> --- a/drivers/net/bonding/bond_options.c\n> +++ b/drivers/net/bonding/bond_options.c\n> @@ -754,6 +754,12 @@ static int bond_option_mode_set(struct bonding *bond,\n> bond->params.miimon);\n> }\n> \n> + /* Guard against transition TLB/tlb_dynamic_lb=0 -> ALB mode.\n> + * In ALB mode, tlb_dynamic_lb must be set to 1.\n> + */\n> + if (newval->value == BOND_MODE_ALB && bond->params.tlb_dynamic_lb != 1)\n> + bond->params.tlb_dynamic_lb = 1;\n> +\n> /* don't cache arp_validate between modes */\n> bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;\n> bond->params.mode = newval->value;\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"U5iWL4Ml\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xq9RC13JQz9sBW\n\tfor ;\n\tSat, 9 Sep 2017 20:29:27 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757427AbdIIK3K (ORCPT );\n\tSat, 9 Sep 2017 06:29:10 -0400","from mail-wm0-f46.google.com ([74.125.82.46]:44144 \"EHLO\n\tmail-wm0-f46.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1757293AbdIIK3I (ORCPT\n\t); Sat, 9 Sep 2017 06:29:08 -0400","by mail-wm0-f46.google.com with SMTP id 137so11095948wmj.1\n\tfor ; Sat, 09 Sep 2017 03:29:07 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\th56sm1575399ede.1.2017.09.09.03.29.04\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSat, 09 Sep 2017 03:29:05 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=54BaCUOi94mtRjjkTnMf7w4kv6r+YIMl+mW4BSV7neA=;\n\tb=U5iWL4Mlp4vn4kcXnvT8GwZG9xxFZQKmSvAaoHq64Id7IbdzWYl2VfoAZtnrOKXf+U\n\tziBrHPuWONit6tdygtu9VKcdD9TmDaNaC31b+VTMa3nS7atudeOvwnOm9fGEyXumQmmN\n\thSRhBslTKLzv171aWsMD8V4+ji9rQDTOixgKQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=54BaCUOi94mtRjjkTnMf7w4kv6r+YIMl+mW4BSV7neA=;\n\tb=lyMUduKL0l8zlGuhxZA3341CvQ+8c0Gj1wAx8PffVOrD4iwbRLMu9tdoF3oJDtSykg\n\tD3wXHfdmQmNbTSKQnNMndr7CvGuThELW4YYVekly3Rj0kpmxEay7N4IenCFNxkmgx8CM\n\trXotS/pQ8ETWZrky+u9bQvgwHQiu3MzCL3M+BmGJh1t4pvfiH8Nhpr5mR+ntiolp2EWu\n\tstDTnQ0srT51pWC7blOkEaRqNRuPfxOrYM7Dl/JPqdA8jKOLpkpHfwjU4ZRFPwiEzjnN\n\tpoHDnmdVcF2N/FTD8sz41aAiq70dF1BQ6RqKs1Ofhw4JfiRqeRffeCQpEQFnCf6oqSxN\n\t2Evw==","X-Gm-Message-State":"AHPjjUiZroS+mZmJx1p33afVn6G8CT+3Z63e2NhRcq6oa8oDFfIHgKwC\n\thc22noj+YM0kOLdF","X-Google-Smtp-Source":"ADKCNb6bFzPxnTRSDPmd4wuzgjrdPH2kmmrfjEwulTI2hbhVNs+gLvccXSHciAtSjcaxsXKMsVQVcg==","X-Received":"by 10.80.213.26 with SMTP id u26mr4162703edi.87.1504952946664;\n\tSat, 09 Sep 2017 03:29:06 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","References":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>\n\t<03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>\n\t","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","From":"Nikolay Aleksandrov ","Message-ID":"<9ae82441-78da-0056-88c1-2f9d863fb81f@cumulusnetworks.com>","Date":"Sat, 9 Sep 2017 13:29:04 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tIcedove/45.6.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765784,"web_url":"http://patchwork.ozlabs.org/comment/1765784/","msgid":"<43356b9d-77e5-4ccb-ee1b-f293d2fb130c@cumulusnetworks.com>","list_archive_url":null,"date":"2017-09-09T11:23:08","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 09/09/17 13:29, Nikolay Aleksandrov wrote:\n> On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote:\n>> On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov\n>> wrote:\n>>> On 08/09/17 17:17, Kosuke Tatsukawa wrote:\n[snip]\n>>>\n>> I think the underlying issue is that tlb_dynamic_lb should be set to 1\n>> for all modes which was not the case when it was getting initialized\n>> only forTLB mode. So from that perspective I prefer Nik's patch with a\n>> small variation that guards the case when mode transitions from TLB to\n>> ALB. The reason why I like that patch is because it's simple and\n>> avoids complications.\n> \n> +1, I think this is the most straight-forward solution as well and\n> safest for -net\n> \n> I will go ahead and submit it in a few minutes.\n> \n> Thanks,\n> Nik\n> \n\nJust FYI, since the second fix (tlb_dynamic_lb in TLB = 0 switch to ALB) is identical\nto this patch, I'm acking this one and will wait until it's in to submit the default\nvalue fix (if we start in non-TLB mode and switch to TLB we'll get dynamic_lb = 0\ncurrently).\n\nI guess this is the simplest way, I didn't want to alter user-configured value on\nmode switch, but it seems better than the alternative especially for -net.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"S+mHFCHj\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xqBdS0Sv4z9sBZ\n\tfor ;\n\tSat, 9 Sep 2017 21:23:24 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757479AbdIILXO (ORCPT );\n\tSat, 9 Sep 2017 07:23:14 -0400","from mail-wm0-f43.google.com ([74.125.82.43]:46782 \"EHLO\n\tmail-wm0-f43.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1756862AbdIILXM (ORCPT\n\t); Sat, 9 Sep 2017 07:23:12 -0400","by mail-wm0-f43.google.com with SMTP id i189so15998652wmf.1\n\tfor ; Sat, 09 Sep 2017 04:23:11 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\tr14sm1798923edd.56.2017.09.09.04.23.09\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSat, 09 Sep 2017 04:23:10 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=HJw+3V7QXyKajjOE+WbgD3SE32weat9VGbCwuM8jNQ8=;\n\tb=S+mHFCHjraMqrkSmwO3Yb5SVYhX3WsxG1eK+cqvW4ukZwJZRmu6bb8yIynM24/Gnmz\n\tjz29TGpZglZ/obWixlTZmcFvdmrdeVCfhkveHeIQKibpkdZsubMxg9JzenVndxjE71pC\n\tDvP/jxaaswuIYELHk2fIp0jifcnfuNLMuOuyU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=HJw+3V7QXyKajjOE+WbgD3SE32weat9VGbCwuM8jNQ8=;\n\tb=pmb+TPbAn/V1KF8yBGKC0xOndrqIa86UZqveM/rlVj6EO4pQ+H0xbxYLdw75MMgjnr\n\tbrlxV9QM8iz0k8FA5z4+VDKBJLXThqUIKshNf0k01bmY+lMoJ3AXh3a2D0l8+zp3j5Xr\n\tYOAgNkEgcpTtpYlj/A38PDzdpjlLdvPfGr3/WDdDamxEo2aLwRgwFV1y/YHvx5vT2GjP\n\tI6/npm2MwDUY5cHIOCmku8MiAqbGIAkF7CPG4hHLOGKYpHvGLVXCnQAQx4hBdpBNsRCK\n\tt9UDTMMvfgnoc9Pj7ulJNyH9AYeeK5r+HdTMx9D5nLp/O2J5CFY4ET9aT5sO0MGV9nDI\n\tr/QQ==","X-Gm-Message-State":"AHPjjUhlFvuPkiytHnUwshcPsiy5HCztrD5OyVzS9Sgem7vFmUpwQRxN\n\tHR7KTpus8Dvz6nZ7","X-Google-Smtp-Source":"ADKCNb4nTLFL4/TybK0Wg0/rgbZxOaJLgvusyCvtLJFZpzrzohVFCTNRwFDdVMRqH/Yie4BB19dJsg==","X-Received":"by 10.80.139.252 with SMTP id n57mr2509910edn.291.1504956191268; \n\tSat, 09 Sep 2017 04:23:11 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","References":"<17EC94B0A072C34B8DCF0D30AD16044A0298684C@BPXM09GP.gisp.nec.co.jp>\n\t<03b61877-053b-2f0e-dc35-8fe31cc90c08@cumulusnetworks.com>\n\t\n\t<9ae82441-78da-0056-88c1-2f9d863fb81f@cumulusnetworks.com>","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","From":"Nikolay Aleksandrov ","X-Enigmail-Draft-Status":"N1110","Message-ID":"<43356b9d-77e5-4ccb-ee1b-f293d2fb130c@cumulusnetworks.com>","Date":"Sat, 9 Sep 2017 14:23:08 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tIcedove/45.6.0","MIME-Version":"1.0","In-Reply-To":"<9ae82441-78da-0056-88c1-2f9d863fb81f@cumulusnetworks.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765785,"web_url":"http://patchwork.ozlabs.org/comment/1765785/","msgid":"","list_archive_url":null,"date":"2017-09-09T11:28:11","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 07/09/17 01:47, Kosuke Tatsukawa wrote:\n> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n> balance-alb mode\") tried to fix transmit dynamic load balancing in\n> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n> (\"bonding: remove hardcoded value\").\n> \n> It turned out that my previous patch only fixed the case when\n> balance-alb was specified as bonding module parameter, and not when\n> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n> common usage). In the latter case, tlb_dynamic_lb was set up according\n> to the default mode of the bonding interface, which happens to be\n> balance-rr.\n> \n> This additional patch addresses this issue by setting up tlb_dynamic_lb\n> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n> \n> I didn't add code to change tlb_balance_lb back to the default value for\n> other modes, because \"mode\" is usually set up only once during\n> initialization, and it's not worthwhile to change the static variable\n> bonding_defaults in bond_main.c to a global variable just for this\n> purpose.\n> \n> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n> balance-tlb mode if it is set up using the sysfs interface. I didn't\n> change that behavior, because the value of tlb_balance_lb can be changed\n> using the sysfs interface for balance-tlb, and I didn't like changing\n> the default value back and forth for balance-tlb.\n> \n> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n> is not an intended usage, so there is little use making it writable at\n> this moment.\n> \n> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n> Reported-by: Reinis Rozitis \n> Signed-off-by: Kosuke Tatsukawa \n> Cc: stable@vger.kernel.org # v4.12+\n> ---\n> drivers/net/bonding/bond_options.c | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n> \n\nThis fix is simpler and more suitable for -net, it fixes the case where\nwe switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll fix the\ndefault tlb_dynamic_lb issue and restore the original behaviour.\n\nAcked-by: Nikolay Aleksandrov ","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"TXjAl6k4\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xqBlK2Mh6z9sBZ\n\tfor ;\n\tSat, 9 Sep 2017 21:28:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757491AbdIIL2Q (ORCPT );\n\tSat, 9 Sep 2017 07:28:16 -0400","from mail-wm0-f43.google.com ([74.125.82.43]:45255 \"EHLO\n\tmail-wm0-f43.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753539AbdIIL2O (ORCPT\n\t); Sat, 9 Sep 2017 07:28:14 -0400","by mail-wm0-f43.google.com with SMTP id f199so16064180wme.0\n\tfor ; Sat, 09 Sep 2017 04:28:13 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\tc25sm706326edb.57.2017.09.09.04.28.11\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSat, 09 Sep 2017 04:28:11 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=azye9nwoU4pFvuKnAAiEY6+lN7+3R/h0PJqROHdiW74=;\n\tb=TXjAl6k4YqMbP88lzW1qbdvF284u2uW9miKdLHw4Zj99ZTDVNU94vQRjaEEMt7yV+O\n\t6ERdX2vILbJOQ/N4WN+Fra6pvIUoD/Xkq5pKu+S/Hyj4XLljIC+Pi5xkTYHxUBq8duHS\n\tbeRamjRbc+KQmYL7VgnbhawO4g8scHgRdoX0M=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=azye9nwoU4pFvuKnAAiEY6+lN7+3R/h0PJqROHdiW74=;\n\tb=F3QmdQqjW7+wctr2/oPM8Zp859u9gInJxzPVsQrm/ZyhKImzFHbmJCjVn84ocG9HyF\n\t0zO2p2aue1KDd3Ef14ue9NK23ACQsy4eEWKuwjGPfuzlwTsUcWKNWtfiSdRzYHv/mddb\n\tOjoXp74eEIPxzKIVfN/9W1NEJHPdT8ogcvaXKe1w5REVFvyj8M5CEhgqK56rNFm9moFX\n\t1I9APphjrISUQTN/B1pMKTzqFEG+1KTTsJWtQhyLbTH6AFlgEEw4shg8HUQb/3/Q+KPO\n\tanAb74pCr/Ss7HdSOLtMIash13tjqe8Rfe/SofNtvTM30rjgBO7O82/a0qoh5WICgWqH\n\tyayA==","X-Gm-Message-State":"AHPjjUjE7qIld08/8NHf27udNgkAcJDBvDrnzLq9K62eSj5u7fHRKwzd\n\tCdJEQX+0Y7SHTYAu","X-Google-Smtp-Source":"ADKCNb4z3w/VcalzD/rurwpbg4h1QSaQEuyVQizq6BaFyJcpo8+W7rqtF98G0b1BUJFX+gzG8ruREA==","X-Received":"by 10.80.212.40 with SMTP id t40mr1339322edh.67.1504956493056;\n\tSat, 09 Sep 2017 04:28:13 -0700 (PDT)","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","From":"Nikolay Aleksandrov ","Message-ID":"","Date":"Sat, 9 Sep 2017 14:28:11 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tIcedove/45.6.0","MIME-Version":"1.0","In-Reply-To":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>","Content-Type":"text/plain; charset=iso-2022-jp","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766376,"web_url":"http://patchwork.ozlabs.org/comment/1766376/","msgid":"","list_archive_url":null,"date":"2017-09-11T16:07:33","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","submitter":{"id":6061,"url":"http://patchwork.ozlabs.org/api/people/6061/","name":"Mahesh Bandewar (महेश बंडेवार)","email":"maheshb@google.com"},"content":"On Sat, Sep 9, 2017 at 4:28 AM, Nikolay Aleksandrov\n wrote:\n> On 07/09/17 01:47, Kosuke Tatsukawa wrote:\n>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>> (\"bonding: remove hardcoded value\").\n>>\n>> It turned out that my previous patch only fixed the case when\n>> balance-alb was specified as bonding module parameter, and not when\n>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n>> common usage). In the latter case, tlb_dynamic_lb was set up according\n>> to the default mode of the bonding interface, which happens to be\n>> balance-rr.\n>>\n>> This additional patch addresses this issue by setting up tlb_dynamic_lb\n>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>\n>> I didn't add code to change tlb_balance_lb back to the default value for\n>> other modes, because \"mode\" is usually set up only once during\n>> initialization, and it's not worthwhile to change the static variable\n>> bonding_defaults in bond_main.c to a global variable just for this\n>> purpose.\n>>\n>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>> balance-tlb mode if it is set up using the sysfs interface. I didn't\n>> change that behavior, because the value of tlb_balance_lb can be changed\n>> using the sysfs interface for balance-tlb, and I didn't like changing\n>> the default value back and forth for balance-tlb.\n>>\n>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n>> is not an intended usage, so there is little use making it writable at\n>> this moment.\n>>\n>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>> Reported-by: Reinis Rozitis \n>> Signed-off-by: Kosuke Tatsukawa \n>> Cc: stable@vger.kernel.org # v4.12+\n>> ---\n>> drivers/net/bonding/bond_options.c | 3 +++\n>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>\n>\n> This fix is simpler and more suitable for -net, it fixes the case where\n> we switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll fix the\n> default tlb_dynamic_lb issue and restore the original behaviour.\n>\nChanging tlb_dyanamic_lb to initialize always is also safe for -net\nand can go in before or after this change (no dependency on this\nchange as such)\n\n> Acked-by: Nikolay Aleksandrov \nAcked-by: Mahesh Bandewar \n>\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"tdkVSNPG\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrXsL0jQsz9s7f\n\tfor ;\n\tTue, 12 Sep 2017 02:08:22 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751520AbdIKQH5 (ORCPT );\n\tMon, 11 Sep 2017 12:07:57 -0400","from mail-yw0-f174.google.com ([209.85.161.174]:34707 \"EHLO\n\tmail-yw0-f174.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750926AbdIKQHz (ORCPT\n\t); Mon, 11 Sep 2017 12:07:55 -0400","by mail-yw0-f174.google.com with SMTP id r85so22562865ywg.1\n\tfor ; Mon, 11 Sep 2017 09:07:55 -0700 (PDT)","by 10.37.161.71 with HTTP; Mon, 11 Sep 2017 09:07:33 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=8XtHc3BDK/yKXWRj2YdMDSBI6cAO8GfPt+dSX7r7XoM=;\n\tb=tdkVSNPGZ1Zo/AiY6hKxw594O6xwMu08bS4B63KNfBHNNQrqXM9z4DdKa/2aXVQYs6\n\t8fnuBCKr63jCRyxzClwp0Q5z2W3Dr8KZt/dcLZgxji3m26sFJpJSNI6uMZa+F5O8vlfb\n\tPNqRj6c6oRmxSatrB8q020eK0xVaGagzJQ3GjWFmTIEnq/3bwiNehWhkhTtChl2naLfp\n\tVXy+uCUe6IVMrykPVLuqbaPD3OF3SYc2WLBPIN0N+KAfGfERGNrJYA3LhX2ND+N6ys2O\n\tvpOwhJxK/L9HwuIZeN4j/lCOuAfOcnYtQl3UWZuLGez46rfZcKMax4tDMRNO7VhuhClz\n\tk4pQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=8XtHc3BDK/yKXWRj2YdMDSBI6cAO8GfPt+dSX7r7XoM=;\n\tb=qc/DW/J0IoZvcaS6+jxu3bP4PZ/eb5pVn7lsDOXNt3XqOLp53xyYnWYPCjTBffXfao\n\tJpz97oi5fK4cVbRKEZQLAIJSMp3sLsIfPwrqlSV9h7ThEyoYW0vxbNh8Y6iHi+nr8y0b\n\tkgcCGgHVXd/vxj0fskupMMO1t4+NpupyM+GBi87jxBeOMy+1AI9Wr6Yk4N4KQhz8ajm0\n\tn4nr9OZ/c8dc38p/Vq7u70GKkBgGgnlogOR9q2QufNkzW5qj5X2ym8eX2B08Rze7ompI\n\tvk+FMFqvgWoQJPOCcEg+5Vek0BaN2p+rqtN82oZlQkNDa1IM/7tXoOubXKNFH5eERBl2\n\tLKxw==","X-Gm-Message-State":"AHPjjUjdeXyuvERIa8dXzglXS8bYeJ84raL7gBUOYnMDehARTr9Y8jFg\n\t79tUImVH0JVZ9qr9rsM2PuaIdJz8He0S","X-Google-Smtp-Source":"AOwi7QAabUXEnEFYYlWwgfJDMbqz7d8vR0AbzvhMBZEyx3cQKArxfP7ne/BZlRCYRZwI7mFLbkkKd02eP0I/C1C7iOE=","X-Received":"by 10.37.128.6 with SMTP id m6mr4822281ybk.73.1505146074479; Mon,\n\t11 Sep 2017 09:07:54 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>\n\t","From":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","Date":"Mon, 11 Sep 2017 09:07:33 -0700","Message-ID":"","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb\n\tmode if specified by sysfs","To":"Nikolay Aleksandrov ","Cc":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766380,"web_url":"http://patchwork.ozlabs.org/comment/1766380/","msgid":"<819E319E-B2EE-44FF-8616-03257E753E6C@cumulusnetworks.com>","list_archive_url":null,"date":"2017-09-11T16:30:08","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 11 September 2017 19:07:33 EEST, \"Mahesh Bandewar (महेश बंडेवार)\" wrote:\n>On Sat, Sep 9, 2017 at 4:28 AM, Nikolay Aleksandrov\n> wrote:\n>> On 07/09/17 01:47, Kosuke Tatsukawa wrote:\n>>> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n>>> balance-alb mode\") tried to fix transmit dynamic load balancing in\n>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n>>> (\"bonding: remove hardcoded value\").\n>>>\n>>> It turned out that my previous patch only fixed the case when\n>>> balance-alb was specified as bonding module parameter, and not when\n>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the\n>most\n>>> common usage). In the latter case, tlb_dynamic_lb was set up\n>according\n>>> to the default mode of the bonding interface, which happens to be\n>>> balance-rr.\n>>>\n>>> This additional patch addresses this issue by setting up\n>tlb_dynamic_lb\n>>> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n>>>\n>>> I didn't add code to change tlb_balance_lb back to the default value\n>for\n>>> other modes, because \"mode\" is usually set up only once during\n>>> initialization, and it's not worthwhile to change the static\n>variable\n>>> bonding_defaults in bond_main.c to a global variable just for this\n>>> purpose.\n>>>\n>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n>>> balance-tlb mode if it is set up using the sysfs interface. I\n>didn't\n>>> change that behavior, because the value of tlb_balance_lb can be\n>changed\n>>> using the sysfs interface for balance-tlb, and I didn't like\n>changing\n>>> the default value back and forth for balance-tlb.\n>>>\n>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot\n>be\n>>> written to. However, I think balance-alb with tlb_dynamic_lb set to\n>0\n>>> is not an intended usage, so there is little use making it writable\n>at\n>>> this moment.\n>>>\n>>> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n>>> Reported-by: Reinis Rozitis \n>>> Signed-off-by: Kosuke Tatsukawa \n>>> Cc: stable@vger.kernel.org # v4.12+\n>>> ---\n>>> drivers/net/bonding/bond_options.c | 3 +++\n>>> 1 files changed, 3 insertions(+), 0 deletions(-)\n>>>\n>>\n>> This fix is simpler and more suitable for -net, it fixes the case\n>where\n>> we switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll\n>fix the\n>> default tlb_dynamic_lb issue and restore the original behaviour.\n>>\n>Changing tlb_dyanamic_lb to initialize always is also safe for -net\n>and can go in before or after this change (no dependency on this\n>change as such)\n\nI never said it was unsafe or dependent, it is simply my preference to wait. :-)\nIf you need it sooner feel free to post it.\n\n>\n>> Acked-by: Nikolay Aleksandrov \n>Acked-by: Mahesh Bandewar \n>>\n>>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"NXFUwIls\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrYLr3KBpz9s7f\n\tfor ;\n\tTue, 12 Sep 2017 02:30:28 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752054AbdIKQaQ (ORCPT );\n\tMon, 11 Sep 2017 12:30:16 -0400","from mail-wm0-f42.google.com ([74.125.82.42]:46991 \"EHLO\n\tmail-wm0-f42.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751214AbdIKQaO (ORCPT\n\t); Mon, 11 Sep 2017 12:30:14 -0400","by mail-wm0-f42.google.com with SMTP id i189so44027722wmf.1\n\tfor ; Mon, 11 Sep 2017 09:30:14 -0700 (PDT)","from [10.139.41.239] ([149.62.200.148])\n\tby smtp.gmail.com with ESMTPSA id\n\t4sm9519102wmg.20.2017.09.11.09.30.11\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tMon, 11 Sep 2017 09:30:12 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=date:user-agent:in-reply-to:references:mime-version\n\t:content-transfer-encoding:subject:to:cc:from:message-id;\n\tbh=kODhV6Mro5Hqs2euyuI7UyQRX3HzccsJj9hE8X9Xvvc=;\n\tb=NXFUwIlsmxvkUm5p2g4f42j9UUc+E0B2F79b6rjoKxy0pNa7Mnxqy5rA+m5xNoKF7w\n\tzExIxVlgmS2tX8kZ8MuOrRY+qlfeT4xRQNZTvBPOVmUXH3t4bhth9MAuCuDU3S4BQarj\n\t21AgXuqNHtShvGuTbVYyMj7ldLBiA22Fsw5B0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:user-agent:in-reply-to:references\n\t:mime-version:content-transfer-encoding:subject:to:cc:from\n\t:message-id;\n\tbh=kODhV6Mro5Hqs2euyuI7UyQRX3HzccsJj9hE8X9Xvvc=;\n\tb=RR2JK6HukSOpZ01nDR1yjmP1vFo3EgMUF5rRDLrrX+o9ffEg6wLv9a6Tv5fepMA7nf\n\tRd59vWMbi6wEnEG47sFY5rjZPDjs8E5X9qk4Jmpt/thNFga/ja//kWoQ2o+lzSoz0Tbg\n\tY5KwxplH99LfLfjggC5twpfI7vTkC7ox1stfiJDzSgFjyJeXpqjOLXUCyvatW5NBi7Rt\n\tyAGI8X93pjRuXzUEgkEOoH0eu/Dpy6f2IWi4yBRzMyjeHYUA4Izd9UW6HADFvZ381S2k\n\tf6p27CZwtGtjAzsoxEATPCdYcAl2BhKbhfn6U5eyarE+D23ffAM/6Yz8w1w7MC5wxcnL\n\tejsA==","X-Gm-Message-State":"AHPjjUimpyVQFr0OKlWJgBvaACOT94+CQGUF+2wFM+iJahQt+A7BYiPR\n\trgtaCD53k7Za1W3VfogP/QiSNg==","X-Google-Smtp-Source":"AOwi7QDmLs6xonSNSji/RvvQ0vhsU/mRYMDhMe3GSRyNc1/sNHsSyua44FNT6Dz0JXfarh+mRpyFlg==","X-Received":"by 10.28.105.157 with SMTP id z29mr7603212wmh.135.1505147413198; \n\tMon, 11 Sep 2017 09:30:13 -0700 (PDT)","Date":"Mon, 11 Sep 2017 19:30:08 +0300","User-Agent":"K-9 Mail for Android","In-Reply-To":"","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain;\n charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs","To":"=?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH?=\n\t=?utf-8?b?4KS14KS+4KSwKQ==?= ","CC":"Kosuke Tatsukawa ,\n\tJay Vosburgh ,\n\tVeaceslav Falico ,\n\tAndy Gospodarek ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tReinis Rozitis ","From":"Nikolay Aleksandrov ","Message-ID":"<819E319E-B2EE-44FF-8616-03257E753E6C@cumulusnetworks.com>","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766566,"web_url":"http://patchwork.ozlabs.org/comment/1766566/","msgid":"<20170911.142600.1782754597359098256.davem@davemloft.net>","list_archive_url":null,"date":"2017-09-11T21:26:00","subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs","submitter":{"id":15,"url":"http://patchwork.ozlabs.org/api/people/15/","name":"David Miller","email":"davem@davemloft.net"},"content":"From: Kosuke Tatsukawa \nDate: Wed, 6 Sep 2017 22:47:59 +0000\n\n> Commit cbf5ecb30560 (\"net: bonding: Fix transmit load balancing in\n> balance-alb mode\") tried to fix transmit dynamic load balancing in\n> balance-alb mode, which wasn't working after commit 8b426dc54cf4\n> (\"bonding: remove hardcoded value\").\n> \n> It turned out that my previous patch only fixed the case when\n> balance-alb was specified as bonding module parameter, and not when\n> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most\n> common usage). In the latter case, tlb_dynamic_lb was set up according\n> to the default mode of the bonding interface, which happens to be\n> balance-rr.\n> \n> This additional patch addresses this issue by setting up tlb_dynamic_lb\n> to 1 if \"mode\" is set to balance-alb through the sysfs interface.\n> \n> I didn't add code to change tlb_balance_lb back to the default value for\n> other modes, because \"mode\" is usually set up only once during\n> initialization, and it's not worthwhile to change the static variable\n> bonding_defaults in bond_main.c to a global variable just for this\n> purpose.\n> \n> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for\n> balance-tlb mode if it is set up using the sysfs interface. I didn't\n> change that behavior, because the value of tlb_balance_lb can be changed\n> using the sysfs interface for balance-tlb, and I didn't like changing\n> the default value back and forth for balance-tlb.\n> \n> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be\n> written to. However, I think balance-alb with tlb_dynamic_lb set to 0\n> is not an intended usage, so there is little use making it writable at\n> this moment.\n> \n> Fixes: 8b426dc54cf4 (\"bonding: remove hardcoded value\")\n> Reported-by: Reinis Rozitis \n> Signed-off-by: Kosuke Tatsukawa \n> Cc: stable@vger.kernel.org # v4.12+\n\nApplied and queued up for -stable, thanks.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrgw84cS6z9sBZ\n\tfor ;\n\tTue, 12 Sep 2017 07:26:16 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751078AbdIKV0C (ORCPT );\n\tMon, 11 Sep 2017 17:26:02 -0400","from shards.monkeyblade.net ([184.105.139.130]:56184 \"EHLO\n\tshards.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750853AbdIKV0B (ORCPT\n\t); Mon, 11 Sep 2017 17:26:01 -0400","from localhost (74-93-104-98-Washington.hfc.comcastbusiness.net\n\t[74.93.104.98]) (using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(Client did not present a certificate)\n\t(Authenticated sender: davem-davemloft)\n\tby shards.monkeyblade.net (Postfix) with ESMTPSA id F3D7710229CEC;\n\tMon, 11 Sep 2017 14:26:00 -0700 (PDT)"],"Date":"Mon, 11 Sep 2017 14:26:00 -0700 (PDT)","Message-Id":"<20170911.142600.1782754597359098256.davem@davemloft.net>","To":"tatsu@ab.jp.nec.com","Cc":"j.vosburgh@gmail.com, vfalico@gmail.com, andy@greyhouse.net,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org, r@roze.lv","Subject":"Re: [PATCH] net: bonding: Fix transmit load balancing in\n\tbalance-alb mode if specified by sysfs","From":"David Miller ","In-Reply-To":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>","References":"<17EC94B0A072C34B8DCF0D30AD16044A02985CB5@BPXM09GP.gisp.nec.co.jp>","X-Mailer":"Mew version 6.7 on Emacs 25.2 / Mule 6.0 (HANACHIRUSATO)","Mime-Version":"1.0","Content-Type":"Text/Plain; charset=us-ascii","Content-Transfer-Encoding":"7bit","X-Greylist":"Sender succeeded SMTP AUTH, not delayed by\n\tmilter-greylist-4.5.12 (shards.monkeyblade.net\n\t[149.20.54.216]); Mon, 11 Sep 2017 14:26:01 -0700 (PDT)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]