[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zn0iI3SPdRkmfnS1@Laptop-X1>
Date: Thu, 27 Jun 2024 16:26:11 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Jay Vosburgh <jay.vosburgh@...onical.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
Andy Gospodarek <andy@...yhouse.net>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
Ido Schimmel <idosch@...dia.com>, Jiri Pirko <jiri@...nulli.us>,
Amit Cohen <amcohen@...dia.com>,
Nikolay Aleksandrov <razor@...ckwall.org>
Subject: Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux
state changed
On Wed, Jun 26, 2024 at 05:06:00PM -0700, Jay Vosburgh wrote:
> >Hits:
> >
> >RTNL: assertion failed at net/core/rtnetlink.c (1823)
Thanks for this hits...
> >
> >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
Powered by blists - more mailing lists