Re: [PATCH 1/2] dma-buf: keep only not signaled fence in reservation_object_add_shared_replace

[Chunming Zhou <zhoucm1@xxxxxxx>]

Any update?


On 2017年10月25日 15:28, Christian König wrote:
> Am 25.10.2017 um 08:42 schrieb Chunming Zhou:
> > On 2017年10月24日 21:55, Christian König wrote:
> > > From: Christian König <christian.koenig@xxxxxxx>
> > >
> > > The amdgpu issue to also need signaled fences in the reservation 
> > > objects
> > > should be fixed by now.
> > >
> > > Optimize the list by keeping only the not signaled yet fences around.
> > >
> > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > ---
> > >   drivers/dma-buf/reservation.c | 31 +++++++++++++++++++++----------
> > >   1 file changed, 21 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/reservation.c 
> > > b/drivers/dma-buf/reservation.c
> > > index b44d9d7db347..4ede77d1bb31 100644
> > > --- a/drivers/dma-buf/reservation.c
> > > +++ b/drivers/dma-buf/reservation.c
> > > @@ -145,8 +145,8 @@ reservation_object_add_shared_replace(struct 
> > > reservation_object *obj,
> > >                         struct reservation_object_list *fobj,
> > >                         struct dma_fence *fence)
> > >   {
> > > -    unsigned i;
> > >       struct dma_fence *old_fence = NULL;
> > > +    unsigned i, j, k;
> > >         dma_fence_get(fence);
> > >   @@ -162,9 +162,7 @@ reservation_object_add_shared_replace(struct 
> > > reservation_object *obj,
> > >        * references from the old struct are carried over to
> > >        * the new.
> > >        */
> > > -    fobj->shared_count = old->shared_count;
> > > -
> > > -    for (i = 0; i < old->shared_count; ++i) {
> > > +    for (i = 0, j = 0, k = old->shared_count; i < 
> > > old->shared_count; ++i) {
> > >           struct dma_fence *check;
> > >             check = rcu_dereference_protected(old->shared[i],
> > > @@ -172,10 +170,14 @@ reservation_object_add_shared_replace(struct 
> > > reservation_object *obj,
> > >             if (!old_fence && check->context == fence->context) {
> > >               old_fence = check;
> > > -            RCU_INIT_POINTER(fobj->shared[i], fence);
> > > -        } else
> > > -            RCU_INIT_POINTER(fobj->shared[i], check);
> > > +            RCU_INIT_POINTER(fobj->shared[j++], fence);
> > > +        } else if (!dma_fence_is_signaled(check)) {
> > > +            RCU_INIT_POINTER(fobj->shared[j++], check);
> > > +        } else {
> > > +            RCU_INIT_POINTER(fobj->shared[--k], check);
> > > +        }
> > >       }
> > > +    fobj->shared_count = j;
> > >       if (!old_fence) {
> > > RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence);
> > Here there is a memory leak for signaled fence slots, since you 
> > re-order the slots, the j'th slot is storing signaled fence, there is 
> > no place to put it when you assign to new one.
> > you cam move it to end or put it here first.
>
> Good point, thanks. Going to respin.
>
> Regards,
> Christian.
>
> > Regards,
> > David Zhou
> > >           fobj->shared_count++;
> > > @@ -192,10 +194,19 @@ reservation_object_add_shared_replace(struct 
> > > reservation_object *obj,
> > >       write_seqcount_end(&obj->seq);
> > >       preempt_enable();
> > >   -    if (old)
> > > -        kfree_rcu(old, rcu);
> > > -
> > >       dma_fence_put(old_fence);
> > > +
> > > +    if (!old)
> > > +        return;
> > > +
> > > +    for (i = fobj->shared_count; i < old->shared_count; ++i) {
> > > +        struct dma_fence *f;
> > > +
> > > +        f = rcu_dereference_protected(fobj->shared[i],
> > > +                          reservation_object_held(obj));
> > > +        dma_fence_put(f);
> > > +    }
> > > +    kfree_rcu(old, rcu);
> > >   }
> > >     /**

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