| Message ID | 1352951574-19228-1-git-send-email-sw0312.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi All, This patch has been tested with only Exynos driver so other maintainers may need to test this. Acked-by: Inki Dae <inki.dae@samsung.com> 2012/11/15 Seung-Woo Kim <sw0312.kim@samsung.com> > Increasing ref counts of both dma-buf and gem for imported dma-buf > come from gem makes memory leak. release function of dma-buf cannot > be called because f_count of dma-buf increased by importing gem and > gem ref count cannot be decrease because of exported dma-buf. > > So I add dma_buf_put() for imported gem come from its own gem into > each drivers having prime_import and prime_export capabilities. With > this, only gem ref count is increased if importing gem exported from > gem of same driver. > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Rob Clark <rob.clark@linaro.org> > Cc: Alex Deucher <alexander.deucher@amd.com> > --- > Mmap failiure in i915 was reported by Jani, and I think it was fixed > with Dave's commit "drm/prime: add exported buffers to current fprivs > imported buffer list (v2)", so I resend this patch. > > I can send exynos only, but this issue is common in all drm driver > having both prime inport and export, IMHO, it's better to send for > all drivers. > > drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 5 +++++ > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 5 +++++ > drivers/gpu/drm/nouveau/nouveau_prime.c | 1 + > drivers/gpu/drm/radeon/radeon_prime.c | 1 + > drivers/staging/omapdrm/omap_gem_dmabuf.c | 5 +++++ > 5 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > index ae13feb..b0897c9 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > @@ -211,7 +211,12 @@ struct drm_gem_object > *exynos_dmabuf_prime_import(struct drm_device *drm_dev, > > /* is it from our device? */ > if (obj->dev == drm_dev) { > + /* > + * Importing dmabuf exported from out own gem > increases > + * refcount on gem itself instead of f_count of > dmabuf. > + */ > drm_gem_object_reference(obj); > + dma_buf_put(dma_buf); > return obj; > } > } > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index aa308e1..32e6287 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct > drm_device *dev, > obj = dma_buf->priv; > /* is it from our device? */ > if (obj->base.dev == dev) { > + /* > + * Importing dmabuf exported from out own gem > increases > + * refcount on gem itself instead of f_count of > dmabuf. > + */ > drm_gem_object_reference(&obj->base); > + dma_buf_put(dma_buf); > return &obj->base; > } > } > diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c > b/drivers/gpu/drm/nouveau/nouveau_prime.c > index a25cf2c..bb653c6 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct > drm_device *dev, > if (nvbo->gem) { > if (nvbo->gem->dev == dev) { > drm_gem_object_reference(nvbo->gem); > + dma_buf_put(dma_buf); > return nvbo->gem; > } > } > diff --git a/drivers/gpu/drm/radeon/radeon_prime.c > b/drivers/gpu/drm/radeon/radeon_prime.c > index 6bef46a..d344a3be 100644 > --- a/drivers/gpu/drm/radeon/radeon_prime.c > +++ b/drivers/gpu/drm/radeon/radeon_prime.c > @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct > drm_device *dev, > bo = dma_buf->priv; > if (bo->gem_base.dev == dev) { > drm_gem_object_reference(&bo->gem_base); > + dma_buf_put(dma_buf); > return &bo->gem_base; > } > } > diff --git a/drivers/staging/omapdrm/omap_gem_dmabuf.c > b/drivers/staging/omapdrm/omap_gem_dmabuf.c > index 42728e0..5b50eb6 100644 > --- a/drivers/staging/omapdrm/omap_gem_dmabuf.c > +++ b/drivers/staging/omapdrm/omap_gem_dmabuf.c > @@ -207,7 +207,12 @@ struct drm_gem_object * omap_gem_prime_import(struct > drm_device *dev, > obj = buffer->priv; > /* is it from our device? */ > if (obj->dev == dev) { > + /* > + * Importing dmabuf exported from out own gem > increases > + * refcount on gem itself instead of f_count of > dmabuf. > + */ > drm_gem_object_reference(obj); > + dma_buf_put(buffer); > return obj; > } > } > -- > 1.7.4.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
Op 15-11-12 04:52, Seung-Woo Kim schreef: > Increasing ref counts of both dma-buf and gem for imported dma-buf > come from gem makes memory leak. release function of dma-buf cannot > be called because f_count of dma-buf increased by importing gem and > gem ref count cannot be decrease because of exported dma-buf. > > So I add dma_buf_put() for imported gem come from its own gem into > each drivers having prime_import and prime_export capabilities. With > this, only gem ref count is increased if importing gem exported from > gem of same driver. > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Rob Clark <rob.clark@linaro.org> > Cc: Alex Deucher <alexander.deucher@amd.com> > --- > Mmap failiure in i915 was reported by Jani, and I think it was fixed > with Dave's commit "drm/prime: add exported buffers to current fprivs > imported buffer list (v2)", so I resend this patch. > > I can send exynos only, but this issue is common in all drm driver > having both prime inport and export, IMHO, it's better to send for > all drivers. I fear that this won't solve the issue completely and keeps open a small race. I don't like the current destruction path either. It really looks like export_dma_buf should be unset when the exported buffer is closed in the original file. Anything else is racy. Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics. So to me it seems instead we should do something like this: - dmabuf_release callback is a noop, no ref is held by the dma-buf. - attach and detach ops increase reference by 1*. - when the buffer is exported, export_dma_buf is set by core code and kept around alive until the gem object is closed. - dma_buf_put is not called by import function. This reference removed as part of gem object close instead. ~Maarten *) Lockdep will rightfully hate this as it reopens a locking inversion, some solution for that is needed. Maybe a post detach callback for dma-buf with all locks dropped would be best here? Other way would be to schedule delayed work with the sole purpose of unreffing in the detach op, similar to how atomic_dec_and_mutex_lock works.
Hi Maarten, On 2012? 11? 19? 19:27, Maarten Lankhorst wrote: > Op 15-11-12 04:52, Seung-Woo Kim schreef: >> Increasing ref counts of both dma-buf and gem for imported dma-buf >> come from gem makes memory leak. release function of dma-buf cannot >> be called because f_count of dma-buf increased by importing gem and >> gem ref count cannot be decrease because of exported dma-buf. >> >> So I add dma_buf_put() for imported gem come from its own gem into >> each drivers having prime_import and prime_export capabilities. With >> this, only gem ref count is increased if importing gem exported from >> gem of same driver. >> >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >> Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com> >> Cc: Inki Dae <inki.dae@samsung.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Rob Clark <rob.clark@linaro.org> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> --- >> Mmap failiure in i915 was reported by Jani, and I think it was fixed >> with Dave's commit "drm/prime: add exported buffers to current fprivs >> imported buffer list (v2)", so I resend this patch. >> >> I can send exynos only, but this issue is common in all drm driver >> having both prime inport and export, IMHO, it's better to send for >> all drivers. > I fear that this won't solve the issue completely and keeps open a small race. I don't believe my patch can fix all issue related with gem prime completely. But it seems that it can solve unrecoverable memory leak caused by re-importing GEM. Anyway, can you give me an example of other race issue? > > I don't like the current destruction path either. It really looks like export_dma_buf > should be unset when the exported buffer is closed in the original file. Anything else is racy. > Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics. I cannot figure out all aspect of delayed fput, but until delayed work is called, dma_buf is not released so export_dma_buf is valid. Considering this, I can't find possible issue of delayed fput. > > So to me it seems instead we should do something like this: > > - dmabuf_release callback is a noop, no ref is held by the dma-buf. > - attach and detach ops increase reference by 1*. > > - when the buffer is exported, export_dma_buf is set by core code and kept around > alive until the gem object is closed. > > - dma_buf_put is not called by import function. This reference removed as part > of gem object close instead. Considering re-import, where drm file priv is different from exporter's file priv, attach and detach are not called because gem object is reused. How do you think remove checking whether dma_buf is came from driver own exporter? Because without this checking, gem object is newly created, and then re-import issue will disappear. Thanks and Regards, - Seung-Woo Kim > > ~Maarten > > *) Lockdep will rightfully hate this as it reopens a locking inversion, some solution for that is needed. > > Maybe a post detach callback for dma-buf with all locks dropped would be best here? > Other way would be to schedule delayed work with the sole purpose of unreffing in the detach op, > similar to how atomic_dec_and_mutex_lock works. > >
Op 20-11-12 02:03, ??? schreef: > Hi Maarten, > > On 2012? 11? 19? 19:27, Maarten Lankhorst wrote: >> Op 15-11-12 04:52, Seung-Woo Kim schreef: >>> Increasing ref counts of both dma-buf and gem for imported dma-buf >>> come from gem makes memory leak. release function of dma-buf cannot >>> be called because f_count of dma-buf increased by importing gem and >>> gem ref count cannot be decrease because of exported dma-buf. >>> >>> So I add dma_buf_put() for imported gem come from its own gem into >>> each drivers having prime_import and prime_export capabilities. With >>> this, only gem ref count is increased if importing gem exported from >>> gem of same driver. >>> >>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >>> Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com> >>> Cc: Inki Dae <inki.dae@samsung.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Rob Clark <rob.clark@linaro.org> >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> --- >>> Mmap failiure in i915 was reported by Jani, and I think it was fixed >>> with Dave's commit "drm/prime: add exported buffers to current fprivs >>> imported buffer list (v2)", so I resend this patch. >>> >>> I can send exynos only, but this issue is common in all drm driver >>> having both prime inport and export, IMHO, it's better to send for >>> all drivers. >> I fear that this won't solve the issue completely and keeps open a small race. > I don't believe my patch can fix all issue related with gem prime > completely. But it seems that it can solve unrecoverable memory leak > caused by re-importing GEM. Anyway, can you give me an example of other > race issue? When the dma_buf is unreffed and refcount drops to 0, work is queued to free it. Until then export_dma_buf member points to something, but refcount is 0 on it. Importing to itself, then closing fd then re-export from the new place would probably trigger it. >> I don't like the current destruction path either. It really looks like export_dma_buf >> should be unset when the exported buffer is closed in the original file. Anything else is racy. >> Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics. > I cannot figure out all aspect of delayed fput, but until delayed work > is called, dma_buf is not released so export_dma_buf is valid. > Considering this, I can't find possible issue of delayed fput. I'm fairly sure that when refcount drops to 0, even though the memory may not be freed yet, you no longer have a guarantee that the memory is still valid. And of course after importing into a different process with its own drm namespace, how does file_priv->prime.lock still protect serialization? I don't see any hierarchy either. export_dma_buf needs to be set to NULL before the dma_buf_put is called that is used for the reference inside export_dma_buf. The release function should only release a reference to whatever backing memory is used. >> So to me it seems instead we should do something like this: >> >> - dmabuf_release callback is a noop, no ref is held by the dma-buf. >> - attach and detach ops increase reference by 1*. >> >> - when the buffer is exported, export_dma_buf is set by core code and kept around >> alive until the gem object is closed. >> >> - dma_buf_put is not called by import function. This reference removed as part >> of gem object close instead. > Considering re-import, where drm file priv is different from exporter's > file priv, attach and detach are not called because gem object is reused. > > How do you think remove checking whether dma_buf is came from driver own > exporter? Because without this checking, gem object is newly created, > and then re-import issue will disappear. The whole refcounting is a circular mess, sadly with no easy solution. :/ It seems to me that using gem reference counts is solving this problem at the wrong layer. A secondary type of reference count is needed for the underlying memory backing. For those who use ttm, I would recommend that the dma_buf increases refcount on ttm_bo by one, so that the gem object can be destroyed and release its reference on the dma_buf. WIthout a secondary refcount you end up in a position where you can't reliably and race free unref the gem object and dma_buf at the same time, since they're both independent interfaces with possibly different lifetimes. It would really help if there were hard rules on lifetime of the export_dma_buf member, until then we're just patching one broken thing with another. ~Maarten
On 2012? 11? 20? 19:26, Maarten Lankhorst wrote: > Op 20-11-12 02:03, ??? schreef: >> Hi Maarten, >> >> On 2012? 11? 19? 19:27, Maarten Lankhorst wrote: >>> Op 15-11-12 04:52, Seung-Woo Kim schreef: >>>> Increasing ref counts of both dma-buf and gem for imported dma-buf >>>> come from gem makes memory leak. release function of dma-buf cannot >>>> be called because f_count of dma-buf increased by importing gem and >>>> gem ref count cannot be decrease because of exported dma-buf. >>>> >>>> So I add dma_buf_put() for imported gem come from its own gem into >>>> each drivers having prime_import and prime_export capabilities. With >>>> this, only gem ref count is increased if importing gem exported from >>>> gem of same driver. >>>> >>>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >>>> Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com> >>>> Cc: Inki Dae <inki.dae@samsung.com> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Cc: Rob Clark <rob.clark@linaro.org> >>>> Cc: Alex Deucher <alexander.deucher@amd.com> >>>> --- >>>> Mmap failiure in i915 was reported by Jani, and I think it was fixed >>>> with Dave's commit "drm/prime: add exported buffers to current fprivs >>>> imported buffer list (v2)", so I resend this patch. >>>> >>>> I can send exynos only, but this issue is common in all drm driver >>>> having both prime inport and export, IMHO, it's better to send for >>>> all drivers. >>> I fear that this won't solve the issue completely and keeps open a small race. >> I don't believe my patch can fix all issue related with gem prime >> completely. But it seems that it can solve unrecoverable memory leak >> caused by re-importing GEM. Anyway, can you give me an example of other >> race issue? > When the dma_buf is unreffed and refcount drops to 0, work is queued to free it. > > Until then export_dma_buf member points to something, but refcount is 0 on it. > > Importing to itself, then closing fd then re-export from the new place would probably trigger it. > Ok, I understood about issue from delayed fput also in your below comment. Anyway, IMO, it is already on current drm prime. >>> I don't like the current destruction path either. It really looks like export_dma_buf >>> should be unset when the exported buffer is closed in the original file. Anything else is racy. >>> Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics. >> I cannot figure out all aspect of delayed fput, but until delayed work >> is called, dma_buf is not released so export_dma_buf is valid. >> Considering this, I can't find possible issue of delayed fput. > I'm fairly sure that when refcount drops to 0, even though the memory may not be freed yet, > you no longer have a guarantee that the memory is still valid. I missed that delayed fput is triggered after recount drops to 0, and now I understood it can cause invalid access. > > And of course after importing into a different process with its own drm namespace, how does > file_priv->prime.lock still protect serialization? > > I don't see any hierarchy either. export_dma_buf needs to be set to NULL before the dma_buf_put > is called that is used for the reference inside export_dma_buf. > > The release function should only release a reference to whatever backing memory is used. > >>> So to me it seems instead we should do something like this: >>> >>> - dmabuf_release callback is a noop, no ref is held by the dma-buf. >>> - attach and detach ops increase reference by 1*. >>> >>> - when the buffer is exported, export_dma_buf is set by core code and kept around >>> alive until the gem object is closed. >>> >>> - dma_buf_put is not called by import function. This reference removed as part >>> of gem object close instead. >> Considering re-import, where drm file priv is different from exporter's >> file priv, attach and detach are not called because gem object is reused. >> >> How do you think remove checking whether dma_buf is came from driver own >> exporter? Because without this checking, gem object is newly created, >> and then re-import issue will disappear. > The whole refcounting is a circular mess, sadly with no easy solution. :/ > > It seems to me that using gem reference counts is solving this problem at the wrong layer. > A secondary type of reference count is needed for the underlying memory backing. > > For those who use ttm, I would recommend that the dma_buf increases refcount on ttm_bo by one, > so that the gem object can be destroyed and release its reference on the dma_buf. > > WIthout a secondary refcount you end up in a position where you can't reliably and race free unref > the gem object and dma_buf at the same time, since they're both independent interfaces with possibly > different lifetimes. If there is no checking whether dma_buf is came from driver own exporter, gem_object is newly allocated and refcount of it can be a secondary refcount at least form mermoy leak issue. So I mentioned about removing check for exporter. But I prefer processing re-import as gem_open without considering dma-buf as current implementation. > > It would really help if there were hard rules on lifetime of the export_dma_buf member, > until then we're just patching one broken thing with another. Issue about memory leak of re-import was also reported on i915 and there was Rob's patch set, but other locking issue blocked the patch. I'm not sure all issue can be solved at once and someone is handling this at the moment. Best Regards, - Seung-Woo Kim > > > ~Maarten > >
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index ae13feb..b0897c9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -211,7 +211,12 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev, /* is it from our device? */ if (obj->dev == drm_dev) { + /* + * Importing dmabuf exported from out own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ drm_gem_object_reference(obj); + dma_buf_put(dma_buf); return obj; } } diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index aa308e1..32e6287 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, obj = dma_buf->priv; /* is it from our device? */ if (obj->base.dev == dev) { + /* + * Importing dmabuf exported from out own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ drm_gem_object_reference(&obj->base); + dma_buf_put(dma_buf); return &obj->base; } } diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index a25cf2c..bb653c6 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev, if (nvbo->gem) { if (nvbo->gem->dev == dev) { drm_gem_object_reference(nvbo->gem); + dma_buf_put(dma_buf); return nvbo->gem; } } diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 6bef46a..d344a3be 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev, bo = dma_buf->priv; if (bo->gem_base.dev == dev) { drm_gem_object_reference(&bo->gem_base); + dma_buf_put(dma_buf); return &bo->gem_base; } } diff --git a/drivers/staging/omapdrm/omap_gem_dmabuf.c b/drivers/staging/omapdrm/omap_gem_dmabuf.c index 42728e0..5b50eb6 100644 --- a/drivers/staging/omapdrm/omap_gem_dmabuf.c +++ b/drivers/staging/omapdrm/omap_gem_dmabuf.c @@ -207,7 +207,12 @@ struct drm_gem_object * omap_gem_prime_import(struct drm_device *dev, obj = buffer->priv; /* is it from our device? */ if (obj->dev == dev) { + /* + * Importing dmabuf exported from out own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ drm_gem_object_reference(obj); + dma_buf_put(buffer); return obj; } }