On Fri, Feb 16, 2007 at 09:20:34PM +0100, Francois Romieu wrote: > Jarek Poplawski <[EMAIL PROTECTED]> : ... > > > @@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct > > > *work) > > > struct net_device *dev = tp->mii.dev; > > > unsigned long thr_delay = next_tick; > > > > > > + rtnl_lock(); > > > + > > > + if (!netif_running(dev)) > > > + goto out_unlock; > > > > I wonder, why you don't do netif_running before > > rtnl_lock ? It's an atomic operation. And I'm not sure if increasing > > rtnl_lock range is really needed here. > > thread A: netif_running() > user task B: rtnl_lock() > user task B: dev->close() > user task B: rtnl_unlock() > thread A: rtnl_lock() > thread A: mess with closed device > > Btw, the thread runs every 3*HZ at most.
You are right (mostly)! But I think rtnl_lock is special and should be spared (even this 3*HZ) and here it's used for some mainly internal purpose (close synchronization). And it looks like mainly for this internal reason holding of rtnl_lock is increased. And because rtnl_lock is quite popular you have to take into consideration that after this 3*HZ it could spend some time waiting for the lock. So, maybe it would be nicer to check this netif_running twice (after rtnl_lock where needed), but maybe it's a mater of taste only, and yours is better, as well. (Btw. I didn't verify this, but I hope you checked that places not under rtnl_lock before the patch are safe from some locking problems now.) Jarek P. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
