- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH] dma-buf: free dmabuf->name in dma_buf_release()

by Andrew Morton

On Thu, 27 Feb 2020 13:38:03 -0800 Cong Wang <xiyou.wangcong(a)gmail.com> wrote: > On Tue, Feb 25, 2020 at 5:54 PM Andrew Morton <akpm(a)linux-foundation.org> wrote: > > > > On Tue, 25 Feb 2020 12:44:46 -0800 Cong Wang <xiyou.wangcong(a)gmail.com> wrote: > > > > > dma-buff name can be set via DMA_BUF_SET_NAME ioctl, but once set > > > it never gets freed. > > > > > > Free it in dma_buf_release(). > > > > > > ... > > > > > > --- a/drivers/dma-buf/dma-buf.c > > > +++ b/drivers/dma-buf/dma-buf.c > > > @@ -108,6 +108,7 @@ static int dma_buf_release(struct inode *inode, struct file *file) > > > dma_resv_fini(dmabuf->resv); > > > > > > module_put(dmabuf->owner); > > > + kfree(dmabuf->name); > > > kfree(dmabuf); > > > return 0; > > > } > > > > ow. Is that ioctl privileged? > > It looks unprivileged to me, as I don't see capable() called along > the path. > OK, thanks. I added cc:stable to my copy.

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2

by Christian König

Am 23.02.20 um 17:54 schrieb Thomas Hellström (VMware): > On 2/23/20 4:45 PM, Christian König wrote: >> Am 21.02.20 um 18:12 schrieb Daniel Vetter: >>> [SNIP] >>> Yeah the Great Plan (tm) is to fully rely on ww_mutex slowly >>> degenerating >>> into essentially a global lock. But only when there's actual contention >>> and thrashing. >> >> Yes exactly. A really big problem in TTM is currently that we drop >> the lock after evicting BOs because they tend to move in again >> directly after that. >> >> From practice I can also confirm that there is exactly zero benefit >> from dropping locks early and reacquire them for example for the VM >> page tables. That's just makes it more likely that somebody needs to >> roll back and this is what we need to avoid in the first place. > > If you have a benchmarking setup available it would be very > interesting for future reference to see how changing from WD to WW > mutexes affects the roll back frequency. WW is known to cause > rollbacks much less frequently but there is more work associated with > each rollback. Not of hand. To be honest I still have a hard time to get a grip on the difference between WD and WW from the algorithm point of view. So I can't judge that difference at all. >> Contention on BO locks during command submission is perfectly fine as >> long as this is as lightweight as possible while we don't have >> trashing. When we have trashing multi submission performance is best >> archived to just favor a single process to finish its business and >> block everybody else. > > Hmm. Sounds like we need a per-manager ww_rwsem protecting manager > allocation, taken in write-mode then there's thrashing. In read-mode > otherwise. That would limit the amount of "unnecessary" locks we'd > have to keep and reduce unwanted side-effects, (see below): Well per-manager (you mean per domain here don't you?) doesn't sound like that useful because we rarely use only one domain, but I'm actually questioning for quite a while if the per BO lock scheme was the right approach. See from the performance aspect the closest to ideal solution I can think of would be a ww_rwsem per user of a resource. In other words we don't lock BOs, but instead a list of all their users and when you want to evict a BO you need to walk that list and inform all users that the BO will be moving. During command submission you then have the fast path which rather just grabs the read side of the user lock and check if all BOs are still in the expected place. If some BOs were evicted you back off and start the slow path, e.g. maybe even copy additional data from userspace then grab the write side of the lock etc.. etc... That approach is similar to what we use in amdgpu with the per-VM BOs, but goes a step further. Problem is that we are so used to per BO locks in the kernel that this is probably not doable any more. >> Because of this I would actually vote for forbidding to release >> individual ww_mutex() locks in a context. > > Yes, I see the problem. > > But my first reaction is that this might have undersirable > side-effects. Let's say somebody wanted to swap the evicted BOs out? Please explain further, I off hand don't see the problem here. In general I actually wanted to re-work TTM in a way that BOs in the SYSTEM/SWAPABLE domain are always backed by a shmem file instead of the struct page array we currently have. > Or cpu-writes to them causing faults, that might also block the > mm_sem, which in turn blocks hugepaged? Mhm, I also only have a higher level view how hugepaged works so why does it grabs the mm_sem on the write side? Thanks, Christian. > > Still it's a fairly simple solution to a problem that seems otherwise > hard to solve efficiently. > > Thanks, > Thomas > > >> >> Regards, >> Christian. >> >>> -Daniel > >

5 years, 3 months

2
4
0 0

Re: [Linaro-mm-sig] [virtio-dev] Re: [PATCH 1/2] virtio: add dma-buf support for exported objects

by Gerd Hoffmann

On Wed, Feb 26, 2020 at 12:56:58PM +0900, David Stevens wrote: > On Tue, Feb 25, 2020 at 3:10 PM Gerd Hoffmann <kraxel(a)redhat.com> wrote: > > > > How about dma_buf_{get,set}_uuid, simliar to dma_buf_set_name? > > While I'm not opposed to such an API, I'm also hesitant to make > changes to the dma-buf API for a single use case. See virtio-wayland discussion. I expect we will see more cases show up. Maybe this should even go one level up, to struct file. cheers, Gerd

5 years, 3 months

1
0
0 0

[PATCH v5 3/3] drm/udl: simplify gem object mapping.

by Gerd Hoffmann

With shmem helpers allowing to update pgprot caching flags via drm_gem_shmem_object.map_cached we can just use that and ditch our own implementations of mmap() and vmap(). We also don't need a special case for imported objects, any map requests are handled by the exporter not udl. Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com> --- drivers/gpu/drm/udl/udl_gem.c | 62 ++--------------------------------- 1 file changed, 3 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index b6e26f98aa0a..7e3a88b25b6b 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -17,72 +17,15 @@ * GEM object funcs */ -static int udl_gem_object_mmap(struct drm_gem_object *obj, - struct vm_area_struct *vma) -{ - int ret; - - ret = drm_gem_shmem_mmap(obj, vma); - if (ret) - return ret; - - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); - if (obj->import_attach) - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); - - return 0; -} - -static void *udl_gem_object_vmap(struct drm_gem_object *obj) -{ - struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); - int ret; - - ret = mutex_lock_interruptible(&shmem->vmap_lock); - if (ret) - return ERR_PTR(ret); - - if (shmem->vmap_use_count++ > 0) - goto out; - - ret = drm_gem_shmem_get_pages(shmem); - if (ret) - goto err_zero_use; - - if (obj->import_attach) - shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf); - else - shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, - VM_MAP, PAGE_KERNEL); - - if (!shmem->vaddr) { - DRM_DEBUG_KMS("Failed to vmap pages\n"); - ret = -ENOMEM; - goto err_put_pages; - } - -out: - mutex_unlock(&shmem->vmap_lock); - return shmem->vaddr; - -err_put_pages: - drm_gem_shmem_put_pages(shmem); -err_zero_use: - shmem->vmap_use_count = 0; - mutex_unlock(&shmem->vmap_lock); - return ERR_PTR(ret); -} - static const struct drm_gem_object_funcs udl_gem_object_funcs = { .free = drm_gem_shmem_free_object, .print_info = drm_gem_shmem_print_info, .pin = drm_gem_shmem_pin, .unpin = drm_gem_shmem_unpin, .get_sg_table = drm_gem_shmem_get_sg_table, - .vmap = udl_gem_object_vmap, + .vmap = drm_gem_shmem_vmap, .vunmap = drm_gem_shmem_vunmap, - .mmap = udl_gem_object_mmap, + .mmap = drm_gem_shmem_mmap, }; /* @@ -101,6 +44,7 @@ struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, obj = &shmem->base; obj->funcs = &udl_gem_object_funcs; + shmem->map_cached = true; return obj; } -- 2.18.2

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: free dmabuf->name in dma_buf_release()

by Andrew Morton

On Tue, 25 Feb 2020 12:44:46 -0800 Cong Wang <xiyou.wangcong(a)gmail.com> wrote: > dma-buff name can be set via DMA_BUF_SET_NAME ioctl, but once set > it never gets freed. > > Free it in dma_buf_release(). > > ... > > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -108,6 +108,7 @@ static int dma_buf_release(struct inode *inode, struct file *file) > dma_resv_fini(dmabuf->resv); > > module_put(dmabuf->owner); > + kfree(dmabuf->name); > kfree(dmabuf); > return 0; > } ow. Is that ioctl privileged?

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Fix missing excl fence waiting

by Christian König

