Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

5 participants
2541 discussions

Re: [Linaro-mm-sig] [PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

by Christian König

Am 28.06.2018 um 11:53 schrieb Zhang, Jerry (Junwei): > On 06/22/2018 10:11 PM, Christian König wrote: >> Add function variants which can be called with the reservation lock >> already held. >> >> v2: reordered, add lockdep asserts, fix kerneldoc >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/dma-buf/dma-buf.c | 57 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/dma-buf.h | 5 +++++ >> 2 files changed, 62 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 852a3928ee71..dc94e76e2e2a 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, >> struct dma_buf_attachment *attach) >> } >> EXPORT_SYMBOL_GPL(dma_buf_detach); >> >> +/** >> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ >> address space >> + * with the reservation lock held. Is a wrapper for map_dma_buf() of >> the >> + * >> + * Returns the scatterlist table of the attachment; >> + * dma_buf_ops. >> + * @attach: [in] attachment whose scatterlist is to be returned >> + * @direction: [in] direction of DMA transfer >> + * >> + * Returns sg_table containing the scatterlist to be returned; >> returns ERR_PTR >> + * on error. May return -EINTR if it is interrupted by a signal. >> + * >> + * A mapping must be unmapped by using >> dma_buf_unmap_attachment_locked(). Note >> + * that the underlying backing storage is pinned for as long as a >> mapping >> + * exists, therefore users/importers should not hold onto a mapping >> for undue >> + * amounts of time. >> + */ >> +struct sg_table * >> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, >> + enum dma_data_direction direction) >> +{ >> + struct sg_table *sg_table; >> + > > Perhaps better to add some error check, like dma_buf_map_attachment() > > WARN_ON(!attach || !attach->dmabuf) Actually I wanted to remove those from the other functions as well. WARN_ON and BUG_ON checks for NULL pointers before using them are totally pointless because they have the same effect as a crash. Regards, Christian. > > Apart from that, it's > Reviewed-by: Junwei Zhang <Jerry.Zhang(a)amd.com> > > Jerry > >> + might_sleep(); >> + reservation_object_assert_held(attach->dmabuf->resv); >> + >> + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); >> + if (!sg_table) >> + sg_table = ERR_PTR(-ENOMEM); >> + >> + return sg_table; >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked); >> + >> /** >> * dma_buf_map_attachment - Returns the scatterlist table of the >> attachment; >> * mapped into _device_ address space. Is a wrapper for >> map_dma_buf() of the >> @@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct >> dma_buf_attachment *attach, >> } >> EXPORT_SYMBOL_GPL(dma_buf_map_attachment); >> >> +/** >> + * dma_buf_unmap_attachment_locked - unmaps the buffer with >> reservation lock >> + * held, should deallocate the associated scatterlist. Is a wrapper for >> + * unmap_dma_buf() of dma_buf_ops. >> + * @attach: [in] attachment to unmap buffer from >> + * @sg_table: [in] scatterlist info of the buffer to unmap >> + * @direction: [in] direction of DMA transfer >> + * >> + * This unmaps a DMA mapping for @attached obtained by >> + * dma_buf_map_attachment_locked(). >> + */ >> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach, >> + struct sg_table *sg_table, >> + enum dma_data_direction direction) >> +{ >> + might_sleep(); >> + reservation_object_assert_held(attach->dmabuf->resv); >> + >> + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, >> + direction); >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked); >> + >> /** >> * dma_buf_unmap_attachment - unmaps and decreases usecount of the >> buffer;might >> * deallocate the scatterlist associated. Is a wrapper for >> unmap_dma_buf() of >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >> index 991787a03199..a25e754ae2f7 100644 >> --- a/include/linux/dma-buf.h >> +++ b/include/linux/dma-buf.h >> @@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags); >> struct dma_buf *dma_buf_get(int fd); >> void dma_buf_put(struct dma_buf *dmabuf); >> >> +struct sg_table *dma_buf_map_attachment_locked(struct >> dma_buf_attachment *, >> + enum dma_data_direction); >> struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, >> enum dma_data_direction); >> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *, >> + struct sg_table *, >> + enum dma_data_direction); >> void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct >> sg_table *, >> enum dma_data_direction); >> int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, >>

6 years, 6 months

Re: [Linaro-mm-sig] [PATCH 3/4] drm/amdgpu: add independent DMA-buf export v3

by Christian König

Am 28.06.2018 um 11:58 schrieb Zhang, Jerry (Junwei): > On 06/22/2018 10:11 PM, Christian König wrote: >> The caching of SGT's done by the DRM code is actually quite harmful and >> should probably removed altogether in the long term. >> >> Start by providing a separate DMA-buf export implementation in >> amdgpu. This is >> also a prerequisite of unpinned DMA-buf handling. >> >> v2: fix unintended recursion, remove debugging leftovers >> v3: split out from unpinned DMA-buf work >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 - >> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 - >> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 >> +++++++++++++------------------ >> 3 files changed, 39 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> index d8e0cc08f9db..5e71af8dd3a7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> @@ -373,7 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object >> *obj, >> void amdgpu_gem_object_close(struct drm_gem_object *obj, >> struct drm_file *file_priv); >> unsigned long amdgpu_gem_timeout(uint64_t timeout_ns); >> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object >> *obj); >> struct drm_gem_object * >> amdgpu_gem_prime_import_sg_table(struct drm_device *dev, >> struct dma_buf_attachment *attach, >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> index a549483032b0..cdf0be85d361 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> @@ -919,7 +919,6 @@ static struct drm_driver kms_driver = { >> .gem_prime_export = amdgpu_gem_prime_export, >> .gem_prime_import = amdgpu_gem_prime_import, >> .gem_prime_res_obj = amdgpu_gem_prime_res_obj, >> - .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table, > > I may miss some patches or important info. > > Even applied the series of patch, I still could find below code in > drm_gem_map_dma_buf(). > In this case, it will cause NULL pointer access. Please help me out of > here. > {{{ > sgt = obj->dev->driver->gem_prime_get_sg_table(obj); > }}} Take a look at the change a bit further down. > > Jerry > >> .gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table, >> .gem_prime_vmap = amdgpu_gem_prime_vmap, >> .gem_prime_vunmap = amdgpu_gem_prime_vunmap, >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> index b2286bc41aec..038a8c8488b7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> @@ -40,22 +40,6 @@ >> >> static const struct dma_buf_ops amdgpu_dmabuf_ops; >> >> -/** >> - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table >> - * implementation >> - * @obj: GEM buffer object >> - * >> - * Returns: >> - * A scatter/gather table for the pinned pages of the buffer >> object's memory. >> - */ >> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object >> *obj) >> -{ >> - struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >> - int npages = bo->tbo.num_pages; >> - >> - return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages); >> -} >> - >> /** >> * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation >> * @obj: GEM buffer object >> @@ -189,35 +173,29 @@ amdgpu_gem_prime_import_sg_table(struct >> drm_device *dev, >> } >> >> /** >> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation >> - * @dma_buf: shared DMA buffer >> - * @target_dev: target device >> + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation >> * @attach: DMA-buf attachment >> + * @dir: DMA direction >> * >> * Makes sure that the shared DMA buffer can be accessed by the >> target device. >> * For now, simply pins it to the GTT domain, where it should be >> accessible by >> * all DMA devices. >> * >> * Returns: >> - * 0 on success or negative error code. >> + * sg_table filled with the DMA addresses to use or ERR_PRT with >> negative error >> + * code. >> */ >> -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, >> - struct dma_buf_attachment *attach) >> +static struct sg_table * >> +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, >> + enum dma_data_direction dir) >> { >> + struct dma_buf *dma_buf = attach->dmabuf; >> struct drm_gem_object *obj = dma_buf->priv; >> struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >> + struct sg_table *sgt; >> long r; >> >> - r = drm_gem_map_attach(dma_buf, attach); >> - if (r) >> - return r; >> - >> - r = amdgpu_bo_reserve(bo, false); >> - if (unlikely(r != 0)) >> - goto error_detach; >> - >> - >> if (attach->dev->driver != adev->dev->driver) { >> /* >> * Wait for all shared fences to complete before we switch >> to future >> @@ -228,54 +206,62 @@ static int amdgpu_gem_map_attach(struct dma_buf >> *dma_buf, >> MAX_SCHEDULE_TIMEOUT); >> if (unlikely(r < 0)) { >> DRM_DEBUG_PRIME("Fence wait failed: %li\n", r); >> - goto error_unreserve; >> + return ERR_PTR(r); >> } >> } >> >> /* pin buffer into GTT */ >> r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); >> if (r) >> - goto error_unreserve; >> + return ERR_PTR(r); >> + >> + sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages); >> + if (IS_ERR(sgt)) >> + return sgt; >> + >> + if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, >> + DMA_ATTR_SKIP_CPU_SYNC)) >> + goto error_free; >> >> if (attach->dev->driver != adev->dev->driver) >> bo->prime_shared_count++; >> >> -error_unreserve: >> - amdgpu_bo_unreserve(bo); >> + return sgt; >> >> -error_detach: >> - if (r) >> - drm_gem_map_detach(dma_buf, attach); >> - return r; >> +error_free: >> + sg_free_table(sgt); >> + kfree(sgt); >> + return ERR_PTR(-ENOMEM); >> } >> >> /** >> - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation >> - * @dma_buf: shared DMA buffer >> + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation >> * @attach: DMA-buf attachment >> + * @sgt: sg_table to unmap >> + * @dir: DMA direction >> * >> * This is called when a shared DMA buffer no longer needs to be >> accessible by >> * the other device. For now, simply unpins the buffer from GTT. >> */ >> -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf, >> - struct dma_buf_attachment *attach) >> +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, >> + struct sg_table *sgt, >> + enum dma_data_direction dir) >> { >> + struct dma_buf *dma_buf = attach->dmabuf; >> struct drm_gem_object *obj = dma_buf->priv; >> struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >> - int ret = 0; >> - >> - ret = amdgpu_bo_reserve(bo, true); >> - if (unlikely(ret != 0)) >> - goto error; >> >> amdgpu_bo_unpin(bo); >> + >> if (attach->dev->driver != adev->dev->driver && >> bo->prime_shared_count) >> bo->prime_shared_count--; >> - amdgpu_bo_unreserve(bo); >> >> -error: >> - drm_gem_map_detach(dma_buf, attach); >> + if (sgt) { >> + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir); >> + sg_free_table(sgt); >> + kfree(sgt); >> + } >> } >> >> /** >> @@ -333,10 +319,8 @@ static int amdgpu_gem_begin_cpu_access(struct >> dma_buf *dma_buf, >> } >> >> static const struct dma_buf_ops amdgpu_dmabuf_ops = { >> - .attach = amdgpu_gem_map_attach, >> - .detach = amdgpu_gem_map_detach, >> - .map_dma_buf = drm_gem_map_dma_buf, >> - .unmap_dma_buf = drm_gem_unmap_dma_buf, Here we are stopping to use drm_gem_map_dma_buf(), so we don't need amdgpu_gem_prime_get_sg_table() any more either. Christian. >> + .map_dma_buf = amdgpu_gem_map_dma_buf, >> + .unmap_dma_buf = amdgpu_gem_unmap_dma_buf, >> .release = drm_gem_dmabuf_release, >> .begin_cpu_access = amdgpu_gem_begin_cpu_access, >> .map = drm_gem_dmabuf_kmap, >>

6 years, 6 months

[PATCH 04/15] dma-fence: Make ->wait callback optional

by Daniel Vetter

Almost everyone uses dma_fence_default_wait. v2: Also remove the BUG_ON(!ops->wait) (Chris). Reviewed-by: Christian König <christian.koenig(a)amd.com> (v1) Signed-off-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-fence-array.c | 1 - drivers/dma-buf/dma-fence.c | 8 +++++--- drivers/dma-buf/sw_sync.c | 1 - include/linux/dma-fence.h | 13 ++++++++----- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index dd1edfb27b61..a8c254497251 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -104,7 +104,6 @@ const struct dma_fence_ops dma_fence_array_ops = { .get_timeline_name = dma_fence_array_get_timeline_name, .enable_signaling = dma_fence_array_enable_signaling, .signaled = dma_fence_array_signaled, - .wait = dma_fence_default_wait, .release = dma_fence_array_release, }; EXPORT_SYMBOL(dma_fence_array_ops); diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 59049375bd19..41ec19c9efc7 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -158,7 +158,10 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) return -EINVAL; trace_dma_fence_wait_start(fence); - ret = fence->ops->wait(fence, intr, timeout); + if (fence->ops->wait) + ret = fence->ops->wait(fence, intr, timeout); + else + ret = dma_fence_default_wait(fence, intr, timeout); trace_dma_fence_wait_end(fence); return ret; } @@ -562,8 +565,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, unsigned seqno) { BUG_ON(!lock); - BUG_ON(!ops || !ops->wait || - !ops->get_driver_name || !ops->get_timeline_name); + BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); kref_init(&fence->refcount); fence->ops = ops; diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 3d78ca89a605..53c1d6d36a64 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -188,7 +188,6 @@ static const struct dma_fence_ops timeline_fence_ops = { .get_timeline_name = timeline_fence_get_timeline_name, .enable_signaling = timeline_fence_enable_signaling, .signaled = timeline_fence_signaled, - .wait = dma_fence_default_wait, .release = timeline_fence_release, .fence_value_str = timeline_fence_value_str, .timeline_value_str = timeline_fence_timeline_value_str, diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index c053d19e1e24..02dba8cd033d 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -191,11 +191,14 @@ struct dma_fence_ops { /** * @wait: * - * Custom wait implementation, or dma_fence_default_wait. + * Custom wait implementation, defaults to dma_fence_default_wait() if + * not set. * - * Must not be NULL, set to dma_fence_default_wait for default implementation. - * the dma_fence_default_wait implementation should work for any fence, as long - * as enable_signaling works correctly. + * The dma_fence_default_wait implementation should work for any fence, as long + * as @enable_signaling works correctly. This hook allows drivers to + * have an optimized version for the case where a process context is + * already available, e.g. if @enable_signaling for the general case + * needs to set up a worker thread. * * Must return -ERESTARTSYS if the wait is intr = true and the wait was * interrupted, and remaining jiffies if fence has signaled, or 0 if wait @@ -203,7 +206,7 @@ struct dma_fence_ops { * which should be treated as if the fence is signaled. For example a hardware * lockup could be reported like that. * - * This callback is mandatory. + * This callback is optional. */ signed long (*wait)(struct dma_fence *fence, bool intr, signed long timeout); -- 2.17.0

