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.
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?
/Thomas
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