Am 23.02.20 um 12:56 schrieb Pan, Xinhui: > If shared fence list is not empty, even we want to test all fences, excl fence is ignored. > That is abviously wrong, so fix it. Yeah that is a known issue and I completely agree with you, but other disagree. See the shared fences are meant to depend on the exclusive fence. So all shared fences must finish only after the exclusive one has finished as well. The problem now is that for error handling this isn't necessary true. In other words when a shared fence completes with an error it is perfectly possible that he does this before the exclusive fence is finished. I'm trying to convince Daniel that this is a problem for years :) Regards, Christian. > > Signed-off-by: xinhui pan <xinhui.pan(a)amd.com> > --- > drivers/dma-buf/dma-resv.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 4264e64788c4..44dc64c547c6 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -632,14 +632,14 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) > */ > bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all) > { > - unsigned seq, shared_count; > + unsigned int seq, shared_count, left; > int ret; > > rcu_read_lock(); > retry: > ret = true; > shared_count = 0; > - seq = read_seqcount_begin(&obj->seq); > + left = seq = read_seqcount_begin(&obj->seq); > > if (test_all) { > unsigned i; > @@ -647,7 +647,7 @@ bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all) > struct dma_resv_list *fobj = rcu_dereference(obj->fence); > > if (fobj) > - shared_count = fobj->shared_count; > + left = shared_count = fobj->shared_count; > > for (i = 0; i < shared_count; ++i) { > struct dma_fence *fence = rcu_dereference(fobj->shared[i]); > @@ -657,13 +657,14 @@ bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all) > goto retry; > else if (!ret) > break; > + left--; > } > > if (read_seqcount_retry(&obj->seq, seq)) > goto retry; > } > > - if (!shared_count) { > + if (!left) { > struct dma_fence *fence_excl = rcu_dereference(obj->fence_excl); > > if (fence_excl) {

5 years, 3 months

2
2
0 0

Re: [Linaro-mm-sig] [PATCH 2/2] drm/virtio: Support virtgpu exported resources

by Gerd Hoffmann

Hi, > +struct dma_buf *virtgpu_gem_prime_export(struct drm_gem_object *obj, > + int flags) > +{ [ ... ] > +} > + > +struct drm_gem_object *virtgpu_gem_prime_import(struct drm_device *dev, > + struct dma_buf *buf) > +{ [ ... ] > +} More code duplication. > diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h > index 0c85914d9369..9c428ef03060 100644 > --- a/include/uapi/linux/virtio_gpu.h > +++ b/include/uapi/linux/virtio_gpu.h API change should go to a separate patch. > +/* > + * VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID > + */ > +#define VIRTIO_GPU_F_CROSS_DEVICE 2 Hmm, how about VIRTIO_GPU_F_RESOURCE_UUID ? > @@ -87,6 +92,7 @@ enum virtio_gpu_ctrl_type { > VIRTIO_GPU_RESP_OK_CAPSET_INFO, > VIRTIO_GPU_RESP_OK_CAPSET, > VIRTIO_GPU_RESP_OK_EDID, > + VIRTIO_GPU_RESP_OK_RESOURCE_ASSIGN_UUID, The "assign" doesn't make sense in the reply. I'd name that VIRTIO_GPU_RESP_OK_RESOURCE_UUID or just VIRTIO_GPU_RESP_OK_UUID, > +/* VIRTIO_GPU_RESP_OK_RESOURCE_ASSIGN_UUID */ > +struct virtio_gpu_resp_resource_assign_uuid { > + struct virtio_gpu_ctrl_hdr hdr; > + __u8 uuid[16]; > +}; Same here. cheers, Gerd

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 1/2] virtio: add dma-buf support for exported objects

by Gerd Hoffmann

On Wed, Feb 19, 2020 at 05:06:36PM +0900, David Stevens wrote: > This change adds a new flavor of dma-bufs that can be used by virtio > drivers to share exported objects. A virtio dma-buf can be queried by > virtio drivers to obtain the UUID which identifies the underlying > exported object. That duplicates a bunch of code from dma-buf.c in virtio_dma_buf.c. How about dma_buf_{get,set}_uuid, simliar to dma_buf_set_name? cheers, Gerd

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2

by Daniel Vetter

On Thu, Feb 20, 2020 at 11:51:07PM +0100, Thomas Hellström (VMware) wrote: > On 2/20/20 9:08 PM, Daniel Vetter wrote: > > On Thu, Feb 20, 2020 at 08:46:27PM +0100, Thomas Hellström (VMware) wrote: > > > On 2/20/20 7:04 PM, Daniel Vetter wrote: > > > > On Thu, Feb 20, 2020 at 10:39:06AM +0100, Thomas Hellström (VMware) wrote: > > > > > On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote: > > > > > > On 2/18/20 10:01 PM, Daniel Vetter wrote: > > > > > > > On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) > > > > > > > <thomas_os(a)shipmail.org> wrote: > > > > > > > > On 2/17/20 6:55 PM, Daniel Vetter wrote: > > > > > > > > > On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: > > > > > > > > > > Implement the importer side of unpinned DMA-buf handling. > > > > > > > > > > > > > > > > > > > > v2: update page tables immediately > > > > > > > > > > > > > > > > > > > > Signed-off-by: Christian König <christian.koenig(a)amd.com> > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 > > > > > > > > > > ++++++++++++++++++++- > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++ > > > > > > > > > > 2 files changed, 71 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > > > index 770baba621b3..48de7624d49c 100644 > > > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > > > @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct > > > > > > > > > > drm_device *dev, struct dma_buf *dma_buf) > > > > > > > > > > return ERR_PTR(ret); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation > > > > > > > > > > + * > > > > > > > > > > + * @attach: the DMA-buf attachment > > > > > > > > > > + * > > > > > > > > > > + * Invalidate the DMA-buf attachment, making sure that > > > > > > > > > > the we re-create the > > > > > > > > > > + * mapping before the next use. > > > > > > > > > > + */ > > > > > > > > > > +static void > > > > > > > > > > +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) > > > > > > > > > > +{ > > > > > > > > > > + struct drm_gem_object *obj = attach->importer_priv; > > > > > > > > > > + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); > > > > > > > > > > + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > > > > > > > > > > + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > > > > > > > > > > + struct ttm_operation_ctx ctx = { false, false }; > > > > > > > > > > + struct ttm_placement placement = {}; > > > > > > > > > > + struct amdgpu_vm_bo_base *bo_base; > > > > > > > > > > + int r; > > > > > > > > > > + > > > > > > > > > > + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) > > > > > > > > > > + return; > > > > > > > > > > + > > > > > > > > > > + r = ttm_bo_validate(&bo->tbo, &placement, &ctx); > > > > > > > > > > + if (r) { > > > > > > > > > > + DRM_ERROR("Failed to invalidate DMA-buf > > > > > > > > > > import (%d))\n", r); > > > > > > > > > > + return; > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { > > > > > > > > > > + struct amdgpu_vm *vm = bo_base->vm; > > > > > > > > > > + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; > > > > > > > > > > + > > > > > > > > > > + if (ticket) { > > > > > > > > > Yeah so this is kinda why I've been a total pain about the > > > > > > > > > exact semantics > > > > > > > > > of the move_notify hook. I think we should flat-out require > > > > > > > > > that importers > > > > > > > > > _always_ have a ticket attach when they call this, and that > > > > > > > > > they can cope > > > > > > > > > with additional locks being taken (i.e. full EDEADLCK) handling. > > > > > > > > > > > > > > > > > > Simplest way to force that contract is to add a dummy 2nd > > > > > > > > > ww_mutex lock to > > > > > > > > > the dma_resv object, which we then can take #ifdef > > > > > > > > > CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket). > > > > > > > > > > > > > > > > > > Now the real disaster is how we handle deadlocks. Two issues: > > > > > > > > > > > > > > > > > > - Ideally we'd keep any lock we've taken locked until the > > > > > > > > > end, it helps > > > > > > > > > needless backoffs. I've played around a bit with that > > > > > > > > > but not even poc > > > > > > > > > level, just an idea: > > > > > > > > > > > > > > > > > > https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d2… > > > > > > > > > > > > > > > > > > > > > > > > > > > Idea is essentially to track a list of objects we had to > > > > > > > > > lock as part of > > > > > > > > > the ttm_bo_validate of the main object. > > > > > > > > > > > > > > > > > > - Second one is if we get a EDEADLCK on one of these > > > > > > > > > sublocks (like the > > > > > > > > > one here). We need to pass that up the entire callchain, > > > > > > > > > including a > > > > > > > > > temporary reference (we have to drop locks to do the > > > > > > > > > ww_mutex_lock_slow > > > > > > > > > call), and need a custom callback to drop that temporary reference > > > > > > > > > (since that's all driver specific, might even be > > > > > > > > > internal ww_mutex and > > > > > > > > > not anything remotely looking like a normal dma_buf). > > > > > > > > > This probably > > > > > > > > > needs the exec util helpers from ttm, but at the > > > > > > > > > dma_resv level, so that > > > > > > > > > we can do something like this: > > > > > > > > > > > > > > > > > > struct dma_resv_ticket { > > > > > > > > > struct ww_acquire_ctx base; > > > > > > > > > > > > > > > > > > /* can be set by anyone (including other drivers) > > > > > > > > > that got hold of > > > > > > > > > * this ticket and had to acquire some new lock. This > > > > > > > > > lock might > > > > > > > > > * protect anything, including driver-internal stuff, and isn't > > > > > > > > > * required to be a dma_buf or even just a dma_resv. */ > > > > > > > > > struct ww_mutex *contended_lock; > > > > > > > > > > > > > > > > > > /* callback which the driver (which might be a dma-buf exporter > > > > > > > > > * and not matching the driver that started this > > > > > > > > > locking ticket) > > > > > > > > > * sets together with @contended_lock, for the main > > > > > > > > > driver to drop > > > > > > > > > * when it calls dma_resv_unlock on the contended_lock. */ > > > > > > > > > void (drop_ref*)(struct ww_mutex *contended_lock); > > > > > > > > > }; > > > > > > > > > > > > > > > > > > This is all supremely nasty (also ttm_bo_validate would need to be > > > > > > > > > improved to handle these sublocks and random new objects > > > > > > > > > that could force > > > > > > > > > a ww_mutex_lock_slow). > > > > > > > > > > > > > > > > > Just a short comment on this: > > > > > > > > > > > > > > > > Neither the currently used wait-die or the wound-wait algorithm > > > > > > > > *strictly* requires a slow lock on the contended lock. For > > > > > > > > wait-die it's > > > > > > > > just very convenient since it makes us sleep instead of spinning with > > > > > > > > -EDEADLK on the contended lock. For wound-wait IIRC one could just > > > > > > > > immediately restart the whole locking transaction after an > > > > > > > > -EDEADLK, and > > > > > > > > the transaction would automatically end up waiting on the contended > > > > > > > > lock, provided the mutex lock stealing is not allowed. There is however > > > > > > > > a possibility that the transaction will be wounded again on another > > > > > > > > lock, taken before the contended lock, but I think there are ways to > > > > > > > > improve the wound-wait algorithm to reduce that probability. > > > > > > > > > > > > > > > > So in short, choosing the wound-wait algorithm instead of wait-die and > > > > > > > > perhaps modifying the ww mutex code somewhat would probably help > > > > > > > > passing > > > > > > > > an -EDEADLK up the call chain without requiring passing the contended > > > > > > > > lock, as long as each locker releases its own locks when receiving an > > > > > > > > -EDEADLK. > > > > > > > Hm this is kinda tempting, since rolling out the full backoff tricker > > > > > > > across driver boundaries is going to be real painful. > > > > > > > > > > > > > > What I'm kinda worried about is the debug/validation checks we're > > > > > > > losing with this. The required backoff has this nice property that > > > > > > > ww_mutex debug code can check that we've fully unwound everything when > > > > > > > we should, that we've blocked on the right lock, and that we're > > > > > > > restarting everything without keeling over. Without that I think we > > > > > > > could end up with situations where a driver in the middle feels like > > > > > > > handling the EDEADLCK, which might go well most of the times (the > > > > > > > deadlock will probably be mostly within a given driver, not across). > > > > > > > Right up to the point where someone creates a deadlock across drivers, > > > > > > > and the lack of full rollback will be felt. > > > > > > > > > > > > > > So not sure whether we can still keep all these debug/validation > > > > > > > checks, or whether this is a step too far towards clever tricks. > > > > > > I think we could definitely find a way to keep debugging to make sure > > > > > > everything is unwound before attempting to restart the locking > > > > > > transaction. But the debug check that we're restarting on the contended > > > > > > lock only really makes sense for wait-die, (and we could easily keep it > > > > > > for wait-die). The lock returning -EDEADLK for wound-wait may actually > > > > > > not be the contending lock but an arbitrary lock that the wounded > > > > > > transaction attempts to take after it is wounded. > > > > > > > > > > > > So in the end IMO this is a tradeoff between added (possibly severe) > > > > > > locking complexity into dma-buf and not being able to switch back to > > > > > > wait-die efficiently if we need / want to do that. > > > > > > > > > > > > /Thomas > > > > > And as a consequence an interface *could* be: > > > > > > > > > > *) We introduce functions > > > > > > > > > > void ww_acquire_relax(struct ww_acquire_ctx *ctx); > > > > > int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx); > > > > > > > > > > that can be used instead of ww_mutex_lock_slow() in the absence of a > > > > > contending lock to avoid spinning on -EDEADLK. While trying to take the > > > > > contending lock is probably the best choice there are various second best > > > > > approaches that can be explored, for example waiting on the contending > > > > > acquire to finish or in the wound-wait case, perhaps do nothing. These > > > > > functions will also help us keep the debugging. > > > > Hm ... I guess this could work. Trouble is, it only gets rid of the > > > > slowpath locking book-keeping headaches, we still have quite a few others. > > > > > > > > > *) A function returning -EDEADLK to a caller *must* have already released > > > > > its own locks. > > > > So this ties to another question, as in should these callbacks have to > > > > drops the locks thei acquire (much simpler code) or not (less thrashing, > > > > if we drop locks we might end up in a situation where threads thrash > > > > around instead of realizing quicker that they're actually deadlocking and > > > > one of them should stop and back off). > > > Hmm.. Could you describe such a thrashing case with an example? > > Ignoring cross device fun and all that, just a simplified example of why > > holding onto locks you've acquired for eviction is useful, at least in a > > slow path. > > > > - one thread trying to do an execbuf with a huge bo > > > > vs. > > > > - an entire pile of thread that try to do execbuf with just a few small bo > > > > First thread is in the eviction loop, selects a bo, wins against all the > > other thread since it's been doing this forever already, gets the bo moved > > out, unlocks. > > > > Since it's competing against lots of other threads with small bo, it'll > > have to do that a lot of times. Often enough to create a contiguous hole. > > If you have a smarter allocator that tries to create that hole more > > actively, just assume that the single huge bo is a substantial part of > > total vram. > > > > The other threads will be quicker in cramming new stuff in, even if they > > occasionally lose the ww dance against the single thread. So the big > > thread livelocks. > > > > If otoh the big thread would keep onto all the locks, eventually it have > > the entire vram locked, and every other thread is guaranteed to lose > > against it in the ww dance and queue up behind. And it could finally but > > its huge bo into vram and execute. > > Hmm, yes this indeed explains why it's beneficial in some cases to keep a > number of locks held across certain operations, but I still fail to see why > we would like *all* locks held across the entire transaction? In the above > case I'd envision us ending up with something like: > > int validate(ctx, bo) > { > > for_each_suitable_bo_to_evict(ebo) { > r = lock(ctx, ebo); > if (r == EDEADLK) > goto out_unlock; > > r = move_notify(ctx, ebo);// locks and unlocks GPU VM bo. Yeah I think for move_notify the "keep the locks" thing is probably not what we want. That's more for when you have to evict stuff and similar things like that (which hopefully no driver needs to do in their ->move_notify). But for placing buffers we kinda want to keep things, and that's also a cross-driver thing (eventually at least I think). > if (r == EDEADLK) > goto out_unlock; > evict(); > } > > place_bo(bo); > //Repeat until success. > > > out_unlock: > for_each_locked_bo(ebo) > unlock(ctx, ebo); So that this unlock loop would need to be moved up to higher levels perhaps. This here would solve the example of a single big bo, but if you have multiple then you still end up with a lot of thrashing until the younger thread realizes that it needs to back off. > } > > > void command_submission() > { > acquire_init(ctx); > > restart: > for_each_bo_in_cs(bo) { > r = lock(ctx, bo); > if (r == -EDEADLK) > goto out_unreserve; > } > > for_each_bo_in_cs(bo) { > r = validate(ctx, bo); > if (r == -EDEADLK) > goto out_unreserve; > }; > > cs(); > > for_each_bo_in_cs(bo) > unlock(ctx, bo); > > acquire_fini(ctx); > return 0; > > out_unreserve: > for_each_locked_bo() > unlock(ctx, bo); > > acquire_relax(); > goto restart; > } > > > > Vary example for multi-gpu and more realism, but that's roughly it. > > > > Aside, a lot of the stuff Christian has been doing in ttm is to improve > > the chances that the competing threads will hit one of the locked objects > > of the big thread, and at least back off a bit. That's at least my > > understanding of what's been happening. > > -Daniel > > OK unserstood. For vmwgfx the crude simplistic idea to avoid that situation > has been to have an rwsem around command submission: When the thread with > the big bo has run a full LRU worth of eviction without succeeding it would > get annoyed and take the rwsem in write mode, blocking competing threads. > But that would of course never work in a dma-buf setting, and IIRC the > implementation is not complete either.... Yeah the Great Plan (tm) is to fully rely on ww_mutex slowly degenerating into essentially a global lock. But only when there's actual contention and thrashing. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 3 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Fix missing excl fence waiting

by Christian König

Am 23.02.20 um 13:21 schrieb Pan, Xinhui: > >> 2020年2月23日 20:04，Koenig, Christian <Christian.Koenig(a)amd.com> 写道： >> >> Am 23.02.20 um 12:56 schrieb Pan, Xinhui: >>> If shared fence list is not empty, even we want to test all fences, excl fence is ignored. >>> That is abviously wrong, so fix it. >> Yeah that is a known issue and I completely agree with you, but other disagree. >> >> See the shared fences are meant to depend on the exclusive fence. So all shared fences must finish only after the exclusive one has finished as well. > fair enough. > >> The problem now is that for error handling this isn't necessary true. In other words when a shared fence completes with an error it is perfectly possible that he does this before the exclusive fence is finished. >> >> I'm trying to convince Daniel that this is a problem for years :) >> > I have met problems, eviction has race with bo relase. system memory is overwried by sDMA. the kernel is 4.19, stable one, LOL. Ok sounds like we add some shared fence which doesn't depend on the exclusive one to finish. That is of course highly problematic for the current handling. It might be that this happens with the TTM move fence, but I don't of hand know either how to prevent that. Question at Daniel and others: Can we finally drop this assumption that all shared fences must finish after the exclusive one? Thanks for pointing this out Xinhui, Christian. > > amdgpu add excl fence to bo to move system memory which is done by the drm scheduler. > after sDMA finish the moving job, the memory might have already been released as dma_resv_test_signaled_rcu did not check excl fence. > > Our local customer report this issue. I took 4 days into it. sigh > > thanks > xinhui > >> Regards, >> Christian. >> >>> Signed-off-by: xinhui pan <xinhui.pan(a)amd.com> >>> --- >>> drivers/dma-buf/dma-resv.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >>> index 4264e64788c4..44dc64c547c6 100644 >>> --- a/drivers/dma-buf/dma-resv.c >>> +++ b/drivers/dma-buf/dma-resv.c >>> @@ -632,14 +632,14 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) >>> */ >>> bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all) >>> { >>> - unsigned seq, shared_count; >>> + unsigned int seq, shared_count, left; >>> int ret; >>> rcu_read_lock(); >>> retry: >>> ret = true; >>> shared_count = 0; >>> - seq = read_seqcount_begin(&obj->seq); >>> + left = seq = read_seqcount_begin(&obj->seq); >>> if (test_all) { >>> unsigned i; >>> @@ -647,7 +647,7 @@ bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all) >>> struct dma_resv_list *fobj = rcu_dereference(obj->fence); >>> if (fobj) >>> - shared_count = fobj->shared_count; >>> + left = shared_count = fobj->shared_count; >>> for (i = 0; i < shared_count; ++i) { >>> struct dma_fence *fence = rcu_dereference(fobj->shared[i]); >>> @@ -657,13 +657,14 @@ bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all) >>> goto retry; >>> else if (!ret) >>> break; >>> + left--; >>> } >>> if (read_seqcount_retry(&obj->seq, seq)) >>> goto retry; >>> } >>> - if (!shared_count) { >>> + if (!left) { >>> struct dma_fence *fence_excl = rcu_dereference(obj->fence_excl); >>> if (fence_excl) {

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2

by Daniel Vetter

On Thu, Feb 20, 2020 at 08:46:27PM +0100, Thomas Hellström (VMware) wrote: > On 2/20/20 7:04 PM, Daniel Vetter wrote: > > On Thu, Feb 20, 2020 at 10:39:06AM +0100, Thomas Hellström (VMware) wrote: > > > On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote: > > > > On 2/18/20 10:01 PM, Daniel Vetter wrote: > > > > > On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) > > > > > <thomas_os(a)shipmail.org> wrote: > > > > > > On 2/17/20 6:55 PM, Daniel Vetter wrote: > > > > > > > On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: > > > > > > > > Implement the importer side of unpinned DMA-buf handling. > > > > > > > > > > > > > > > > v2: update page tables immediately > > > > > > > > > > > > > > > > Signed-off-by: Christian König <christian.koenig(a)amd.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 > > > > > > > > ++++++++++++++++++++- > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++ > > > > > > > > 2 files changed, 71 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > index 770baba621b3..48de7624d49c 100644 > > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > > > @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct > > > > > > > > drm_device *dev, struct dma_buf *dma_buf) > > > > > > > > return ERR_PTR(ret); > > > > > > > > } > > > > > > > > > > > > > > > > +/** > > > > > > > > + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation > > > > > > > > + * > > > > > > > > + * @attach: the DMA-buf attachment > > > > > > > > + * > > > > > > > > + * Invalidate the DMA-buf attachment, making sure that > > > > > > > > the we re-create the > > > > > > > > + * mapping before the next use. > > > > > > > > + */ > > > > > > > > +static void > > > > > > > > +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) > > > > > > > > +{ > > > > > > > > + struct drm_gem_object *obj = attach->importer_priv; > > > > > > > > + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); > > > > > > > > + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > > > > > > > > + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > > > > > > > > + struct ttm_operation_ctx ctx = { false, false }; > > > > > > > > + struct ttm_placement placement = {}; > > > > > > > > + struct amdgpu_vm_bo_base *bo_base; > > > > > > > > + int r; > > > > > > > > + > > > > > > > > + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) > > > > > > > > + return; > > > > > > > > + > > > > > > > > + r = ttm_bo_validate(&bo->tbo, &placement, &ctx); > > > > > > > > + if (r) { > > > > > > > > + DRM_ERROR("Failed to invalidate DMA-buf > > > > > > > > import (%d))\n", r); > > > > > > > > + return; > > > > > > > > + } > > > > > > > > + > > > > > > > > + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { > > > > > > > > + struct amdgpu_vm *vm = bo_base->vm; > > > > > > > > + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; > > > > > > > > + > > > > > > > > + if (ticket) { > > > > > > > Yeah so this is kinda why I've been a total pain about the > > > > > > > exact semantics > > > > > > > of the move_notify hook. I think we should flat-out require > > > > > > > that importers > > > > > > > _always_ have a ticket attach when they call this, and that > > > > > > > they can cope > > > > > > > with additional locks being taken (i.e. full EDEADLCK) handling. > > > > > > > > > > > > > > Simplest way to force that contract is to add a dummy 2nd > > > > > > > ww_mutex lock to > > > > > > > the dma_resv object, which we then can take #ifdef > > > > > > > CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket). > > > > > > > > > > > > > > Now the real disaster is how we handle deadlocks. Two issues: > > > > > > > > > > > > > > - Ideally we'd keep any lock we've taken locked until the > > > > > > > end, it helps > > > > > > > needless backoffs. I've played around a bit with that > > > > > > > but not even poc > > > > > > > level, just an idea: > > > > > > > > > > > > > > https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d2… > > > > > > > > > > > > > > > > > > > > > Idea is essentially to track a list of objects we had to > > > > > > > lock as part of > > > > > > > the ttm_bo_validate of the main object. > > > > > > > > > > > > > > - Second one is if we get a EDEADLCK on one of these > > > > > > > sublocks (like the > > > > > > > one here). We need to pass that up the entire callchain, > > > > > > > including a > > > > > > > temporary reference (we have to drop locks to do the > > > > > > > ww_mutex_lock_slow > > > > > > > call), and need a custom callback to drop that temporary reference > > > > > > > (since that's all driver specific, might even be > > > > > > > internal ww_mutex and > > > > > > > not anything remotely looking like a normal dma_buf). > > > > > > > This probably > > > > > > > needs the exec util helpers from ttm, but at the > > > > > > > dma_resv level, so that > > > > > > > we can do something like this: > > > > > > > > > > > > > > struct dma_resv_ticket { > > > > > > > struct ww_acquire_ctx base; > > > > > > > > > > > > > > /* can be set by anyone (including other drivers) > > > > > > > that got hold of > > > > > > > * this ticket and had to acquire some new lock. This > > > > > > > lock might > > > > > > > * protect anything, including driver-internal stuff, and isn't > > > > > > > * required to be a dma_buf or even just a dma_resv. */ > > > > > > > struct ww_mutex *contended_lock; > > > > > > > > > > > > > > /* callback which the driver (which might be a dma-buf exporter > > > > > > > * and not matching the driver that started this > > > > > > > locking ticket) > > > > > > > * sets together with @contended_lock, for the main > > > > > > > driver to drop > > > > > > > * when it calls dma_resv_unlock on the contended_lock. */ > > > > > > > void (drop_ref*)(struct ww_mutex *contended_lock); > > > > > > > }; > > > > > > > > > > > > > > This is all supremely nasty (also ttm_bo_validate would need to be > > > > > > > improved to handle these sublocks and random new objects > > > > > > > that could force > > > > > > > a ww_mutex_lock_slow). > > > > > > > > > > > > > Just a short comment on this: > > > > > > > > > > > > Neither the currently used wait-die or the wound-wait algorithm > > > > > > *strictly* requires a slow lock on the contended lock. For > > > > > > wait-die it's > > > > > > just very convenient since it makes us sleep instead of spinning with > > > > > > -EDEADLK on the contended lock. For wound-wait IIRC one could just > > > > > > immediately restart the whole locking transaction after an > > > > > > -EDEADLK, and > > > > > > the transaction would automatically end up waiting on the contended > > > > > > lock, provided the mutex lock stealing is not allowed. There is however > > > > > > a possibility that the transaction will be wounded again on another > > > > > > lock, taken before the contended lock, but I think there are ways to > > > > > > improve the wound-wait algorithm to reduce that probability. > > > > > > > > > > > > So in short, choosing the wound-wait algorithm instead of wait-die and > > > > > > perhaps modifying the ww mutex code somewhat would probably help > > > > > > passing > > > > > > an -EDEADLK up the call chain without requiring passing the contended > > > > > > lock, as long as each locker releases its own locks when receiving an > > > > > > -EDEADLK. > > > > > Hm this is kinda tempting, since rolling out the full backoff tricker > > > > > across driver boundaries is going to be real painful. > > > > > > > > > > What I'm kinda worried about is the debug/validation checks we're > > > > > losing with this. The required backoff has this nice property that > > > > > ww_mutex debug code can check that we've fully unwound everything when > > > > > we should, that we've blocked on the right lock, and that we're > > > > > restarting everything without keeling over. Without that I think we > > > > > could end up with situations where a driver in the middle feels like > > > > > handling the EDEADLCK, which might go well most of the times (the > > > > > deadlock will probably be mostly within a given driver, not across). > > > > > Right up to the point where someone creates a deadlock across drivers, > > > > > and the lack of full rollback will be felt. > > > > > > > > > > So not sure whether we can still keep all these debug/validation > > > > > checks, or whether this is a step too far towards clever tricks. > > > > I think we could definitely find a way to keep debugging to make sure > > > > everything is unwound before attempting to restart the locking > > > > transaction. But the debug check that we're restarting on the contended > > > > lock only really makes sense for wait-die, (and we could easily keep it > > > > for wait-die). The lock returning -EDEADLK for wound-wait may actually > > > > not be the contending lock but an arbitrary lock that the wounded > > > > transaction attempts to take after it is wounded. > > > > > > > > So in the end IMO this is a tradeoff between added (possibly severe) > > > > locking complexity into dma-buf and not being able to switch back to > > > > wait-die efficiently if we need / want to do that. > > > > > > > > /Thomas > > > And as a consequence an interface *could* be: > > > > > > *) We introduce functions > > > > > > void ww_acquire_relax(struct ww_acquire_ctx *ctx); > > > int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx); > > > > > > that can be used instead of ww_mutex_lock_slow() in the absence of a > > > contending lock to avoid spinning on -EDEADLK. While trying to take the > > > contending lock is probably the best choice there are various second best > > > approaches that can be explored, for example waiting on the contending > > > acquire to finish or in the wound-wait case, perhaps do nothing. These > > > functions will also help us keep the debugging. > > Hm ... I guess this could work. Trouble is, it only gets rid of the > > slowpath locking book-keeping headaches, we still have quite a few others. > > > > > *) A function returning -EDEADLK to a caller *must* have already released > > > its own locks. > > So this ties to another question, as in should these callbacks have to > > drops the locks thei acquire (much simpler code) or not (less thrashing, > > if we drop locks we might end up in a situation where threads thrash > > around instead of realizing quicker that they're actually deadlocking and > > one of them should stop and back off). > > Hmm.. Could you describe such a thrashing case with an example? Ignoring cross device fun and all that, just a simplified example of why holding onto locks you've acquired for eviction is useful, at least in a slow path. - one thread trying to do an execbuf with a huge bo vs. - an entire pile of thread that try to do execbuf with just a few small bo First thread is in the eviction loop, selects a bo, wins against all the other thread since it's been doing this forever already, gets the bo moved out, unlocks. Since it's competing against lots of other threads with small bo, it'll have to do that a lot of times. Often enough to create a contiguous hole. If you have a smarter allocator that tries to create that hole more actively, just assume that the single huge bo is a substantial part of total vram. The other threads will be quicker in cramming new stuff in, even if they occasionally lose the ww dance against the single thread. So the big thread livelocks. If otoh the big thread would keep onto all the locks, eventually it have the entire vram locked, and every other thread is guaranteed to lose against it in the ww dance and queue up behind. And it could finally but its huge bo into vram and execute. Vary example for multi-gpu and more realism, but that's roughly it. Aside, a lot of the stuff Christian has been doing in ttm is to improve the chances that the competing threads will hit one of the locked objects of the big thread, and at least back off a bit. That's at least my understanding of what's been happening. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2

by Daniel Vetter

On Thu, Feb 20, 2020 at 10:39:06AM +0100, Thomas Hellström (VMware) wrote: > On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote: > > On 2/18/20 10:01 PM, Daniel Vetter wrote: > > > On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) > > > <thomas_os(a)shipmail.org> wrote: > > > > On 2/17/20 6:55 PM, Daniel Vetter wrote: > > > > > On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: > > > > > > Implement the importer side of unpinned DMA-buf handling. > > > > > > > > > > > > v2: update page tables immediately > > > > > > > > > > > > Signed-off-by: Christian König <christian.koenig(a)amd.com> > > > > > > --- > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 > > > > > > ++++++++++++++++++++- > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++ > > > > > > 2 files changed, 71 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > index 770baba621b3..48de7624d49c 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > > > > > > @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct > > > > > > drm_device *dev, struct dma_buf *dma_buf) > > > > > > return ERR_PTR(ret); > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation > > > > > > + * > > > > > > + * @attach: the DMA-buf attachment > > > > > > + * > > > > > > + * Invalidate the DMA-buf attachment, making sure that > > > > > > the we re-create the > > > > > > + * mapping before the next use. > > > > > > + */ > > > > > > +static void > > > > > > +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) > > > > > > +{ > > > > > > + struct drm_gem_object *obj = attach->importer_priv; > > > > > > + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); > > > > > > + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > > > > > > + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > > > > > > + struct ttm_operation_ctx ctx = { false, false }; > > > > > > + struct ttm_placement placement = {}; > > > > > > + struct amdgpu_vm_bo_base *bo_base; > > > > > > + int r; > > > > > > + > > > > > > + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) > > > > > > + return; > > > > > > + > > > > > > + r = ttm_bo_validate(&bo->tbo, &placement, &ctx); > > > > > > + if (r) { > > > > > > + DRM_ERROR("Failed to invalidate DMA-buf > > > > > > import (%d))\n", r); > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { > > > > > > + struct amdgpu_vm *vm = bo_base->vm; > > > > > > + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; > > > > > > + > > > > > > + if (ticket) { > > > > > Yeah so this is kinda why I've been a total pain about the > > > > > exact semantics > > > > > of the move_notify hook. I think we should flat-out require > > > > > that importers > > > > > _always_ have a ticket attach when they call this, and that > > > > > they can cope > > > > > with additional locks being taken (i.e. full EDEADLCK) handling. > > > > > > > > > > Simplest way to force that contract is to add a dummy 2nd > > > > > ww_mutex lock to > > > > > the dma_resv object, which we then can take #ifdef > > > > > CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket). > > > > > > > > > > Now the real disaster is how we handle deadlocks. Two issues: > > > > > > > > > > - Ideally we'd keep any lock we've taken locked until the > > > > > end, it helps > > > > > needless backoffs. I've played around a bit with that > > > > > but not even poc > > > > > level, just an idea: > > > > > > > > > > https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d2… > > > > > > > > > > > > > > > Idea is essentially to track a list of objects we had to > > > > > lock as part of > > > > > the ttm_bo_validate of the main object. > > > > > > > > > > - Second one is if we get a EDEADLCK on one of these > > > > > sublocks (like the > > > > > one here). We need to pass that up the entire callchain, > > > > > including a > > > > > temporary reference (we have to drop locks to do the > > > > > ww_mutex_lock_slow > > > > > call), and need a custom callback to drop that temporary reference > > > > > (since that's all driver specific, might even be > > > > > internal ww_mutex and > > > > > not anything remotely looking like a normal dma_buf). > > > > > This probably > > > > > needs the exec util helpers from ttm, but at the > > > > > dma_resv level, so that > > > > > we can do something like this: > > > > > > > > > > struct dma_resv_ticket { > > > > > struct ww_acquire_ctx base; > > > > > > > > > > /* can be set by anyone (including other drivers) > > > > > that got hold of > > > > > * this ticket and had to acquire some new lock. This > > > > > lock might > > > > > * protect anything, including driver-internal stuff, and isn't > > > > > * required to be a dma_buf or even just a dma_resv. */ > > > > > struct ww_mutex *contended_lock; > > > > > > > > > > /* callback which the driver (which might be a dma-buf exporter > > > > > * and not matching the driver that started this > > > > > locking ticket) > > > > > * sets together with @contended_lock, for the main > > > > > driver to drop > > > > > * when it calls dma_resv_unlock on the contended_lock. */ > > > > > void (drop_ref*)(struct ww_mutex *contended_lock); > > > > > }; > > > > > > > > > > This is all supremely nasty (also ttm_bo_validate would need to be > > > > > improved to handle these sublocks and random new objects > > > > > that could force > > > > > a ww_mutex_lock_slow). > > > > > > > > > Just a short comment on this: > > > > > > > > Neither the currently used wait-die or the wound-wait algorithm > > > > *strictly* requires a slow lock on the contended lock. For > > > > wait-die it's > > > > just very convenient since it makes us sleep instead of spinning with > > > > -EDEADLK on the contended lock. For wound-wait IIRC one could just > > > > immediately restart the whole locking transaction after an > > > > -EDEADLK, and > > > > the transaction would automatically end up waiting on the contended > > > > lock, provided the mutex lock stealing is not allowed. There is however > > > > a possibility that the transaction will be wounded again on another > > > > lock, taken before the contended lock, but I think there are ways to > > > > improve the wound-wait algorithm to reduce that probability. > > > > > > > > So in short, choosing the wound-wait algorithm instead of wait-die and > > > > perhaps modifying the ww mutex code somewhat would probably help > > > > passing > > > > an -EDEADLK up the call chain without requiring passing the contended > > > > lock, as long as each locker releases its own locks when receiving an > > > > -EDEADLK. > > > Hm this is kinda tempting, since rolling out the full backoff tricker > > > across driver boundaries is going to be real painful. > > > > > > What I'm kinda worried about is the debug/validation checks we're > > > losing with this. The required backoff has this nice property that > > > ww_mutex debug code can check that we've fully unwound everything when > > > we should, that we've blocked on the right lock, and that we're > > > restarting everything without keeling over. Without that I think we > > > could end up with situations where a driver in the middle feels like > > > handling the EDEADLCK, which might go well most of the times (the > > > deadlock will probably be mostly within a given driver, not across). > > > Right up to the point where someone creates a deadlock across drivers, > > > and the lack of full rollback will be felt. > > > > > > So not sure whether we can still keep all these debug/validation > > > checks, or whether this is a step too far towards clever tricks. > > > > I think we could definitely find a way to keep debugging to make sure > > everything is unwound before attempting to restart the locking > > transaction. But the debug check that we're restarting on the contended > > lock only really makes sense for wait-die, (and we could easily keep it > > for wait-die). The lock returning -EDEADLK for wound-wait may actually > > not be the contending lock but an arbitrary lock that the wounded > > transaction attempts to take after it is wounded. > > > > So in the end IMO this is a tradeoff between added (possibly severe) > > locking complexity into dma-buf and not being able to switch back to > > wait-die efficiently if we need / want to do that. > > > > /Thomas > > And as a consequence an interface *could* be: > > *) We introduce functions > > void ww_acquire_relax(struct ww_acquire_ctx *ctx); > int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx); > > that can be used instead of ww_mutex_lock_slow() in the absence of a > contending lock to avoid spinning on -EDEADLK. While trying to take the > contending lock is probably the best choice there are various second best > approaches that can be explored, for example waiting on the contending > acquire to finish or in the wound-wait case, perhaps do nothing. These > functions will also help us keep the debugging. Hm ... I guess this could work. Trouble is, it only gets rid of the slowpath locking book-keeping headaches, we still have quite a few others. > *) A function returning -EDEADLK to a caller *must* have already released > its own locks. So this ties to another question, as in should these callbacks have to drops the locks thei acquire (much simpler code) or not (less thrashing, if we drop locks we might end up in a situation where threads thrash around instead of realizing quicker that they're actually deadlocking and one of them should stop and back off). But keeping locks locked means massive amounts of book-keeping in dma_resv layer, so goes all downhill from there. > *) move_notify() explicitly takes a struct ww_acquire_ctx * to make sure > there is no ambiguity. (I think it would be valuable if we could do the same > for ttm_bo_validate()). Yeah I think more explicit locking ctx would be really good no matter what. Implicitly fishing the acquire_ctx out of the lock for the object you're called on is kinda nasty. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2

by Daniel Vetter

On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) <thomas_os(a)shipmail.org> wrote: > > On 2/17/20 6:55 PM, Daniel Vetter wrote: > > On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: > >> Implement the importer side of unpinned DMA-buf handling. > >> > >> v2: update page tables immediately > >> > >> Signed-off-by: Christian König <christian.koenig(a)amd.com> > >> --- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 ++++++++++++++++++++- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++ > >> 2 files changed, 71 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> index 770baba621b3..48de7624d49c 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf) > >> return ERR_PTR(ret); > >> } > >> > >> +/** > >> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation > >> + * > >> + * @attach: the DMA-buf attachment > >> + * > >> + * Invalidate the DMA-buf attachment, making sure that the we re-create the > >> + * mapping before the next use. > >> + */ > >> +static void > >> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) > >> +{ > >> + struct drm_gem_object *obj = attach->importer_priv; > >> + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); > >> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > >> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > >> + struct ttm_operation_ctx ctx = { false, false }; > >> + struct ttm_placement placement = {}; > >> + struct amdgpu_vm_bo_base *bo_base; > >> + int r; > >> + > >> + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) > >> + return; > >> + > >> + r = ttm_bo_validate(&bo->tbo, &placement, &ctx); > >> + if (r) { > >> + DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r); > >> + return; > >> + } > >> + > >> + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { > >> + struct amdgpu_vm *vm = bo_base->vm; > >> + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; > >> + > >> + if (ticket) { > > Yeah so this is kinda why I've been a total pain about the exact semantics > > of the move_notify hook. I think we should flat-out require that importers > > _always_ have a ticket attach when they call this, and that they can cope > > with additional locks being taken (i.e. full EDEADLCK) handling. > > > > Simplest way to force that contract is to add a dummy 2nd ww_mutex lock to > > the dma_resv object, which we then can take #ifdef > > CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket). > > > > Now the real disaster is how we handle deadlocks. Two issues: > > > > - Ideally we'd keep any lock we've taken locked until the end, it helps > > needless backoffs. I've played around a bit with that but not even poc > > level, just an idea: > > > > https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d2… > > > > Idea is essentially to track a list of objects we had to lock as part of > > the ttm_bo_validate of the main object. > > > > - Second one is if we get a EDEADLCK on one of these sublocks (like the > > one here). We need to pass that up the entire callchain, including a > > temporary reference (we have to drop locks to do the ww_mutex_lock_slow > > call), and need a custom callback to drop that temporary reference > > (since that's all driver specific, might even be internal ww_mutex and > > not anything remotely looking like a normal dma_buf). This probably > > needs the exec util helpers from ttm, but at the dma_resv level, so that > > we can do something like this: > > > > struct dma_resv_ticket { > > struct ww_acquire_ctx base; > > > > /* can be set by anyone (including other drivers) that got hold of > > * this ticket and had to acquire some new lock. This lock might > > * protect anything, including driver-internal stuff, and isn't > > * required to be a dma_buf or even just a dma_resv. */ > > struct ww_mutex *contended_lock; > > > > /* callback which the driver (which might be a dma-buf exporter > > * and not matching the driver that started this locking ticket) > > * sets together with @contended_lock, for the main driver to drop > > * when it calls dma_resv_unlock on the contended_lock. */ > > void (drop_ref*)(struct ww_mutex *contended_lock); > > }; > > > > This is all supremely nasty (also ttm_bo_validate would need to be > > improved to handle these sublocks and random new objects that could force > > a ww_mutex_lock_slow). > > > Just a short comment on this: > > Neither the currently used wait-die or the wound-wait algorithm > *strictly* requires a slow lock on the contended lock. For wait-die it's > just very convenient since it makes us sleep instead of spinning with > -EDEADLK on the contended lock. For wound-wait IIRC one could just > immediately restart the whole locking transaction after an -EDEADLK, and > the transaction would automatically end up waiting on the contended > lock, provided the mutex lock stealing is not allowed. There is however > a possibility that the transaction will be wounded again on another > lock, taken before the contended lock, but I think there are ways to > improve the wound-wait algorithm to reduce that probability. > > So in short, choosing the wound-wait algorithm instead of wait-die and > perhaps modifying the ww mutex code somewhat would probably help passing > an -EDEADLK up the call chain without requiring passing the contended > lock, as long as each locker releases its own locks when receiving an > -EDEADLK. Hm this is kinda tempting, since rolling out the full backoff tricker across driver boundaries is going to be real painful. What I'm kinda worried about is the debug/validation checks we're losing with this. The required backoff has this nice property that ww_mutex debug code can check that we've fully unwound everything when we should, that we've blocked on the right lock, and that we're restarting everything without keeling over. Without that I think we could end up with situations where a driver in the middle feels like handling the EDEADLCK, which might go well most of the times (the deadlock will probably be mostly within a given driver, not across). Right up to the point where someone creates a deadlock across drivers, and the lack of full rollback will be felt. So not sure whether we can still keep all these debug/validation checks, or whether this is a step too far towards clever tricks. But definitely a neat idea ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years, 3 months

1
0
0 0

RFC: Unpinned DMA-buf handling

by Christian König

The basic idea stayed the same since the last version of those patches. The exporter can provide explicit pin/unpin functions and the importer a move_notify callback. This allows us to avoid pinning buffers while importers have a mapping for them. In difference to the last version the locking changes were separated from this patchset and committed to drm-misc-next. This allows drivers to implement the new locking semantics without the extra unpinned handling, but of course the changed locking semantics is still a prerequisite to the unpinned handling. The last time this set was send out the discussion ended by questioning if the move_notify callback was really the right approach of notifying the importers that a buffer is about to change its placement. A possible alternative would be to add a special crafted fence object instead. Let's discuss on the different approaches once more, Christian.

5 years, 3 months

3
14
0 0

RFC: Unpinned DMA-buf handling

by Christian König

Hi everyone, hopefully the last iteration of those patches. For now I've addressed the issue of unmapping imported BOs from the amdgpu page tables immediately by locking the page tables in place. For HMM handling we are getting the ability to invalidate BOs without locking the VM anyway, so this last TODO will probably go away rather soon. Place comment, Christian.

5 years, 3 months

2
9
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Fix a typo in Kconfig

by Daniel Vetter

On Sun, Feb 16, 2020 at 12:47:08PM +0100, Christophe JAILLET wrote: > A 'h' ismissing in' syncronization' > > Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> Applied, thanks for your patch. -Daniel > --- > drivers/dma-buf/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > index 0613bb7770f5..e7d820ce0724 100644 > --- a/drivers/dma-buf/Kconfig > +++ b/drivers/dma-buf/Kconfig > @@ -6,7 +6,7 @@ config SYNC_FILE > default n > select DMA_SHARED_BUFFER > ---help--- > - The Sync File Framework adds explicit syncronization via > + The Sync File Framework adds explicit synchronization via > userspace. It enables send/receive 'struct dma_fence' objects to/from > userspace via Sync File fds for synchronization between drivers via > userspace components. It has been ported from Android. > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH -next] drm/panfrost: Remove set but not used variable 'bo'

by Rob Herring

On Mon, Feb 3, 2020 at 9:33 AM YueHaibing <yuehaibing(a)huawei.com> wrote: > > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/gpu/drm/panfrost/panfrost_job.c: In function 'panfrost_job_cleanup': > drivers/gpu/drm/panfrost/panfrost_job.c:278:31: warning: > variable 'bo' set but not used [-Wunused-but-set-variable] > > commit bdefca2d8dc0 ("drm/panfrost: Add the panfrost_gem_mapping concept") > involved this unused variable. > > Reported-by: Hulk Robot <hulkci(a)huawei.com> > Signed-off-by: YueHaibing <yuehaibing(a)huawei.com> > --- > drivers/gpu/drm/panfrost/panfrost_job.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) Applied to drm-misc-fixes. Rob