6 years, 6 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

by Chris Wilson

Quoting Michel Dänzer (2018-06-26 15:31:47) > From: Michel Dänzer <michel.daenzer(a)amd.com> > > Fixes the BUG_ON spuriously triggering under the following > circumstances: > > * ttm_eu_reserve_buffers processes a list containing multiple BOs using > the same reservation object, so it calls > reservation_object_reserve_shared with that reservation object once > for each such BO. > * In reservation_object_reserve_shared, old->shared_count == > old->shared_max - 1, so obj->staged is freed in preparation of an > in-place update. > * ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence > once for each of the BOs above, always with the same fence. > * The first call adds the fence in the remaining free slot, after which > old->shared_count == old->shared_max. > > In the next call to reservation_object_add_shared_fence, the BUG_ON > triggers. However, nothing bad would happen in > reservation_object_add_shared_inplace, since the fence is already in the > reservation object. > > Prevent this by moving the BUG_ON to where an overflow would actually > happen (e.g. if a buggy caller didn't call > reservation_object_reserve_shared before). > > Cc: stable(a)vger.kernel.org > Signed-off-by: Michel Dänzer <michel.daenzer(a)amd.com> I've convinced myself (or rather have not found a valid argument against) that being able to call reserve_shared + add_shared multiple times for the same fence is an intended part of reservation_object API I'd double check with Christian though. Reviewed-by: Chris Wilson <chris(a)chris-wilson.co.uk> > drivers/dma-buf/reservation.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 314eb1071cce..532545b9488e 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -141,6 +141,7 @@ reservation_object_add_shared_inplace(struct reservation_object *obj, > if (signaled) { > RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > + BUG_ON(fobj->shared_count >= fobj->shared_max); Personally I would just let kasan detect this and throw away the BUG_ON or at least move it behind some DMABUF_BUG_ON(). -Chris

6 years, 6 months

Re: [Linaro-mm-sig] [PATCH v2] dma-buf/fence: Take refcount on the module that owns the fence

by Daniel Vetter

On Mon, Jun 25, 2018 at 09:21:15PM +0530, Akhil P Oommen wrote: > > > On 6/25/2018 1:20 PM, Daniel Vetter wrote: > > On Fri, Jun 22, 2018 at 11:08:48AM +0100, Chris Wilson wrote: > > > Quoting Gustavo Padovan (2018-06-22 11:04:16) > > > > Hi Akhil, > > > > > > > > On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > > > > > Each fence object holds function pointers of the module that > > > > > initialized > > > > > it. Allowing the module to unload before this fence's release is > > > > > catastrophic. So, keep a refcount on the module until the fence is > > > > > released. > > > > > > > > > > Signed-off-by: Akhil P Oommen <akhilpo(a)codeaurora.org> > > > > > --- > > > > > Changes in v2: > > > > > - added description for the new function parameter. > > > > > > > > > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > > > > > include/linux/dma-fence.h | 10 ++++++++-- > > > > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > > > fence.c > > > > > index 4edb9fd..2aaa44e 100644 > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > @@ -18,6 +18,7 @@ > > > > > * more details. > > > > > */ > > > > > +#include <linux/module.h> > > > > > #include <linux/slab.h> > > > > > #include <linux/export.h> > > > > > #include <linux/atomic.h> > > > > > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > > > > > { > > > > > struct dma_fence *fence = > > > > > container_of(kref, struct dma_fence, refcount); > > > > > + struct module *module = fence->owner; > > > > > trace_dma_fence_destroy(fence); > > > > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > > > > > fence->ops->release(fence); > > > > > else > > > > > dma_fence_free(fence); > > > > > + > > > > > + module_put(module); > > > > > } > > > > > EXPORT_SYMBOL(dma_fence_release); > > > > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > > > > /** > > > > > * dma_fence_init - Initialize a custom fence. > > > > > + * @module: [in] the module that calls this API > > > > > * @fence: [in] the fence to initialize > > > > > * @ops: [in] the dma_fence_ops for operations on this > > > > > fence > > > > > * @lock: [in] the irqsafe spinlock to use for locking > > > > > this fence > > > > > @@ -556,8 +561,9 @@ struct default_wait_cb { > > > > > * to check which fence is later by simply using dma_fence_later. > > > > > */ > > > > > void > > > > > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > > > > > *ops, > > > > > - spinlock_t *lock, u64 context, unsigned seqno) > > > > > +_dma_fence_init(struct module *module, struct dma_fence *fence, > > > > > + const struct dma_fence_ops *ops, spinlock_t *lock, > > > > > + u64 context, unsigned seqno) > > > > > { > > > > > BUG_ON(!lock); > > > > > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > > > > > @@ -571,7 +577,11 @@ struct default_wait_cb { > > > > > fence->seqno = seqno; > > > > > fence->flags = 0UL; > > > > > fence->error = 0; > > > > > + fence->owner = module; > > > > > + > > > > > + if (!try_module_get(module)) > > > > > + fence->owner = NULL; > > > > > trace_dma_fence_init(fence); > > > > > } > > > > > -EXPORT_SYMBOL(dma_fence_init); > > > > > +EXPORT_SYMBOL(_dma_fence_init); > > > > Do we still need to export the symbol, it won't be called from outside > > > > anymore? Other than that looks good to me: > > > There's a big drawback in that a module reference is often insufficient, > > > and that a reference on the driver (or whatever is required for the > > > lifetime of the fence) will already hold the module reference. > > > > > > Considering that we want a few 100k fences in flight per second, is > > > there no other way to only export a fence with a module reference? > > We'd need to make the timeline a full-blown object (Maarten owes me one > > for that design screw-up), and then we could stuff all these things in > > there. > > > > And I think that's the right fix, since try_module_get for every > > dma_fence_init just ain't cool really :-) > > -Daniel > Thanks for the feedback, Daniel. > I see your point, but I am not sure how much impact an extra refcounting > would create considering the whole effort of setting up a new fence. Also, > this refcounting is not required for built-in modules. > > As of now, unloading a kernel module that uses fence_init() is an easy way > to bring down the system. This patch simply fixes that. What you have > suggested sounds like a non-trivial effort which someone who is more > familiar with this code base can do a better job than me. Perhaps we can > take this patch now to fix the issue at hand and later somebody else can > share a more optimal solution. :) Module unload is a developer-only feature for a reason. Given that I don't think fixing this with a hack is the right approach. And dma_fence_init() is supposed to be really fast. Also note that you can fix this already for your own driver by simply waiting for all pending dma_fences to get released, so I don't think it's super-important to land this asap. Yes the real fix is a bit more involved, but shouldn't be too hard to pull off really. -Daniel > > @Gustavo & @Sumit, I would like the maintainers to take a decision here. > > -Akhil. > _______________________________________________ > 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

