| Message ID | 0639f1e3d366c5098d561a947fd416fa5277e7f4.1660572700.git.jtoppins@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bonding: 802.3ad: fix no transmission of LACPDUs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 85 this patch: 85 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 85 this patch: 85 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Aug 15, 2022 at 11:08:35AM -0400, Jonathan Toppins wrote: > This is caused by the global variable ad_ticks_per_sec being zero as > demonstrated by the reproducer script discussed below. This causes > all timer values in __ad_timer_to_ticks to be zero, resulting > in the periodic timer to never fire. > > To reproduce: > Run the script in > `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which > puts bonding into a state where it never transmits LACPDUs. > > line 44: ip link add fbond type bond mode 4 miimon 200 \ > xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast > setting bond param: ad_actor_sys_prio > given: > params.ad_actor_system = 0 > call stack: > bond_option_ad_actor_sys_prio() > -> bond_3ad_update_ad_actor_settings() > -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio > -> ad.system.sys_mac_addr = bond->dev->dev_addr; because > params.ad_actor_system == 0 > results: > ad.system.sys_mac_addr = bond->dev->dev_addr > > line 48: ip link set fbond address 52:54:00:3B:7C:A6 > setting bond MAC addr > call stack: > bond->dev->dev_addr = new_mac > > line 52: ip link set fbond type bond ad_actor_sys_prio 65535 > setting bond param: ad_actor_sys_prio > given: > params.ad_actor_system = 0 > call stack: > bond_option_ad_actor_sys_prio() > -> bond_3ad_update_ad_actor_settings() > -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio > -> ad.system.sys_mac_addr = bond->dev->dev_addr; because > params.ad_actor_system == 0 > results: > ad.system.sys_mac_addr = bond->dev->dev_addr > > line 60: ip link set veth1-bond down master fbond > given: > params.ad_actor_system = 0 > params.mode = BOND_MODE_8023AD > ad.system.sys_mac_addr == bond->dev->dev_addr > call stack: > bond_enslave > -> bond_3ad_initialize(); because first slave > -> if ad.system.sys_mac_addr != bond->dev->dev_addr > return > results: > Nothing is run in bond_3ad_initialize() because dev_add equals > sys_mac_addr leaving the global ad_ticks_per_sec zero as it is > never initialized anywhere else. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> > --- > > Notes: > v2: > * split this fix from the reproducer > v3: > * rebased to latest net/master > > drivers/net/bonding/bond_3ad.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c > index d7fb33c078e8..957d30db6f95 100644 > --- a/drivers/net/bonding/bond_3ad.c > +++ b/drivers/net/bonding/bond_3ad.c > @@ -84,7 +84,8 @@ enum ad_link_speed_type { > static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = { > 0, 0, 0, 0, 0, 0 > }; > -static u16 ad_ticks_per_sec; > + > +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL; > static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000; > > static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned = > -- > 2.31.1 > Acked-by: Hangbin Liu <liuhangbin@gmail.com>
Jonathan Toppins <jtoppins@redhat.com> wrote: >This is caused by the global variable ad_ticks_per_sec being zero as >demonstrated by the reproducer script discussed below. This causes >all timer values in __ad_timer_to_ticks to be zero, resulting >in the periodic timer to never fire. > >To reproduce: >Run the script in >`tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which >puts bonding into a state where it never transmits LACPDUs. > >line 44: ip link add fbond type bond mode 4 miimon 200 \ > xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast >setting bond param: ad_actor_sys_prio >given: > params.ad_actor_system = 0 >call stack: > bond_option_ad_actor_sys_prio() > -> bond_3ad_update_ad_actor_settings() > -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio > -> ad.system.sys_mac_addr = bond->dev->dev_addr; because > params.ad_actor_system == 0 >results: > ad.system.sys_mac_addr = bond->dev->dev_addr > >line 48: ip link set fbond address 52:54:00:3B:7C:A6 >setting bond MAC addr >call stack: > bond->dev->dev_addr = new_mac > >line 52: ip link set fbond type bond ad_actor_sys_prio 65535 >setting bond param: ad_actor_sys_prio >given: > params.ad_actor_system = 0 >call stack: > bond_option_ad_actor_sys_prio() > -> bond_3ad_update_ad_actor_settings() > -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio > -> ad.system.sys_mac_addr = bond->dev->dev_addr; because > params.ad_actor_system == 0 >results: > ad.system.sys_mac_addr = bond->dev->dev_addr > >line 60: ip link set veth1-bond down master fbond >given: > params.ad_actor_system = 0 > params.mode = BOND_MODE_8023AD > ad.system.sys_mac_addr == bond->dev->dev_addr >call stack: > bond_enslave > -> bond_3ad_initialize(); because first slave > -> if ad.system.sys_mac_addr != bond->dev->dev_addr > return >results: > Nothing is run in bond_3ad_initialize() because dev_add equals > sys_mac_addr leaving the global ad_ticks_per_sec zero as it is > never initialized anywhere else. > >Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >--- > >Notes: > v2: > * split this fix from the reproducer > v3: > * rebased to latest net/master > > drivers/net/bonding/bond_3ad.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >index d7fb33c078e8..957d30db6f95 100644 >--- a/drivers/net/bonding/bond_3ad.c >+++ b/drivers/net/bonding/bond_3ad.c >@@ -84,7 +84,8 @@ enum ad_link_speed_type { > static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = { > 0, 0, 0, 0, 0, 0 > }; >-static u16 ad_ticks_per_sec; >+ >+static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL; > static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000; I still feel like this is kind of a hack, as it's not really fixing bond_3ad_initialize to actually work (which is the real problem as I understand it). If it's ok to skip all that for this case, then why do we ever need to call bond_3ad_initialize? -J > static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned = >-- >2.31.1 > --- -Jay Vosburgh, jay.vosburgh@canonical.com
On 8/16/22 09:11, Jay Vosburgh wrote: > Jonathan Toppins <jtoppins@redhat.com> wrote: > >> This is caused by the global variable ad_ticks_per_sec being zero as >> demonstrated by the reproducer script discussed below. This causes >> all timer values in __ad_timer_to_ticks to be zero, resulting >> in the periodic timer to never fire. >> >> To reproduce: >> Run the script in >> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which >> puts bonding into a state where it never transmits LACPDUs. >> >> line 44: ip link add fbond type bond mode 4 miimon 200 \ >> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast >> setting bond param: ad_actor_sys_prio >> given: >> params.ad_actor_system = 0 >> call stack: >> bond_option_ad_actor_sys_prio() >> -> bond_3ad_update_ad_actor_settings() >> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio >> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because >> params.ad_actor_system == 0 >> results: >> ad.system.sys_mac_addr = bond->dev->dev_addr >> >> line 48: ip link set fbond address 52:54:00:3B:7C:A6 >> setting bond MAC addr >> call stack: >> bond->dev->dev_addr = new_mac >> >> line 52: ip link set fbond type bond ad_actor_sys_prio 65535 >> setting bond param: ad_actor_sys_prio >> given: >> params.ad_actor_system = 0 >> call stack: >> bond_option_ad_actor_sys_prio() >> -> bond_3ad_update_ad_actor_settings() >> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio >> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because >> params.ad_actor_system == 0 >> results: >> ad.system.sys_mac_addr = bond->dev->dev_addr >> >> line 60: ip link set veth1-bond down master fbond >> given: >> params.ad_actor_system = 0 >> params.mode = BOND_MODE_8023AD >> ad.system.sys_mac_addr == bond->dev->dev_addr >> call stack: >> bond_enslave >> -> bond_3ad_initialize(); because first slave >> -> if ad.system.sys_mac_addr != bond->dev->dev_addr >> return >> results: >> Nothing is run in bond_3ad_initialize() because dev_add equals >> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is >> never initialized anywhere else. >> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >> --- >> >> Notes: >> v2: >> * split this fix from the reproducer >> v3: >> * rebased to latest net/master >> >> drivers/net/bonding/bond_3ad.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >> index d7fb33c078e8..957d30db6f95 100644 >> --- a/drivers/net/bonding/bond_3ad.c >> +++ b/drivers/net/bonding/bond_3ad.c >> @@ -84,7 +84,8 @@ enum ad_link_speed_type { >> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = { >> 0, 0, 0, 0, 0, 0 >> }; >> -static u16 ad_ticks_per_sec; >> + >> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL; >> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000; > > I still feel like this is kind of a hack, as it's not really > fixing bond_3ad_initialize to actually work (which is the real problem > as I understand it). If it's ok to skip all that for this case, then > why do we ever need to call bond_3ad_initialize? > The way it is currently written you still need to call bond_3ad_initialize() just not for setting the tick resolution. The issue here is ad_ticks_per_sec is used in several places to calculate timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad protocol. And if this variable, ad_ticks_per_sec, is left uninitialized all of these timer periods go to zero. Since the value passed in bond_3ad_initialize() is an immediate value I simply moved it off of the call stack and set the static global variable instead. To fix bond_3ad_initialize(), probably something like the below is needed, but I do not understand why the guard if check was placed in bond_3ad_initialize(). diff --git i/drivers/net/bonding/bond_3ad.c w/drivers/net/bonding/bond_3ad.c index d7fb33c078e8..5b5146f5c4ea 100644 --- i/drivers/net/bonding/bond_3ad.c +++ w/drivers/net/bonding/bond_3ad.c @@ -2005,32 +2005,21 @@ void bond_3ad_initiate_agg_selection(struct bonding *bond, int timeout) * * Can be called only after the mac address of the bond is set. */ -void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution) +void bond_3ad_initialize(struct bonding *bond) { - /* check that the bond is not initialized yet */ - if (!MAC_ADDRESS_EQUAL(&(BOND_AD_INFO(bond).system.sys_mac_addr), - bond->dev->dev_addr)) { - - BOND_AD_INFO(bond).aggregator_identifier = 0; - - BOND_AD_INFO(bond).system.sys_priority = - bond->params.ad_actor_sys_prio; - if (is_zero_ether_addr(bond->params.ad_actor_system)) - BOND_AD_INFO(bond).system.sys_mac_addr = - *((struct mac_addr *)bond->dev->dev_addr); - else - BOND_AD_INFO(bond).system.sys_mac_addr = - *((struct mac_addr *)bond->params.ad_actor_system); - - /* initialize how many times this module is called in one - * second (should be about every 100ms) - */ - ad_ticks_per_sec = tick_resolution; + BOND_AD_INFO(bond).aggregator_identifier = 0; + BOND_AD_INFO(bond).system.sys_priority = + bond->params.ad_actor_sys_prio; + if (is_zero_ether_addr(bond->params.ad_actor_system)) + BOND_AD_INFO(bond).system.sys_mac_addr = + *((struct mac_addr *)bond->dev->dev_addr); + else + BOND_AD_INFO(bond).system.sys_mac_addr = + *((struct mac_addr *)bond->params.ad_actor_system); - bond_3ad_initiate_agg_selection(bond, - AD_AGGREGATOR_SELECTION_TIMER * - ad_ticks_per_sec); - } + bond_3ad_initiate_agg_selection(bond, + AD_AGGREGATOR_SELECTION_TIMER * + ad_ticks_per_sec); } /** diff --git i/drivers/net/bonding/bond_main.c w/drivers/net/bonding/bond_main.c index 50e60843020c..5f56af9dc3ba 100644 --- i/drivers/net/bonding/bond_main.c +++ w/drivers/net/bonding/bond_main.c @@ -2078,10 +2078,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, /* if this is the first slave */ if (!prev_slave) { SLAVE_AD_INFO(new_slave)->id = 1; - /* Initialize AD with the number of times that the AD timer is called in 1 second - * can be called only after the mac address of the bond is set + /* can be called only after the mac address of the + * bond is set */ - bond_3ad_initialize(bond, 1000/AD_TIMER_INTERVAL); + bond_3ad_initialize(bond); } else { SLAVE_AD_INFO(new_slave)->id = SLAVE_AD_INFO(prev_slave)->id + 1; diff --git i/include/net/bond_3ad.h w/include/net/bond_3ad.h index 184105d68294..be2992e6de5d 100644 --- i/include/net/bond_3ad.h +++ w/include/net/bond_3ad.h @@ -290,7 +290,7 @@ static inline const char *bond_3ad_churn_desc(churn_state_t state) } /* ========== AD Exported functions to the main bonding code ========== */ -void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution); +void bond_3ad_initialize(struct bonding *bond); void bond_3ad_bind_slave(struct slave *slave); void bond_3ad_unbind_slave(struct slave *slave); void bond_3ad_state_machine_handler(struct work_struct *); -Jon
On 8/16/22 09:41, Jonathan Toppins wrote: > On 8/16/22 09:11, Jay Vosburgh wrote: >> Jonathan Toppins <jtoppins@redhat.com> wrote: >> >>> This is caused by the global variable ad_ticks_per_sec being zero as >>> demonstrated by the reproducer script discussed below. This causes >>> all timer values in __ad_timer_to_ticks to be zero, resulting >>> in the periodic timer to never fire. >>> >>> To reproduce: >>> Run the script in >>> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` >>> which >>> puts bonding into a state where it never transmits LACPDUs. >>> >>> line 44: ip link add fbond type bond mode 4 miimon 200 \ >>> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast >>> setting bond param: ad_actor_sys_prio >>> given: >>> params.ad_actor_system = 0 >>> call stack: >>> bond_option_ad_actor_sys_prio() >>> -> bond_3ad_update_ad_actor_settings() >>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio >>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because >>> params.ad_actor_system == 0 >>> results: >>> ad.system.sys_mac_addr = bond->dev->dev_addr >>> >>> line 48: ip link set fbond address 52:54:00:3B:7C:A6 >>> setting bond MAC addr >>> call stack: >>> bond->dev->dev_addr = new_mac >>> >>> line 52: ip link set fbond type bond ad_actor_sys_prio 65535 >>> setting bond param: ad_actor_sys_prio >>> given: >>> params.ad_actor_system = 0 >>> call stack: >>> bond_option_ad_actor_sys_prio() >>> -> bond_3ad_update_ad_actor_settings() >>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio >>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because >>> params.ad_actor_system == 0 >>> results: >>> ad.system.sys_mac_addr = bond->dev->dev_addr >>> >>> line 60: ip link set veth1-bond down master fbond >>> given: >>> params.ad_actor_system = 0 >>> params.mode = BOND_MODE_8023AD >>> ad.system.sys_mac_addr == bond->dev->dev_addr >>> call stack: >>> bond_enslave >>> -> bond_3ad_initialize(); because first slave >>> -> if ad.system.sys_mac_addr != bond->dev->dev_addr >>> return >>> results: >>> Nothing is run in bond_3ad_initialize() because dev_add equals >>> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is >>> never initialized anywhere else. >>> >>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >>> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >>> --- >>> >>> Notes: >>> v2: >>> * split this fix from the reproducer >>> v3: >>> * rebased to latest net/master >>> >>> drivers/net/bonding/bond_3ad.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/bonding/bond_3ad.c >>> b/drivers/net/bonding/bond_3ad.c >>> index d7fb33c078e8..957d30db6f95 100644 >>> --- a/drivers/net/bonding/bond_3ad.c >>> +++ b/drivers/net/bonding/bond_3ad.c >>> @@ -84,7 +84,8 @@ enum ad_link_speed_type { >>> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = { >>> 0, 0, 0, 0, 0, 0 >>> }; >>> -static u16 ad_ticks_per_sec; >>> + >>> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL; >>> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000; >> >> I still feel like this is kind of a hack, as it's not really >> fixing bond_3ad_initialize to actually work (which is the real problem >> as I understand it). If it's ok to skip all that for this case, then >> why do we ever need to call bond_3ad_initialize? >> > > The way it is currently written you still need to call > bond_3ad_initialize() just not for setting the tick resolution. The > issue here is ad_ticks_per_sec is used in several places to calculate > timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad > protocol. And if this variable, ad_ticks_per_sec, is left uninitialized > all of these timer periods go to zero. Since the value passed in > bond_3ad_initialize() is an immediate value I simply moved it off of the > call stack and set the static global variable instead. > > To fix bond_3ad_initialize(), probably something like the below is > needed, but I do not understand why the guard if check was placed in > bond_3ad_initialize(). I looked at the history of the if guard in bond_3ad_initialize and it has existed since the creation of the git tree. It appears since commit 5ee14e6d336f ("bonding: 3ad: apply ad_actor settings changes immediately") the if guard is no longer needed and removing the if guard would also fix the problem, I have not tested yet. I think this patch series can be accepted as-is because, it does fix the issue as demonstrated by the kselftest accompanying the series and is the smallest change to fix the issue. Further, I don't see why we want to set the file-scoped variable, ad_ticks_per_sec, inside bond_3ad_initialize() as ad_ticks_per_sec is utilized across all bonds. It seems like ad_ticks_per_sec should be changed to a const and set at the top of the file. I see no value in passing the value as an unnamed constant on the stack when bond_3ad_initialize is called. These changes however could be done in the net-next tree as follow-on cleanup patches. Jay, how would you like to proceed? [...] Thanks, -Jon
Jonathan Toppins <jtoppins@redhat.com> wrote: >On 8/16/22 09:41, Jonathan Toppins wrote: >> On 8/16/22 09:11, Jay Vosburgh wrote: >>> Jonathan Toppins <jtoppins@redhat.com> wrote: >>> >>>> This is caused by the global variable ad_ticks_per_sec being zero as >>>> demonstrated by the reproducer script discussed below. This causes >>>> all timer values in __ad_timer_to_ticks to be zero, resulting >>>> in the periodic timer to never fire. >>>> >>>> To reproduce: >>>> Run the script in >>>> `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` >>>> which >>>> puts bonding into a state where it never transmits LACPDUs. >>>> >>>> line 44: ip link add fbond type bond mode 4 miimon 200 \ >>>> xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast >>>> setting bond param: ad_actor_sys_prio >>>> given: >>>> params.ad_actor_system = 0 >>>> call stack: >>>> bond_option_ad_actor_sys_prio() >>>> -> bond_3ad_update_ad_actor_settings() >>>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio >>>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because >>>> params.ad_actor_system == 0 >>>> results: >>>> ad.system.sys_mac_addr = bond->dev->dev_addr >>>> >>>> line 48: ip link set fbond address 52:54:00:3B:7C:A6 >>>> setting bond MAC addr >>>> call stack: >>>> bond->dev->dev_addr = new_mac >>>> >>>> line 52: ip link set fbond type bond ad_actor_sys_prio 65535 >>>> setting bond param: ad_actor_sys_prio >>>> given: >>>> params.ad_actor_system = 0 >>>> call stack: >>>> bond_option_ad_actor_sys_prio() >>>> -> bond_3ad_update_ad_actor_settings() >>>> -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio >>>> -> ad.system.sys_mac_addr = bond->dev->dev_addr; because >>>> params.ad_actor_system == 0 >>>> results: >>>> ad.system.sys_mac_addr = bond->dev->dev_addr >>>> >>>> line 60: ip link set veth1-bond down master fbond >>>> given: >>>> params.ad_actor_system = 0 >>>> params.mode = BOND_MODE_8023AD >>>> ad.system.sys_mac_addr == bond->dev->dev_addr >>>> call stack: >>>> bond_enslave >>>> -> bond_3ad_initialize(); because first slave >>>> -> if ad.system.sys_mac_addr != bond->dev->dev_addr >>>> return >>>> results: >>>> Nothing is run in bond_3ad_initialize() because dev_add equals >>>> sys_mac_addr leaving the global ad_ticks_per_sec zero as it is >>>> never initialized anywhere else. >>>> >>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >>>> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >>>> --- >>>> >>>> Notes: >>>> v2: >>>> * split this fix from the reproducer >>>> v3: >>>> * rebased to latest net/master >>>> >>>> drivers/net/bonding/bond_3ad.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/bonding/bond_3ad.c >>>> b/drivers/net/bonding/bond_3ad.c >>>> index d7fb33c078e8..957d30db6f95 100644 >>>> --- a/drivers/net/bonding/bond_3ad.c >>>> +++ b/drivers/net/bonding/bond_3ad.c >>>> @@ -84,7 +84,8 @@ enum ad_link_speed_type { >>>> static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = { >>>> 0, 0, 0, 0, 0, 0 >>>> }; >>>> -static u16 ad_ticks_per_sec; >>>> + >>>> +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL; >>>> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000; >>> >>> I still feel like this is kind of a hack, as it's not really >>> fixing bond_3ad_initialize to actually work (which is the real problem >>> as I understand it). If it's ok to skip all that for this case, then >>> why do we ever need to call bond_3ad_initialize? >>> >> The way it is currently written you still need to call >> bond_3ad_initialize() just not for setting the tick resolution. The >> issue here is ad_ticks_per_sec is used in several places to calculate >> timer periods, __ad_timer_to_ticks(), for various timers in the 802.3ad >> protocol. And if this variable, ad_ticks_per_sec, is left uninitialized >> all of these timer periods go to zero. Since the value passed in >> bond_3ad_initialize() is an immediate value I simply moved it off of the >> call stack and set the static global variable instead. >> To fix bond_3ad_initialize(), probably something like the below is >> needed, but I do not understand why the guard if check was placed in >> bond_3ad_initialize(). > >I looked at the history of the if guard in bond_3ad_initialize and it has >existed since the creation of the git tree. It appears since commit >5ee14e6d336f ("bonding: 3ad: apply ad_actor settings changes immediately") >the if guard is no longer needed and removing the if guard would also fix >the problem, I have not tested yet. The logic in bond_3ad_initialize probably came that way when the code was contributed sometime during 2.4.x. Curiosity got the better of me, and I looked at the 2.4.35 code. I'm not sure what the point of the test was even then, since bond_3ad_initialize is only called when adding a first interface to the bond, and there wasn't a way to tweak the sys_mac_addr then. In the current code, I think the if test could fail if sys_mac_addr is set manually to be equal to the bond's MAC prior to adding the first interface to the bond. As far as I can tell, the only result of failing the MAC test would be that the agg selection timer wouldn't be started, which is an optimization to reduce LACP aggregator flailing when multiple interfaces are added at the same time (IEEE 802.1AX-2000 6.4.9 and 6.4.12.1.n). >I think this patch series can be accepted as-is because, it does fix the >issue as demonstrated by the kselftest accompanying the series and is the >smallest change to fix the issue. > >Further, I don't see why we want to set the file-scoped variable, >ad_ticks_per_sec, inside bond_3ad_initialize() as ad_ticks_per_sec is >utilized across all bonds. It seems like ad_ticks_per_sec should be >changed to a const and set at the top of the file. I see no value in >passing the value as an unnamed constant on the stack when >bond_3ad_initialize is called. These changes however could be done in the >net-next tree as follow-on cleanup patches. > >Jay, how would you like to proceed? I don't have issue with moving ad_ticks_per_sec to a file scope constant. The minimal change here, though, is effectively making the tick_resolution parameter to bond_3ad_initialize be ignored, even though the compiler won't complain about it. Given that there is already mystery in how some of this works, I'd prefer the patches to make the code clearer, so my vote is for the "fix it right" method, i.e., make ad_ticks_per_sec a real constant, remove the tick_resolution parameter from bond_3ad_initialize and drop the "if MAC_ADDRESS_COMPARE" test therein. -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index d7fb33c078e8..957d30db6f95 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -84,7 +84,8 @@ enum ad_link_speed_type { static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = { 0, 0, 0, 0, 0, 0 }; -static u16 ad_ticks_per_sec; + +static u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL; static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000; static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned =
This is caused by the global variable ad_ticks_per_sec being zero as demonstrated by the reproducer script discussed below. This causes all timer values in __ad_timer_to_ticks to be zero, resulting in the periodic timer to never fire. To reproduce: Run the script in `tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh` which puts bonding into a state where it never transmits LACPDUs. line 44: ip link add fbond type bond mode 4 miimon 200 \ xmit_hash_policy 1 ad_actor_sys_prio 65535 lacp_rate fast setting bond param: ad_actor_sys_prio given: params.ad_actor_system = 0 call stack: bond_option_ad_actor_sys_prio() -> bond_3ad_update_ad_actor_settings() -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio -> ad.system.sys_mac_addr = bond->dev->dev_addr; because params.ad_actor_system == 0 results: ad.system.sys_mac_addr = bond->dev->dev_addr line 48: ip link set fbond address 52:54:00:3B:7C:A6 setting bond MAC addr call stack: bond->dev->dev_addr = new_mac line 52: ip link set fbond type bond ad_actor_sys_prio 65535 setting bond param: ad_actor_sys_prio given: params.ad_actor_system = 0 call stack: bond_option_ad_actor_sys_prio() -> bond_3ad_update_ad_actor_settings() -> set ad.system.sys_priority = bond->params.ad_actor_sys_prio -> ad.system.sys_mac_addr = bond->dev->dev_addr; because params.ad_actor_system == 0 results: ad.system.sys_mac_addr = bond->dev->dev_addr line 60: ip link set veth1-bond down master fbond given: params.ad_actor_system = 0 params.mode = BOND_MODE_8023AD ad.system.sys_mac_addr == bond->dev->dev_addr call stack: bond_enslave -> bond_3ad_initialize(); because first slave -> if ad.system.sys_mac_addr != bond->dev->dev_addr return results: Nothing is run in bond_3ad_initialize() because dev_add equals sys_mac_addr leaving the global ad_ticks_per_sec zero as it is never initialized anywhere else. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> --- Notes: v2: * split this fix from the reproducer v3: * rebased to latest net/master drivers/net/bonding/bond_3ad.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)