Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux state changed
From: Hangbin Liu <hidden>
Date: 2024-06-27 08:26:19
Subsystem:
bonding driver, networking drivers, networking [general], the rest · Maintainers:
Jay Vosburgh, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Wed, Jun 26, 2024 at 05:06:00PM -0700, Jay Vosburgh wrote:
quoted
Hits: RTNL: assertion failed at net/core/rtnetlink.c (1823)
Thanks for this hits...
quoted
On two selftests. Please run the selftests on a debug kernel..
OK, I will try run my tests on debug kernel in future.
Oh, I forgot about needing RTNL. We cannot simply acquire RTNL in ad_mux_machine(), as the bond->mode_lock is already held, and the lock ordering must be RTNL first, then mode_lock, lest we deadlock. Hangbin, I'd suggest you look at how bond_netdev_notify_work() complies with the lock ordering (basically, doing the actual work out of line in a workqueue event), or how the "should_notify" flag is used in bond_3ad_state_machine_handler(). The first is more complicated, but won't skip events; the second may miss intermediate state transitions if it cannot acquire RTNL and has to delay the notification.
I think the administer doesn't want to loose the state change info. So how about something like:
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c6807e473ab7..046f11c5c47a 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c@@ -1185,6 +1185,8 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr) default: break; } + + port->slave->should_notify_lacp = 1; } }
@@ -2527,6 +2529,9 @@ void bond_3ad_state_machine_handler(struct work_struct *work) bond_slave_state_notify(bond); rtnl_unlock(); } + + /* Notify the mux state changes */ + bond_slave_link_notify(bond); queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks); }
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3c3fcce4acd4..db8f2fb613df 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c@@ -1748,11 +1748,19 @@ static void bond_netdev_notify_work(struct work_struct *_work) notify_work.work); if (rtnl_trylock()) { - struct netdev_bonding_info binfo; + if (slave->should_notify_link) { + struct netdev_bonding_info binfo; + bond_fill_ifslave(slave, &binfo.slave); + bond_fill_ifbond(slave->bond, &binfo.master); + netdev_bonding_info_change(slave->dev, &binfo); + slave->should_notify_link = 0; + } + + if (slave->should_notify_lacp) { + rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL, 0, NULL); + slave->should_notify_lacp = 0; + } - bond_fill_ifslave(slave, &binfo.slave); - bond_fill_ifbond(slave->bond, &binfo.master); - netdev_bonding_info_change(slave->dev, &binfo); rtnl_unlock(); } else { queue_delayed_work(slave->bond->wq, &slave->notify_work, 1);
diff --git a/include/net/bonding.h b/include/net/bonding.h
index b61fb1aa3a56..38d37ea2382c 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h@@ -170,7 +170,8 @@ struct slave { inactive:1, /* indicates inactive slave */ rx_disabled:1, /* indicates whether slave's Rx is disabled */ should_notify:1, /* indicates whether the state changed */ - should_notify_link:1; /* indicates whether the link changed */ + should_notify_link:1, /* indicates whether the link changed */ + should_notify_lacp:1; /* indicates whether the lacp state changed */ u8 duplex; u32 original_mtu; u32 link_failure_count;
@@ -641,11 +642,10 @@ static inline void bond_slave_link_notify(struct bonding *bond) struct slave *tmp; bond_for_each_slave(bond, tmp, iter) { - if (tmp->should_notify_link) { + if (tmp->should_notify_link || tmp->should_notify_lacp) bond_queue_slave_event(tmp); + if (tmp->should_notify_link) bond_lower_state_changed(tmp); - tmp->should_notify_link = 0; - } } }
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index eabfc8290f5e..4507bb8d5264 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c@@ -4116,6 +4116,7 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change, rtmsg_ifinfo_event(type, dev, change, rtnl_get_event(0), flags, NULL, 0, portid, nlh); } +EXPORT_SYMBOL(rtmsg_ifinfo); void rtmsg_ifinfo_newnet(int type, struct net_device *dev, unsigned int change, gfp_t flags, int *new_nsid, int new_ifindex)
--
2.45.0
Thanks
Hangbin