6 years, 6 months

Re: [Linaro-mm-sig] [PATCH v2] dma-buf/fence: Take refcount on the module that owns the fence

by Gustavo Padovan

Hi Akhil, On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > Each fence object holds function pointers of the module that > initialized > it. Allowing the module to unload before this fence's release is > catastrophic. So, keep a refcount on the module until the fence is > released. > > Signed-off-by: Akhil P Oommen <akhilpo(a)codeaurora.org> > --- > Changes in v2: > - added description for the new function parameter. > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > include/linux/dma-fence.h | 10 ++++++++-- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > fence.c > index 4edb9fd..2aaa44e 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -18,6 +18,7 @@ > * more details. > */ > > +#include <linux/module.h> > #include <linux/slab.h> > #include <linux/export.h> > #include <linux/atomic.h> > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > { > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > + struct module *module = fence->owner; > > trace_dma_fence_destroy(fence); > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > fence->ops->release(fence); > else > dma_fence_free(fence); > + > + module_put(module); > } > EXPORT_SYMBOL(dma_fence_release); > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > /** > * dma_fence_init - Initialize a custom fence. > + * @module: [in] the module that calls this API > * @fence: [in] the fence to initialize > * @ops: [in] the dma_fence_ops for operations on this > fence > * @lock: [in] the irqsafe spinlock to use for locking > this fence > @@ -556,8 +561,9 @@ struct default_wait_cb { > * to check which fence is later by simply using dma_fence_later. > */ > void > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > *ops, > - spinlock_t *lock, u64 context, unsigned seqno) > +_dma_fence_init(struct module *module, struct dma_fence *fence, > + const struct dma_fence_ops *ops, spinlock_t *lock, > + u64 context, unsigned seqno) > { > BUG_ON(!lock); > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > @@ -571,7 +577,11 @@ struct default_wait_cb { > fence->seqno = seqno; > fence->flags = 0UL; > fence->error = 0; > + fence->owner = module; > + > + if (!try_module_get(module)) > + fence->owner = NULL; > > trace_dma_fence_init(fence); > } > -EXPORT_SYMBOL(dma_fence_init); > +EXPORT_SYMBOL(_dma_fence_init); Do we still need to export the symbol, it won't be called from outside anymore? Other than that looks good to me: Reviewed-by: Gustavo Padovan <gustavo.padovan(a)collabora.com> Gustavo