5 years, 3 months

1
0
0 0

[PATCH v2 3/4] drm/virtio: move mapping teardown to virtio_gpu_cleanup_object()

by Gerd Hoffmann

Stop sending DETACH_BACKING commands, that will happening anyway when releasing resources via UNREF. Handle guest-side cleanup in virtio_gpu_cleanup_object(), called when the host finished processing the UNREF command. Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com> --- drivers/gpu/drm/virtio/virtgpu_drv.h | 2 -- drivers/gpu/drm/virtio/virtgpu_object.c | 14 ++++++-- drivers/gpu/drm/virtio/virtgpu_vq.c | 46 ------------------------- 3 files changed, 12 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index 1bc13f6b161b..d37ddd7644f6 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -281,8 +281,6 @@ void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev, int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev, struct virtio_gpu_object *obj, struct virtio_gpu_fence *fence); -void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev, - struct virtio_gpu_object *obj); int virtio_gpu_attach_status_page(struct virtio_gpu_device *vgdev); int virtio_gpu_detach_status_page(struct virtio_gpu_device *vgdev); void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev, diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 28a161af7503..bce2b3d843fe 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -23,6 +23,7 @@ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/dma-mapping.h> #include <linux/moduleparam.h> #include "virtgpu_drv.h" @@ -65,6 +66,17 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo) { struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; + if (bo->pages) { + if (bo->mapped) { + dma_unmap_sg(vgdev->vdev->dev.parent, + bo->pages->sgl, bo->mapped, + DMA_TO_DEVICE); + bo->mapped = 0; + } + sg_free_table(bo->pages); + bo->pages = NULL; + drm_gem_shmem_unpin(&bo->base.base); + } virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle); drm_gem_shmem_free_object(&bo->base.base); } @@ -74,8 +86,6 @@ static void virtio_gpu_free_object(struct drm_gem_object *obj) struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; - if (bo->pages) - virtio_gpu_object_detach(vgdev, bo); if (bo->created) { virtio_gpu_cmd_unref_resource(vgdev, bo); /* completion handler calls virtio_gpu_cleanup_object() */ diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index 4e22c3914f94..87c439156151 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -545,22 +545,6 @@ void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev, virtio_gpu_queue_ctrl_buffer(vgdev, vbuf); } -static void virtio_gpu_cmd_resource_inval_backing(struct virtio_gpu_device *vgdev, - uint32_t resource_id, - struct virtio_gpu_fence *fence) -{ - struct virtio_gpu_resource_detach_backing *cmd_p; - struct virtio_gpu_vbuffer *vbuf; - - cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p)); - memset(cmd_p, 0, sizeof(*cmd_p)); - - cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING); - cmd_p->resource_id = cpu_to_le32(resource_id); - - virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence); -} - void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev, uint32_t scanout_id, uint32_t resource_id, uint32_t width, uint32_t height, @@ -1155,36 +1139,6 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev, return 0; } -void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev, - struct virtio_gpu_object *obj) -{ - bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev); - - if (WARN_ON_ONCE(!obj->pages)) - return; - - if (use_dma_api && obj->mapped) { - struct virtio_gpu_fence *fence = virtio_gpu_fence_alloc(vgdev); - /* detach backing and wait for the host process it ... */ - virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, fence); - dma_fence_wait(&fence->f, true); - dma_fence_put(&fence->f); - - /* ... then tear down iommu mappings */ - dma_unmap_sg(vgdev->vdev->dev.parent, - obj->pages->sgl, obj->mapped, - DMA_TO_DEVICE); - obj->mapped = 0; - } else { - virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, NULL); - } - - sg_free_table(obj->pages); - obj->pages = NULL; - - drm_gem_shmem_unpin(&obj->base.base); -} - void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev, struct virtio_gpu_output *output) { -- 2.18.1

