Re: [Bonding-devel] [PATCH] bonding: cancel_delayed_work() -> cancel_delayed_work_sync()

[Jarek Poplawski <jarkao2@gmail.com>]

On Thu, Dec 17, 2009 at 11:37:42AM -0800, Jay Vosburgh wrote:
> Laurent Chavey <chavey@google.com> wrote:
> 
> >one instance that could be a problem
> >
> >__exit bonding_exit(void)
> >    bond_free_all()
> >      	bond_work_cancel_all(bond);
> >        unregister_netdevice(bond_dev)
> >
> >could the above result in an invalid pointer when trying
> >to use bond-> in one of the timer CB ?
> 
> 	The bonding teardown logic was reworked in October, and there is
> no longer a bond_free_all in the current mainline.  What kernel are you
> looking at?
> 
> 	The bond_close function will stop the various work items, and
> the ndo_uninit (bond_uninit) will call bond_work_cancel_all as well.
> 
> 	Actually, on looking at it (it being current mainline),
> bond_uninit might need some kind of logic to wait and insure that all
> timers have completed before returning.  It comes from unregister, so
> the next thing that happens after it returns is that the memory will be
> freed (via netdev_run_todo, during rtnl_unlock, if I'm following it
> correctly).
> 
> 	The bond_uninit function is called under RTNL, though, so the
> timer functions (bond_mii_monitor, et al) may need additional checks for
> kill_timers to insure they don't attempt to acquire RTNL if a cancel is
> pending.
> 
> 	That's kind of tricky itself, since the lock ordering requires
> RTNL to be acquired first, so there's no way for bond_mii_monitor (et
> al) to check for kill_timers prior to already having RTNL (because the
> function acquires RTNL conditionally, only if needed; to do that, it
> unlocks the bond lock, then acquires RTNL, then re-locks the bond lock).
> 
> 	So, the lock dance to acquire RTNL in bond_mii_monitor (et al)
> would need some trickery, perhaps a rtnl_trylock loop, that checks
> kill_timers each time the trylock fails, e.g.,
> 
> 	if (bond_miimon_inspect(bond)) {
> 		read_unlock(&bond->lock);
> 		while (!rtnl_trylock) {
> 			read_lock(&bond->lock);
> 			if (bond->kill_timers)
> 				goto out;
> 			read_unlock(&bond->lock);
> 			/* msleep ? */
> 		}
> 
> 		bond_miimon_commit(bond);
> 		[...]
> 
> 	So, with the above (and similar changes to the other delayed
> work functions, and a big honkin' comment somewhere to explain this), I
> suspect that bond_work_cancel_all could use the _sync variant to cancel
> the work, as long as kill_timers is set before the cancel_sync is
> called.
> 
> 	Am I missing anything?  Does this seem rational?

It seems OK to me ...if there is nothing better ;-) But such endless
loops are tricky (they omit lockdep, plus can hide some hidden
dependancies between different tasks, even in the future). If it's
possible we could consider a limited loop with re-arming on failure;
then cancel_delayed_work_sync() (with its standard logic) could be
used everywhere, and kill_timers might be useless too (if there is no
re-arming between different works).

Jarek P.