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

[Jay Vosburgh <fubar@us.ibm.com>]

Jarek Poplawski <jarkao2@gmail.com> wrote:

>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).

	A less evil alternative would be to punt and reschedule the work
if the rtnl_trylock failed, e.g.,

	if (bond_miimon_inspect(bond)) {
		read_unlock(&bond->lock);
		if (!rtnl_trylock()) {
			queue_work(...);
			return;
		}

		read_lock(&bond->lock);

		bond_miimon_commit(bond);
		[...]

	I'm not sure what the usual contention level on rtnl is (and,
therefore, how often this will punt for the normal case that's not the
race we're trying to avoid here).

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com