5 years, 4 months

1
0
0 0

[PATCH 3/4] drm/virtio: move mapping teardown to virtio_gpu_cleanup_object()

by Gerd Hoffmann

Stop sending DETACH_BACKING commands, that will happening anyway when releasing resources via UNREF. Handle guest-side cleanup in virtio_gpu_cleanup_object(), called when the host finished processing the UNREF command. Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com> --- drivers/gpu/drm/virtio/virtgpu_drv.h | 2 -- drivers/gpu/drm/virtio/virtgpu_object.c | 14 ++++++-- drivers/gpu/drm/virtio/virtgpu_vq.c | 46 ------------------------- 3 files changed, 12 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index 372dd248cf02..15fb3c12f22f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -280,8 +280,6 @@ void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev, int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev, struct virtio_gpu_object *obj, struct virtio_gpu_fence *fence); -void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev, - struct virtio_gpu_object *obj); int virtio_gpu_attach_status_page(struct virtio_gpu_device *vgdev); int virtio_gpu_detach_status_page(struct virtio_gpu_device *vgdev); void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev, diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 28a161af7503..bce2b3d843fe 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -23,6 +23,7 @@ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/dma-mapping.h> #include <linux/moduleparam.h> #include "virtgpu_drv.h" @@ -65,6 +66,17 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo) { struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; + if (bo->pages) { + if (bo->mapped) { + dma_unmap_sg(vgdev->vdev->dev.parent, + bo->pages->sgl, bo->mapped, + DMA_TO_DEVICE); + bo->mapped = 0; + } + sg_free_table(bo->pages); + bo->pages = NULL; + drm_gem_shmem_unpin(&bo->base.base); + } virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle); drm_gem_shmem_free_object(&bo->base.base); } @@ -74,8 +86,6 @@ static void virtio_gpu_free_object(struct drm_gem_object *obj) struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private; - if (bo->pages) - virtio_gpu_object_detach(vgdev, bo); if (bo->created) { virtio_gpu_cmd_unref_resource(vgdev, bo); /* completion handler calls virtio_gpu_cleanup_object() */ diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index 6e8097e4c214..e258186bedb2 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -538,22 +538,6 @@ void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev, virtio_gpu_queue_ctrl_buffer(vgdev, vbuf); } -static void virtio_gpu_cmd_resource_inval_backing(struct virtio_gpu_device *vgdev, - uint32_t resource_id, - struct virtio_gpu_fence *fence) -{ - struct virtio_gpu_resource_detach_backing *cmd_p; - struct virtio_gpu_vbuffer *vbuf; - - cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p)); - memset(cmd_p, 0, sizeof(*cmd_p)); - - cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING); - cmd_p->resource_id = cpu_to_le32(resource_id); - - virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence); -} - void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev, uint32_t scanout_id, uint32_t resource_id, uint32_t width, uint32_t height, @@ -1148,36 +1132,6 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev, return 0; } -void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev, - struct virtio_gpu_object *obj) -{ - bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev); - - if (WARN_ON_ONCE(!obj->pages)) - return; - - if (use_dma_api && obj->mapped) { - struct virtio_gpu_fence *fence = virtio_gpu_fence_alloc(vgdev); - /* detach backing and wait for the host process it ... */ - virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, fence); - dma_fence_wait(&fence->f, true); - dma_fence_put(&fence->f); - - /* ... then tear down iommu mappings */ - dma_unmap_sg(vgdev->vdev->dev.parent, - obj->pages->sgl, obj->mapped, - DMA_TO_DEVICE); - obj->mapped = 0; - } else { - virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, NULL); - } - - sg_free_table(obj->pages); - obj->pages = NULL; - - drm_gem_shmem_unpin(&obj->base.base); -} - void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev, struct virtio_gpu_output *output) { -- 2.18.1

