Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Kosuke Tatsukawa
Date: Wed, 6 Sep 2017 22:47:59 +
> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
> balance-alb mode") tried to fix transmit dynamic load balancing in
> balance-alb mode, which wasn't working after commit 8b426dc54cf4
> ("bonding: remove hardcoded value").
>
> It turned out that my previous patch only fixed the case when
> balance-alb was specified as bonding module parameter, and not when
> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
> common usage). In the latter case, tlb_dynamic_lb was set up according
> to the default mode of the bonding interface, which happens to be
> balance-rr.
>
> This additional patch addresses this issue by setting up tlb_dynamic_lb
> to 1 if "mode" is set to balance-alb through the sysfs interface.
>
> I didn't add code to change tlb_balance_lb back to the default value for
> other modes, because "mode" is usually set up only once during
> initialization, and it's not worthwhile to change the static variable
> bonding_defaults in bond_main.c to a global variable just for this
> purpose.
>
> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
> balance-tlb mode if it is set up using the sysfs interface. I didn't
> change that behavior, because the value of tlb_balance_lb can be changed
> using the sysfs interface for balance-tlb, and I didn't like changing
> the default value back and forth for balance-tlb.
>
> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
> is not an intended usage, so there is little use making it writable at
> this moment.
>
> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
> Reported-by: Reinis Rozitis
> Signed-off-by: Kosuke Tatsukawa
> Cc: sta...@vger.kernel.org # v4.12+
Applied and queued up for -stable, thanks.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 11 September 2017 19:07:33 EEST, "Mahesh Bandewar (महेश बंडेवार)"
wrote:
>On Sat, Sep 9, 2017 at 4:28 AM, Nikolay Aleksandrov
> wrote:
>> On 07/09/17 01:47, Kosuke Tatsukawa wrote:
>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>> ("bonding: remove hardcoded value").
>>>
>>> It turned out that my previous patch only fixed the case when
>>> balance-alb was specified as bonding module parameter, and not when
>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the
>most
>>> common usage). In the latter case, tlb_dynamic_lb was set up
>according
>>> to the default mode of the bonding interface, which happens to be
>>> balance-rr.
>>>
>>> This additional patch addresses this issue by setting up
>tlb_dynamic_lb
>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>
>>> I didn't add code to change tlb_balance_lb back to the default value
>for
>>> other modes, because "mode" is usually set up only once during
>>> initialization, and it's not worthwhile to change the static
>variable
>>> bonding_defaults in bond_main.c to a global variable just for this
>>> purpose.
>>>
>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>> balance-tlb mode if it is set up using the sysfs interface. I
>didn't
>>> change that behavior, because the value of tlb_balance_lb can be
>changed
>>> using the sysfs interface for balance-tlb, and I didn't like
>changing
>>> the default value back and forth for balance-tlb.
>>>
>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot
>be
>>> written to. However, I think balance-alb with tlb_dynamic_lb set to
>0
>>> is not an intended usage, so there is little use making it writable
>at
>>> this moment.
>>>
>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>> Reported-by: Reinis Rozitis
>>> Signed-off-by: Kosuke Tatsukawa
>>> Cc: sta...@vger.kernel.org # v4.12+
>>> ---
>>> drivers/net/bonding/bond_options.c |3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>
>> This fix is simpler and more suitable for -net, it fixes the case
>where
>> we switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll
>fix the
>> default tlb_dynamic_lb issue and restore the original behaviour.
>>
>Changing tlb_dyanamic_lb to initialize always is also safe for -net
>and can go in before or after this change (no dependency on this
>change as such)
I never said it was unsafe or dependent, it is simply my preference to wait. :-)
If you need it sooner feel free to post it.
>
>> Acked-by: Nikolay Aleksandrov
>Acked-by: Mahesh Bandewar
>>
>>
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On Sat, Sep 9, 2017 at 4:28 AM, Nikolay Aleksandrov
wrote:
> On 07/09/17 01:47, Kosuke Tatsukawa wrote:
>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>> balance-alb mode") tried to fix transmit dynamic load balancing in
>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>> ("bonding: remove hardcoded value").
>>
>> It turned out that my previous patch only fixed the case when
>> balance-alb was specified as bonding module parameter, and not when
>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>> common usage). In the latter case, tlb_dynamic_lb was set up according
>> to the default mode of the bonding interface, which happens to be
>> balance-rr.
>>
>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>
>> I didn't add code to change tlb_balance_lb back to the default value for
>> other modes, because "mode" is usually set up only once during
>> initialization, and it's not worthwhile to change the static variable
>> bonding_defaults in bond_main.c to a global variable just for this
>> purpose.
>>
>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>> change that behavior, because the value of tlb_balance_lb can be changed
>> using the sysfs interface for balance-tlb, and I didn't like changing
>> the default value back and forth for balance-tlb.
>>
>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>> is not an intended usage, so there is little use making it writable at
>> this moment.
>>
>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>> Reported-by: Reinis Rozitis
>> Signed-off-by: Kosuke Tatsukawa
>> Cc: sta...@vger.kernel.org # v4.12+
>> ---
>> drivers/net/bonding/bond_options.c |3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>
> This fix is simpler and more suitable for -net, it fixes the case where
> we switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll fix the
> default tlb_dynamic_lb issue and restore the original behaviour.
>
Changing tlb_dyanamic_lb to initialize always is also safe for -net
and can go in before or after this change (no dependency on this
change as such)
> Acked-by: Nikolay Aleksandrov
Acked-by: Mahesh Bandewar
>
>
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 07/09/17 01:47, Kosuke Tatsukawa wrote:
> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
> balance-alb mode") tried to fix transmit dynamic load balancing in
> balance-alb mode, which wasn't working after commit 8b426dc54cf4
> ("bonding: remove hardcoded value").
>
> It turned out that my previous patch only fixed the case when
> balance-alb was specified as bonding module parameter, and not when
> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
> common usage). In the latter case, tlb_dynamic_lb was set up according
> to the default mode of the bonding interface, which happens to be
> balance-rr.
>
> This additional patch addresses this issue by setting up tlb_dynamic_lb
> to 1 if "mode" is set to balance-alb through the sysfs interface.
>
> I didn't add code to change tlb_balance_lb back to the default value for
> other modes, because "mode" is usually set up only once during
> initialization, and it's not worthwhile to change the static variable
> bonding_defaults in bond_main.c to a global variable just for this
> purpose.
>
> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
> balance-tlb mode if it is set up using the sysfs interface. I didn't
> change that behavior, because the value of tlb_balance_lb can be changed
> using the sysfs interface for balance-tlb, and I didn't like changing
> the default value back and forth for balance-tlb.
>
> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
> is not an intended usage, so there is little use making it writable at
> this moment.
>
> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
> Reported-by: Reinis Rozitis
> Signed-off-by: Kosuke Tatsukawa
> Cc: sta...@vger.kernel.org # v4.12+
> ---
> drivers/net/bonding/bond_options.c |3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
This fix is simpler and more suitable for -net, it fixes the case where
we switch to ALB mode with tlb_dynamic_lb = 0. After it is in I'll fix the
default tlb_dynamic_lb issue and restore the original behaviour.
Acked-by: Nikolay Aleksandrov
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 09/09/17 13:29, Nikolay Aleksandrov wrote: > On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote: >> On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov >> wrote: >>> On 08/09/17 17:17, Kosuke Tatsukawa wrote: [snip] >>> >> I think the underlying issue is that tlb_dynamic_lb should be set to 1 >> for all modes which was not the case when it was getting initialized >> only forTLB mode. So from that perspective I prefer Nik's patch with a >> small variation that guards the case when mode transitions from TLB to >> ALB. The reason why I like that patch is because it's simple and >> avoids complications. > > +1, I think this is the most straight-forward solution as well and > safest for -net > > I will go ahead and submit it in a few minutes. > > Thanks, > Nik > Just FYI, since the second fix (tlb_dynamic_lb in TLB = 0 switch to ALB) is identical to this patch, I'm acking this one and will wait until it's in to submit the default value fix (if we start in non-TLB mode and switch to TLB we'll get dynamic_lb = 0 currently). I guess this is the simplest way, I didn't want to alter user-configured value on mode switch, but it seems better than the alternative especially for -net.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 09/09/17 02:54, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov
> wrote:
>> On 08/09/17 17:17, Kosuke Tatsukawa wrote:
>>> Hi,
>>>
On 08/09/17 13:10, Nikolay Aleksandrov wrote:
> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>> Hi,
>>
>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").
It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.
I didn't add code to change tlb_balance_lb back to the default value
for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.
Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface. I didn't
change that behavior, because the value of tlb_balance_lb can be
changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.
As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to. However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
Reported-by: Reinis Rozitis
Signed-off-by: Kosuke Tatsukawa
Cc: sta...@vger.kernel.org # v4.12+
---
drivers/net/bonding/bond_options.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> I don't believe this to be the right solution, hardcoding it like this
>>> changes user-visible behaviour. The issue is that if someone configured
>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>>> override their config if they switch the mode to alb. Also it robs users
>>> from their choice.
>>>
>>> If you think this should be settable in ALB mode, then IMO you should
>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>>> That would also be consistent with how it's handled in TLB mode.
>>
>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>> this point. All the current commits regarding tlb_dynamic_lb are for
>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>> to 0 is an intended usage.
>>
>>
>>> Going back and looking at your previous fix I'd argue that it is also
>>> wrong, you should've removed the mode check altogether to return the
>>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>>> default and then ALB mode would've had it, of course that would've left
>>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>>> is a different issue.
>>
>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>> tlb_dynamic_lb is referenced in the following functions.
>>
>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
>> + bond_tlb_xmit() -- Only used by balance-tlb
>> + bond_open() -- Used by both balance-tlb and balance-alb
>> + bond_check_params() -- Used during module initialization
>> + bond_fill_info() -- Used to get/set value
>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>>
>> The following untested patch adds code to make balance-alb work as if
>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
>> also reverts my previous patch.
>>
>> What do you think about this approach?
>> ---
>> Kosuke TATSUKAWA | 1st Platform Software Division
>> | NEC Solution Innovators
>> | ta...@ab.jp.nec.com
>>
>
> Logically the approach looks good, that being said
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On Fri, Sep 8, 2017 at 7:30 AM, Nikolay Aleksandrov
wrote:
> On 08/09/17 17:17, Kosuke Tatsukawa wrote:
>> Hi,
>>
>>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
On 08/09/17 05:06, Kosuke Tatsukawa wrote:
> Hi,
>
>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>> ("bonding: remove hardcoded value").
>>>
>>> It turned out that my previous patch only fixed the case when
>>> balance-alb was specified as bonding module parameter, and not when
>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>> common usage). In the latter case, tlb_dynamic_lb was set up according
>>> to the default mode of the bonding interface, which happens to be
>>> balance-rr.
>>>
>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>
>>> I didn't add code to change tlb_balance_lb back to the default value for
>>> other modes, because "mode" is usually set up only once during
>>> initialization, and it's not worthwhile to change the static variable
>>> bonding_defaults in bond_main.c to a global variable just for this
>>> purpose.
>>>
>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>>> change that behavior, because the value of tlb_balance_lb can be changed
>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>> the default value back and forth for balance-tlb.
>>>
>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>>> is not an intended usage, so there is little use making it writable at
>>> this moment.
>>>
>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>> Reported-by: Reinis Rozitis
>>> Signed-off-by: Kosuke Tatsukawa
>>> Cc: sta...@vger.kernel.org # v4.12+
>>> ---
>>> drivers/net/bonding/bond_options.c |3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>
>> I don't believe this to be the right solution, hardcoding it like this
>> changes user-visible behaviour. The issue is that if someone configured
>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>> override their config if they switch the mode to alb. Also it robs users
>> from their choice.
>>
>> If you think this should be settable in ALB mode, then IMO you should
>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>> That would also be consistent with how it's handled in TLB mode.
>
> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
> this point. All the current commits regarding tlb_dynamic_lb are for
> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
> to 0 is an intended usage.
>
>
>> Going back and looking at your previous fix I'd argue that it is also
>> wrong, you should've removed the mode check altogether to return the
>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>> default and then ALB mode would've had it, of course that would've left
>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>> is a different issue.
>
> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
> tlb_dynamic_lb is referenced in the following functions.
>
> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
> + bond_tlb_xmit() -- Only used by balance-tlb
> + bond_open() -- Used by both balance-tlb and balance-alb
> + bond_check_params() -- Used during module initialization
> + bond_fill_info() -- Used to get/set value
> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>
> The following untested patch adds code to make balance-alb work as if
> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
> also reverts my previous patch.
>
> What do you think about this approach?
> ---
> Kosuke TATSUKAWA | 1st Platform Software Division
> | NEC Solution Innovators
> | ta...@ab.jp.nec.com
>
Logically the approach looks good, that being said it adds unnecessary
tests in
the fast path, why not just something like the patch below ? That only
leaves the
problem if it is zeroed in TLB and switched
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 08/09/17 17:17, Kosuke Tatsukawa wrote:
> Hi,
>
>> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
Hi,
> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>> balance-alb mode") tried to fix transmit dynamic load balancing in
>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>> ("bonding: remove hardcoded value").
>>
>> It turned out that my previous patch only fixed the case when
>> balance-alb was specified as bonding module parameter, and not when
>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>> common usage). In the latter case, tlb_dynamic_lb was set up according
>> to the default mode of the bonding interface, which happens to be
>> balance-rr.
>>
>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>
>> I didn't add code to change tlb_balance_lb back to the default value for
>> other modes, because "mode" is usually set up only once during
>> initialization, and it's not worthwhile to change the static variable
>> bonding_defaults in bond_main.c to a global variable just for this
>> purpose.
>>
>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>> change that behavior, because the value of tlb_balance_lb can be changed
>> using the sysfs interface for balance-tlb, and I didn't like changing
>> the default value back and forth for balance-tlb.
>>
>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>> is not an intended usage, so there is little use making it writable at
>> this moment.
>>
>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>> Reported-by: Reinis Rozitis
>> Signed-off-by: Kosuke Tatsukawa
>> Cc: sta...@vger.kernel.org # v4.12+
>> ---
>> drivers/net/bonding/bond_options.c |3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>
> I don't believe this to be the right solution, hardcoding it like this
> changes user-visible behaviour. The issue is that if someone configured
> it to be 0 in tlb mode, suddenly it will become 1 and will silently
> override their config if they switch the mode to alb. Also it robs users
> from their choice.
>
> If you think this should be settable in ALB mode, then IMO you should
> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
> That would also be consistent with how it's handled in TLB mode.
No, I don't think tlb_dynamic_lb should be settable in balance-alb at
this point. All the current commits regarding tlb_dynamic_lb are for
balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
to 0 is an intended usage.
> Going back and looking at your previous fix I'd argue that it is also
> wrong, you should've removed the mode check altogether to return the
> original behaviour where the dynamic_lb is set to 1 (enabled) by
> default and then ALB mode would've had it, of course that would've left
> the case of setting it to 0 in TLB mode and switching to ALB, but that
> is a different issue.
Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
tlb_dynamic_lb is referenced in the following functions.
+ bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
+ bond_tlb_xmit() -- Only used by balance-tlb
+ bond_open() -- Used by both balance-tlb and balance-alb
+ bond_check_params() -- Used during module initialization
+ bond_fill_info() -- Used to get/set value
+ bond_option_tlb_dynamic_lb_set() -- Used to get/set value
+ bonding_show_tlb_dynamic_lb() -- Used to get/set value
+ bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
The following untested patch adds code to make balance-alb work as if
tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
also reverts my previous patch.
What do you think about this approach?
---
Kosuke TATSUKAWA | 1st Platform Software Division
| NEC Solution Innovators
| ta...@ab.jp.nec.com
>>>
>>> Logically the approach looks good, that being said it adds unnecessary
>>> tests in
>>> the fast path, why not just something like the patch below ? That only
>>> leaves the
>>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one
>>> line
>>> fix to unsuppmodes just allow it to be set for that specific case. The below
>>> returns the default behaviour befo
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
Hi,
> On 08/09/17 13:10, Nikolay Aleksandrov wrote:
>> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>>> Hi,
>>>
On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
> balance-alb mode") tried to fix transmit dynamic load balancing in
> balance-alb mode, which wasn't working after commit 8b426dc54cf4
> ("bonding: remove hardcoded value").
>
> It turned out that my previous patch only fixed the case when
> balance-alb was specified as bonding module parameter, and not when
> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
> common usage). In the latter case, tlb_dynamic_lb was set up according
> to the default mode of the bonding interface, which happens to be
> balance-rr.
>
> This additional patch addresses this issue by setting up tlb_dynamic_lb
> to 1 if "mode" is set to balance-alb through the sysfs interface.
>
> I didn't add code to change tlb_balance_lb back to the default value for
> other modes, because "mode" is usually set up only once during
> initialization, and it's not worthwhile to change the static variable
> bonding_defaults in bond_main.c to a global variable just for this
> purpose.
>
> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
> balance-tlb mode if it is set up using the sysfs interface. I didn't
> change that behavior, because the value of tlb_balance_lb can be changed
> using the sysfs interface for balance-tlb, and I didn't like changing
> the default value back and forth for balance-tlb.
>
> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
> is not an intended usage, so there is little use making it writable at
> this moment.
>
> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
> Reported-by: Reinis Rozitis
> Signed-off-by: Kosuke Tatsukawa
> Cc: sta...@vger.kernel.org # v4.12+
> ---
> drivers/net/bonding/bond_options.c |3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
I don't believe this to be the right solution, hardcoding it like this
changes user-visible behaviour. The issue is that if someone configured
it to be 0 in tlb mode, suddenly it will become 1 and will silently
override their config if they switch the mode to alb. Also it robs users
from their choice.
If you think this should be settable in ALB mode, then IMO you should
edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
That would also be consistent with how it's handled in TLB mode.
>>>
>>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>>> this point. All the current commits regarding tlb_dynamic_lb are for
>>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>>> to 0 is an intended usage.
>>>
>>>
Going back and looking at your previous fix I'd argue that it is also
wrong, you should've removed the mode check altogether to return the
original behaviour where the dynamic_lb is set to 1 (enabled) by
default and then ALB mode would've had it, of course that would've left
the case of setting it to 0 in TLB mode and switching to ALB, but that
is a different issue.
>>>
>>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>>> tlb_dynamic_lb is referenced in the following functions.
>>>
>>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
>>> + bond_tlb_xmit() -- Only used by balance-tlb
>>> + bond_open() -- Used by both balance-tlb and balance-alb
>>> + bond_check_params() -- Used during module initialization
>>> + bond_fill_info() -- Used to get/set value
>>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
>>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
>>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>>>
>>> The following untested patch adds code to make balance-alb work as if
>>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
>>> also reverts my previous patch.
>>>
>>> What do you think about this approach?
>>> ---
>>> Kosuke TATSUKAWA | 1st Platform Software Division
>>> | NEC Solution Innovators
>>> | ta...@ab.jp.nec.com
>>>
>>
>> Logically the approach looks good, that being said it adds unnecessary tests
>> in
>> the fast path, why not just something like the patch below ? That only
>> leaves the
>> problem if it is zeroed in TLB and switched to ALB mode, and that is a one
>> line
>> fix to unsuppmodes just allow it to be set for that specific case. The below
>> returns the default behaviour before the commit in your Fixes tag.
>>
>>
>
> Actually I'm fine with your approach, too. It will fix this regardless of the
> value of tlb_dynamic
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 08/09/17 13:10, Nikolay Aleksandrov wrote:
> On 08/09/17 05:06, Kosuke Tatsukawa wrote:
>> Hi,
>>
>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").
It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.
I didn't add code to change tlb_balance_lb back to the default value for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.
Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface. I didn't
change that behavior, because the value of tlb_balance_lb can be changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.
As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to. However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
Reported-by: Reinis Rozitis
Signed-off-by: Kosuke Tatsukawa
Cc: sta...@vger.kernel.org # v4.12+
---
drivers/net/bonding/bond_options.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> I don't believe this to be the right solution, hardcoding it like this
>>> changes user-visible behaviour. The issue is that if someone configured
>>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>>> override their config if they switch the mode to alb. Also it robs users
>>> from their choice.
>>>
>>> If you think this should be settable in ALB mode, then IMO you should
>>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>>> That would also be consistent with how it's handled in TLB mode.
>>
>> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
>> this point. All the current commits regarding tlb_dynamic_lb are for
>> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
>> to 0 is an intended usage.
>>
>>
>>> Going back and looking at your previous fix I'd argue that it is also
>>> wrong, you should've removed the mode check altogether to return the
>>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>>> default and then ALB mode would've had it, of course that would've left
>>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>>> is a different issue.
>>
>> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
>> tlb_dynamic_lb is referenced in the following functions.
>>
>> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
>> + bond_tlb_xmit() -- Only used by balance-tlb
>> + bond_open() -- Used by both balance-tlb and balance-alb
>> + bond_check_params() -- Used during module initialization
>> + bond_fill_info() -- Used to get/set value
>> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
>> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
>> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>>
>> The following untested patch adds code to make balance-alb work as if
>> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
>> also reverts my previous patch.
>>
>> What do you think about this approach?
>> ---
>> Kosuke TATSUKAWA | 1st Platform Software Division
>> | NEC Solution Innovators
>> | ta...@ab.jp.nec.com
>>
>
> Logically the approach looks good, that being said it adds unnecessary tests
> in
> the fast path, why not just something like the patch below ? That only leaves
> the
> problem if it is zeroed in TLB and switched to ALB mode, and that is a one
> line
> fix to unsuppmodes just allow it to be set for that specific case. The below
> returns the default behaviour before the commit in your Fixes tag.
>
>
Actually I'm fine with your approach, too. It will fix this regardless of the
value of tlb_dynamic_lb which sounds good to me for the price of a test in
the fast path.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 08/09/17 05:06, Kosuke Tatsukawa wrote:
> Hi,
>
>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>> ("bonding: remove hardcoded value").
>>>
>>> It turned out that my previous patch only fixed the case when
>>> balance-alb was specified as bonding module parameter, and not when
>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>> common usage). In the latter case, tlb_dynamic_lb was set up according
>>> to the default mode of the bonding interface, which happens to be
>>> balance-rr.
>>>
>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>
>>> I didn't add code to change tlb_balance_lb back to the default value for
>>> other modes, because "mode" is usually set up only once during
>>> initialization, and it's not worthwhile to change the static variable
>>> bonding_defaults in bond_main.c to a global variable just for this
>>> purpose.
>>>
>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>>> change that behavior, because the value of tlb_balance_lb can be changed
>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>> the default value back and forth for balance-tlb.
>>>
>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>>> is not an intended usage, so there is little use making it writable at
>>> this moment.
>>>
>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>> Reported-by: Reinis Rozitis
>>> Signed-off-by: Kosuke Tatsukawa
>>> Cc: sta...@vger.kernel.org # v4.12+
>>> ---
>>> drivers/net/bonding/bond_options.c |3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>
>> I don't believe this to be the right solution, hardcoding it like this
>> changes user-visible behaviour. The issue is that if someone configured
>> it to be 0 in tlb mode, suddenly it will become 1 and will silently
>> override their config if they switch the mode to alb. Also it robs users
>> from their choice.
>>
>> If you think this should be settable in ALB mode, then IMO you should
>> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
>> That would also be consistent with how it's handled in TLB mode.
>
> No, I don't think tlb_dynamic_lb should be settable in balance-alb at
> this point. All the current commits regarding tlb_dynamic_lb are for
> balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
> to 0 is an intended usage.
>
>
>> Going back and looking at your previous fix I'd argue that it is also
>> wrong, you should've removed the mode check altogether to return the
>> original behaviour where the dynamic_lb is set to 1 (enabled) by
>> default and then ALB mode would've had it, of course that would've left
>> the case of setting it to 0 in TLB mode and switching to ALB, but that
>> is a different issue.
>
> Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
> tlb_dynamic_lb is referenced in the following functions.
>
> + bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
> + bond_tlb_xmit() -- Only used by balance-tlb
> + bond_open() -- Used by both balance-tlb and balance-alb
> + bond_check_params() -- Used during module initialization
> + bond_fill_info() -- Used to get/set value
> + bond_option_tlb_dynamic_lb_set() -- Used to get/set value
> + bonding_show_tlb_dynamic_lb() -- Used to get/set value
> + bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
>
> The following untested patch adds code to make balance-alb work as if
> tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
> also reverts my previous patch.
>
> What do you think about this approach?
> ---
> Kosuke TATSUKAWA | 1st Platform Software Division
> | NEC Solution Innovators
> | ta...@ab.jp.nec.com
>
Logically the approach looks good, that being said it adds unnecessary tests in
the fast path, why not just something like the patch below ? That only leaves
the
problem if it is zeroed in TLB and switched to ALB mode, and that is a one line
fix to unsuppmodes just allow it to be set for that specific case. The below
returns the default behaviour before the commit in your Fixes tag.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index fc63992ab0e0..c99dc59d729b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4289,7 +4289,7 @@ static int bond_check_params(struct bond_params *params)
int bond_mode = BOND_MODE_ROUNDROBIN;
int xmit_hashtype = BOND_XMIT_POLIC
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
Hi,
> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>> balance-alb mode") tried to fix transmit dynamic load balancing in
>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>> ("bonding: remove hardcoded value").
>>
>> It turned out that my previous patch only fixed the case when
>> balance-alb was specified as bonding module parameter, and not when
>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>> common usage). In the latter case, tlb_dynamic_lb was set up according
>> to the default mode of the bonding interface, which happens to be
>> balance-rr.
>>
>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>
>> I didn't add code to change tlb_balance_lb back to the default value for
>> other modes, because "mode" is usually set up only once during
>> initialization, and it's not worthwhile to change the static variable
>> bonding_defaults in bond_main.c to a global variable just for this
>> purpose.
>>
>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>> change that behavior, because the value of tlb_balance_lb can be changed
>> using the sysfs interface for balance-tlb, and I didn't like changing
>> the default value back and forth for balance-tlb.
>>
>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>> is not an intended usage, so there is little use making it writable at
>> this moment.
>>
>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>> Reported-by: Reinis Rozitis
>> Signed-off-by: Kosuke Tatsukawa
>> Cc: sta...@vger.kernel.org # v4.12+
>> ---
>> drivers/net/bonding/bond_options.c |3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>
> I don't believe this to be the right solution, hardcoding it like this
> changes user-visible behaviour. The issue is that if someone configured
> it to be 0 in tlb mode, suddenly it will become 1 and will silently
> override their config if they switch the mode to alb. Also it robs users
> from their choice.
>
> If you think this should be settable in ALB mode, then IMO you should
> edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
> That would also be consistent with how it's handled in TLB mode.
No, I don't think tlb_dynamic_lb should be settable in balance-alb at
this point. All the current commits regarding tlb_dynamic_lb are for
balance-tlb mode, so I don't think balance-alb with tlb_dynamic_lb set
to 0 is an intended usage.
> Going back and looking at your previous fix I'd argue that it is also
> wrong, you should've removed the mode check altogether to return the
> original behaviour where the dynamic_lb is set to 1 (enabled) by
> default and then ALB mode would've had it, of course that would've left
> the case of setting it to 0 in TLB mode and switching to ALB, but that
> is a different issue.
Maybe balance-alb shouldn't be dependent on tlb_dynamic_lb.
tlb_dynamic_lb is referenced in the following functions.
+ bond_do_alb_xmit() -- Used by both balance-tlb and balance-alb
+ bond_tlb_xmit() -- Only used by balance-tlb
+ bond_open() -- Used by both balance-tlb and balance-alb
+ bond_check_params() -- Used during module initialization
+ bond_fill_info() -- Used to get/set value
+ bond_option_tlb_dynamic_lb_set() -- Used to get/set value
+ bonding_show_tlb_dynamic_lb() -- Used to get/set value
+ bond_is_nondyn_tlb() -- Only referenced if balance-tlb mode
The following untested patch adds code to make balance-alb work as if
tlb_dynamic_lb==1 for the functions which affect balance-alb mode. It
also reverts my previous patch.
What do you think about this approach?
---
Kosuke TATSUKAWA | 1st Platform Software Division
| NEC Solution Innovators
| ta...@ab.jp.nec.com
drivers/net/bonding/bond_alb.c |6 --
drivers/net/bonding/bond_main.c |5 +++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index c02cc81..a9229b5 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1325,7 +1325,8 @@ static int bond_do_alb_xmit(struct sk_buff *skb, struct
bonding *bond,
if (!tx_slave) {
/* unbalanced or unassigned, send through primary */
tx_slave = rcu_dereference(bond->curr_active_slave);
- if (bond->params.tlb_dynamic_lb)
+ if (bond->params.tlb_dynamic_lb ||
+ (BOND_MODE(bond) == BOND_MODE_ALB))
bond_info->unbalanced_load += skb->len;
}
@@ -1339,7 +1340,8 @@ s
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On Thu, Sep 7, 2017 at 5:47 PM, Mahesh Bandewar (महेश बंडेवार)
wrote:
> On Thu, Sep 7, 2017 at 5:39 PM, Mahesh Bandewar (महेश बंडेवार)
> wrote:
>> On Thu, Sep 7, 2017 at 4:09 PM, Nikolay Aleksandrov
>> wrote:
>>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").
It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.
I didn't add code to change tlb_balance_lb back to the default value for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.
Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface. I didn't
change that behavior, because the value of tlb_balance_lb can be changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.
As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to. However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>>
>> This is wrong! I think you are confusing various things here. ALB is
>> different mode from TLB and TLB-dynamic-lb is *only* a special case of
>> TLB. Your earlier patch is also wrong for the same reasons. However,
>> since the default value of tlb_dynamic_lb is set to 0 it's not
>> causing issues for ALB mode otherwise it would break ALB mode.
> I take this back. The default value is 1 so ALB is broken because of
> the referenced change.
>
>> tlb_dynamic_lb has absolutely nothing to do with ALB mode. Please
>> revert the earlier change (cbf5ecb30560).
>>
>> It's not clear to me what you saw as broken, so can't really suggest
>> what really need to be done.
Please scratch all I said.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On Thu, Sep 7, 2017 at 5:39 PM, Mahesh Bandewar (महेश बंडेवार)
wrote:
> On Thu, Sep 7, 2017 at 4:09 PM, Nikolay Aleksandrov
> wrote:
>> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>>> balance-alb mode") tried to fix transmit dynamic load balancing in
>>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>>> ("bonding: remove hardcoded value").
>>>
>>> It turned out that my previous patch only fixed the case when
>>> balance-alb was specified as bonding module parameter, and not when
>>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>>> common usage). In the latter case, tlb_dynamic_lb was set up according
>>> to the default mode of the bonding interface, which happens to be
>>> balance-rr.
>>>
>>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>>
>>> I didn't add code to change tlb_balance_lb back to the default value for
>>> other modes, because "mode" is usually set up only once during
>>> initialization, and it's not worthwhile to change the static variable
>>> bonding_defaults in bond_main.c to a global variable just for this
>>> purpose.
>>>
>>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>>> change that behavior, because the value of tlb_balance_lb can be changed
>>> using the sysfs interface for balance-tlb, and I didn't like changing
>>> the default value back and forth for balance-tlb.
>>>
>>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>>> is not an intended usage, so there is little use making it writable at
>>> this moment.
>>>
>>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
>
> This is wrong! I think you are confusing various things here. ALB is
> different mode from TLB and TLB-dynamic-lb is *only* a special case of
> TLB. Your earlier patch is also wrong for the same reasons. However,
> since the default value of tlb_dynamic_lb is set to 0 it's not
> causing issues for ALB mode otherwise it would break ALB mode.
I take this back. The default value is 1 so ALB is broken because of
the referenced change.
> tlb_dynamic_lb has absolutely nothing to do with ALB mode. Please
> revert the earlier change (cbf5ecb30560).
>
> It's not clear to me what you saw as broken, so can't really suggest
> what really need to be done.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On Thu, Sep 7, 2017 at 4:09 PM, Nikolay Aleksandrov
wrote:
> On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
>> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
>> balance-alb mode") tried to fix transmit dynamic load balancing in
>> balance-alb mode, which wasn't working after commit 8b426dc54cf4
>> ("bonding: remove hardcoded value").
>>
>> It turned out that my previous patch only fixed the case when
>> balance-alb was specified as bonding module parameter, and not when
>> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
>> common usage). In the latter case, tlb_dynamic_lb was set up according
>> to the default mode of the bonding interface, which happens to be
>> balance-rr.
>>
>> This additional patch addresses this issue by setting up tlb_dynamic_lb
>> to 1 if "mode" is set to balance-alb through the sysfs interface.
>>
>> I didn't add code to change tlb_balance_lb back to the default value for
>> other modes, because "mode" is usually set up only once during
>> initialization, and it's not worthwhile to change the static variable
>> bonding_defaults in bond_main.c to a global variable just for this
>> purpose.
>>
>> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
>> balance-tlb mode if it is set up using the sysfs interface. I didn't
>> change that behavior, because the value of tlb_balance_lb can be changed
>> using the sysfs interface for balance-tlb, and I didn't like changing
>> the default value back and forth for balance-tlb.
>>
>> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
>> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
>> is not an intended usage, so there is little use making it writable at
>> this moment.
>>
>> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
This is wrong! I think you are confusing various things here. ALB is
different mode from TLB and TLB-dynamic-lb is *only* a special case of
TLB. Your earlier patch is also wrong for the same reasons. However,
since the default value of tlb_dynamic_lb is set to 0 it's not
causing issues for ALB mode otherwise it would break ALB mode.
tlb_dynamic_lb has absolutely nothing to do with ALB mode. Please
revert the earlier change (cbf5ecb30560).
It's not clear to me what you saw as broken, so can't really suggest
what really need to be done.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
On 7.09.2017 01:47, Kosuke Tatsukawa wrote:
> Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
> balance-alb mode") tried to fix transmit dynamic load balancing in
> balance-alb mode, which wasn't working after commit 8b426dc54cf4
> ("bonding: remove hardcoded value").
>
> It turned out that my previous patch only fixed the case when
> balance-alb was specified as bonding module parameter, and not when
> balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
> common usage). In the latter case, tlb_dynamic_lb was set up according
> to the default mode of the bonding interface, which happens to be
> balance-rr.
>
> This additional patch addresses this issue by setting up tlb_dynamic_lb
> to 1 if "mode" is set to balance-alb through the sysfs interface.
>
> I didn't add code to change tlb_balance_lb back to the default value for
> other modes, because "mode" is usually set up only once during
> initialization, and it's not worthwhile to change the static variable
> bonding_defaults in bond_main.c to a global variable just for this
> purpose.
>
> Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
> balance-tlb mode if it is set up using the sysfs interface. I didn't
> change that behavior, because the value of tlb_balance_lb can be changed
> using the sysfs interface for balance-tlb, and I didn't like changing
> the default value back and forth for balance-tlb.
>
> As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
> written to. However, I think balance-alb with tlb_dynamic_lb set to 0
> is not an intended usage, so there is little use making it writable at
> this moment.
>
> Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
> Reported-by: Reinis Rozitis
> Signed-off-by: Kosuke Tatsukawa
> Cc: sta...@vger.kernel.org # v4.12+
> ---
> drivers/net/bonding/bond_options.c |3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
I don't believe this to be the right solution, hardcoding it like this
changes user-visible behaviour. The issue is that if someone configured
it to be 0 in tlb mode, suddenly it will become 1 and will silently
override their config if they switch the mode to alb. Also it robs users
from their choice.
If you think this should be settable in ALB mode, then IMO you should
edit tlb_dynamic_lb option's unsuppmodes and allow it to be set in ALB.
That would also be consistent with how it's handled in TLB mode.
Going back and looking at your previous fix I'd argue that it is also
wrong, you should've removed the mode check altogether to return the
original behaviour where the dynamic_lb is set to 1 (enabled) by
default and then ALB mode would've had it, of course that would've left
the case of setting it to 0 in TLB mode and switching to ALB, but that
is a different issue.
Cheers,
Nik
[PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").
It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.
I didn't add code to change tlb_balance_lb back to the default value for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.
Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface. I didn't
change that behavior, because the value of tlb_balance_lb can be changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.
As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to. However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
Reported-by: Reinis Rozitis
Signed-off-by: Kosuke Tatsukawa
Cc: sta...@vger.kernel.org # v4.12+
---
drivers/net/bonding/bond_options.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c
b/drivers/net/bonding/bond_options.c
index a12d603..5931aa2 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -754,6 +754,9 @@ static int bond_option_mode_set(struct bonding *bond,
bond->params.miimon);
}
+ if (newval->value == BOND_MODE_ALB)
+ bond->params.tlb_dynamic_lb = 1;
+
/* don't cache arp_validate between modes */
bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
bond->params.mode = newval->value;
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode
Reinis Rozitis reported that tlb_dynamic_lb was still 0 in balance-alb
mode even when using linux-4.12.10 kernels, which includes this patch.
It turned out that my previous patch only fixed the case when
balance-alb mode was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
I'll send another patch to address this case.
In the meantime, a workaround for this issue is to specify balance-tlb
mode as a module parameter for bonding. This will change the value of
tlb_balance_lb to 1, and make balance-alb work like pre-4.12 kernels.
Best regards.
> balance-alb mode used to have transmit dynamic load balancing feature
> enabled by default. However, transmit dynamic load balancing no longer
> works in balance-alb after commit 8b426dc54cf4 ("bonding: remove
> hardcoded value").
>
> Both balance-tlb and balance-alb use the function bond_do_alb_xmit() to
> send packets. This function uses the parameter tlb_dynamic_lb.
> tlb_dynamic_lb used to have the default value of 1 for balance-alb, but
> now the value is set to 0 except in balance-tlb.
>
> Re-enable transmit dyanmic load balancing by initializing tlb_dynamic_lb
> for balance-alb similar to balance-tlb.
>
> Signed-off-by: Kosuke Tatsukawa
> Cc: sta...@vger.kernel.org
> ---
> drivers/net/bonding/bond_main.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 14ff622..181839d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4596,7 +4596,7 @@ static int bond_check_params(struct bond_params *params)
> }
> ad_user_port_key = valptr->value;
>
> - if (bond_mode == BOND_MODE_TLB) {
> + if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
> bond_opt_initstr(&newval, "default");
> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> &newval);
---
Kosuke TATSUKAWA | 1st Platform Software Division
| NEC Solution Innovators
| ta...@ab.jp.nec.com
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode
From: Kosuke Tatsukawa
Date: Thu, 20 Jul 2017 05:20:40 +
> balance-alb mode used to have transmit dynamic load balancing feature
> enabled by default. However, transmit dynamic load balancing no longer
> works in balance-alb after commit 8b426dc54cf4 ("bonding: remove
> hardcoded value").
>
> Both balance-tlb and balance-alb use the function bond_do_alb_xmit() to
> send packets. This function uses the parameter tlb_dynamic_lb.
> tlb_dynamic_lb used to have the default value of 1 for balance-alb, but
> now the value is set to 0 except in balance-tlb.
>
> Re-enable transmit dyanmic load balancing by initializing tlb_dynamic_lb
> for balance-alb similar to balance-tlb.
>
> Signed-off-by: Kosuke Tatsukawa
Applied and queued up for -stable, thanks.
Andy, thanks for the Fixes: tag.
Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode
On Thu, Jul 20, 2017 at 1:20 AM, Kosuke Tatsukawa wrote:
> balance-alb mode used to have transmit dynamic load balancing feature
> enabled by default. However, transmit dynamic load balancing no longer
> works in balance-alb after commit 8b426dc54cf4 ("bonding: remove
> hardcoded value").
>
> Both balance-tlb and balance-alb use the function bond_do_alb_xmit() to
> send packets. This function uses the parameter tlb_dynamic_lb.
> tlb_dynamic_lb used to have the default value of 1 for balance-alb, but
> now the value is set to 0 except in balance-tlb.
>
> Re-enable transmit dyanmic load balancing by initializing tlb_dynamic_lb
> for balance-alb similar to balance-tlb.
>
> Signed-off-by: Kosuke Tatsukawa
> Cc: sta...@vger.kernel.org
You probably should add:
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value").
Otherwise this looks reasonable to me.
Acked-by: Andy Gospodarek
> ---
> drivers/net/bonding/bond_main.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 14ff622..181839d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4596,7 +4596,7 @@ static int bond_check_params(struct bond_params *params)
> }
> ad_user_port_key = valptr->value;
>
> - if (bond_mode == BOND_MODE_TLB) {
> + if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
> bond_opt_initstr(&newval, "default");
> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> &newval);
>
[PATCH] net: bonding: Fix transmit load balancing in balance-alb mode
balance-alb mode used to have transmit dynamic load balancing feature
enabled by default. However, transmit dynamic load balancing no longer
works in balance-alb after commit 8b426dc54cf4 ("bonding: remove
hardcoded value").
Both balance-tlb and balance-alb use the function bond_do_alb_xmit() to
send packets. This function uses the parameter tlb_dynamic_lb.
tlb_dynamic_lb used to have the default value of 1 for balance-alb, but
now the value is set to 0 except in balance-tlb.
Re-enable transmit dyanmic load balancing by initializing tlb_dynamic_lb
for balance-alb similar to balance-tlb.
Signed-off-by: Kosuke Tatsukawa
Cc: sta...@vger.kernel.org
---
drivers/net/bonding/bond_main.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 14ff622..181839d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4596,7 +4596,7 @@ static int bond_check_params(struct bond_params *params)
}
ad_user_port_key = valptr->value;
- if (bond_mode == BOND_MODE_TLB) {
+ if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
bond_opt_initstr(&newval, "default");
valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
&newval);
