[PATCH] instmem/gk20a: fix race conditions

* [PATCH] instmem/gk20a: fix race conditions
@ 2015-11-09  7:37 Alexandre Courbot
       [not found] ` <1447054673-1500-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: Alexandre Courbot @ 2015-11-09  7:37 UTC (permalink / raw)
  To: Ben Skeggs; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

The LRU list used for recycling CPU mappings was handling concurrency
very poorly. For instance, if an instobj was acquired twice before being
released once, it would end up into the LRU list even though there is
still a client accessing it.

This patch fixes this by properly counting how many clients are
currently using a given instobj.

While at it, we also raise errors when inconsistencies are detected, and
factorize some code.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drm/nouveau/nvkm/subdev/instmem/gk20a.c | 66 ++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drm/nouveau/nvkm/subdev/instmem/gk20a.c
index fc419bb8eab7..681b2541229a 100644
--- a/drm/nouveau/nvkm/subdev/instmem/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/instmem/gk20a.c
@@ -57,6 +57,8 @@ struct gk20a_instobj {
 	/* CPU mapping */
 	u32 *vaddr;
 	struct list_head vaddr_node;
+	/* How many clients are using vaddr? */
+	u32 use_cpt;
 };
 #define gk20a_instobj(p) container_of((p), struct gk20a_instobj, memory)
 
@@ -158,27 +160,35 @@ gk20a_instobj_cpu_map_iommu(struct nvkm_memory *memory)
 }
 
 /*
- * Must be called while holding gk20a_instmem_lock
+ * Recycle the vaddr of obj. Must be called with gk20a_instmem::lock held.
+ */
+static void
+gk20a_instobj_recycle_vaddr(struct gk20a_instobj *obj)
+{
+	struct gk20a_instmem *imem = obj->imem;
+	/* there should not be any user left... */
+	WARN_ON(obj->use_cpt);
+	list_del(&obj->vaddr_node);
+	vunmap(obj->vaddr);
+	obj->vaddr = NULL;
+	imem->vaddr_use -= nvkm_memory_size(&obj->memory);
+	nvkm_debug(&imem->base.subdev, "vaddr used: %x/%x\n", imem->vaddr_use,
+		   imem->vaddr_max);
+}
+
+/*
+ * Must be called while holding gk20a_instmem::lock
  */
 static void
 gk20a_instmem_vaddr_gc(struct gk20a_instmem *imem, const u64 size)
 {
 	while (imem->vaddr_use + size > imem->vaddr_max) {
-		struct gk20a_instobj *obj;
-
 		/* no candidate that can be unmapped, abort... */
 		if (list_empty(&imem->vaddr_lru))
 			break;
 
-		obj = list_first_entry(&imem->vaddr_lru, struct gk20a_instobj,
-				       vaddr_node);
-		list_del(&obj->vaddr_node);
-		vunmap(obj->vaddr);
-		obj->vaddr = NULL;
-		imem->vaddr_use -= nvkm_memory_size(&obj->memory);
-		nvkm_debug(&imem->base.subdev, "(GC) vaddr used: %x/%x\n",
-			   imem->vaddr_use, imem->vaddr_max);
-
+		gk20a_instobj_recycle_vaddr(list_first_entry(&imem->vaddr_lru,
+					     struct gk20a_instobj, vaddr_node));
 	}
 }
 
@@ -196,9 +206,10 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
 	spin_lock_irqsave(&imem->lock, flags);
 
 	if (node->vaddr) {
-		/* remove us from the LRU list since we cannot be unmapped */
-		list_del(&node->vaddr_node);
-
+		if (!node->use_cpt) {
+			/* remove from LRU list since mapping in use again */
+			list_del(&node->vaddr_node);
+		}
 		goto out;
 	}
 
@@ -218,6 +229,7 @@ gk20a_instobj_acquire(struct nvkm_memory *memory)
 		   imem->vaddr_use, imem->vaddr_max);
 
 out:
+	node->use_cpt++;
 	spin_unlock_irqrestore(&imem->lock, flags);
 
 	return node->vaddr;
@@ -233,9 +245,15 @@ gk20a_instobj_release(struct nvkm_memory *memory)
 
 	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);
+	/* we should at least have one user to release... */
+	if (WARN_ON(node->use_cpt == 0))
+		goto out;
+
+	/* add unused objs to the LRU list to recycle their mapping */
+	if (--node->use_cpt == 0)
+		list_add_tail(&node->vaddr_node, &imem->vaddr_lru);
 
+out:
 	spin_unlock_irqrestore(&imem->lock, flags);
 
 	wmb();
@@ -273,25 +291,15 @@ static void
 gk20a_instobj_dtor(struct gk20a_instobj *node)
 {
 	struct gk20a_instmem *imem = node->imem;
-	struct gk20a_instobj *obj;
 	unsigned long flags;
 
 	spin_lock_irqsave(&imem->lock, flags);
 
+	/* vaddr has already been recycled */
 	if (!node->vaddr)
 		goto out;
 
-	list_for_each_entry(obj, &imem->vaddr_lru, vaddr_node) {
-		if (obj == node) {
-			list_del(&obj->vaddr_node);
-			break;
-		}
-	}
-	vunmap(node->vaddr);
-	node->vaddr = NULL;
-	imem->vaddr_use -= nvkm_memory_size(&node->memory);
-	nvkm_debug(&imem->base.subdev, "vaddr used: %x/%x\n",
-		   imem->vaddr_use, imem->vaddr_max);
+	gk20a_instobj_recycle_vaddr(node);
 
 out:
 	spin_unlock_irqrestore(&imem->lock, flags);
-- 
2.6.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

^ permalink raw reply related	[flat|nested] 4+ messages in thread