Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux state changed

[Hangbin Liu <liuhangbin@gmail.com>]

On Thu, Jun 27, 2024 at 11:29:21AM +0300, Nikolay Aleksandrov wrote:
> On 27/06/2024 11:26, Hangbin Liu wrote:
> > 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.
> >>
> 
> +1 & facepalm, completely forgot it was running without 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:
> > 
> 
> You can (and will) miss events with the below code. It is kind of best effort,
> but if the notification is not run before the next state change, you will
> lose the intermediate changes.

Yes, but at least the admin could get the latest state. With the following
code the admin may not get the latest update if lock rtnl failed.

        if (should_notify_rtnl && rtnl_trylock()) {
                bond_slave_lacp_notify(bond);
                rtnl_unlock();
	}

Thanks
Hangbin