5 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH -next] drm/panfrost: Remove set but not used variable 'bo'

by Steven Price

On Mon, Feb 03, 2020 at 03:27:24PM +0000, YueHaibing wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/gpu/drm/panfrost/panfrost_job.c: In function 'panfrost_job_cleanup': > drivers/gpu/drm/panfrost/panfrost_job.c:278:31: warning: > variable 'bo' set but not used [-Wunused-but-set-variable] > > commit bdefca2d8dc0 ("drm/panfrost: Add the panfrost_gem_mapping concept") > involved this unused variable. > > Reported-by: Hulk Robot <hulkci(a)huawei.com> > Signed-off-by: YueHaibing <yuehaibing(a)huawei.com> I'm not sure how I didn't spot that before! Thanks for fixing it. Note commit bdefca2d8dc0 is actually in v5.5 and not (yet) in drm-misc-next. Reviewed-by: Steven Price <steven.price(a)arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_job.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index 7c36ec675b73..ccb8546a9342 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -275,12 +275,8 @@ static void panfrost_job_cleanup(struct kref *ref) > } > > if (job->bos) { > - struct panfrost_gem_object *bo; > - > - for (i = 0; i < job->bo_count; i++) { > - bo = to_panfrost_bo(job->bos[i]); > + for (i = 0; i < job->bo_count; i++) > drm_gem_object_put_unlocked(job->bos[i]); > - } > > kvfree(job->bos); > } > > >

