Broken locking in ttm_bo_swapout

Broken locking in ttm_bo_swapout

[Matthew Bullock]

I have a fairly simple opengl application running on nouveau. It 
effectively just plots a couple of very large images that change regularly.

I've been getting an easily repeatable failure, hitting the
BUG_ON(in_interrupt) in vfree() that is called during ttm_shrink_work().
Obviously vfree() is being called with a lock held...

Looking at the locking up through the stack trace, there's an obvious 
mistake:

There should be a:
spin_lock(&glob->lru_lock);
before the continue in ttm_bo_swapout() in drivers/gpu/drm/ttm/ttm_bo.c

--- ttm_bo.c~   2011-01-05 00:50:19.000000000 +0000
+++ ttm_bo.c    2011-01-20 18:11:57.262813765 +0000
@@ -1761,6 +1761,7 @@ static int ttm_bo_swapout(struct ttm_mem
                         spin_unlock(&glob->lru_lock);
                         (void) ttm_bo_cleanup_refs(bo, false, false, 
false);
                         kref_put(&bo->list_kref, ttm_bo_release_list);
+                       spin_lock(&glob->lru_lock);
                         continue;
                 }

That fixes the problem for me.

Matthew

Re: Broken locking in ttm_bo_swapout

[Maarten Maathuis]

On Thu, Jan 20, 2011 at 7:53 PM, Matthew Bullock
<matthew.bullock@tadpole.com> wrote:
> I have a fairly simple opengl application running on nouveau. It effectively
> just plots a couple of very large images that change regularly.
>
> I've been getting an easily repeatable failure, hitting the
> BUG_ON(in_interrupt) in vfree() that is called during ttm_shrink_work().
> Obviously vfree() is being called with a lock held...
>
> Looking at the locking up through the stack trace, there's an obvious
> mistake:
>
> There should be a:
> spin_lock(&glob->lru_lock);
> before the continue in ttm_bo_swapout() in drivers/gpu/drm/ttm/ttm_bo.c
>
> --- ttm_bo.c~   2011-01-05 00:50:19.000000000 +0000
> +++ ttm_bo.c    2011-01-20 18:11:57.262813765 +0000
> @@ -1761,6 +1761,7 @@ static int ttm_bo_swapout(struct ttm_mem
>                        spin_unlock(&glob->lru_lock);
>                        (void) ttm_bo_cleanup_refs(bo, false, false, false);
>                        kref_put(&bo->list_kref, ttm_bo_release_list);
> +                       spin_lock(&glob->lru_lock);
>                        continue;
>                }
>
> That fixes the problem for me.
>
> Matthew
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

This seems (very) plausible, maybe someone who commits drm patches
frequently will see this message now.

It would be a shame to forget about this e-mail.

Maarten.

-- 
Far away from the primal instinct, the song seems to fade away, the
river get wider between your thoughts and the things we do and say.

Re: Broken locking in ttm_bo_swapout

[Thomas Hellstrom]

On 01/20/2011 07:53 PM, Matthew Bullock wrote:
> I have a fairly simple opengl application running on nouveau. It 
> effectively just plots a couple of very large images that change 
> regularly.
>
> I've been getting an easily repeatable failure, hitting the
> BUG_ON(in_interrupt) in vfree() that is called during ttm_shrink_work().
> Obviously vfree() is being called with a lock held...
>
> Looking at the locking up through the stack trace, there's an obvious 
> mistake:
>
> There should be a:
> spin_lock(&glob->lru_lock);
> before the continue in ttm_bo_swapout() in drivers/gpu/drm/ttm/ttm_bo.c
>
> --- ttm_bo.c~   2011-01-05 00:50:19.000000000 +0000
> +++ ttm_bo.c    2011-01-20 18:11:57.262813765 +0000
> @@ -1761,6 +1761,7 @@ static int ttm_bo_swapout(struct ttm_mem
>                         spin_unlock(&glob->lru_lock);
>                         (void) ttm_bo_cleanup_refs(bo, false, false, 
> false);
>                         kref_put(&bo->list_kref, ttm_bo_release_list);
> +                       spin_lock(&glob->lru_lock);
>                         continue;
>                 }
>
> That fixes the problem for me.
>
> Matthew
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Indeed this fix looks correct. Sorry for taking so long to respond.
Could you spin a patch against Dave's drm-fixes tree or do you want me 
to do that?

Thanks,
Thomas

[PATCH] Fix broken locking in ttm_bo_swapout()

[Matthew Bullock]

Signed-off-by: Matthew Bullock <matthew.bullock@tadpole.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index af61fc2..e4695db 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1803,6 +1803,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 			spin_unlock(&glob->lru_lock);
 			(void) ttm_bo_cleanup_refs(bo, false, false, false);
 			kref_put(&bo->list_kref, ttm_bo_release_list);
+			spin_lock(&glob->lru_lock);
 			continue;
 		}
 
-- 
1.7.2.3