Nouveau - Oct 2015 - [PATCH] instmem/gk20a: exclusively acquire instobjs

Although I would not have expected this to happen, we seem to run into
race conditions if instobjs are accessed concurrently. Use a global lock
for safety.

Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
 drm/nouveau/nvkm/subdev/instmem/gk20a.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/instmem/gk20a.c
b/drm/nouveau/nvkm/subdev/instmem/gk20a.c
index fc419bb8eab7..d015633b8edd 100644
--- a/drm/nouveau/nvkm/subdev/instmem/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/instmem/gk20a.c
@@ -92,6 +92,7 @@ struct gk20a_instmem {
 
 	/* protects vaddr_* and gk20a_instobj::vaddr* */
 	spinlock_t lock;
+	unsigned long flags;
 
 	/* CPU mappings LRU */
 	unsigned int vaddr_use;
@@ -188,12 +189,11 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
 	struct gk20a_instobj *node = gk20a_instobj(memory);
 	struct gk20a_instmem *imem = node->imem;
 	struct nvkm_ltc *ltc = imem->base.subdev.device->ltc;
-	const u64 size = nvkm_memory_size(memory);
-	unsigned long flags;
+	u64 size;
 
 	nvkm_ltc_flush(ltc);
 
-	spin_lock_irqsave(&imem->lock, flags);
+	spin_lock_irqsave(&imem->lock, imem->flags);
 
 	if (node->vaddr) {
 		/* remove us from the LRU list since we cannot be unmapped */
@@ -202,6 +202,8 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
 		goto out;
 	}
 
+	size = nvkm_memory_size(memory);
+
 	/* try to free some address space if we reached the limit */
 	gk20a_instmem_vaddr_gc(imem, size);
 
@@ -218,8 +220,6 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
 		   imem->vaddr_use, imem->vaddr_max);
 
 out:
-	spin_unlock_irqrestore(&imem->lock, flags);
-
 	return node->vaddr;
 }
 
@@ -229,14 +229,11 @@ gk20a_instobj_release(struct nvkm_memory *memory)
 	struct gk20a_instobj *node = gk20a_instobj(memory);
 	struct gk20a_instmem *imem = node->imem;
 	struct nvkm_ltc *ltc = imem->base.subdev.device->ltc;
-	unsigned long flags;
-
-	spin_lock_irqsave(&imem->lock, flags);
 
 	/* add ourselves to the LRU list so our CPU mapping can be freed */
 	list_add_tail(&node->vaddr_node, &imem->vaddr_lru);
 
-	spin_unlock_irqrestore(&imem->lock, flags);
+	spin_unlock_irqrestore(&imem->lock, imem->flags);
 
 	wmb();
 	nvkm_ltc_invalidate(ltc);
-- 
2.6.1

On 10/26/2015 03:54 PM, Alexandre Courbot wrote:
> Although I would not have expected this to happen, we seem to run into
> race conditions if instobjs are accessed concurrently. Use a global lock
> for safety.
I wouldn't expect this to be an issue either.

Before merging such a large hammer of a fix, I'd strongly prefer to see
at least a better justification for why this is happening rather than
potentially papering over a larger issue.

Ben.
> 
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drm/nouveau/nvkm/subdev/instmem/gk20a.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drm/nouveau/nvkm/subdev/instmem/gk20a.c
b/drm/nouveau/nvkm/subdev/instmem/gk20a.c
> index fc419bb8eab7..d015633b8edd 100644
> --- a/drm/nouveau/nvkm/subdev/instmem/gk20a.c
> +++ b/drm/nouveau/nvkm/subdev/instmem/gk20a.c
> @@ -92,6 +92,7 @@ struct gk20a_instmem {
>  
>  	/* protects vaddr_* and gk20a_instobj::vaddr* */
>  	spinlock_t lock;
> +	unsigned long flags;
>  
>  	/* CPU mappings LRU */
>  	unsigned int vaddr_use;
> @@ -188,12 +189,11 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
>  	struct gk20a_instobj *node = gk20a_instobj(memory);
>  	struct gk20a_instmem *imem = node->imem;
>  	struct nvkm_ltc *ltc = imem->base.subdev.device->ltc;
> -	const u64 size = nvkm_memory_size(memory);
> -	unsigned long flags;
> +	u64 size;
>  
>  	nvkm_ltc_flush(ltc);
>  
> -	spin_lock_irqsave(&imem->lock, flags);
> +	spin_lock_irqsave(&imem->lock, imem->flags);
>  
>  	if (node->vaddr) {
>  		/* remove us from the LRU list since we cannot be unmapped */
> @@ -202,6 +202,8 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
>  		goto out;
>  	}
>  
> +	size = nvkm_memory_size(memory);
> +
>  	/* try to free some address space if we reached the limit */
>  	gk20a_instmem_vaddr_gc(imem, size);
>  
> @@ -218,8 +220,6 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
>  		   imem->vaddr_use, imem->vaddr_max);
>  
>  out:
> -	spin_unlock_irqrestore(&imem->lock, flags);
> -
>  	return node->vaddr;
>  }
>  
> @@ -229,14 +229,11 @@ gk20a_instobj_release(struct nvkm_memory *memory)
>  	struct gk20a_instobj *node = gk20a_instobj(memory);
>  	struct gk20a_instmem *imem = node->imem;
>  	struct nvkm_ltc *ltc = imem->base.subdev.device->ltc;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&imem->lock, flags);
>  
>  	/* add ourselves to the LRU list so our CPU mapping can be freed */
>  	list_add_tail(&node->vaddr_node, &imem->vaddr_lru);
>  
> -	spin_unlock_irqrestore(&imem->lock, flags);
> +	spin_unlock_irqrestore(&imem->lock, imem->flags);
>  
>  	wmb();
>  	nvkm_ltc_invalidate(ltc);
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20151105/4f513a82/attachment.sig>

On Thu, Nov 5, 2015 at 6:19 AM, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On 10/26/2015 03:54 PM, Alexandre Courbot wrote:
>> Although I would not have expected this to happen, we seem to run into
>> race conditions if instobjs are accessed concurrently. Use a global
lock
>> for safety.
> I wouldn't expect this to be an issue either.
>
> Before merging such a large hammer of a fix, I'd strongly prefer to see
> at least a better justification for why this is happening rather than
> potentially papering over a larger issue.
I was afraid you would say that. ;)

But you're right. I am really busy with lots of stuff but will try to
rootcause this more precisely...