5 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] KASAN: use-after-free Read in vgem_gem_dumb_create

by Daniel Vetter

On Fri, Jan 31, 2020 at 11:28 PM syzbot <syzbot+0dc4444774d419e916c8(a)syzkaller.appspotmail.com> wrote: > > Hello, > > syzbot found the following crash on: > > HEAD commit: 39bed42d Merge tag 'for-linus-hmm' of git://git.kernel.org.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=179465bee00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=2646535f8818ae25 > dashboard link: https://syzkaller.appspot.com/bug?extid=0dc4444774d419e916c8 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16251279e00000 > > The bug was bisected to: > > commit 7611750784664db46d0db95631e322aeb263dde7 > Author: Alex Deucher <alexander.deucher(a)amd.com> > Date: Wed Jun 21 16:31:41 2017 +0000 > > drm/amdgpu: use kernel is_power_of_2 rather than local version > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11628df1e00000 > final crash: https://syzkaller.appspot.com/x/report.txt?x=13628df1e00000 > console output: https://syzkaller.appspot.com/x/log.txt?x=15628df1e00000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+0dc4444774d419e916c8(a)syzkaller.appspotmail.com > Fixes: 761175078466 ("drm/amdgpu: use kernel is_power_of_2 rather than local version") Aside: This bisect line is complete nonsense ... I'm kinda at the point where I'm assuming that syzbot bisect results are garbage, which is maybe not what we want. I guess much stricter filtering for noise is needed, dunno. -Danile > > ================================================================== > BUG: KASAN: use-after-free in vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > Read of size 8 at addr ffff88809fa67908 by task syz-executor.0/14871 > > CPU: 0 PID: 14871 Comm: syz-executor.0 Not tainted 5.5.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x197/0x210 lib/dump_stack.c:118 > print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374 > __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506 > kasan_report+0x12/0x20 mm/kasan/common.c:639 > __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135 > vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > RIP: 0033:0x45b349 > Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00007f871af46c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 00007f871af476d4 RCX: 000000000045b349 > RDX: 0000000020000180 RSI: 00000000c02064b2 RDI: 0000000000000003 > RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff > R13: 0000000000000285 R14: 00000000004d14d0 R15: 000000000075bf2c > > Allocated by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > __kasan_kmalloc mm/kasan/common.c:513 [inline] > __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:486 > kasan_kmalloc+0x9/0x10 mm/kasan/common.c:527 > kmem_cache_alloc_trace+0x158/0x790 mm/slab.c:3551 > kmalloc include/linux/slab.h:556 [inline] > kzalloc include/linux/slab.h:670 [inline] > __vgem_gem_create+0x49/0x100 drivers/gpu/drm/vgem/vgem_drv.c:165 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:194 [inline] > vgem_gem_dumb_create+0xd7/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Freed by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > kasan_set_free_info mm/kasan/common.c:335 [inline] > __kasan_slab_free+0x102/0x150 mm/kasan/common.c:474 > kasan_slab_free+0xe/0x10 mm/kasan/common.c:483 > __cache_free mm/slab.c:3426 [inline] > kfree+0x10a/0x2c0 mm/slab.c:3757 > vgem_gem_free_object+0xbe/0xe0 drivers/gpu/drm/vgem/vgem_drv.c:68 > drm_gem_object_free+0x100/0x220 drivers/gpu/drm/drm_gem.c:983 > kref_put include/linux/kref.h:65 [inline] > drm_gem_object_put_unlocked drivers/gpu/drm/drm_gem.c:1017 [inline] > drm_gem_object_put_unlocked+0x196/0x1c0 drivers/gpu/drm/drm_gem.c:1002 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:199 [inline] > vgem_gem_dumb_create+0x115/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > The buggy address belongs to the object at ffff88809fa67800 > which belongs to the cache kmalloc-1k of size 1024 > The buggy address is located 264 bytes inside of > 1024-byte region [ffff88809fa67800, ffff88809fa67c00) > The buggy address belongs to the page: > page:ffffea00027e99c0 refcount:1 mapcount:0 mapping:ffff8880aa400c40 index:0x0 > raw: 00fffe0000000200 ffffea0002293548 ffffea00023e1f08 ffff8880aa400c40 > raw: 0000000000000000 ffff88809fa67000 0000000100000002 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff88809fa67800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > >ffff88809fa67900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff88809fa67980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== > > > --- > This bug is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller(a)googlegroups.com. > > syzbot will keep track of this bug report. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. > For information about bisection process see: https://goo.gl/tpsmEJ#bisection > syzbot can test patches for this bug, for details see: > https://goo.gl/tpsmEJ#testing-patches -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years, 4 months