6 years, 6 months

[PATCH 1/5] dma_buf: remove device parameter from attach callback

by Christian König

The device parameter is completely unused because it is available in the attachment structure as well. Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-buf.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 3 +-- drivers/gpu/drm/drm_prime.c | 3 +-- drivers/gpu/drm/udl/udl_dmabuf.c | 1 - drivers/gpu/drm/vmwgfx/vmwgfx_prime.c | 1 - drivers/media/common/videobuf2/videobuf2-dma-contig.c | 2 +- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +- drivers/media/common/videobuf2/videobuf2-vmalloc.c | 2 +- include/drm/drm_prime.h | 2 +- include/linux/dma-buf.h | 3 +-- 10 files changed, 8 insertions(+), 13 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d78d5fc173dc..e99a8d19991b 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -568,7 +568,7 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, mutex_lock(&dmabuf->lock); if (dmabuf->ops->attach) { - ret = dmabuf->ops->attach(dmabuf, dev, attach); + ret = dmabuf->ops->attach(dmabuf, attach); if (ret) goto err_attach; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index 4683626b065f..f1500f1ec0f5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -133,7 +133,6 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, } static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, - struct device *target_dev, struct dma_buf_attachment *attach) { struct drm_gem_object *obj = dma_buf->priv; @@ -141,7 +140,7 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); long r; - r = drm_gem_map_attach(dma_buf, target_dev, attach); + r = drm_gem_map_attach(dma_buf, attach); if (r) return r; diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7856a9b3f8a8..4a3a232fea67 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -186,7 +186,6 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri /** * drm_gem_map_attach - dma_buf attach implementation for GEM * @dma_buf: buffer to attach device to - * @target_dev: not used * @attach: buffer attachment data * * Allocates &drm_prime_attachment and calls &drm_driver.gem_prime_pin for @@ -195,7 +194,7 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri * * Returns 0 on success, negative error code on failure. */ -int drm_gem_map_attach(struct dma_buf *dma_buf, struct device *target_dev, +int drm_gem_map_attach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach) { struct drm_prime_attachment *prime_attach; diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 2867ed155ff6..5fdc8bdc2026 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -29,7 +29,6 @@ struct udl_drm_dmabuf_attachment { }; static int udl_attach_dma_buf(struct dma_buf *dmabuf, - struct device *dev, struct dma_buf_attachment *attach) { struct udl_drm_dmabuf_attachment *udl_attach; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c index 0d42a46521fc..fbffb37ccf42 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c @@ -40,7 +40,6 @@ */ static int vmw_prime_map_attach(struct dma_buf *dma_buf, - struct device *target_dev, struct dma_buf_attachment *attach) { return -ENOSYS; diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index f1178f6f434d..12d0072c52c2 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -222,7 +222,7 @@ struct vb2_dc_attachment { enum dma_data_direction dma_dir; }; -static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, +static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct dma_buf_attachment *dbuf_attach) { struct vb2_dc_attachment *attach; diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 753ed3138dcc..cf94765e593f 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -371,7 +371,7 @@ struct vb2_dma_sg_attachment { enum dma_data_direction dma_dir; }; -static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, +static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf *dbuf, struct dma_buf_attachment *dbuf_attach) { struct vb2_dma_sg_attachment *attach; diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c index 3a7c80cd1a17..298ffb9ecdae 100644 --- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c +++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c @@ -209,7 +209,7 @@ struct vb2_vmalloc_attachment { enum dma_data_direction dma_dir; }; -static int vb2_vmalloc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, +static int vb2_vmalloc_dmabuf_ops_attach(struct dma_buf *dbuf, struct dma_buf_attachment *dbuf_attach) { struct vb2_vmalloc_attachment *attach; diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index 4d5f5d6cf6a6..ef338151cea8 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -82,7 +82,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, struct dma_buf_export_info *exp_info); void drm_gem_dmabuf_release(struct dma_buf *dma_buf); -int drm_gem_map_attach(struct dma_buf *dma_buf, struct device *target_dev, +int drm_gem_map_attach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach); void drm_gem_map_detach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 085db2fee2d7..346caf77937f 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -77,8 +77,7 @@ struct dma_buf_ops { * to signal that backing storage is already allocated and incompatible * with the requirements of requesting device. */ - int (*attach)(struct dma_buf *, struct device *, - struct dma_buf_attachment *); + int (*attach)(struct dma_buf *, struct dma_buf_attachment *); /** * @detach: -- 2.14.1

6 years, 6 months

[PATCH] staging: android: ion: fix ion_dma_buf_attach signatur

by Christian König

Fixup for "dma_buf: remove device parameter from attach callback v2". I missed this driver, sorry for the noise. Patch is not even compile tested. Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/staging/android/ion/ion.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index a5220445b5e8..3d4d35558937 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -201,7 +201,7 @@ struct ion_dma_buf_attachment { struct list_head list; }; -static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev, +static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment) { struct ion_dma_buf_attachment *a; @@ -219,7 +219,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev, } a->table = table; - a->dev = dev; + a->dev = attachment->dev; INIT_LIST_HEAD(&a->list); attachment->priv = a; -- 2.14.1

6 years, 6 months

Re: [Linaro-mm-sig] [PATCH] dma-buf/fence: Take refcount on the module that owns the fence

by kbuild test robot

Hi Akhil, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.18-rc1 next-20180618] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Akhil-P-Oommen/dma-buf-fence-Take-… reproduce: make htmldocs All warnings (new ones prefixed by >>): WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) mm/mempool.c:228: warning: Function parameter or member 'pool' not described in 'mempool_init' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev' include/net/cfg80211.h:4279: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev' include/net/mac80211.h:2282: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw' include/net/mac80211.h:2282: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw' include/net/mac80211.h:955: warning: Function parameter or member 'control.rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.rts_cts_rate_idx' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.use_rts' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.use_cts_prot' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.short_preamble' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.skip_table' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.jiffies' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.vif' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.hw_key' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.flags' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'control.enqueue_time' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'ack' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'ack.cookie' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.ack_signal' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info' include/net/mac80211.h:955: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info' net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info' kernel/sched/fair.c:3760: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg' include/linux/device.h:93: warning: bad line: this bus. include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf' include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf' >> drivers/dma-buf/dma-fence.c:567: warning: Function parameter or member 'module' not described in '_dma_fence_init' include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array' include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip' include/linux/iio/hw-consumer.h:1: warning: no structured comments found include/linux/device.h:94: warning: bad line: this bus. include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry' include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume_early' not described in 'regulator_ops' drivers/regulator/core.c:4465: warning: Excess function parameter 'state' description in 'regulator_suspend_late' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb' arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb' drivers/usb/dwc3/gadget.c:510: warning: Excess function parameter 'dwc' description in 'dwc3_gadget_start_config' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver' include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver' drivers/gpu/drm/i915/i915_vma.h:48: warning: cannot understand function prototype: 'struct i915_vma ' drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found include/drm/tinydrm/tinydrm.h:34: warning: Function parameter or member 'fb_dirty' not described in 'tinydrm_device' drivers/gpu/drm/tinydrm/mipi-dbi.c:272: warning: Function parameter or member 'crtc_state' not described in 'mipi_dbi_enable_flush' drivers/gpu/drm/tinydrm/mipi-dbi.c:272: warning: Function parameter or member 'plane_state' not described in 'mipi_dbi_enable_flush' vim +567 drivers/dma-buf/dma-fence.c a519435a drivers/dma-buf/fence.c Christian König 2015-10-20 545 e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 546 /** f54d1867 drivers/dma-buf/dma-fence.c Chris Wilson 2016-10-25 547 * dma_fence_init - Initialize a custom fence. e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 548 * @fence: [in] the fence to initialize f54d1867 drivers/dma-buf/dma-fence.c Chris Wilson 2016-10-25 549 * @ops: [in] the dma_fence_ops for operations on this fence e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 550 * @lock: [in] the irqsafe spinlock to use for locking this fence e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 551 * @context: [in] the execution context this fence is run on e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 552 * @seqno: [in] a linear increasing sequence number for this context e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 553 * e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 554 * Initializes an allocated fence, the caller doesn't have to keep its e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 555 * refcount after committing with this fence, but it will need to hold a f54d1867 drivers/dma-buf/dma-fence.c Chris Wilson 2016-10-25 556 * refcount again if dma_fence_ops.enable_signaling gets called. This can e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 557 * be used for other implementing other types of fence. e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 558 * e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 559 * context and seqno are used for easy comparison between fences, allowing f54d1867 drivers/dma-buf/dma-fence.c Chris Wilson 2016-10-25 560 * to check which fence is later by simply using dma_fence_later. e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 561 */ e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 562 void 9c7d6561 drivers/dma-buf/dma-fence.c Akhil P Oommen 2018-06-19 563 _dma_fence_init(struct module *module, struct dma_fence *fence, 9c7d6561 drivers/dma-buf/dma-fence.c Akhil P Oommen 2018-06-19 564 const struct dma_fence_ops *ops, spinlock_t *lock, 9c7d6561 drivers/dma-buf/dma-fence.c Akhil P Oommen 2018-06-19 565 u64 context, unsigned seqno) e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 566 { e941759c drivers/dma-buf/fence.c Maarten Lankhorst 2014-07-01 @567 BUG_ON(!lock); :::::: The code at line 567 was first introduced by commit :::::: e941759c74a44d6ac2eed21bb0a38b21fe4559e2 fence: dma-buf cross-device synchronization (v18) :::::: TO: Maarten Lankhorst <maarten.lankhorst(a)canonical.com> :::::: CC: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

6 years, 6 months

[PATCH 1/2] locking: WW mutex cleanup

by Thomas Hellstrom

From: Peter Ziljstra <peterz(a)infradead.org> Make the WW mutex code more readable by adding comments, splitting up functions and pointing out that we're actually using the Wait-Die algorithm. Cc: Ingo Molnar <mingo(a)redhat.com> Cc: Jonathan Corbet <corbet(a)lwn.net> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Sean Paul <seanpaul(a)chromium.org> Cc: David Airlie <airlied(a)linux.ie> Cc: Davidlohr Bueso <dave(a)stgolabs.net> Cc: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com> Cc: Josh Triplett <josh(a)joshtriplett.org> Cc: Thomas Gleixner <tglx(a)linutronix.de> Cc: Kate Stewart <kstewart(a)linuxfoundation.org> Cc: Philippe Ombredanne <pombredanne(a)nexb.com> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: linux-doc(a)vger.kernel.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Co-authored-by: Thomas Hellstrom <thellstrom(a)vmware.com> Signed-off-by: Thomas Hellstrom <thellstrom(a)vmware.com> --- Documentation/locking/ww-mutex-design.txt | 12 +- include/linux/ww_mutex.h | 28 ++--- kernel/locking/mutex.c | 202 ++++++++++++++++++------------ 3 files changed, 145 insertions(+), 97 deletions(-) diff --git a/Documentation/locking/ww-mutex-design.txt b/Documentation/locking/ww-mutex-design.txt index 34c3a1b50b9a..2fd7f2a2af21 100644 --- a/Documentation/locking/ww-mutex-design.txt +++ b/Documentation/locking/ww-mutex-design.txt @@ -32,10 +32,10 @@ the oldest task) wins, and the one with the higher reservation id (i.e. the younger task) unlocks all of the buffers that it has already locked, and then tries again. -In the RDBMS literature this deadlock handling approach is called wait/wound: +In the RDBMS literature this deadlock handling approach is called wait/die: The older tasks waits until it can acquire the contended lock. The younger tasks needs to back off and drop all the locks it is currently holding, i.e. the -younger task is wounded. +younger task dies. Concepts -------- @@ -56,9 +56,9 @@ Furthermore there are three different class of w/w lock acquire functions: * Normal lock acquisition with a context, using ww_mutex_lock. -* Slowpath lock acquisition on the contending lock, used by the wounded task - after having dropped all already acquired locks. These functions have the - _slow postfix. +* Slowpath lock acquisition on the contending lock, used by the task that just + killed its transaction after having dropped all already acquired locks. + These functions have the _slow postfix. From a simple semantics point-of-view the _slow functions are not strictly required, since simply calling the normal ww_mutex_lock functions on the @@ -220,7 +220,7 @@ mutexes are a natural fit for such a case for two reasons: Note that this approach differs in two important ways from the above methods: - Since the list of objects is dynamically constructed (and might very well be - different when retrying due to hitting the -EDEADLK wound condition) there's + different when retrying due to hitting the -EDEADLK die condition) there's no need to keep any object on a persistent list when it's not locked. We can therefore move the list_head into the object itself. - On the other hand the dynamic object list construction also means that the -EALREADY return diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h index 39fda195bf78..f82fce2229c8 100644 --- a/include/linux/ww_mutex.h +++ b/include/linux/ww_mutex.h @@ -6,7 +6,7 @@ * * Copyright (C) 2004, 2005, 2006 Red Hat, Inc., Ingo Molnar <mingo(a)redhat.com> * - * Wound/wait implementation: + * Wait/Die implementation: * Copyright (C) 2013 Canonical Ltd. * * This file contains the main data structure and API definitions. @@ -28,9 +28,9 @@ struct ww_class { struct ww_acquire_ctx { struct task_struct *task; unsigned long stamp; - unsigned acquired; + unsigned int acquired; #ifdef CONFIG_DEBUG_MUTEXES - unsigned done_acquire; + unsigned int done_acquire; struct ww_class *ww_class; struct ww_mutex *contending_lock; #endif @@ -38,8 +38,8 @@ struct ww_acquire_ctx { struct lockdep_map dep_map; #endif #ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH - unsigned deadlock_inject_interval; - unsigned deadlock_inject_countdown; + unsigned int deadlock_inject_interval; + unsigned int deadlock_inject_countdown; #endif }; @@ -102,7 +102,7 @@ static inline void ww_mutex_init(struct ww_mutex *lock, * * Context-based w/w mutex acquiring can be done in any order whatsoever within * a given lock class. Deadlocks will be detected and handled with the - * wait/wound logic. + * wait/die logic. * * Mixing of context-based w/w mutex acquiring and single w/w mutex locking can * result in undetected deadlocks and is so forbidden. Mixing different contexts @@ -195,13 +195,13 @@ static inline void ww_acquire_fini(struct ww_acquire_ctx *ctx) * Lock the w/w mutex exclusively for this task. * * Deadlocks within a given w/w class of locks are detected and handled with the - * wait/wound algorithm. If the lock isn't immediately avaiable this function + * wait/die algorithm. If the lock isn't immediately available this function * will either sleep until it is (wait case). Or it selects the current context - * for backing off by returning -EDEADLK (wound case). Trying to acquire the + * for backing off by returning -EDEADLK (die case). Trying to acquire the * same lock with the same context twice is also detected and signalled by * returning -EALREADY. Returns 0 if the mutex was successfully acquired. * - * In the wound case the caller must release all currently held w/w mutexes for + * In the die case the caller must release all currently held w/w mutexes for * the given context and then wait for this contending lock to be available by * calling ww_mutex_lock_slow. Alternatively callers can opt to not acquire this * lock and proceed with trying to acquire further w/w mutexes (e.g. when @@ -226,14 +226,14 @@ extern int /* __must_check */ ww_mutex_lock(struct ww_mutex *lock, struct ww_acq * Lock the w/w mutex exclusively for this task. * * Deadlocks within a given w/w class of locks are detected and handled with the - * wait/wound algorithm. If the lock isn't immediately avaiable this function + * wait/die algorithm. If the lock isn't immediately available this function * will either sleep until it is (wait case). Or it selects the current context - * for backing off by returning -EDEADLK (wound case). Trying to acquire the + * for backing off by returning -EDEADLK (die case). Trying to acquire the * same lock with the same context twice is also detected and signalled by * returning -EALREADY. Returns 0 if the mutex was successfully acquired. If a * signal arrives while waiting for the lock then this function returns -EINTR. * - * In the wound case the caller must release all currently held w/w mutexes for + * In the die case the caller must release all currently held w/w mutexes for * the given context and then wait for this contending lock to be available by * calling ww_mutex_lock_slow_interruptible. Alternatively callers can opt to * not acquire this lock and proceed with trying to acquire further w/w mutexes @@ -256,7 +256,7 @@ extern int __must_check ww_mutex_lock_interruptible(struct ww_mutex *lock, * @lock: the mutex to be acquired * @ctx: w/w acquire context * - * Acquires a w/w mutex with the given context after a wound case. This function + * Acquires a w/w mutex with the given context after a die case. This function * will sleep until the lock becomes available. * * The caller must have released all w/w mutexes already acquired with the @@ -290,7 +290,7 @@ ww_mutex_lock_slow(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) * @lock: the mutex to be acquired * @ctx: w/w acquire context * - * Acquires a w/w mutex with the given context after a wound case. This function + * Acquires a w/w mutex with the given context after a die case. This function * will sleep until the lock becomes available and returns 0 when the lock has * been acquired. If a signal arrives while waiting for the lock then this * function returns -EINTR. diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index 2048359f33d2..412b4fc08235 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -243,6 +243,17 @@ void __sched mutex_lock(struct mutex *lock) EXPORT_SYMBOL(mutex_lock); #endif +/* + * Wait-Die: + * The newer transactions are killed when: + * It (the new transaction) makes a request for a lock being held + * by an older transaction. + */ + +/* + * Associate the ww_mutex @ww with the context @ww_ctx under which we acquired + * it. + */ static __always_inline void ww_mutex_lock_acquired(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx) { @@ -281,26 +292,53 @@ ww_mutex_lock_acquired(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx) DEBUG_LOCKS_WARN_ON(ww_ctx->ww_class != ww->ww_class); #endif ww_ctx->acquired++; + ww->ctx = ww_ctx; } +/* + * Determine if context @a is 'after' context @b. IOW, @a is a younger + * transaction than @b and depending on algorithm either needs to wait for + * @b or die. + */ static inline bool __sched __ww_ctx_stamp_after(struct ww_acquire_ctx *a, struct ww_acquire_ctx *b) { - return a->stamp - b->stamp <= LONG_MAX && - (a->stamp != b->stamp || a > b); + + return (signed long)(a->stamp - b->stamp) > 0; +} + +/* + * Wait-Die; wake a younger waiter context (when locks held) such that it can + * die. + * + * Among waiters with context, only the first one can have other locks acquired + * already (ctx->acquired > 0), because __ww_mutex_add_waiter() and + * __ww_mutex_check_kill() wake any but the earliest context. + */ +static bool __sched +__ww_mutex_die(struct mutex *lock, struct mutex_waiter *waiter, + struct ww_acquire_ctx *ww_ctx) +{ + if (waiter->ww_ctx->acquired > 0 && + __ww_ctx_stamp_after(waiter->ww_ctx, ww_ctx)) { + debug_mutex_wake_waiter(lock, waiter); + wake_up_process(waiter->task); + } + + return true; } /* - * Wake up any waiters that may have to back off when the lock is held by the - * given context. + * We just acquired @lock under @ww_ctx, if there are later contexts waiting + * behind us on the wait-list, check if they need to die. * - * Due to the invariants on the wait list, this can only affect the first - * waiter with a context. + * See __ww_mutex_add_waiter() for the list-order construction; basically the + * list is ordered by stamp, smallest (oldest) first. * * The current task must not be on the wait list. */ static void __sched -__ww_mutex_wakeup_for_backoff(struct mutex *lock, struct ww_acquire_ctx *ww_ctx) +__ww_mutex_check_waiters(struct mutex *lock, struct ww_acquire_ctx *ww_ctx) { struct mutex_waiter *cur; @@ -310,30 +348,23 @@ __ww_mutex_wakeup_for_backoff(struct mutex *lock, struct ww_acquire_ctx *ww_ctx) if (!cur->ww_ctx) continue; - if (cur->ww_ctx->acquired > 0 && - __ww_ctx_stamp_after(cur->ww_ctx, ww_ctx)) { - debug_mutex_wake_waiter(lock, cur); - wake_up_process(cur->task); - } - - break; + if (__ww_mutex_die(lock, cur, ww_ctx)) + break; } } /* - * After acquiring lock with fastpath or when we lost out in contested - * slowpath, set ctx and wake up any waiters so they can recheck. + * After acquiring lock with fastpath, where we do not hold wait_lock, set ctx + * and wake up any waiters so they can recheck. */ static __always_inline void ww_mutex_set_context_fastpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) { ww_mutex_lock_acquired(lock, ctx); - lock->ctx = ctx; - /* * The lock->ctx update should be visible on all cores before - * the atomic read is done, otherwise contended waiters might be + * the WAITERS check is done, otherwise contended waiters might be * missed. The contended waiters will either see ww_ctx == NULL * and keep spinning, or it will acquire wait_lock, add itself * to waiter list and sleep. @@ -347,29 +378,14 @@ ww_mutex_set_context_fastpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) return; /* - * Uh oh, we raced in fastpath, wake up everyone in this case, - * so they can see the new lock->ctx. + * Uh oh, we raced in fastpath, check if any of the waiters need to + * die. */ spin_lock(&lock->base.wait_lock); - __ww_mutex_wakeup_for_backoff(&lock->base, ctx); + __ww_mutex_check_waiters(&lock->base, ctx); spin_unlock(&lock->base.wait_lock); } -/* - * After acquiring lock in the slowpath set ctx. - * - * Unlike for the fast path, the caller ensures that waiters are woken up where - * necessary. - * - * Callers must hold the mutex wait_lock. - */ -static __always_inline void -ww_mutex_set_context_slowpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) -{ - ww_mutex_lock_acquired(lock, ctx); - lock->ctx = ctx; -} - #ifdef CONFIG_MUTEX_SPIN_ON_OWNER static inline @@ -645,37 +661,73 @@ void __sched ww_mutex_unlock(struct ww_mutex *lock) } EXPORT_SYMBOL(ww_mutex_unlock); + +static __always_inline int __sched +__ww_mutex_kill(struct mutex *lock, struct ww_acquire_ctx *ww_ctx) +{ + if (ww_ctx->acquired > 0) { +#ifdef CONFIG_DEBUG_MUTEXES + struct ww_mutex *ww; + + ww = container_of(lock, struct ww_mutex, base); + DEBUG_LOCKS_WARN_ON(ww_ctx->contending_lock); + ww_ctx->contending_lock = ww; +#endif + return -EDEADLK; + } + + return 0; +} + + +/* + * Check whether we need to kill the transaction for the current lock acquire. + * + * Wait-Die: If we're trying to acquire a lock already held by an older + * context, kill ourselves. + * + * Since __ww_mutex_add_waiter() orders the wait-list on stamp, we only have to + * look at waiters before us in the wait-list. + */ static inline int __sched -__ww_mutex_lock_check_stamp(struct mutex *lock, struct mutex_waiter *waiter, - struct ww_acquire_ctx *ctx) +__ww_mutex_check_kill(struct mutex *lock, struct mutex_waiter *waiter, + struct ww_acquire_ctx *ctx) { struct ww_mutex *ww = container_of(lock, struct ww_mutex, base); struct ww_acquire_ctx *hold_ctx = READ_ONCE(ww->ctx); struct mutex_waiter *cur; + if (ctx->acquired == 0) + return 0; + if (hold_ctx && __ww_ctx_stamp_after(ctx, hold_ctx)) - goto deadlock; + return __ww_mutex_kill(lock, ctx); /* * If there is a waiter in front of us that has a context, then its - * stamp is earlier than ours and we must back off. + * stamp is earlier than ours and we must kill ourself. */ cur = waiter; list_for_each_entry_continue_reverse(cur, &lock->wait_list, list) { - if (cur->ww_ctx) - goto deadlock; + if (!cur->ww_ctx) + continue; + + return __ww_mutex_kill(lock, ctx); } return 0; - -deadlock: -#ifdef CONFIG_DEBUG_MUTEXES - DEBUG_LOCKS_WARN_ON(ctx->contending_lock); - ctx->contending_lock = ww; -#endif - return -EDEADLK; } +/* + * Add @waiter to the wait-list, keep the wait-list ordered by stamp, smallest + * first. Such that older contexts are preferred to acquire the lock over + * younger contexts. + * + * Waiters without context are interspersed in FIFO order. + * + * Furthermore, for Wait-Die kill ourself immediately when possible (there are + * older contexts already waiting) to avoid unnecessary waiting. + */ static inline int __sched __ww_mutex_add_waiter(struct mutex_waiter *waiter, struct mutex *lock, @@ -692,7 +744,7 @@ __ww_mutex_add_waiter(struct mutex_waiter *waiter, /* * Add the waiter before the first waiter with a higher stamp. * Waiters without a context are skipped to avoid starving - * them. + * them. Wait-Die waiters may die here. */ pos = &lock->wait_list; list_for_each_entry_reverse(cur, &lock->wait_list, list) { @@ -700,34 +752,27 @@ __ww_mutex_add_waiter(struct mutex_waiter *waiter, continue; if (__ww_ctx_stamp_after(ww_ctx, cur->ww_ctx)) { - /* Back off immediately if necessary. */ - if (ww_ctx->acquired > 0) { -#ifdef CONFIG_DEBUG_MUTEXES - struct ww_mutex *ww; + /* + * Wait-Die: if we find an older context waiting, there + * is no point in queueing behind it, as we'd have to + * die the moment it would acquire the lock. + */ + int ret = __ww_mutex_kill(lock, ww_ctx); - ww = container_of(lock, struct ww_mutex, base); - DEBUG_LOCKS_WARN_ON(ww_ctx->contending_lock); - ww_ctx->contending_lock = ww; -#endif - return -EDEADLK; - } + if (ret) + return ret; break; } pos = &cur->list; - /* - * Wake up the waiter so that it gets a chance to back - * off. - */ - if (cur->ww_ctx->acquired > 0) { - debug_mutex_wake_waiter(lock, cur); - wake_up_process(cur->task); - } + /* Wait-Die: ensure younger waiters die. */ + __ww_mutex_die(lock, cur, ww_ctx); } list_add_tail(&waiter->list, pos); + return 0; } @@ -771,7 +816,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, */ if (__mutex_trylock(lock)) { if (use_ww_ctx && ww_ctx) - __ww_mutex_wakeup_for_backoff(lock, ww_ctx); + __ww_mutex_check_waiters(lock, ww_ctx); goto skip_wait; } @@ -789,10 +834,13 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, waiter.ww_ctx = MUTEX_POISON_WW_CTX; #endif } else { - /* Add in stamp order, waking up waiters that must back off. */ + /* + * Add in stamp order, waking up waiters that must kill + * themselves. + */ ret = __ww_mutex_add_waiter(&waiter, lock, ww_ctx); if (ret) - goto err_early_backoff; + goto err_early_kill; waiter.ww_ctx = ww_ctx; } @@ -814,7 +862,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, goto acquired; /* - * Check for signals and wound conditions while holding + * Check for signals and kill conditions while holding * wait_lock. This ensures the lock cancellation is ordered * against mutex_unlock() and wake-ups do not go missing. */ @@ -823,8 +871,8 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, goto err; } - if (use_ww_ctx && ww_ctx && ww_ctx->acquired > 0) { - ret = __ww_mutex_lock_check_stamp(lock, &waiter, ww_ctx); + if (use_ww_ctx && ww_ctx) { + ret = __ww_mutex_check_kill(lock, &waiter, ww_ctx); if (ret) goto err; } @@ -869,7 +917,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, lock_acquired(&lock->dep_map, ip); if (use_ww_ctx && ww_ctx) - ww_mutex_set_context_slowpath(ww, ww_ctx); + ww_mutex_lock_acquired(ww, ww_ctx); spin_unlock(&lock->wait_lock); preempt_enable(); @@ -878,7 +926,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, err: __set_current_state(TASK_RUNNING); mutex_remove_waiter(lock, &waiter, current); -err_early_backoff: +err_early_kill: spin_unlock(&lock->wait_lock); debug_mutex_free_waiter(&waiter); mutex_release(&lock->dep_map, 1, ip); -- 2.14.3

6 years, 6 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig