Skip to contentSkip to conversation

instmem/gk20a: exclusively acquire instobjs

PATCH In review Alexandre Courbot 3 participants 4 messages Oct 26, 2015 – Nov 9, 2015 3 messages not in the archive Arm STM32

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 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.

---
 drm/nouveau/nvkm/subdev/instmem/gk20a.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
drm/nouveau/nvkm/subdev/instmem/gk20a.c +6 -9
index fc419bb8eab7..d015633b8edd 100644
@@ -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);
Signature · 6 lines
2.6.1

_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 1445838895-28342-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

#2 lore raw
Quoted 3 lines · Alexandre Courbot

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.

Quoted 58 lines
---
 drm/nouveau/nvkm/subdev/instmem/gk20a.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
drm/nouveau/nvkm/subdev/instmem/gk20a.c +5 -7
index fc419bb8eab7..d015633b8edd 100644
@@ -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;
-

drm/nouveau/nvkm/subdev/instmem/gk20a.c gk20a_instobj_release(struct nvkm_memory *memory) · line 239

Quoted 10 lines
drm/nouveau/nvkm/subdev/instmem/gk20a.c +1 -2
-	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);

_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw
Quoted 9 lines · Ben Skeggs
Quoted 3 lines · Alexandre Courbot

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...
_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): CAAVeFuJAvb9GDxr=WBcpfvkXbiiTbju=aU2kwzhUA-fXgMXXkQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#6 lore raw
Quoted 7 lines · Alexandre Courbot
Quoted 6 lines · Ben Skeggs
Quoted 3 lines · Alexandre Courbot

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.

Quoted 8 lines
Quoted 3 lines

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...

Turns out this was just me not knowing how to handle concurrency properly. >_<

Fortunately this means that the error was just confined to the gk20a implementation and is not part of something larger. I will send the correct fixup patch later today.

Thanks for refusing my lazy hacks! :)
_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau