Hi Thomas, > 1. You need the a lock with the order lock->mmap_sem, cause otherwise > you run into trouble when drm_vma_node_unmap() needs to be called. > Why is this? unmap_mapping_range() never takes the mmap_sem. It takes > a finer-level per-address-space lock. Well, that is interesting. Where do we actually then have the order of reservation->mmap_sem defined? > Do you mean to temporarily pin the bo while page-table-entries are set > up? That is basically what we already do by taking the reservation lock. > Otherwise whatever lock we introduce in the fault handler will also > need to block the bo from being moved.. Correct, let me explain it a bit differently: We need one lock which is held while the BO backing store is replaced to do faults and other stuff. And we need another lock which is held while we allocate backing store for the BO, do command submission and prepare moves. As far as I can see the first one should be a simple mutex while the second is the reservation object. Regards, Christian. Am 15.10.2018 um 10:20 schrieb Thomas Hellstrom: > Hi! > > Interesting disscussion. Some comments below. > > On 10/15/2018 09:41 AM, Koenig, Christian wrote: >>> Now amdgpu switched over >> Well exactly that's the point. amdgpu has *NOT* switched the order over. >> >>> like i915 did yearsearlier >> I actually consider this a problem in i915 which should be fixed sooner >> or later. >> >> The locking in the fault handler as exposed by TTM is actually pointing >> out a deeper issue which nobody seems to have correctly understood so >> far. >> >> Let me explain a bit: >> 1. You need the a lock with the order lock->mmap_sem, cause otherwise >> you run into trouble when drm_vma_node_unmap() needs to be called. > Why is this? unmap_mapping_range() never takes the mmap_sem. It takes > a finer-level per-address-space lock. > >> 2. But for CPU fault processing you also need a lock to determine the >> actual address where a BO is currently located > Do you mean to temporarily pin the bo while page-table-entries are set > up? > >> >> i915 has solved this by separating the locks and using the reservation >> lock as fault processing lock. >> >> Problem is that this clashes with filling reservation objects on demand >> when another driver wants to use it. > > Could you elaborate a bit on this, what locking scenarios are > conflicting etc. > >> >> So what we should probably do is to fix i915 to not use the reservation >> object as fault processing lock any more, then separate the TTM handling >> into two locks as well. > > This sounds a bit strange to me. What we want to do in the fault > handler is to temporarily pin down the object so that we can set up > page-table entries, which is exactly what we accomplish with > reservation objects. Otherwise whatever lock we introduce in the fault > handler will also need to block the bo from being moved.. > > Thanks, Thomas. > >> And last fix the the reservation object logic to allow pinning DMA-bufs >> on demand. >> >> Christian. >> >> Am 15.10.2018 um 08:55 schrieb Daniel Vetter: >>> On Sun, Oct 14, 2018 at 8:20 PM Koenig, Christian >>> <Christian.Koenig@xxxxxxx> wrote: >>>> Hi Thomas, >>>> >>>>> that the access() handler took a shortcut when the new locking order >>>>> was established >>>> There is no new locking order, the access handler is just for >>>> debugging >>>> and ignoring the correct locking order between mmap_sem and >>>> bo_reserve. >>>> >>>> That this is throwing a lockdep warning is perfectly possible. We >>>> should >>>> probably move that to a trylock instead. >>>> >>>>> bo_reserve() >>>>> copy_to_user() / copy_from_user() >>>>> bo_unreserve() >>>> That one is illegal for a completely different reason. >>>> >>>> The address accessed by copy_to_user()/copy_from_user() could be a BO >>>> itself, so to resolve this we could end up locking a BO twice. >>>> >>>> Adding a might_lock() to the beginning of ttm_bo_vm_fault as you >>>> suggested doesn't work either, because at this point the mmap_sem is >>>> still locked. >>>> >>>> So lockdep would complain about the incorrect bo_reserve and >>>> mmap_sem order. >>> I think Thomas' point is the one below: >>> >>>> Christian. >>>> >>>> Am 13.10.2018 um 21:04 schrieb Thomas Hellstrom: >>>>> Hi, Christian, >>>>> >>>>> On 10/13/2018 07:36 PM, Christian König wrote: >>>>>> Hi Thomas, >>>>>> >>>>>>> bo_reserve() >>>>>>> copy_to_user() / copy_from_user() >>>>>>> bo_unreserve() >>>>>> That pattern is illegal for a number of reasons and the mmap_sem is >>>>>> only one of it. >>>>>> >>>>>> So the locking order must always be mmap_sem->bo_reservation. See >>>>>> the >>>>>> userptr implementation in amdgpu as well. >>>>>> >>>>>> Christian. >>>>> I'm not arguing against that, and since vmwgfx doesn't use that >>>>> pattern, the locking order doesn't really matter to me since it's >>>>> even >>>>> possible to make the TTM fault() handler more well-behaved if we were >>>>> to fix the locking order to mmap_sem->bo_reserve. >>>>> >>>>> My concern is, since the _opposite_ locking order is (admittedly >>>>> vaguely) documented in the fault handler, that the access() handler >>>>> took a shortcut when the new locking order was established possibly >>>>> without auditing of the other TTM drivers for locking inversion: For >>>>> example it looks from a quick glance like >>>>> nouveau_gem_pushbuf_reloc_apply() calls copy_from_user() with bo's >>>>> reserved (which IIRC was the typical use-case at the time this was >>>>> last lifted). And lockdep won't trip unless the access() callback is >>>>> actually called. >>>>> >>>>> My point is if AMD wants to enforce this locking order, then IMHO the >>>>> other drivers need to be audited and corrected if they are assuming >>>>> the locking order documented in fault(). A good way to catch such >>>>> drivers would be to add that might_lock(). >>> ^^ This one here. There's a bunch of drivers which try-lock in the >>> fault handler, so that the _can_ do the >>> >>> bo_reserve() >>> copy*user() >>> bo_unreserve() >>> >>> pattern. Yes the trylock will just loop forever if you copy*user() >>> hits a bo itself that's already in the CS. Iirc I've complained about >>> this years back. Now amdgpu switched over (like i915 did years >>> earlier), because it's the only thing that reliably works even when >>> facing evil userspace, but there's still radeon&noveau. I think Thomas >>> argues that we should fix those, and I agree. >>> >>> Once those are fixed I also think that a might_lock in the fault >>> handler should not blow up anymore. If it does, you have an inversion >>> still somewhere. >>> >>> Aside: I think it'd be good to document this as part of struct >>> reservation_object, preferrably with lockdep annotations, to make sure >>> no one gets this wrong. Since we need _every_ driver to obey this, or >>> you just need the right buffer sharing combination to deadlock. >>> >>> Cheers, Daniel >>> >>>>> Thanks, >>>>> Thomas >>>>> >>>>> >>>>>> Am 12.10.2018 um 16:52 schrieb Thomas Hellstrom: >>>>>>> Hi, Felix, >>>>>>> >>>>>>> It looks like there is a locking inversion in ttm_bo_vm_access() >>>>>>> where we take a sleeping bo_reserve() while holding mmap_sem(). >>>>>>> >>>>>>> Previously we've been assuming the other way around or at least >>>>>>> undefined allowing for drivers to do >>>>>>> >>>>>>> bo_reserve() >>>>>>> copy_to_user() / copy_from_user() >>>>>>> bo_unreserve() >>>>>>> >>>>>>> I'm not sure the latter pattern is used in any drivers, though, and >>>>>>> I guess there are ways around it. So it might make sense to fix the >>>>>>> locking order at this point. In that case, perhaps one should add a >>>>>>> >>>>>>> might_lock(&bo->resv->lock.base); >>>>>>> >>>>>>> at the start of the TTM fault handler to trip lockdep on locking >>>>>>> order violations in situations where the access() callback isn't >>>>>>> commonly used... >>>>>>> >>>>>>> /Thomas >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> dri-devel mailing list >>>>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
