| Message ID | 1423270311-9181-1-git-send-email-maheshb@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Feb 06, 2015 at 04:51:51PM -0800, Mahesh Bandewar wrote: >This patches implements the poll_controller support for all >bonding driver. If the slaves have poll_controller net_op defined, >this implementation calls them. This is mode agnostic implementation >and iterates through all slaves (based on mode) and calls respective >handler. > >Signed-off-by: Mahesh Bandewar <maheshb@google.com> Really good patchset, thank you. Two nitpicks below, nothing serious. Also, please, add Doc/bonding.txt changes for every user-visible change/enhancement, this doc is a go-to for a lot of users. Otherwise, with the doc and other issues, pointed by Jay and Andy, fixed, I guess it's good to go. The 0/ patch would be welcome too, btw. >--- > drivers/net/bonding/bond_3ad.c | 24 +++++++++++++++++++++ > drivers/net/bonding/bond_main.c | 47 +++++++++++++++++++++++++++++++++++++++++ > include/net/bond_3ad.h | 1 + > 3 files changed, 72 insertions(+) > >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >index 9b436696b95e..14f2ebe786c5 100644 >--- a/drivers/net/bonding/bond_3ad.c >+++ b/drivers/net/bonding/bond_3ad.c >@@ -2477,6 +2477,30 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info) > return ret; > } > >+#define BOND_3AD_PORT_OPERATIONAL \ >+ (AD_STATE_DISTRIBUTING | AD_STATE_COLLECTING | \ >+ AD_STATE_SYNCHRONIZATION | AD_STATE_AGGREGATION) >+ >+static int bond_3ad_port_operational(struct slave *slave) >+{ >+ port_t *port = &SLAVE_AD_INFO(slave)->port; >+ >+ return bond_slave_can_tx(slave) && >+ (port->actor_oper_port_state & port->partner_oper.port_state & >+ BOND_3AD_PORT_OPERATIONAL) == BOND_3AD_PORT_OPERATIONAL; >+} >+ >+/* bond_3ad_port_is_active - check if a slave port is active or not. A port >+ * is active when it can forward traffic. >+ * >+ * @slave: slave port to check state for. >+ * Returns: 0 if not active else is active. >+ */ >+int bond_3ad_port_is_active(struct slave *slave) >+{ >+ return bond_3ad_port_operational(slave); >+} >+ > int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, > struct slave *slave) > { >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index c9e519cb9214..a50ec87486f3 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -928,6 +928,53 @@ static inline void slave_disable_netpoll(struct slave *slave) > > static void bond_poll_controller(struct net_device *bond_dev) > { >+ struct bonding *bond = netdev_priv(bond_dev); >+ struct slave *slave = NULL; >+ struct list_head *iter; >+ struct ad_info ad_info; >+ struct aggregator *agg; >+ const struct net_device_ops *ops; >+ bool call_slave_netpoll; >+ >+ if (BOND_MODE(bond) == BOND_MODE_8023AD) >+ if (bond_3ad_get_active_agg_info(bond, &ad_info)) >+ return; >+ >+ bond_for_each_slave(bond, slave, iter) { >+ call_slave_netpoll = false; >+ switch (BOND_MODE(bond)) { >+ case BOND_MODE_8023AD: >+ agg = SLAVE_AD_INFO(slave)->port.aggregator; >+ if (!bond_slave_is_up(slave)) bond_3ad_port_is_active() already contains the check for being up. >+ break; >+ if (agg && agg->aggregator_identifier != Hm, should we poll it without an aggregator? >+ ad_info.aggregator_id) >+ break; >+ if (!bond_3ad_port_is_active(slave) && >+ ad_info.ports != 1) >+ break; >+ >+ call_slave_netpoll = true; >+ break; >+ default: >+ if (bond_slave_is_up(slave)) >+ call_slave_netpoll = true; >+ break; >+ } >+ >+ if (call_slave_netpoll) { >+ ops = slave->dev->netdev_ops; >+ if (ops->ndo_poll_controller) { It's weird to see this check so down the road. Maybe reorg the logic to something like: bond_for_each_slave() { if (!bond_slave_is_up() || !bond_slave_has_ndo_poll()) continue; if (BOND_MODE(bond) == 3AD && !bond_3ad_port_is_active()) continue; slave-do_ndo_poll_stuff(); } Just as a thought, might be easier to read/shorter. >+ struct netpoll_info *ni = >+ rcu_dereference_bh(slave->dev->npinfo); >+ >+ if (down_trylock(&ni->dev_lock)) >+ continue; >+ ops->ndo_poll_controller(slave->dev); >+ up(&ni->dev_lock); >+ } >+ } >+ } > } > > static void bond_netpoll_cleanup(struct net_device *bond_dev) >diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h >index f04cdbb7848e..6c455c646d61 100644 >--- a/include/net/bond_3ad.h >+++ b/include/net/bond_3ad.h >@@ -278,5 +278,6 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, > struct slave *slave); > int bond_3ad_set_carrier(struct bonding *bond); > void bond_3ad_update_lacp_rate(struct bonding *bond); >+int bond_3ad_port_is_active(struct slave *slave); > #endif /* _NET_BOND_3AD_H */ > >-- >2.2.0.rc0.207.ga3a616c > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 7, 2015 at 1:04 AM, Veaceslav Falico <vfalico@gmail.com> wrote: > On Fri, Feb 06, 2015 at 04:51:51PM -0800, Mahesh Bandewar wrote: >> >> This patches implements the poll_controller support for all >> bonding driver. If the slaves have poll_controller net_op defined, >> this implementation calls them. This is mode agnostic implementation >> and iterates through all slaves (based on mode) and calls respective >> handler. >> >> Signed-off-by: Mahesh Bandewar <maheshb@google.com> > > > Really good patchset, thank you. Two nitpicks below, nothing serious. Also, > please, add Doc/bonding.txt changes for every user-visible > change/enhancement, this doc is a go-to for a lot of users. > > Otherwise, with the doc and other issues, pointed by Jay and Andy, fixed, I > guess it's good to go. The 0/ patch would be welcome too, btw. > > >> --- >> drivers/net/bonding/bond_3ad.c | 24 +++++++++++++++++++++ >> drivers/net/bonding/bond_main.c | 47 >> +++++++++++++++++++++++++++++++++++++++++ >> include/net/bond_3ad.h | 1 + >> 3 files changed, 72 insertions(+) >> >> diff --git a/drivers/net/bonding/bond_3ad.c >> b/drivers/net/bonding/bond_3ad.c >> index 9b436696b95e..14f2ebe786c5 100644 >> --- a/drivers/net/bonding/bond_3ad.c >> +++ b/drivers/net/bonding/bond_3ad.c >> @@ -2477,6 +2477,30 @@ int bond_3ad_get_active_agg_info(struct bonding >> *bond, struct ad_info *ad_info) >> return ret; >> } >> >> +#define BOND_3AD_PORT_OPERATIONAL \ >> + (AD_STATE_DISTRIBUTING | AD_STATE_COLLECTING | \ >> + AD_STATE_SYNCHRONIZATION | AD_STATE_AGGREGATION) >> + >> +static int bond_3ad_port_operational(struct slave *slave) >> +{ >> + port_t *port = &SLAVE_AD_INFO(slave)->port; >> + >> + return bond_slave_can_tx(slave) && >> + (port->actor_oper_port_state & >> port->partner_oper.port_state & >> + BOND_3AD_PORT_OPERATIONAL) == BOND_3AD_PORT_OPERATIONAL; >> +} >> + >> +/* bond_3ad_port_is_active - check if a slave port is active or not. A >> port >> + * is active when it can forward traffic. >> + * >> + * @slave: slave port to check state for. >> + * Returns: 0 if not active else is active. >> + */ >> +int bond_3ad_port_is_active(struct slave *slave) >> +{ >> + return bond_3ad_port_operational(slave); >> +} >> + >> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, >> struct slave *slave) >> { >> diff --git a/drivers/net/bonding/bond_main.c >> b/drivers/net/bonding/bond_main.c >> index c9e519cb9214..a50ec87486f3 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -928,6 +928,53 @@ static inline void slave_disable_netpoll(struct slave >> *slave) >> >> static void bond_poll_controller(struct net_device *bond_dev) >> { >> + struct bonding *bond = netdev_priv(bond_dev); >> + struct slave *slave = NULL; >> + struct list_head *iter; >> + struct ad_info ad_info; >> + struct aggregator *agg; >> + const struct net_device_ops *ops; >> + bool call_slave_netpoll; >> + >> + if (BOND_MODE(bond) == BOND_MODE_8023AD) >> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) >> + return; >> + >> + bond_for_each_slave(bond, slave, iter) { >> + call_slave_netpoll = false; >> + switch (BOND_MODE(bond)) { >> + case BOND_MODE_8023AD: >> + agg = SLAVE_AD_INFO(slave)->port.aggregator; >> + if (!bond_slave_is_up(slave)) > > > bond_3ad_port_is_active() already contains the check for being up. > >> + break; >> + if (agg && agg->aggregator_identifier != > > > Hm, should we poll it without an aggregator? > >> + ad_info.aggregator_id) >> + break; >> + if (!bond_3ad_port_is_active(slave) && >> + ad_info.ports != 1) >> + break; >> + >> + call_slave_netpoll = true; >> + break; >> + default: >> + if (bond_slave_is_up(slave)) >> + call_slave_netpoll = true; >> + break; >> + } >> + >> + if (call_slave_netpoll) { >> + ops = slave->dev->netdev_ops; >> + if (ops->ndo_poll_controller) { > > > It's weird to see this check so down the road. Maybe reorg the logic to > something like: > > bond_for_each_slave() { > if (!bond_slave_is_up() || !bond_slave_has_ndo_poll()) > continue; > > if (BOND_MODE(bond) == 3AD && !bond_3ad_port_is_active()) > continue; > > slave-do_ndo_poll_stuff(); > } > > Just as a thought, might be easier to read/shorter. > > OK, I'll try that. thanks >> + struct netpoll_info *ni = >> + >> rcu_dereference_bh(slave->dev->npinfo); >> + >> + if (down_trylock(&ni->dev_lock)) >> + continue; >> + ops->ndo_poll_controller(slave->dev); >> + up(&ni->dev_lock); >> + } >> + } >> + } >> } >> >> static void bond_netpoll_cleanup(struct net_device *bond_dev) >> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h >> index f04cdbb7848e..6c455c646d61 100644 >> --- a/include/net/bond_3ad.h >> +++ b/include/net/bond_3ad.h >> @@ -278,5 +278,6 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, >> struct bonding *bond, >> struct slave *slave); >> int bond_3ad_set_carrier(struct bonding *bond); >> void bond_3ad_update_lacp_rate(struct bonding *bond); >> +int bond_3ad_port_is_active(struct slave *slave); >> #endif /* _NET_BOND_3AD_H */ >> >> -- >> 2.2.0.rc0.207.ga3a616c >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 7, 2015 at 1:04 AM, Veaceslav Falico <vfalico@gmail.com> wrote: > On Fri, Feb 06, 2015 at 04:51:51PM -0800, Mahesh Bandewar wrote: >> >> This patches implements the poll_controller support for all >> bonding driver. If the slaves have poll_controller net_op defined, >> this implementation calls them. This is mode agnostic implementation >> and iterates through all slaves (based on mode) and calls respective >> handler. >> >> Signed-off-by: Mahesh Bandewar <maheshb@google.com> > > > Really good patchset, thank you. Two nitpicks below, nothing serious. Also, > please, add Doc/bonding.txt changes for every user-visible > change/enhancement, this doc is a go-to for a lot of users. > > Otherwise, with the doc and other issues, pointed by Jay and Andy, fixed, I > guess it's good to go. The 0/ patch would be welcome too, btw. > > >> --- >> drivers/net/bonding/bond_3ad.c | 24 +++++++++++++++++++++ >> drivers/net/bonding/bond_main.c | 47 >> +++++++++++++++++++++++++++++++++++++++++ >> include/net/bond_3ad.h | 1 + >> 3 files changed, 72 insertions(+) >> >> diff --git a/drivers/net/bonding/bond_3ad.c >> b/drivers/net/bonding/bond_3ad.c >> index 9b436696b95e..14f2ebe786c5 100644 >> --- a/drivers/net/bonding/bond_3ad.c >> +++ b/drivers/net/bonding/bond_3ad.c >> @@ -2477,6 +2477,30 @@ int bond_3ad_get_active_agg_info(struct bonding >> *bond, struct ad_info *ad_info) >> return ret; >> } >> >> +#define BOND_3AD_PORT_OPERATIONAL \ >> + (AD_STATE_DISTRIBUTING | AD_STATE_COLLECTING | \ >> + AD_STATE_SYNCHRONIZATION | AD_STATE_AGGREGATION) >> + >> +static int bond_3ad_port_operational(struct slave *slave) >> +{ >> + port_t *port = &SLAVE_AD_INFO(slave)->port; >> + >> + return bond_slave_can_tx(slave) && >> + (port->actor_oper_port_state & >> port->partner_oper.port_state & >> + BOND_3AD_PORT_OPERATIONAL) == BOND_3AD_PORT_OPERATIONAL; >> +} >> + >> +/* bond_3ad_port_is_active - check if a slave port is active or not. A >> port >> + * is active when it can forward traffic. >> + * >> + * @slave: slave port to check state for. >> + * Returns: 0 if not active else is active. >> + */ >> +int bond_3ad_port_is_active(struct slave *slave) >> +{ >> + return bond_3ad_port_operational(slave); >> +} >> + >> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, >> struct slave *slave) >> { >> diff --git a/drivers/net/bonding/bond_main.c >> b/drivers/net/bonding/bond_main.c >> index c9e519cb9214..a50ec87486f3 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -928,6 +928,53 @@ static inline void slave_disable_netpoll(struct slave >> *slave) >> >> static void bond_poll_controller(struct net_device *bond_dev) >> { >> + struct bonding *bond = netdev_priv(bond_dev); >> + struct slave *slave = NULL; >> + struct list_head *iter; >> + struct ad_info ad_info; >> + struct aggregator *agg; >> + const struct net_device_ops *ops; >> + bool call_slave_netpoll; >> + >> + if (BOND_MODE(bond) == BOND_MODE_8023AD) >> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) >> + return; >> + >> + bond_for_each_slave(bond, slave, iter) { >> + call_slave_netpoll = false; >> + switch (BOND_MODE(bond)) { >> + case BOND_MODE_8023AD: >> + agg = SLAVE_AD_INFO(slave)->port.aggregator; >> + if (!bond_slave_is_up(slave)) > > > bond_3ad_port_is_active() already contains the check for being up. > >> + break; >> + if (agg && agg->aggregator_identifier != > > > Hm, should we poll it without an aggregator? > I think I missed this comment earlier, but it would be wrong. I mean we have to perform this check otherwise it would be wrong. >> + ad_info.aggregator_id) >> + break; >> + if (!bond_3ad_port_is_active(slave) && >> + ad_info.ports != 1) >> + break; >> + >> + call_slave_netpoll = true; >> + break; >> + default: >> + if (bond_slave_is_up(slave)) >> + call_slave_netpoll = true; >> + break; >> + } >> + >> + if (call_slave_netpoll) { >> + ops = slave->dev->netdev_ops; >> + if (ops->ndo_poll_controller) { > > > It's weird to see this check so down the road. Maybe reorg the logic to > something like: > > bond_for_each_slave() { > if (!bond_slave_is_up() || !bond_slave_has_ndo_poll()) > continue; > > if (BOND_MODE(bond) == 3AD && !bond_3ad_port_is_active()) > continue; > > slave-do_ndo_poll_stuff(); > } > > Just as a thought, might be easier to read/shorter. > I tried this and it does save one bool variable but not much different. Here it is - bond_for_each_slave(bond, slave, iter) { ops = slave->dev->netdev_ops; if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller) continue; if (BOND_MODE(bond) == BOND_MODE_8023AD) { struct aggregator *agg = SLAVE_AD_INFO(slave)->port.aggregator; if (agg && agg->aggregator_identifier != ad_info.aggregator_id) continue; if (!bond_3ad_port_is_active(slave) || ad_info.ports != 1) continue; } ni = rcu_dereference_bh(slave->dev->npinfo); if (down_trylock(&ni->dev_lock)) continue; ops->ndo_poll_controller(slave->dev); up(&ni->dev_lock); } I'm not sure if it's any different! BTW I did keep the aggregator check since I'm not convinced that it's OK to eliminate the check. > >> + struct netpoll_info *ni = >> + >> rcu_dereference_bh(slave->dev->npinfo); >> + >> + if (down_trylock(&ni->dev_lock)) >> + continue; >> + ops->ndo_poll_controller(slave->dev); >> + up(&ni->dev_lock); >> + } >> + } >> + } >> } >> >> static void bond_netpoll_cleanup(struct net_device *bond_dev) >> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h >> index f04cdbb7848e..6c455c646d61 100644 >> --- a/include/net/bond_3ad.h >> +++ b/include/net/bond_3ad.h >> @@ -278,5 +278,6 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, >> struct bonding *bond, >> struct slave *slave); >> int bond_3ad_set_carrier(struct bonding *bond); >> void bond_3ad_update_lacp_rate(struct bonding *bond); >> +int bond_3ad_port_is_active(struct slave *slave); >> #endif /* _NET_BOND_3AD_H */ >> >> -- >> 2.2.0.rc0.207.ga3a616c >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mahesh Bandewar <maheshb@google.com> wrote: >On Sat, Feb 7, 2015 at 1:04 AM, Veaceslav Falico <vfalico@gmail.com> wrote: >> On Fri, Feb 06, 2015 at 04:51:51PM -0800, Mahesh Bandewar wrote: >>> >>> This patches implements the poll_controller support for all >>> bonding driver. If the slaves have poll_controller net_op defined, >>> this implementation calls them. This is mode agnostic implementation >>> and iterates through all slaves (based on mode) and calls respective >>> handler. >>> >>> Signed-off-by: Mahesh Bandewar <maheshb@google.com> >> >> >> Really good patchset, thank you. Two nitpicks below, nothing serious. Also, >> please, add Doc/bonding.txt changes for every user-visible >> change/enhancement, this doc is a go-to for a lot of users. >> >> Otherwise, with the doc and other issues, pointed by Jay and Andy, fixed, I >> guess it's good to go. The 0/ patch would be welcome too, btw. >> >> >>> --- >>> drivers/net/bonding/bond_3ad.c | 24 +++++++++++++++++++++ >>> drivers/net/bonding/bond_main.c | 47 >>> +++++++++++++++++++++++++++++++++++++++++ >>> include/net/bond_3ad.h | 1 + >>> 3 files changed, 72 insertions(+) >>> >>> diff --git a/drivers/net/bonding/bond_3ad.c >>> b/drivers/net/bonding/bond_3ad.c >>> index 9b436696b95e..14f2ebe786c5 100644 >>> --- a/drivers/net/bonding/bond_3ad.c >>> +++ b/drivers/net/bonding/bond_3ad.c >>> @@ -2477,6 +2477,30 @@ int bond_3ad_get_active_agg_info(struct bonding >>> *bond, struct ad_info *ad_info) >>> return ret; >>> } >>> >>> +#define BOND_3AD_PORT_OPERATIONAL \ >>> + (AD_STATE_DISTRIBUTING | AD_STATE_COLLECTING | \ >>> + AD_STATE_SYNCHRONIZATION | AD_STATE_AGGREGATION) >>> + >>> +static int bond_3ad_port_operational(struct slave *slave) >>> +{ >>> + port_t *port = &SLAVE_AD_INFO(slave)->port; >>> + >>> + return bond_slave_can_tx(slave) && >>> + (port->actor_oper_port_state & >>> port->partner_oper.port_state & >>> + BOND_3AD_PORT_OPERATIONAL) == BOND_3AD_PORT_OPERATIONAL; >>> +} >>> + >>> +/* bond_3ad_port_is_active - check if a slave port is active or not. A >>> port >>> + * is active when it can forward traffic. >>> + * >>> + * @slave: slave port to check state for. >>> + * Returns: 0 if not active else is active. >>> + */ >>> +int bond_3ad_port_is_active(struct slave *slave) >>> +{ >>> + return bond_3ad_port_operational(slave); >>> +} >>> + >>> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, >>> struct slave *slave) >>> { >>> diff --git a/drivers/net/bonding/bond_main.c >>> b/drivers/net/bonding/bond_main.c >>> index c9e519cb9214..a50ec87486f3 100644 >>> --- a/drivers/net/bonding/bond_main.c >>> +++ b/drivers/net/bonding/bond_main.c >>> @@ -928,6 +928,53 @@ static inline void slave_disable_netpoll(struct slave >>> *slave) >>> >>> static void bond_poll_controller(struct net_device *bond_dev) >>> { >>> + struct bonding *bond = netdev_priv(bond_dev); >>> + struct slave *slave = NULL; >>> + struct list_head *iter; >>> + struct ad_info ad_info; >>> + struct aggregator *agg; >>> + const struct net_device_ops *ops; >>> + bool call_slave_netpoll; >>> + >>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) >>> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) >>> + return; >>> + >>> + bond_for_each_slave(bond, slave, iter) { >>> + call_slave_netpoll = false; >>> + switch (BOND_MODE(bond)) { >>> + case BOND_MODE_8023AD: >>> + agg = SLAVE_AD_INFO(slave)->port.aggregator; >>> + if (!bond_slave_is_up(slave)) >> >> >> bond_3ad_port_is_active() already contains the check for being up. >> >>> + break; >>> + if (agg && agg->aggregator_identifier != >> >> >> Hm, should we poll it without an aggregator? >> >I think I missed this comment earlier, but it would be wrong. I mean >we have to perform this check otherwise it would be wrong. This looks to be a check to determine if the slave is a member of the active aggregator. I don't believe we'll want to transmit on a slave that's not a member of the active agg. FWIW, bond_3ad_xmit_xor has similar logic. Slaves will generally be a member of some aggregator (active or otherwise), except for short periods of time immediately after enslavement (before the next run of the state machine) and during port selection logic during transit from one agg to another. Since the port.aggregator can be NULL, I believe the test as-is is necessary. -J >>> + ad_info.aggregator_id) >>> + break; >>> + if (!bond_3ad_port_is_active(slave) && >>> + ad_info.ports != 1) >>> + break; >>> + >>> + call_slave_netpoll = true; >>> + break; >>> + default: >>> + if (bond_slave_is_up(slave)) >>> + call_slave_netpoll = true; >>> + break; >>> + } >>> + >>> + if (call_slave_netpoll) { >>> + ops = slave->dev->netdev_ops; >>> + if (ops->ndo_poll_controller) { >> >> >> It's weird to see this check so down the road. Maybe reorg the logic to >> something like: >> >> bond_for_each_slave() { >> if (!bond_slave_is_up() || !bond_slave_has_ndo_poll()) >> continue; >> >> if (BOND_MODE(bond) == 3AD && !bond_3ad_port_is_active()) >> continue; >> >> slave-do_ndo_poll_stuff(); >> } >> >> Just as a thought, might be easier to read/shorter. >> >I tried this and it does save one bool variable but not much >different. Here it is - > > bond_for_each_slave(bond, slave, iter) { > ops = slave->dev->netdev_ops; > if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller) > continue; > > if (BOND_MODE(bond) == BOND_MODE_8023AD) { > struct aggregator *agg = > SLAVE_AD_INFO(slave)->port.aggregator; > > if (agg && agg->aggregator_identifier != > ad_info.aggregator_id) > continue; > if (!bond_3ad_port_is_active(slave) || ad_info.ports != 1) > continue; > } > > ni = rcu_dereference_bh(slave->dev->npinfo); > if (down_trylock(&ni->dev_lock)) > continue; > ops->ndo_poll_controller(slave->dev); > up(&ni->dev_lock); > } > >I'm not sure if it's any different! BTW I did keep the aggregator >check since I'm not convinced that it's OK to eliminate the check. > > >> >>> + struct netpoll_info *ni = >>> + >>> rcu_dereference_bh(slave->dev->npinfo); >>> + >>> + if (down_trylock(&ni->dev_lock)) >>> + continue; >>> + ops->ndo_poll_controller(slave->dev); >>> + up(&ni->dev_lock); >>> + } >>> + } >>> + } >>> } >>> >>> static void bond_netpoll_cleanup(struct net_device *bond_dev) >>> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h >>> index f04cdbb7848e..6c455c646d61 100644 >>> --- a/include/net/bond_3ad.h >>> +++ b/include/net/bond_3ad.h >>> @@ -278,5 +278,6 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, >>> struct bonding *bond, >>> struct slave *slave); >>> int bond_3ad_set_carrier(struct bonding *bond); >>> void bond_3ad_update_lacp_rate(struct bonding *bond); >>> +int bond_3ad_port_is_active(struct slave *slave); >>> #endif /* _NET_BOND_3AD_H */ >>> >>> -- >>> 2.2.0.rc0.207.ga3a616c >>> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index 9b436696b95e..14f2ebe786c5 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -2477,6 +2477,30 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info) return ret; } +#define BOND_3AD_PORT_OPERATIONAL \ + (AD_STATE_DISTRIBUTING | AD_STATE_COLLECTING | \ + AD_STATE_SYNCHRONIZATION | AD_STATE_AGGREGATION) + +static int bond_3ad_port_operational(struct slave *slave) +{ + port_t *port = &SLAVE_AD_INFO(slave)->port; + + return bond_slave_can_tx(slave) && + (port->actor_oper_port_state & port->partner_oper.port_state & + BOND_3AD_PORT_OPERATIONAL) == BOND_3AD_PORT_OPERATIONAL; +} + +/* bond_3ad_port_is_active - check if a slave port is active or not. A port + * is active when it can forward traffic. + * + * @slave: slave port to check state for. + * Returns: 0 if not active else is active. + */ +int bond_3ad_port_is_active(struct slave *slave) +{ + return bond_3ad_port_operational(slave); +} + int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave) { diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index c9e519cb9214..a50ec87486f3 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -928,6 +928,53 @@ static inline void slave_disable_netpoll(struct slave *slave) static void bond_poll_controller(struct net_device *bond_dev) { + struct bonding *bond = netdev_priv(bond_dev); + struct slave *slave = NULL; + struct list_head *iter; + struct ad_info ad_info; + struct aggregator *agg; + const struct net_device_ops *ops; + bool call_slave_netpoll; + + if (BOND_MODE(bond) == BOND_MODE_8023AD) + if (bond_3ad_get_active_agg_info(bond, &ad_info)) + return; + + bond_for_each_slave(bond, slave, iter) { + call_slave_netpoll = false; + switch (BOND_MODE(bond)) { + case BOND_MODE_8023AD: + agg = SLAVE_AD_INFO(slave)->port.aggregator; + if (!bond_slave_is_up(slave)) + break; + if (agg && agg->aggregator_identifier != + ad_info.aggregator_id) + break; + if (!bond_3ad_port_is_active(slave) && + ad_info.ports != 1) + break; + + call_slave_netpoll = true; + break; + default: + if (bond_slave_is_up(slave)) + call_slave_netpoll = true; + break; + } + + if (call_slave_netpoll) { + ops = slave->dev->netdev_ops; + if (ops->ndo_poll_controller) { + struct netpoll_info *ni = + rcu_dereference_bh(slave->dev->npinfo); + + if (down_trylock(&ni->dev_lock)) + continue; + ops->ndo_poll_controller(slave->dev); + up(&ni->dev_lock); + } + } + } } static void bond_netpoll_cleanup(struct net_device *bond_dev) diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h index f04cdbb7848e..6c455c646d61 100644 --- a/include/net/bond_3ad.h +++ b/include/net/bond_3ad.h @@ -278,5 +278,6 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave); int bond_3ad_set_carrier(struct bonding *bond); void bond_3ad_update_lacp_rate(struct bonding *bond); +int bond_3ad_port_is_active(struct slave *slave); #endif /* _NET_BOND_3AD_H */
This patches implements the poll_controller support for all bonding driver. If the slaves have poll_controller net_op defined, this implementation calls them. This is mode agnostic implementation and iterates through all slaves (based on mode) and calls respective handler. Signed-off-by: Mahesh Bandewar <maheshb@google.com> --- drivers/net/bonding/bond_3ad.c | 24 +++++++++++++++++++++ drivers/net/bonding/bond_main.c | 47 +++++++++++++++++++++++++++++++++++++++++ include/net/bond_3ad.h | 1 + 3 files changed, 72 insertions(+)