Re: [PATCH 1/2] dma-buf: make reservation_object_copy_fences rcu save

[zhoucm1 <david1.zhou@xxxxxxx>]

On 2017年09月07日 15:13, Christian König wrote:
> Ping? David can you take a look?
>
> Alex is on vacation and that is a rather important bug fix.
>
> Thanks,
> Christian.
>
> Am 04.09.2017 um 21:02 schrieb Christian König:
> > From: Christian König <christian.koenig@xxxxxxx>
> >
> > Stop requiring that the src reservation object is locked for this 
> > operation.
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > ---
> >   drivers/dma-buf/reservation.c | 56 
> > ++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 42 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma-buf/reservation.c 
> > b/drivers/dma-buf/reservation.c
> > index dec3a81..b44d9d7 100644
> > --- a/drivers/dma-buf/reservation.c
> > +++ b/drivers/dma-buf/reservation.c
> > @@ -266,8 +266,7 @@ EXPORT_SYMBOL(reservation_object_add_excl_fence);
> >   * @dst: the destination reservation object
> >   * @src: the source reservation object
> >   *
> > -* Copy all fences from src to dst. Both src->lock as well as 
> > dst-lock must be
> > -* held.
> > +* Copy all fences from src to dst. dst-lock must be held.
> >   */
> >   int reservation_object_copy_fences(struct reservation_object *dst,
> >                      struct reservation_object *src)
> > @@ -277,33 +276,62 @@ int reservation_object_copy_fences(struct 
> > reservation_object *dst,
> >       size_t size;
> >       unsigned i;
> >   -    src_list = reservation_object_get_list(src);
> > +    rcu_read_lock();
> > +    src_list = rcu_dereference(src->fence);
> >   +retry:
> >       if (src_list) {
> > -        size = offsetof(typeof(*src_list),
> > -                shared[src_list->shared_count]);
> > +        unsigned shared_count = src_list->shared_count;
> > +
> > +        size = offsetof(typeof(*src_list), shared[shared_count]);
> > +        rcu_read_unlock();
> > +
> >           dst_list = kmalloc(size, GFP_KERNEL);
> >           if (!dst_list)
> >               return -ENOMEM;
> >   -        dst_list->shared_count = src_list->shared_count;
> > -        dst_list->shared_max = src_list->shared_count;
> > -        for (i = 0; i < src_list->shared_count; ++i)
> > -            dst_list->shared[i] =
> > -                dma_fence_get(src_list->shared[i]);
> > +        rcu_read_lock();
> > +        src_list = rcu_dereference(src->fence);
> > +        if (!src_list || src_list->shared_count > shared_count) {
> > +            kfree(dst_list);
> > +            goto retry;
> > +        }
> > +
> > +        dst_list->shared_count = 0;
> > +        dst_list->shared_max = shared_count;
> > +        for (i = 0; i < src_list->shared_count; ++i) {
> > +            struct dma_fence *fence;
> > +
> > +            fence = rcu_dereference(src_list->shared[i]);
> > +            if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > +                     &fence->flags))
seems here is duplicated with the below dma_fence_is_signaled, can it be 
removed?

And I'm not sure the locking, but it looks good, so Acked-by: Chunming 
Zhou <david1.zhou@xxxxxxx>
> > +                continue;
> > +
> > +            if (!dma_fence_get_rcu(fence)) {
> > +                kfree(dst_list);
> > +                src_list = rcu_dereference(src->fence);
> > +                goto retry;
> > +            }
> > +
> > +            if (dma_fence_is_signaled(fence)) {
> > +                dma_fence_put(fence);
> > +                continue;
> > +            }
> > +
> > +            dst_list->shared[dst_list->shared_count++] = fence;
> > +        }
> >       } else {
> >           dst_list = NULL;
> >       }
> >   +    new = dma_fence_get_rcu_safe(&src->fence_excl);
> > +    rcu_read_unlock();
> > +
> >       kfree(dst->staged);
> >       dst->staged = NULL;
> >         src_list = reservation_object_get_list(dst);
> > -
> >       old = reservation_object_get_excl(dst);
> > -    new = reservation_object_get_excl(src);
> > -
> > -    dma_fence_get(new);
> >         preempt_disable();
> >       write_seqcount_begin(&dst->seq);

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel