op 21-10-13 10:48, Thomas Hellstrom schreef: > Hi! > > As discussed previously the current locking order in TTM of these locks is > bo::reserve -> vm::mmap_sem. This leads to a hack in > the TTM fault() handle to try and revert the locking order. If a tryreserve > failed, we tried to have the vm code release the mmap_sem() and then > schedule, to give the holder of bo::reserve a chance to release the lock. > This solution is no longer legal, since we've been more or less kindly asked > to remove the set_need_resched() call. > > Maarten has proposed to invert the locking order. I've previously said I had > no strong preference. The current locking order dates back from the time when > TTM wasn't using unmap_mapping_range() but walked the page tables itself, > updating PTEs as needed. Furthermore it was needed for user bos that used > get_user_pages() in the TTM populate and swap-in methods. User-bos were > removed some time ago but I'm looking at re-adding them. They would suite the > VMware model of cached-only pages very well. I see uses both in the gallium > API, XA's DMA functionality and openCL. > > We would then need a somewhat nicer way to invert the locking order. I've > attached a solution that ups the mmap_sem and then reserves, but due to how > the fault API is done, we then need to release the reserve and retry the > fault. This of course opens up for starvation, but I don't think starvation > at this point is very likely: One thread being refused to write or read from > a buffer object because the GPU is continously busy with it. If this *would* > become a problem, it's probably possible to modify the fault code to allow us > to hold locks until the retried fault, but that would be a bit invasive, > since it touches the arch code.... > > Basically I'm proposing to keep the current locking order.
I'm not sure why we have to worry about mmap_sem lock being taken before bo::reserve. If we already hold mmap_sem, no extra locking is needed for get_user_pages. Releasing it is a bit silly. I think we should keep mmap_sem as outer lock, and have bo::reserve as inner, even if it might complicate support for user-bo's. I'm not sure what you can do with user-bo's that can't be done by allocating the same bo from kernel first and map + populate it. ~Maarten
