On Wed, Jul 03, 2019 at 10:28:57PM +0200, Christoph Hellwig wrote: > On Wed, Jul 03, 2019 at 07:00:50PM +0000, Jason Gunthorpe wrote: > > I don't think the API should be encouraging some shortcut here.. > > > > We can't do the above pattern because the old hmm_vma API didn't allow > > it, which is presumably a reason why it is obsolete. > > > > I'd rather see drivers move to a consistent pattern so we can then > > easily hoist the seqcount lock scheme into some common mmu notifier > > code, as discussed. > > So you don't like the version in amdgpu_ttm_tt_get_user_pages_done in > linux-next either? I looked at this for 5 mins, and I can't see the key elements of the collision retry lock: - Where is the retry loop? - Where is the lock around the final test to valid prior to using the output of range? For instance looking at amdgpu_gem_userptr_ioctl().. We can't be holding a lock when we do hmm_range_wait_until_valid() (inside amdgpu_ttm_tt_get_user_pages), otherwise it deadlocks, and there are not other locks that would encompass the final is_valid check. And amdgpu_gem_userptr_ioctl() looks like a syscall entry point, so having it fail just because the lock collided (ie is_valid == false) can't possibly be the right thing. I'm also unclear when the device data is updated in that sequence.. So.. I think this locking is wrong. Maybe AMD team can explain how it should work? Jason
