Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
On 8/12/22 14:34, Christian König wrote: > > > Am 10.08.22 um 20:53 schrieb Dmitry Osipenko: >> On 8/10/22 21:25, Christian König wrote: >>> Am 10.08.22 um 19:49 schrieb Dmitry Osipenko: On 8/10/22 14:30, Christian König wrote: > Am 25.07.22 um 17:18 schrieb Dmitry Osipenko: >> This patch moves the non-dynamic dma-buf users over to the dynamic >> locking specification. The strict locking convention prevents >> deadlock >> situation for dma-buf importers and exporters. >> >> Previously the "unlocked" versions of the dma-buf API functions >> weren't >> taking the reservation lock and this patch makes them to take the >> lock. >> >> Intel and AMD GPU drivers already were mapping imported dma-bufs >> under >> the held lock, hence the "locked" variant of the functions are added >> for them and the drivers are updated to use the "locked" versions. > In general "Yes, please", but that won't be that easy. > > You not only need to change amdgpu and i915, but all drivers > implementing the map_dma_buf(), unmap_dma_buf() callbacks. > > Auditing all that code is a huge bunch of work. Hm, neither of drivers take the resv lock in map_dma_buf/unmap_dma_buf. It's easy to audit them all and I did it. So either I'm missing something or it doesn't take much time to check them all. Am I really missing something? >>> Ok, so this is only changing map/unmap now? >> It also vmap/vunmap and attach/detach: In the previous patch I added the >> _unlocked postfix to the func names and in this patch I made them all to >> actually take the lock. > > > Take your patch "[PATCH v2 2/5] drm/gem: Take reservation lock for > vmap/vunmap operations" as a blueprint on how to approach it. > > E.g. one callback at a time and then document the result in the end. Yeah, I'll do it for v3. I'm vaguely recalling that there was a problem when I wanted to split this patch in the past, but don't remember what it was.. maybe that problem is gone now, will see :) -- Best regards, Dmitry
Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
Am 10.08.22 um 20:53 schrieb Dmitry Osipenko: On 8/10/22 21:25, Christian König wrote: Am 10.08.22 um 19:49 schrieb Dmitry Osipenko: On 8/10/22 14:30, Christian König wrote: Am 25.07.22 um 17:18 schrieb Dmitry Osipenko: This patch moves the non-dynamic dma-buf users over to the dynamic locking specification. The strict locking convention prevents deadlock situation for dma-buf importers and exporters. Previously the "unlocked" versions of the dma-buf API functions weren't taking the reservation lock and this patch makes them to take the lock. Intel and AMD GPU drivers already were mapping imported dma-bufs under the held lock, hence the "locked" variant of the functions are added for them and the drivers are updated to use the "locked" versions. In general "Yes, please", but that won't be that easy. You not only need to change amdgpu and i915, but all drivers implementing the map_dma_buf(), unmap_dma_buf() callbacks. Auditing all that code is a huge bunch of work. Hm, neither of drivers take the resv lock in map_dma_buf/unmap_dma_buf. It's easy to audit them all and I did it. So either I'm missing something or it doesn't take much time to check them all. Am I really missing something? Ok, so this is only changing map/unmap now? It also vmap/vunmap and attach/detach: In the previous patch I added the _unlocked postfix to the func names and in this patch I made them all to actually take the lock. Take your patch "[PATCH v2 2/5] drm/gem: Take reservation lock for vmap/vunmap operations" as a blueprint on how to approach it. E.g. one callback at a time and then document the result in the end. Regards, Christian. In this case please separate this from the documentation change. I'll factor out the doc in the v3. I would also drop the _locked postfix from the function name, just having _unlocked on all functions which are supposed to be called with the lock held should be sufficient. Noted for the v3. Thanks for looking into this, Christian. Thank you for the review.
Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
On 8/10/22 21:25, Christian König wrote: > Am 10.08.22 um 19:49 schrieb Dmitry Osipenko: >> On 8/10/22 14:30, Christian König wrote: >>> Am 25.07.22 um 17:18 schrieb Dmitry Osipenko: This patch moves the non-dynamic dma-buf users over to the dynamic locking specification. The strict locking convention prevents deadlock situation for dma-buf importers and exporters. Previously the "unlocked" versions of the dma-buf API functions weren't taking the reservation lock and this patch makes them to take the lock. Intel and AMD GPU drivers already were mapping imported dma-bufs under the held lock, hence the "locked" variant of the functions are added for them and the drivers are updated to use the "locked" versions. >>> In general "Yes, please", but that won't be that easy. >>> >>> You not only need to change amdgpu and i915, but all drivers >>> implementing the map_dma_buf(), unmap_dma_buf() callbacks. >>> >>> Auditing all that code is a huge bunch of work. >> Hm, neither of drivers take the resv lock in map_dma_buf/unmap_dma_buf. >> It's easy to audit them all and I did it. So either I'm missing >> something or it doesn't take much time to check them all. Am I really >> missing something? > > Ok, so this is only changing map/unmap now? It also vmap/vunmap and attach/detach: In the previous patch I added the _unlocked postfix to the func names and in this patch I made them all to actually take the lock. > In this case please separate this from the documentation change. I'll factor out the doc in the v3. > I would also drop the _locked postfix from the function name, just > having _unlocked on all functions which are supposed to be called with > the lock held should be sufficient. Noted for the v3. > Thanks for looking into this, > Christian. Thank you for the review. -- Best regards, Dmitry
Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
Am 10.08.22 um 19:49 schrieb Dmitry Osipenko: On 8/10/22 14:30, Christian König wrote: Am 25.07.22 um 17:18 schrieb Dmitry Osipenko: This patch moves the non-dynamic dma-buf users over to the dynamic locking specification. The strict locking convention prevents deadlock situation for dma-buf importers and exporters. Previously the "unlocked" versions of the dma-buf API functions weren't taking the reservation lock and this patch makes them to take the lock. Intel and AMD GPU drivers already were mapping imported dma-bufs under the held lock, hence the "locked" variant of the functions are added for them and the drivers are updated to use the "locked" versions. In general "Yes, please", but that won't be that easy. You not only need to change amdgpu and i915, but all drivers implementing the map_dma_buf(), unmap_dma_buf() callbacks. Auditing all that code is a huge bunch of work. Hm, neither of drivers take the resv lock in map_dma_buf/unmap_dma_buf. It's easy to audit them all and I did it. So either I'm missing something or it doesn't take much time to check them all. Am I really missing something? Ok, so this is only changing map/unmap now? In this case please separate this from the documentation change. I would also drop the _locked postfix from the function name, just having _unlocked on all functions which are supposed to be called with the lock held should be sufficient. Thanks for looking into this, Christian. https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2FA%2Fident%2Fmap_dma_bufdata=05%7C01%7Cchristian.koenig%40amd.com%7C70fd52d0a82a477bfbfe08da7af8bec7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637957506041914442%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=K47uCULsoiURjze0H0ksUa4vzJ%2BxqgoShH9106FvyyA%3Dreserved=0
Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
On 8/10/22 14:30, Christian König wrote: >> + * - dma_buf_move_notify() > > This one is called by the exporter, not the importer. Good catch, thank you! -- Best regards, Dmitry
Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
On 8/10/22 14:30, Christian König wrote: > Am 25.07.22 um 17:18 schrieb Dmitry Osipenko: >> This patch moves the non-dynamic dma-buf users over to the dynamic >> locking specification. The strict locking convention prevents deadlock >> situation for dma-buf importers and exporters. >> >> Previously the "unlocked" versions of the dma-buf API functions weren't >> taking the reservation lock and this patch makes them to take the lock. >> >> Intel and AMD GPU drivers already were mapping imported dma-bufs under >> the held lock, hence the "locked" variant of the functions are added >> for them and the drivers are updated to use the "locked" versions. > > In general "Yes, please", but that won't be that easy. > > You not only need to change amdgpu and i915, but all drivers > implementing the map_dma_buf(), unmap_dma_buf() callbacks. > > Auditing all that code is a huge bunch of work. Hm, neither of drivers take the resv lock in map_dma_buf/unmap_dma_buf. It's easy to audit them all and I did it. So either I'm missing something or it doesn't take much time to check them all. Am I really missing something? https://elixir.bootlin.com/linux/latest/A/ident/map_dma_buf -- Best regards, Dmitry
Re: [Linaro-mm-sig] [PATCH v2 3/5] dma-buf: Move all dma-bufs to dynamic locking specification
Am 25.07.22 um 17:18 schrieb Dmitry Osipenko: This patch moves the non-dynamic dma-buf users over to the dynamic locking specification. The strict locking convention prevents deadlock situation for dma-buf importers and exporters. Previously the "unlocked" versions of the dma-buf API functions weren't taking the reservation lock and this patch makes them to take the lock. Intel and AMD GPU drivers already were mapping imported dma-bufs under the held lock, hence the "locked" variant of the functions are added for them and the drivers are updated to use the "locked" versions. In general "Yes, please", but that won't be that easy. You not only need to change amdgpu and i915, but all drivers implementing the map_dma_buf(), unmap_dma_buf() callbacks. Auditing all that code is a huge bunch of work. Signed-off-by: Dmitry Osipenko --- Documentation/driver-api/dma-buf.rst | 6 + drivers/dma-buf/dma-buf.c | 186 - drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4 +- drivers/gpu/drm/drm_prime.c| 4 +- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 8 +- include/linux/dma-buf.h| 28 ++-- 6 files changed, 179 insertions(+), 57 deletions(-) diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index 36a76cbe9095..622b8156d212 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -119,6 +119,12 @@ DMA Buffer ioctls .. kernel-doc:: include/uapi/linux/dma-buf.h +DMA-BUF locking convention +~ + +.. kernel-doc:: drivers/dma-buf/dma-buf.c + :doc: locking convention + Kernel Functions and Structures Reference ~ diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d16237a6ffaa..bfdd551c7571 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) * 2. Userspace passes this file-descriptors to all drivers it wants this buffer *to share with: First the file descriptor is converted to a _buf using *dma_buf_get(). Then the buffer is attached to the device using - *dma_buf_attach(). + *dma_buf_attach_unlocked(). * *Up to this stage the exporter is still free to migrate or reallocate the *backing storage. @@ -569,8 +569,8 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) *dma_buf_map_attachment() and dma_buf_unmap_attachment(). * * 4. Once a driver is done with a shared buffer it needs to call - *dma_buf_detach() (after cleaning up any mappings) and then release the - *reference acquired with dma_buf_get() by calling dma_buf_put(). + *dma_buf_detach_unlocked() (after cleaning up any mappings) and then + *release the reference acquired with dma_buf_get() by calling dma_buf_put(). * * For the detailed semantics exporters are expected to implement see * _buf_ops. @@ -794,6 +794,63 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, return sg_table; } +/** + * DOC: locking convention + * + * In order to avoid deadlock situations between dma-buf exports and importers, + * all dma-buf API users must follow the common dma-buf locking convention. + * + * Convention for importers + * + * 1. Importers must hold the dma-buf reservation lock when calling these + *functions: + * + * - dma_buf_pin() + * - dma_buf_unpin() + * - dma_buf_move_notify() This one is called by the exporter, not the importer. Regards, Christian. + * - dma_buf_map_attachment_locked() + * - dma_buf_unmap_attachment_locked() + * + * 2. Importers must not hold the dma-buf reservation lock when calling these + *functions: + * + * - dma_buf_attach_unlocked() + * - dma_buf_dynamic_attach_unlocked() + * - dma_buf_detach_unlocked() + * - dma_buf_export( + * - dma_buf_fd() + * - dma_buf_get() + * - dma_buf_put() + * - dma_buf_begin_cpu_access() + * - dma_buf_end_cpu_access() + * - dma_buf_map_attachment_unlocked() + * - dma_buf_unmap_attachment_unlocked() + * - dma_buf_vmap_unlocked() + * - dma_buf_vunmap_unlocked() + * + * Convention for exporters + * + * 1. These _buf_ops callbacks are invoked with unlocked dma-buf + *reservation and exporter can take the lock: + * + * - _buf_ops.attach() + * - _buf_ops.detach() + * - _buf_ops.release() + * - _buf_ops.begin_cpu_access() + * - _buf_ops.end_cpu_access() + * + * 2. These _buf_ops callbacks are invoked with locked dma-buf + *reservation and exporter can't take the lock: + * + * - _buf_ops.pin() + * - _buf_ops.unpin() + * - _buf_ops.map_dma_buf() + * - _buf_ops.unmap_dma_buf() + * - _buf_ops.mmap() + * - _buf_ops.vmap() + * - _buf_ops.vunmap() + */
