| Message ID | 20171024135552.2139-1-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017年10月24日 21:55, Christian König wrote: > From: Christian König <christian.koenig@amd.com> > > 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@amd.com> > --- > 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. 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); > } > > /**
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@amd.com> >> >> 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@amd.com> >> --- >> 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); >> } >> /** >
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@amd.com> >>> >>> 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@amd.com> >>> --- >>> 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); >>> } >>> /** >> >
Looks like v2 never made it to the list. I've just send the V2 patches once more. Christian. Am 31.10.2017 um 08:26 schrieb Chunming Zhou: > 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@amd.com> >>>> >>>> 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@amd.com> >>>> --- >>>> 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); >>>> } >>>> /** >>> >> > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 31/10/17 09:42 AM, Christian König wrote: > Looks like v2 never made it to the list. I've just send the V2 patches > once more. FWIW, I received your v2 patches yesterday, and now twice today.
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); 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); } /**