3
2
0 0

Re: [Linaro-mm-sig] KASAN: use-after-free Read in vgem_gem_dumb_create

by Hillf Danton

Fri, 31 Jan 2020 14:28:10 -0800 (PST) > syzbot found the following crash on: > > HEAD commit: 39bed42d Merge tag 'for-linus-hmm' of git://git.kernel.org.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=179465bee00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=2646535f8818ae25 > dashboard link: https://syzkaller.appspot.com/bug?extid=0dc4444774d419e916c8 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16251279e00000 > > The bug was bisected to: > > commit 7611750784664db46d0db95631e322aeb263dde7 > Author: Alex Deucher <alexander.deucher(a)amd.com> > Date: Wed Jun 21 16:31:41 2017 +0000 > > drm/amdgpu: use kernel is_power_of_2 rather than local version > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11628df1e00000 > final crash: https://syzkaller.appspot.com/x/report.txt?x=13628df1e00000 > console output: https://syzkaller.appspot.com/x/log.txt?x=15628df1e00000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+0dc4444774d419e916c8(a)syzkaller.appspotmail.com > Fixes: 761175078466 ("drm/amdgpu: use kernel is_power_of_2 rather than local version") > > ================================================================== > BUG: KASAN: use-after-free in vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > Read of size 8 at addr ffff88809fa67908 by task syz-executor.0/14871 > > CPU: 0 PID: 14871 Comm: syz-executor.0 Not tainted 5.5.0-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x197/0x210 lib/dump_stack.c:118 > print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374 > __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506 > kasan_report+0x12/0x20 mm/kasan/common.c:639 > __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135 > vgem_gem_dumb_create+0x238/0x250 drivers/gpu/drm/vgem/vgem_drv.c:221 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > RIP: 0033:0x45b349 > Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00007f871af46c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 00007f871af476d4 RCX: 000000000045b349 > RDX: 0000000020000180 RSI: 00000000c02064b2 RDI: 0000000000000003 > RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff > R13: 0000000000000285 R14: 00000000004d14d0 R15: 000000000075bf2c > > Allocated by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > __kasan_kmalloc mm/kasan/common.c:513 [inline] > __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:486 > kasan_kmalloc+0x9/0x10 mm/kasan/common.c:527 > kmem_cache_alloc_trace+0x158/0x790 mm/slab.c:3551 > kmalloc include/linux/slab.h:556 [inline] > kzalloc include/linux/slab.h:670 [inline] > __vgem_gem_create+0x49/0x100 drivers/gpu/drm/vgem/vgem_drv.c:165 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:194 [inline] > vgem_gem_dumb_create+0xd7/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Freed by task 14871: > save_stack+0x23/0x90 mm/kasan/common.c:72 > set_track mm/kasan/common.c:80 [inline] > kasan_set_free_info mm/kasan/common.c:335 [inline] > __kasan_slab_free+0x102/0x150 mm/kasan/common.c:474 > kasan_slab_free+0xe/0x10 mm/kasan/common.c:483 > __cache_free mm/slab.c:3426 [inline] > kfree+0x10a/0x2c0 mm/slab.c:3757 > vgem_gem_free_object+0xbe/0xe0 drivers/gpu/drm/vgem/vgem_drv.c:68 > drm_gem_object_free+0x100/0x220 drivers/gpu/drm/drm_gem.c:983 > kref_put include/linux/kref.h:65 [inline] > drm_gem_object_put_unlocked drivers/gpu/drm/drm_gem.c:1017 [inline] > drm_gem_object_put_unlocked+0x196/0x1c0 drivers/gpu/drm/drm_gem.c:1002 > vgem_gem_create drivers/gpu/drm/vgem/vgem_drv.c:199 [inline] > vgem_gem_dumb_create+0x115/0x250 drivers/gpu/drm/vgem/vgem_drv.c:217 > drm_mode_create_dumb+0x282/0x310 drivers/gpu/drm/drm_dumb_buffers.c:94 > drm_mode_create_dumb_ioctl+0x26/0x30 drivers/gpu/drm/drm_dumb_buffers.c:100 > drm_ioctl_kernel+0x244/0x300 drivers/gpu/drm/drm_ioctl.c:786 > drm_ioctl+0x54e/0xa60 drivers/gpu/drm/drm_ioctl.c:886 > vfs_ioctl fs/ioctl.c:47 [inline] > ksys_ioctl+0x123/0x180 fs/ioctl.c:747 > __do_sys_ioctl fs/ioctl.c:756 [inline] > __se_sys_ioctl fs/ioctl.c:754 [inline] > __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 > do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > The buggy address belongs to the object at ffff88809fa67800 > which belongs to the cache kmalloc-1k of size 1024 > The buggy address is located 264 bytes inside of > 1024-byte region [ffff88809fa67800, ffff88809fa67c00) > The buggy address belongs to the page: > page:ffffea00027e99c0 refcount:1 mapcount:0 mapping:ffff8880aa400c40 index:0x0 > raw: 00fffe0000000200 ffffea0002293548 ffffea00023e1f08 ffff8880aa400c40 > raw: 0000000000000000 ffff88809fa67000 0000000100000002 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff88809fa67800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > >ffff88809fa67900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff88809fa67980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff88809fa67a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== Release obj in error path. --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -196,10 +196,10 @@ static struct drm_gem_object *vgem_gem_c return ERR_CAST(obj); ret = drm_gem_handle_create(file, &obj->base, handle); - drm_gem_object_put_unlocked(&obj->base); - if (ret) + if (ret) { + drm_gem_object_put_unlocked(&obj->base); return ERR_PTR(ret); - + } return &obj->base; }

