On Friday, October 3, 2008 1:40 pm Thomas Hellström wrote: > Jesse Barnes wrote: > > On Wednesday, September 17, 2008 5:48 am Thomas Hellström wrote: > >> Jesse Barnes wrote: > >>> Ok, here's an updated patch (note: only compile tested!) which I think > >>> addresses all of the comments I received. I switched over to using > >>> dev_mapping for unmapping and tried to cleanup the mm mmap & private > >>> fields a bit. > >>> > >>> How does it look? > >>> > >>> Thanks, > >>> Jesse > >> > >> Oops, sorry for the previous spam, I hit "send" too soon. > >> > >> This looks better, only the > >> unmap_mapping_range() <-> fault() > >> race remains. Should be easily fixable with a mutex, though. > > > > I think airlied is almost ready to integrate this, so I took a look at > > the locking requirements. I can't figure out what I'm protecting against > > though. It looks like in the vm_insert_pfn case we'll already have the > > mmap_sem protecting us from other vma updates, but maybe > > unmap_mapping_range doesn't honor that? Other callers of vm_insert_pfn > > don't seem to do anything special (though I could only find one), and > > neither do callers of > > unmap_mapping_range(). > > > > As long as the core isn't broken I think the race is harmless; we'll get > > an extra page fault if vm_insert_pfn goes first, and avoid one if > > unmap_mapping_range ends up first (again assuming the core does the right > > thing here). > > > > Can either of you shed some light on this? > > > > Thanks, > > Jesse > > Jesse, > The mmap_sem() is taken in read mode only, this means that there can be > any number of threads simultaneously in fault() accessing the same > buffer object, and I don't think unmap_mapping_range takes any > driver-visible locks at all, except there used to be a sequence > increment that made old nopage() back off if it raced with > unmap_mapping_range. > > So we have this sequence in fault: > 1a) bind_to_gtt > 1b) insert_pfn() > > And this sequence in mapping teardown: > 2a) unmap_mapping_range() > 2b) unbind_from_gtt(). > > So 1b) may well happen while unmap_mapping_range() is running, and even > between 2a) and 2b) leaving you with ptes pointing to bogus data. You > may even have the sequence 2a) 1a) 2b) 1b).
Ah right, I was only thinking of the page table updates and not the GTT update, of course. I'll add a lock for this unless GEM already has a good one. Thanks, -- Jesse Barnes, Intel Open Source Technology Center ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ -- _______________________________________________ Dri-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/dri-devel
