From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gustavo Padovan Subject: Re: [RFC 26/29] dma-buf/fence: remove pointless fence_timeline_signal at destroy phase Date: Fri, 15 Jan 2016 16:02:07 -0200 Message-ID: <20160115180207.GD2664@joana> References: <1452869739-3304-1-git-send-email-gustavo@padovan.org> <1452869739-3304-27-git-send-email-gustavo@padovan.org> <569930FF.1090601@Intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <569930FF.1090601@Intel.com> Sender: linux-kernel-owner@vger.kernel.org To: John Harrison Cc: Gustavo Padovan , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, daniels@collabora.com, Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , Maarten Lankhorst List-Id: dri-devel@lists.freedesktop.org 2016-01-15 John Harrison : > On 15/01/2016 14:55, Gustavo Padovan wrote: > >From: Gustavo Padovan > > > >All changes to timeline value come through the user via > >fence_timeline_signal() calls. When fence_timeline_destroy() is called no > >changes on timeline->value happens hence call fence_timeline_signal() with > >no increment is pointless. > > > >Signed-off-by: Gustavo Padovan > >--- > > drivers/dma-buf/fence.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > >diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c > >index 7a5fc9b..26f5f0f 100644 > >--- a/drivers/dma-buf/fence.c > >+++ b/drivers/dma-buf/fence.c > >@@ -136,7 +136,7 @@ EXPORT_SYMBOL(fence_timeline_put); > > * fence_timeline_destroy - destroy a fence_timeline > > * @timeline [in] the fence_timeline to destroy > > * > >- * This function destroys a timeline. It signals any active fence first. > >+ * This function destroys a timeline. > > The implementation for this was certainly broken but I would say it should > be fixed to match the comment rather than just abandoned completely. That > is, what happens if a timeline owner destroys their timeline while there are > outstanding fences which other drivers are waiting on? That is presumably a > bug in the code that called destroy prematurely, but bugs happen. > > The old implementation simply leaked the fences. Doing a debugfs dump would > show the timeline with all its outstanding fences still floating around > forever after. Worse, anything waiting on them would never be signalled and > is therefore potentially deadlocked. > > Note that I haven't had chance to look through the entire patch series yet > so maybe this has been fixed up elsewhere. If not, then I think it > definitely needs looking into. > Patches 27 and 28 are attempt to fix that. I assumed that if some code is calling fence_timeline_destroy() it wants to stop everything so I worked on a solution that stops any waiter and allows the timeline to be destroyed. No one is using fence_timeline_destroy() in mainline now, so it is definately a behaviour we can discuss. Gustavo