| Message ID | 20210917123513.1106-4-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2 | expand |
On Fri, Sep 17, 2021 at 02:34:50PM +0200, Christian König wrote: > This makes the function much simpler since the complex > retry logic is now handled else where. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-resv.c | 86 ++++++++++++++++---------------------- > 1 file changed, 35 insertions(+), 51 deletions(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index a3c79a99fb44..406150dea5e4 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -426,74 +426,58 @@ EXPORT_SYMBOL_GPL(dma_resv_iter_walk); > */ > int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) > { > - struct dma_resv_list *src_list, *dst_list; > - struct dma_fence *old, *new; > - unsigned int i; > + struct dma_resv_iter cursor; > + struct dma_resv_list *list; > + struct dma_fence *f, *excl; > > dma_resv_assert_held(dst); > > - rcu_read_lock(); > - src_list = dma_resv_shared_list(src); > - > -retry: > - if (src_list) { > - unsigned int shared_count = src_list->shared_count; > - > - rcu_read_unlock(); > + list = NULL; > + excl = NULL; > > - dst_list = dma_resv_list_alloc(shared_count); > - if (!dst_list) > - return -ENOMEM; > + rcu_read_lock(); > + dma_resv_iter_begin(&cursor, src, true); > + dma_resv_for_each_fence_unlocked(&cursor, f) { > > - rcu_read_lock(); > - src_list = dma_resv_shared_list(src); > - if (!src_list || src_list->shared_count > shared_count) { > - kfree(dst_list); > - goto retry; > - } > + if (cursor.is_first) { Maybe have a wrapper for this, like dma_resv_iter_is_reset or is_first or is_restart (my preference) with some nice docs that this returns true everytime we had to restart the sequence? Otherwise I fully agree, this is so much better with all the hairy restarting and get_rcu and test_bit shovelled away somewhere. Either way (but I much prefer a wrapper for is_first): Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + dma_resv_list_free(list); > + dma_fence_put(excl); > > - dst_list->shared_count = 0; > - for (i = 0; i < src_list->shared_count; ++i) { > - struct dma_fence __rcu **dst; > - struct dma_fence *fence; > + if (cursor.fences) { > + unsigned int cnt = cursor.fences->shared_count; > > - fence = rcu_dereference(src_list->shared[i]); > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > - &fence->flags)) > - continue; > + rcu_read_unlock(); > + list = dma_resv_list_alloc(cnt); > + if (!list) { > + dma_resv_iter_end(&cursor); > + return -ENOMEM; > + } > > - if (!dma_fence_get_rcu(fence)) { > - dma_resv_list_free(dst_list); > - src_list = dma_resv_shared_list(src); > - goto retry; > - } > + list->shared_count = 0; > + rcu_read_lock(); > > - if (dma_fence_is_signaled(fence)) { > - dma_fence_put(fence); > - continue; > + } else { > + list = NULL; > } > - > - dst = &dst_list->shared[dst_list->shared_count++]; > - rcu_assign_pointer(*dst, fence); > + excl = NULL; > } > - } else { > - dst_list = NULL; > - } > > - new = dma_fence_get_rcu_safe(&src->fence_excl); > + dma_fence_get(f); > + if (dma_resv_iter_is_exclusive(&cursor)) > + excl = f; > + else > + RCU_INIT_POINTER(list->shared[list->shared_count++], f); > + } > + dma_resv_iter_end(&cursor); > rcu_read_unlock(); > > - src_list = dma_resv_shared_list(dst); > - old = dma_resv_excl_fence(dst); > - > 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); > + excl = rcu_replace_pointer(dst->fence_excl, excl, dma_resv_held(dst)); > + list = rcu_replace_pointer(dst->fence, list, dma_resv_held(dst)); > write_seqcount_end(&dst->seq); > > - dma_resv_list_free(src_list); > - dma_fence_put(old); > + dma_resv_list_free(list); > + dma_fence_put(excl); > > return 0; > } > -- > 2.25.1 >
Am 17.09.21 um 16:35 schrieb Daniel Vetter: > On Fri, Sep 17, 2021 at 02:34:50PM +0200, Christian König wrote: >> This makes the function much simpler since the complex >> retry logic is now handled else where. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/dma-resv.c | 86 ++++++++++++++++---------------------- >> 1 file changed, 35 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >> index a3c79a99fb44..406150dea5e4 100644 >> --- a/drivers/dma-buf/dma-resv.c >> +++ b/drivers/dma-buf/dma-resv.c >> @@ -426,74 +426,58 @@ EXPORT_SYMBOL_GPL(dma_resv_iter_walk); >> */ >> int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) >> { >> - struct dma_resv_list *src_list, *dst_list; >> - struct dma_fence *old, *new; >> - unsigned int i; >> + struct dma_resv_iter cursor; >> + struct dma_resv_list *list; >> + struct dma_fence *f, *excl; >> >> dma_resv_assert_held(dst); >> >> - rcu_read_lock(); >> - src_list = dma_resv_shared_list(src); >> - >> -retry: >> - if (src_list) { >> - unsigned int shared_count = src_list->shared_count; >> - >> - rcu_read_unlock(); >> + list = NULL; >> + excl = NULL; >> >> - dst_list = dma_resv_list_alloc(shared_count); >> - if (!dst_list) >> - return -ENOMEM; >> + rcu_read_lock(); >> + dma_resv_iter_begin(&cursor, src, true); >> + dma_resv_for_each_fence_unlocked(&cursor, f) { >> >> - rcu_read_lock(); >> - src_list = dma_resv_shared_list(src); >> - if (!src_list || src_list->shared_count > shared_count) { >> - kfree(dst_list); >> - goto retry; >> - } >> + if (cursor.is_first) { > Maybe have a wrapper for this, like dma_resv_iter_is_reset or is_first or > is_restart (my preference) with some nice docs that this returns true > everytime we had to restart the sequence? Exactly that's what I wanted to avoid since the is_first (or whatever) function should only be used inside of the dma_resv.c code. On the other hand I can just make that static here and document that it should never be exported. Christian. > > Otherwise I fully agree, this is so much better with all the hairy > restarting and get_rcu and test_bit shovelled away somewhere. > > Either way (but I much prefer a wrapper for is_first): > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> + dma_resv_list_free(list); >> + dma_fence_put(excl); >> >> - dst_list->shared_count = 0; >> - for (i = 0; i < src_list->shared_count; ++i) { >> - struct dma_fence __rcu **dst; >> - struct dma_fence *fence; >> + if (cursor.fences) { >> + unsigned int cnt = cursor.fences->shared_count; >> >> - fence = rcu_dereference(src_list->shared[i]); >> - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >> - &fence->flags)) >> - continue; >> + rcu_read_unlock(); >> + list = dma_resv_list_alloc(cnt); >> + if (!list) { >> + dma_resv_iter_end(&cursor); >> + return -ENOMEM; >> + } >> >> - if (!dma_fence_get_rcu(fence)) { >> - dma_resv_list_free(dst_list); >> - src_list = dma_resv_shared_list(src); >> - goto retry; >> - } >> + list->shared_count = 0; >> + rcu_read_lock(); >> >> - if (dma_fence_is_signaled(fence)) { >> - dma_fence_put(fence); >> - continue; >> + } else { >> + list = NULL; >> } >> - >> - dst = &dst_list->shared[dst_list->shared_count++]; >> - rcu_assign_pointer(*dst, fence); >> + excl = NULL; >> } >> - } else { >> - dst_list = NULL; >> - } >> >> - new = dma_fence_get_rcu_safe(&src->fence_excl); >> + dma_fence_get(f); >> + if (dma_resv_iter_is_exclusive(&cursor)) >> + excl = f; >> + else >> + RCU_INIT_POINTER(list->shared[list->shared_count++], f); >> + } >> + dma_resv_iter_end(&cursor); >> rcu_read_unlock(); >> >> - src_list = dma_resv_shared_list(dst); >> - old = dma_resv_excl_fence(dst); >> - >> 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); >> + excl = rcu_replace_pointer(dst->fence_excl, excl, dma_resv_held(dst)); >> + list = rcu_replace_pointer(dst->fence, list, dma_resv_held(dst)); >> write_seqcount_end(&dst->seq); >> >> - dma_resv_list_free(src_list); >> - dma_fence_put(old); >> + dma_resv_list_free(list); >> + dma_fence_put(excl); >> >> return 0; >> } >> -- >> 2.25.1 >>
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index a3c79a99fb44..406150dea5e4 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -426,74 +426,58 @@ EXPORT_SYMBOL_GPL(dma_resv_iter_walk); */ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) { - struct dma_resv_list *src_list, *dst_list; - struct dma_fence *old, *new; - unsigned int i; + struct dma_resv_iter cursor; + struct dma_resv_list *list; + struct dma_fence *f, *excl; dma_resv_assert_held(dst); - rcu_read_lock(); - src_list = dma_resv_shared_list(src); - -retry: - if (src_list) { - unsigned int shared_count = src_list->shared_count; - - rcu_read_unlock(); + list = NULL; + excl = NULL; - dst_list = dma_resv_list_alloc(shared_count); - if (!dst_list) - return -ENOMEM; + rcu_read_lock(); + dma_resv_iter_begin(&cursor, src, true); + dma_resv_for_each_fence_unlocked(&cursor, f) { - rcu_read_lock(); - src_list = dma_resv_shared_list(src); - if (!src_list || src_list->shared_count > shared_count) { - kfree(dst_list); - goto retry; - } + if (cursor.is_first) { + dma_resv_list_free(list); + dma_fence_put(excl); - dst_list->shared_count = 0; - for (i = 0; i < src_list->shared_count; ++i) { - struct dma_fence __rcu **dst; - struct dma_fence *fence; + if (cursor.fences) { + unsigned int cnt = cursor.fences->shared_count; - fence = rcu_dereference(src_list->shared[i]); - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, - &fence->flags)) - continue; + rcu_read_unlock(); + list = dma_resv_list_alloc(cnt); + if (!list) { + dma_resv_iter_end(&cursor); + return -ENOMEM; + } - if (!dma_fence_get_rcu(fence)) { - dma_resv_list_free(dst_list); - src_list = dma_resv_shared_list(src); - goto retry; - } + list->shared_count = 0; + rcu_read_lock(); - if (dma_fence_is_signaled(fence)) { - dma_fence_put(fence); - continue; + } else { + list = NULL; } - - dst = &dst_list->shared[dst_list->shared_count++]; - rcu_assign_pointer(*dst, fence); + excl = NULL; } - } else { - dst_list = NULL; - } - new = dma_fence_get_rcu_safe(&src->fence_excl); + dma_fence_get(f); + if (dma_resv_iter_is_exclusive(&cursor)) + excl = f; + else + RCU_INIT_POINTER(list->shared[list->shared_count++], f); + } + dma_resv_iter_end(&cursor); rcu_read_unlock(); - src_list = dma_resv_shared_list(dst); - old = dma_resv_excl_fence(dst); - 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); + excl = rcu_replace_pointer(dst->fence_excl, excl, dma_resv_held(dst)); + list = rcu_replace_pointer(dst->fence, list, dma_resv_held(dst)); write_seqcount_end(&dst->seq); - dma_resv_list_free(src_list); - dma_fence_put(old); + dma_resv_list_free(list); + dma_fence_put(excl); return 0; }
This makes the function much simpler since the complex retry logic is now handled else where. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 86 ++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 51 deletions(-)