On Mon, Oct 15, 2018 at 5:24 PM Thomas Hellstrom <thomas@xxxxxxxxxxxx> wrote: > On 10/15/2018 04:47 PM, Daniel Vetter wrote: > > On Mon, Oct 15, 2018 at 2:30 PM Thomas Hellstrom <thomas@xxxxxxxxxxxx> wrote: > >> On 10/15/2018 12:55 PM, Koenig, Christian wrote: > >>> Am 15.10.2018 um 12:42 schrieb Thomas Hellstrom: > >>>> On 10/15/2018 10:43 AM, Koenig, Christian wrote: > >>>>> 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? > >>>> TTM doesn't. But some drivers using TTM do, which prompted the wording > >>>> in the fault handler comments. > >>>> But as Daniel mentioned, to be able to share buffers throughout DRM, > >>>> we at some point need to agree on a locking order, and when we do it's > >>>> important that we consider all relevant use-cases and workarounds. > >>>> > >>>> To me it certainly looks like the vm_ops implementations would be more > >>>> straightforward if we were to use mmap_sem->bo_reserve, but that would > >>>> again require a number of drivers to rework their copy_*_user code. > >>>> If those drivers, however, insisted on keeping their copy_*_user code, > >>>> they probably need to fix the recursive bo reservation up anyway, for > >>>> example by making sure not to copy from a vma that has VM_IO set. > >>> Yeah, completely agree. If you ask me using copy_(from|to)_user while a > >>> BO is reserved is currently completely forbidden. > >> So if we all agree that this should be completely forbidden, and this is > >> what requires the bo_reserve->mmap_sem locking order, then I guess the > >> logical follow up is trying to enforce mmap_sem->bo_reserve and pursuade > >> driver maintainers to convert. Perhaps as a configurable lockdep option? > > The problem with adding that under e.g. CONFIG_PROVE_LOCKING or > > similar is that we have drivers violating it right now. And last time > > around this entire discussion came up no one wanted to do the work to > > convert drivers, since it means you get to add a slowpath to the most > > complex ioctl they have. > > -Daniel > > Hmm. I actually had something like CONFIG_DRM_PROVE_LOCKING in mind; To > be enabled only for DRM developers. dma-buf is shared outside of DRM, imo that's where all the troubles lie. Especially once we have Christian's work merged to dynamically pin/unpin shared dma-bufs. We really need everyone to follow the locking rules around dma-buf/reservation_obj, or things won't really work. > But regardless, that slowpath is not hit frequently enough to affect > real-life performance, right? So it's more a matter of coding effort > rather than valid performance concerns? Validating it was the big concern for i915 at least. We typed tons of tests to make sure it keeps working, plus clever tricks to make sure we can actually test it - we heuristically prefault, which reduces the real-world hit rate for the slow-path to 0. Perfect for hiding a security bug :-/ -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