5 years, 4 months

3
4
0 0

Re: [Linaro-mm-sig] KASAN: use-after-free Read in vgem_gem_dumb_create

by Dan Carpenter

I don't totally understand the stack trace but I do see a double free bug. drivers/gpu/drm/vgem/vgem_drv.c 186 static struct drm_gem_object *vgem_gem_create(struct drm_device *dev, 187 struct drm_file *file, 188 unsigned int *handle, 189 unsigned long size) 190 { 191 struct drm_vgem_gem_object *obj; 192 int ret; 193 194 obj = __vgem_gem_create(dev, size); obj->base.handle_count is zero. 195 if (IS_ERR(obj)) 196 return ERR_CAST(obj); 197 198 ret = drm_gem_handle_create(file, &obj->base, handle); We bump it +1 and then the error handling calls drm_gem_object_handle_put_unlocked(obj); which calls drm_gem_object_put_unlocked(); which frees obj. 199 drm_gem_object_put_unlocked(&obj->base); So this is a double free. Could someone check my thinking and send a patch? It's just a one liner. Otherwise I can send it on Monday. 200 if (ret) 201 return ERR_PTR(ret); 202 203 return &obj->base; 204 } regards, dan carpenter

5 years, 4 months

1
0
0 0

[PATCH] dma-buf: fix locking in sync_print_obj()

by Dan Carpenter

This is always called with IRQs disabled and we don't actually want to enable IRQs at the end. Fixes: a6aa8fca4d79 ("dma-buf/sw-sync: Reduce irqsave/irqrestore from known context") Signed-off-by: Dan Carpenter <dan.carpenter(a)oracle.com> --- drivers/dma-buf/sync_debug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index 101394f16930..952331344b1c 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -107,15 +107,16 @@ static void sync_print_fence(struct seq_file *s, static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj) { struct list_head *pos; + unsigned long flags; seq_printf(s, "%s: %d\n", obj->name, obj->value); - spin_lock_irq(&obj->lock); + spin_lock_irqsave(&obj->lock, flags); list_for_each(pos, &obj->pt_list) { struct sync_pt *pt = container_of(pos, struct sync_pt, link); sync_print_fence(s, &pt->base, false); } - spin_unlock_irq(&obj->lock); + spin_unlock_irqrestore(&obj->lock, flags); } static void sync_print_sync_file(struct seq_file *s, -- 2.11.0

5 years, 4 months

2
3
0 0

[PATCH] dma-heap: Make the symbol 'dma_heap_ioctl_cmds' static

by Sumit Semwal

From: zhong jiang <zhongjiang(a)huawei.com> Fix the following sparse warning. drivers/dma-buf/dma-heap.c:109:14: warning: symbol 'dma_heap_ioctl_cmds' was not declared. Should it be static? Acked-by: Andrew F. Davis <afd(a)ti.com> Acked-by: John Stultz <john.stultz(a)linaro.org> Signed-off-by: zhong jiang <zhongjiang(a)huawei.com> Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org> [sumits: rebased over IOCTL rename patches] --- drivers/dma-buf/dma-heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 1886aee46131..afd22c9dbdcf 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -106,7 +106,7 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data) return 0; } -unsigned int dma_heap_ioctl_cmds[] = { +static unsigned int dma_heap_ioctl_cmds[] = { DMA_HEAP_IOCTL_ALLOC, }; -- 2.18.0

5 years, 5 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig