On Wed, 11 Jul 2007, Tejun Heo wrote: > >> How high performance device addition/removal should be? Wouldn't rwlock > >> be enough? > > > > rwlock is a type of spinlock, right? Hence it can't sleep and can't be > > used for device addition/removal. Also rwlocks aren't fair and are > > subject to livelock. > > rwsem of course. Sorry about the confusion. Fairness doesn't matter > here. The synchronization is suspend vs. addition with priority on > suspend (suspend does down_write, removal does down_read_trylock).
Right. I think device addition/removal is infrequent enough that either SRCU or and rwsem could be used, without any significant slowdown. > I'm still not a big fan of locking (or downing) all device semaphores > when the same effect can be achieved with a rwsem. One thing I like > about rwsem approach is that it can never cause a deadlock because one > side uses trylock exclusively. But an rwsem _doesn't_ obtain the same effect. The major difference is that holding all the device semaphores blocks binding of drivers, which we need to prevent during suspend. A minor difference is that it won't prevent adding a new device, since you don't need to hold any existing semaphores before calling device_add(). > If you lock all the device sems, failing addition becomes more difficult > (can be done but then you need something like the rwsem anyway). That was the idea: First acquire the rwsem write lock so that all existing calls to device_add will have finished and new ones will fail, and then acquire all the semaphores. The purpose of holding all the semaphores is not to prevent device addition. More like the other way around: In order to be sure of holding _all_ the semaphores, we have to prevent new devices from being added. > Without special handling of dev->sem, when a driver screws up (try to > add a child device from ->suspend), it will deadlock while suspend is in > progress, which isn't pretty. No it won't. If it calls device_add from within its suspend method, the down_read_trylock will fail and the call will return immediately. With a WARN_ON to let people know which driver tried to do device_add at an illegal time. > Please also note that currently dev->sem does not protect against adding > children. It says it does in the comment on the definition of the field > but it's never acquired during device_add(). That's why we need the rwsem. > Yeah, I'm all for centralizing things. I just didn't see what could be > simplified in LLDs by freezing sysfs access. Can you point me to any > example node which can benefit from such thing? For USB devices, writes to the /sys/.../bConfigurationValue node will call usb_set_configuration(), which registers child devices. We don't want these calls to fail because a suspend is underway, since suspends are supposed to be transparent to userspace. So they need to block until the suspend is over. Access (either read or write) to any sysfs node which actually touches the device hardware will fail while the device is suspended. Again, for transparency such accesses should block. > > Using a freezable workqueue for the callbacks would also localize the > > suspend handling. > > But the freezable workqueue thing isn't really necessary if any of the > discussed solutions is implemented, right? It isn't necessary if the callbacks aren't needed (i.e., if we give methods a way to unregister their attribute directly). But if we do keep the callbacks, then for safety's sake their workqueue should be freezable. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
