On Wed, Feb 18, 2009 at 5:42 PM, Thomas Hellström <tho...@shipmail.org> wrote: > Kristian Høgsberg wrote: >> >> On Wed, Feb 18, 2009 at 12:36 PM, Eric Anholt <e...@anholt.net> wrote: >> >>> >>> On Wed, 2009-02-18 at 11:02 -0500, k...@bitplanet.net wrote: >>> >>>> >>>> From: Kristian Høgsberg <k...@redhat.com> >>>> >>>> A number of GEM operations (and legacy drm ones) want to copy data to >>>> or from userspace while holding the struct_mutex lock. However, the >>>> fault handler calls us with the mmap_sem held and thus enforces the >>>> opposite locking order. This patch downs the mmap_sem up front for >>>> those operations that access userspace data under the struct_mutex >>>> lock to ensure the locking order is consistent. >>>> >>>> Signed-off-by: Kristian Høgsberg <k...@redhat.com> >>>> >>> >>> Have you tested this against actually faulting? My understanding was >>> that you can't recurse on mmap_sem. >>> >> >> I tested it and it worked, but didn't add code to detect contention so >> I can't say for sure I hit that case. mmap_sem is a read/write >> semaphore, so while we can't recurse, we can get away with taking two >> reader locks. >> >> cheers, >> Kristian >> > > Kristian, > This seems a bit odd to me. The extra lock taking does not prevent any > deadlocks, so is this done to only silence a warning message? A reversed > locking order between a mutex and an rwsem can never deadlock if the rwsem > is only taken in read mode. If the mutex is taken simultaneously as the > rwsem in _write_ mode then that locking order must be preserved across the > code, even when the rwsem is taken in read mode.
If all we ever did was read, we wouldn't need the mmap_sem at all... The point is that somebody could be holding the semaphore in write mode in which case taking the mmap_sem in read mode blocks. So if we're called with the mmap_sem held in write mode and we going to take the struct_lock mutex, but gem_execbuffer preempts, takes the struct_mutex and then tries to call copy_from_user(), we deadlock. Taking the mmap_sem in read mode early as in my patch, prevents this. In this case, I guess the fault handler is always only called with mmap_sem held in read mode, and then yes, there's no deadlock and the warning is harmless. That doesn't mean that it's a good idea - enforcing a consistent lock order is good practice that keeps the code clean and makes it easy to verify locking without knowing every part of the system in detail. And should the fault callback mmap_sem assumptions change in the future, gem will certainly be more robust if we keep the locking simple. cheers, Kristian ------------------------------------------------------------------------------ Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA -OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise -Strategies to boost innovation and cut costs with open source participation -Receive a $600 discount off the registration fee with the source code: SFAD http://p.sf.net/sfu/XcvMzF8H -- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
