| Subject | Re: [PATCH v1 13/25] dma-buf: Use sequence counter with associated wound/wait mutex | From | Christian König <> | Date | Wed, 20 May 2020 12:48:10 +0200 |
| |
Am 19.05.20 um 23:45 schrieb Ahmed S. Darwish: > A sequence counter write side critical section must be protected by some > form of locking to serialize writers. If the serialization primitive is > not disabling preemption implicitly, preemption has to be explicitly > disabled before entering the sequence counter write side critical > section. > > The dma-buf reservation subsystem uses plain sequence counters to manage > updates to reservations. Writer serialization is accomplished through a > wound/wait mutex. > > Acquiring a wound/wait mutex does not disable preemption, so this needs > to be done manually before and after the write side critical section. > > Use the newly-added seqcount_ww_mutex_t instead: > > - It associates the ww_mutex with the sequence count, which enables > lockdep to validate that the write side critical section is properly > serialized. > > - It removes the need to explicitly add preempt_disable/enable() > around the write side critical section because the write_begin/end() > functions for this new data type automatically do this. > > If lockdep is disabled this ww_mutex lock association is compiled out > and has neither storage size nor runtime overhead.
Mhm, is the dma_resv object the only user of this new seqcount_ww_mutex variant ?
If yes we are trying to get rid of this sequence counter for quite some time, so I would rather invest the additional time to finish this.
Regards, Christian.
> > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> > --- > drivers/dma-buf/dma-resv.c | 8 +------- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 -- > include/linux/dma-resv.h | 2 +- > 3 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 590ce7ad60a0..3aba2b2bfc48 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -128,7 +128,7 @@ subsys_initcall(dma_resv_lockdep); > void dma_resv_init(struct dma_resv *obj) > { > ww_mutex_init(&obj->lock, &reservation_ww_class); > - seqcount_init(&obj->seq); > + seqcount_ww_mutex_init(&obj->seq, &obj->lock); > > RCU_INIT_POINTER(obj->fence, NULL); > RCU_INIT_POINTER(obj->fence_excl, NULL); > @@ -259,7 +259,6 @@ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) > fobj = dma_resv_get_list(obj); > count = fobj->shared_count; > > - preempt_disable(); > write_seqcount_begin(&obj->seq); > > for (i = 0; i < count; ++i) { > @@ -281,7 +280,6 @@ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) > smp_store_mb(fobj->shared_count, count); > > write_seqcount_end(&obj->seq); > - preempt_enable(); > dma_fence_put(old); > } > EXPORT_SYMBOL(dma_resv_add_shared_fence); > @@ -308,14 +306,12 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) > if (fence) > dma_fence_get(fence); > > - preempt_disable(); > write_seqcount_begin(&obj->seq); > /* write_seqcount_begin provides the necessary memory barrier */ > RCU_INIT_POINTER(obj->fence_excl, fence); > if (old) > old->shared_count = 0; > write_seqcount_end(&obj->seq); > - preempt_enable(); > > /* inplace update, no shared fences */ > while (i--) > @@ -393,13 +389,11 @@ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) > src_list = dma_resv_get_list(dst); > old = dma_resv_get_excl(dst); > > - preempt_disable(); > write_seqcount_begin(&dst->seq); > /* write_seqcount_begin provides the necessary memory barrier */ > RCU_INIT_POINTER(dst->fence_excl, new); > RCU_INIT_POINTER(dst->fence, dst_list); > write_seqcount_end(&dst->seq); > - preempt_enable(); > > dma_resv_list_free(src_list); > dma_fence_put(old); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > index 9dff792c9290..87fd32aae8f9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > @@ -258,11 +258,9 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo, > new->shared_count = k; > > /* Install the new fence list, seqcount provides the barriers */ > - preempt_disable(); > write_seqcount_begin(&resv->seq); > RCU_INIT_POINTER(resv->fence, new); > write_seqcount_end(&resv->seq); > - preempt_enable(); > > /* Drop the references to the removed fences or move them to ef_list */ > for (i = j, k = 0; i < old->shared_count; ++i) { > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > index a6538ae7d93f..d44a77e8a7e3 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -69,7 +69,7 @@ struct dma_resv_list { > */ > struct dma_resv { > struct ww_mutex lock; > - seqcount_t seq; > + seqcount_ww_mutex_t seq; > > struct dma_fence __rcu *fence_excl; > struct dma_resv_list __rcu *fence;
|