Stability improvements to error state capture

Using safe list iterators alleviates the problem of unsynchronized driver list manipulations while error state capture is in the process of traversing lists.

Quoted 2 lines · Tomas Elf

Using safe list iterators alleviates the problem of unsynchronized driver list manipulations while error state capture is in the process of traversing lists.

Wrong, this does not make anything safe.
-Chris

Using safe list iterators alleviates the problem of unsynchronized driver list manipulations while error state capture is in the process of traversing lists.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 38 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 2f04e4f..32c1799 100644

drivers/gpu/drm/i915/i915_gpu_error.c +17 -17

 		capture_bo(err++, vma);
 		if (++i == count)
 			break;

@@ -734,17 +734,17 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 			     int count, struct list_head *head,
 			     struct i915_address_space *vm)
 {
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj, *tmpobj;
 	struct drm_i915_error_buffer * const first = err;
 	struct drm_i915_error_buffer * const last = err + count;
 
-	list_for_each_entry(obj, head, global_list) {
-		struct i915_vma *vma;
+	list_for_each_entry_safe(obj, tmpobj, head, global_list) {
+		struct i915_vma *vma, *tmpvma;
 
 		if (err == last)
 			break;
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
+		list_for_each_entry_safe(vma, tmpvma, &obj->vma_list, vma_link)
 			if (vma->vm == vm && vma->pin_count > 0)
 				capture_bo(err++, vma);
 	}

@@ -958,13 +958,13 @@ static void i915_gem_record_active_context(struct intel_engine_cs *ring,
 					   struct drm_i915_error_ring *ering)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj, *tmpobj;
 
 	/* Currently render ring is the only HW context user */
 	if (ring->id != RCS || !error->ccid)
 		return;
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+	list_for_each_entry_safe(obj, tmpobj, &dev_priv->mm.bound_list, global_list) {
 		if (!i915_gem_obj_ggtt_bound(obj))
 			continue;
 

@@ -979,7 +979,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 				  struct drm_i915_error_state *error)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_request *request;
+	struct drm_i915_gem_request *request, *tmpreq;
 	int i, count;
 
 	for (i = 0; i < I915_NUM_RINGS; i++) {

@@ -1055,7 +1055,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 		i915_gem_record_active_context(ring, error, &error->ring[i]);
 
 		count = 0;
-		list_for_each_entry(request, &ring->request_list, list)
+		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list)
 			count++;
 
 		error->ring[i].num_requests = count;

@@ -1068,7 +1068,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
 		}
 
 		count = 0;
-		list_for_each_entry(request, &ring->request_list, list) {
+		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
 			struct drm_i915_error_request *erq;
 
 			erq = &error->ring[i].requests[count++];

@@ -1088,17 +1088,17 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 				const int ndx)
 {
 	struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
-	struct drm_i915_gem_object *obj;
-	struct i915_vma *vma;
+	struct drm_i915_gem_object *obj, *tmpobj;
+	struct i915_vma *vma, *tmpvma;
 	int i;
 
 	i = 0;
-	list_for_each_entry(vma, &vm->active_list, mm_list)
+	list_for_each_entry_safe(vma, tmpvma, &vm->active_list, mm_list)
 		i++;
 	error->active_bo_count[ndx] = i;
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
+	list_for_each_entry_safe(obj, tmpobj, &dev_priv->mm.bound_list, global_list) {
+		list_for_each_entry_safe(vma, tmpvma, &obj->vma_list, vma_link)
 			if (vma->vm == vm && vma->pin_count > 0)
 				i++;
 	}

@@ -1128,10 +1128,10 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
 				     struct drm_i915_error_state *error)
 {
-	struct i915_address_space *vm;
+	struct i915_address_space *vm, *tmpvm;
 	int cnt = 0, i = 0;
 
-	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
+	list_for_each_entry_safe(vm, tmpvm, &dev_priv->vm_list, global_link)
 		cnt++;
 
 	error->active_bo = kcalloc(cnt, sizeof(*error->active_bo), GFP_ATOMIC);

@@ -1155,7 +1155,7 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
 		error->pinned_bo = NULL;
 		error->pinned_bo_count = NULL;
 	} else {
-		list_for_each_entry(vm, &dev_priv->vm_list, global_link)
+		list_for_each_entry_safe(vm, tmpvm, &dev_priv->vm_list, global_link)
 			i915_gem_capture_vm(dev_priv, error, vm, i++);
 
 		error->vm_count = cnt;

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 13 lines · Tomas Elf

Using safe list iterators alleviates the problem of unsynchronized driver list manipulations while error state capture is in the process of traversing lists.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c | 38 +++++++++++++++++------------------
  1 file changed, 19 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 2f04e4f..32c1799 100644

drivers/gpu/drm/i915/i915_gpu_error.c static void capture_bo(struct drm_i915_error_buffer *err, · line 724

Quoted 9 lines

drivers/gpu/drm/i915/i915_gpu_error.c +2 -2

@@ -718,10 +718,10 @@ static void capture_bo(struct drm_i915_error_buffer *err,
  static u32 capture_active_bo(struct drm_i915_error_buffer *err,
  			     int count, struct list_head *head)
  {
-	struct i915_vma *vma;
+	struct i915_vma *vma, *tmpvma;
  	int i = 0;
 
-	list_for_each_entry(vma, head, mm_list) {
+	list_for_each_entry_safe(vma, tmpvma, head, mm_list) {

_safe isn't safe against anything, it's only safe against removal of the current list item done with a list_del/list_move from this thread. I.e. the only thing it does is have a temporary pointer to the next list element so if you delete the current one it won't fall over.

It doesn't protect against anything else, and especially not against other threads totally stomping the list.

So we need proper protection for these lists, independent of dev->struct_mutex. The least invasive way to do that is probably rcu, which only requires us that we release the memory for any object sitting on these lists with kfree_rcu instead of normal kfree. Another option might be a spinlock, but that would be a lot more invasive, and Chris won't like the execbuf overhead this causes ;-)
-Daniel

Quoted 113 lines

drivers/gpu/drm/i915/i915_gpu_error.c +17 -17

  		capture_bo(err++, vma);
  		if (++i == count)
  			break;

@@ -734,17 +734,17 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
  			     int count, struct list_head *head,
  			     struct i915_address_space *vm)
  {
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj, *tmpobj;
  	struct drm_i915_error_buffer * const first = err;
  	struct drm_i915_error_buffer * const last = err + count;
 
-	list_for_each_entry(obj, head, global_list) {
-		struct i915_vma *vma;
+	list_for_each_entry_safe(obj, tmpobj, head, global_list) {
+		struct i915_vma *vma, *tmpvma;
 
  		if (err == last)
  			break;
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
+		list_for_each_entry_safe(vma, tmpvma, &obj->vma_list, vma_link)
  			if (vma->vm == vm && vma->pin_count > 0)
  				capture_bo(err++, vma);
  	}

@@ -958,13 +958,13 @@ static void i915_gem_record_active_context(struct intel_engine_cs *ring,
  					   struct drm_i915_error_ring *ering)
  {
  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj, *tmpobj;
 
  	/* Currently render ring is the only HW context user */
  	if (ring->id != RCS || !error->ccid)
  		return;
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+	list_for_each_entry_safe(obj, tmpobj, &dev_priv->mm.bound_list, global_list) {
  		if (!i915_gem_obj_ggtt_bound(obj))
  			continue;
 

@@ -979,7 +979,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
  				  struct drm_i915_error_state *error)
  {
  	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_request *request;
+	struct drm_i915_gem_request *request, *tmpreq;
  	int i, count;
 
  	for (i = 0; i < I915_NUM_RINGS; i++) {

@@ -1055,7 +1055,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
  		i915_gem_record_active_context(ring, error, &error->ring[i]);
 
  		count = 0;
-		list_for_each_entry(request, &ring->request_list, list)
+		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list)
  			count++;
 
  		error->ring[i].num_requests = count;

@@ -1068,7 +1068,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
  		}
 
  		count = 0;
-		list_for_each_entry(request, &ring->request_list, list) {
+		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
  			struct drm_i915_error_request *erq;
 
  			erq = &error->ring[i].requests[count++];

@@ -1088,17 +1088,17 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
  				const int ndx)
  {
  	struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
-	struct drm_i915_gem_object *obj;
-	struct i915_vma *vma;
+	struct drm_i915_gem_object *obj, *tmpobj;
+	struct i915_vma *vma, *tmpvma;
  	int i;
 
  	i = 0;
-	list_for_each_entry(vma, &vm->active_list, mm_list)
+	list_for_each_entry_safe(vma, tmpvma, &vm->active_list, mm_list)
  		i++;
  	error->active_bo_count[ndx] = i;
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
+	list_for_each_entry_safe(obj, tmpobj, &dev_priv->mm.bound_list, global_list) {
+		list_for_each_entry_safe(vma, tmpvma, &obj->vma_list, vma_link)
  			if (vma->vm == vm && vma->pin_count > 0)
  				i++;
  	}

@@ -1128,10 +1128,10 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
  static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
  				     struct drm_i915_error_state *error)
  {
-	struct i915_address_space *vm;
+	struct i915_address_space *vm, *tmpvm;
  	int cnt = 0, i = 0;
 
-	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
+	list_for_each_entry_safe(vm, tmpvm, &dev_priv->vm_list, global_link)
  		cnt++;
 
  	error->active_bo = kcalloc(cnt, sizeof(*error->active_bo), GFP_ATOMIC);

@@ -1155,7 +1155,7 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
  		error->pinned_bo = NULL;
  		error->pinned_bo_count = NULL;
  	} else {
-		list_for_each_entry(vm, &dev_priv->vm_list, global_link)
+		list_for_each_entry_safe(vm, tmpvm, &dev_priv->vm_list, global_link)
  			i915_gem_capture_vm(dev_priv, error, vm, i++);
 

Quoted 8 lines

drivers/gpu/drm/i915/i915_gpu_error.c +0 -1

  		error->vm_count = cnt;
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 29 lines · Daniel Vetter

Quoted 13 lines · Tomas Elf

Using safe list iterators alleviates the problem of unsynchronized driver list manipulations while error state capture is in the process of traversing lists.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 38 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 2f04e4f..32c1799 100644

drivers/gpu/drm/i915/i915_gpu_error.c static void capture_bo(struct drm_i915_error_buffer *err, · line 724

Quoted 9 lines

drivers/gpu/drm/i915/i915_gpu_error.c +2 -2

@@ -718,10 +718,10 @@ static void capture_bo(struct drm_i915_error_buffer *err,
 static u32 capture_active_bo(struct drm_i915_error_buffer *err,
 			     int count, struct list_head *head)
 {
-	struct i915_vma *vma;
+	struct i915_vma *vma, *tmpvma;
 	int i = 0;
 
-	list_for_each_entry(vma, head, mm_list) {
+	list_for_each_entry_safe(vma, tmpvma, head, mm_list) {

_safe isn't safe against anything, it's only safe against removal of the current list item done with a list_del/list_move from this thread. I.e. the only thing it does is have a temporary pointer to the next list element so if you delete the current one it won't fall over.

Quoted 2 lines

It doesn't protect against anything else, and especially not against other threads totally stomping the list.

Absolutely! But it's good enough to make the test not fall over within an hour of testing and instead allow it to run for 12+ hours during continuous long-duration testing, which is what I need for the TDR validation.

Quoted 7 lines

So we need proper protection for these lists, independent of dev->struct_mutex. The least invasive way to do that is probably rcu, which only requires us that we release the memory for any object sitting on these lists with kfree_rcu instead of normal kfree. Another option might be a spinlock, but that would be a lot more invasive, and Chris won't like the execbuf overhead this causes ;-)
-Daniel

Awesome! Let's go with that then.

Quoted 30 lines · Tomas Elf

Quoted 29 lines · Daniel Vetter

Quoted 13 lines · Tomas Elf

Using safe list iterators alleviates the problem of unsynchronized driver list manipulations while error state capture is in the process of traversing lists.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 38 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 2f04e4f..32c1799 100644

drivers/gpu/drm/i915/i915_gpu_error.c static void capture_bo(struct drm_i915_error_buffer *err, · line 724

Quoted 9 lines

drivers/gpu/drm/i915/i915_gpu_error.c +2 -2

@@ -718,10 +718,10 @@ static void capture_bo(struct drm_i915_error_buffer *err,
 static u32 capture_active_bo(struct drm_i915_error_buffer *err,
 			     int count, struct list_head *head)
 {
-	struct i915_vma *vma;
+	struct i915_vma *vma, *tmpvma;
 	int i = 0;
 
-	list_for_each_entry(vma, head, mm_list) {
+	list_for_each_entry_safe(vma, tmpvma, head, mm_list) {

_safe isn't safe against anything, it's only safe against removal of the current list item done with a list_del/list_move from this thread. I.e. the only thing it does is have a temporary pointer to the next list element so if you delete the current one it won't fall over.

Quoted 6 lines

Quoted 2 lines

It doesn't protect against anything else, and especially not against other threads totally stomping the list.

Absolutely! But it's good enough to make the test not fall over within an hour of testing and instead allow it to run for 12+ hours during continuous long-duration testing, which is what I need for the TDR validation.

Well it looks really suspicious, since the only thing this protects against is against deleting the element we look at right now. The only sensible theory I can come up with is that this slightly helps when starting the list walk, in case someone comes around and deletes the first element in the list from the retire worker. Since the retire worker tends to do more work per list item than we do that's enough to make the race really unlikely. But it's still just duct-tape.

Quoted 9 lines

Quoted 7 lines

So we need proper protection for these lists, independent of dev->struct_mutex. The least invasive way to do that is probably rcu, which only requires us that we release the memory for any object sitting on these lists with kfree_rcu instead of normal kfree. Another option might be a spinlock, but that would be a lot more invasive, and Chris won't like the execbuf overhead this causes ;-)
-Daniel

Awesome! Let's go with that then.

See our massive irc discussion ... it's more tricky :(

In short rcu works perfectly if you only have the following lifetime sequence:
- kmalloc object
- add to list
- remove from list (eventually)
- free object

If you readd an object to a list, or even worse, move it then the rcu list helpers won't work. What could happen on the read side is:

- you walk the rcu list, keeping track of the head element to know when to stop
- while looking at that list one of the yet untraversed items gets moved to a new list

-> you'll traverse the new list forever since that one will never hit the head element.

Not a problem for requests/vmas, but a problem for some of the object lists.

Note that the problem isn't that we re-add the element (if that happens we might end up walking parts of the list twice or parts of it not at all), but moving to a different list where there's a different head element.

I haven't checked, but maybe we're lucky and all the object lists we're looking at will always have the same head element. Then I think it'll work out (albeit racy, but who cares about that for the error capture) and with the guarantee (when using rcu delayed freeing) that'll we'll never chase freed memory.

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 32c1799..cc75ca4 100644

Quoted 20 lines · Tomas Elf

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 32c1799..cc75ca4 100644
@@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev,
  		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
  			struct drm_i915_error_request *erq;
 

drivers/gpu/drm/i915/i915_gpu_error.c static void i915_gem_record_rings(struct drm_device *dev, · line 1074

Quoted 1 line

drivers/gpu/drm/i915/i915_gpu_error.c +1 -0

+			if (WARN_ON(!request || count >= error->ring[i].num_requests)) {

Request cannot be null, count can legitmately be more, the WARN on is inappropriate. Again, I sent several patches over the past couple of years to fix this.
-Chris

Quoted 21 lines · Chris Wilson

Quoted 20 lines · Tomas Elf

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 32c1799..cc75ca4 100644
@@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev,
 		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
 			struct drm_i915_error_request *erq;
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_gpu_error.c static void i915_gem_record_rings(struct drm_device *dev, · line 1074

Quoted 1 line

drivers/gpu/drm/i915/i915_gpu_error.c +1 -0

+			if (WARN_ON(!request || count >= error->ring[i].num_requests)) {

Request cannot be null, count can legitmately be more, the WARN on is inappropriate. Again, I sent several patches over the past couple of years to fix this.
-Chris

Ok, so having the driver request list change grow in between the point where we allocate and set up the error state request list to the same size as the driver request list (since that's what count being larger than the list size implies) is legitimate? Traversing into unallocated memory seems pretty dodgy to me but if you say so.

Quoted 30 lines · Tomas Elf

Quoted 21 lines · Chris Wilson

Quoted 20 lines · Tomas Elf

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 32c1799..cc75ca4 100644
@@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev,
  		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
  			struct drm_i915_error_request *erq;
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_gpu_error.c static void i915_gem_record_rings(struct drm_device *dev, · line 1074

Quoted 1 line

drivers/gpu/drm/i915/i915_gpu_error.c +1 -0

+			if (WARN_ON(!request || count >= error->ring[i].num_requests)) {

Request cannot be null, count can legitmately be more, the WARN on is inappropriate. Again, I sent several patches over the past couple of years to fix this.
-Chris

Quoted 5 lines

Ok, so having the driver request list change grow in between the point where we allocate and set up the error state request list to the same size as the driver request list (since that's what count being larger than the list size implies) is legitimate? Traversing into unallocated memory seems pretty dodgy to me but if you say so.

We still need to handle it ofc, but just not WARN on this condition since it can happen.
-Daniel

Quoted 37 lines · Daniel Vetter

Quoted 30 lines · Tomas Elf

Quoted 21 lines · Chris Wilson

Quoted 20 lines · Tomas Elf

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index 32c1799..cc75ca4 100644
@@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev,
 		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
 			struct drm_i915_error_request *erq;
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_gpu_error.c static void i915_gem_record_rings(struct drm_device *dev, · line 1074

Quoted 1 line

drivers/gpu/drm/i915/i915_gpu_error.c +1 -0

+			if (WARN_ON(!request || count >= error->ring[i].num_requests)) {

Request cannot be null, count can legitmately be more, the WARN on is inappropriate. Again, I sent several patches over the past couple of years to fix this.
-Chris

Quoted 5 lines

Ok, so having the driver request list change grow in between the point where we allocate and set up the error state request list to the same size as the driver request list (since that's what count being larger than the list size implies) is legitimate? Traversing into unallocated memory seems pretty dodgy to me but if you say so.

Quoted 4 lines

We still need to handle it ofc, but just not WARN on this condition since it can happen.
-Daniel

With the RCU discussion ongoing I guess we should we just drop this patch? I agree that what I've been seeing looks like a side-effect of concurrent memory deallocation. Whatever solution you reach should make this patch pointless.

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +13 -0

index 32c1799..cc75ca4 100644
@@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev,
 		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
 			struct drm_i915_error_request *erq;
 
+			if (WARN_ON(!request || count >= error->ring[i].num_requests)) {
+				/*
+				 * If the ring request list was changed in
+				 * between the point where the error request
+				 * list was created and dimensioned and this
+				 * point then just update the num_requests
+				 * field to reflect this.
+				 */
+				error->ring[i].num_requests =
+					min(count, error->ring[i].num_requests);
+				break;
+			}
+
 			erq = &error->ring[i].requests[count++];
 			erq->seqno = request->seqno;
 			erq->jiffies = request->emitted_jiffies;

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 6 lines · Tomas Elf

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Please don't throw a WARNING since this is expected to occasionally happen. DRM_DEBUG_DRIVER is enough imo.
-Daniel

Quoted 34 lines

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +13 -1

index 32c1799..cc75ca4 100644
@@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev,
  		list_for_each_entry_safe(request, tmpreq, &ring->request_list, list) {
  			struct drm_i915_error_request *erq;
 
+			if (WARN_ON(!request || count >= error->ring[i].num_requests)) {
+				/*
+				 * If the ring request list was changed in
+				 * between the point where the error request
+				 * list was created and dimensioned and this
+				 * point then just update the num_requests
+				 * field to reflect this.
+				 */
+				error->ring[i].num_requests =
+					min(count, error->ring[i].num_requests);
+				break;
+			}
+
  			erq = &error->ring[i].requests[count++];
  			erq->seqno = request->seqno;
  			erq->jiffies = request->emitted_jiffies;
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 11 lines · Daniel Vetter

Quoted 6 lines · Tomas Elf

Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable.

Please don't throw a WARNING since this is expected to occasionally happen. DRM_DEBUG_DRIVER is enough imo.
-Daniel

I still don't see how it could happen without leading to reads of unallocated memory. The error state request list has been allocated to a certain size equal to num_requests and this loop seems to assume that the error state request list maintains the same size as the driver request list, which is not the case - leading to crashes, which is how I happened to notice it.

I can obviously remove the warning but are you saying we shouldn't even take action if it happens? Such as early exit?

In the past vmas have sometimes turned out to be NULL when capturing buffer objects during error state capture. To avoid NULL pointer exceptions throw a WARNING and exit early and be prepared for the error state not being fully accurate following this point.

Quoted 4 lines · Tomas Elf

In the past vmas have sometimes turned out to be NULL when capturing buffer objects during error state capture. To avoid NULL pointer exceptions throw a WARNING and exit early and be prepared for the error state not being fully accurate following this point.

No, fix the root cause.
-Chris

In the past vmas have sometimes turned out to be NULL when capturing buffer objects during error state capture. To avoid NULL pointer exceptions throw a WARNING and exit early and be prepared for the error state not being fully accurate following this point.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index cc75ca4..ae24971 100644

drivers/gpu/drm/i915/i915_gpu_error.c +3 -0

+
+	obj = vma->obj;
+
 	err->size = obj->base.size;
 	err->name = obj->base.name;
 	for (i = 0; i < I915_NUM_RINGS; i++)

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 15 lines · Tomas Elf

In the past vmas have sometimes turned out to be NULL when capturing buffer objects during error state capture. To avoid NULL pointer exceptions throw a WARNING and exit early and be prepared for the error state not being fully accurate following this point.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index cc75ca4..ae24971 100644

drivers/gpu/drm/i915/i915_gpu_error.c unwind: · line 698

Quoted 9 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -1

@@ -691,9 +691,17 @@ unwind:
 static void capture_bo(struct drm_i915_error_buffer *err,
 		       struct i915_vma *vma)
 {
-	struct drm_i915_gem_object *obj = vma->obj;
+	struct drm_i915_gem_object *obj;
 	int i;
 
+	if (WARN_ON(!vma))
+	    return;

If we rcu-protect the vma lists and vmas themselves then this can't happen.

drivers/gpu/drm/i915/i915_gpu_error.c unwind: · line 701

Quoted 3 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -0

+
+	if (WARN_ON(!vma->obj))
+	    return;

Again we probably need to rcu-protect this, and more important we need to make sure the compiler doesn't do crap. So

	obj = rcu_derefence(vma->obj)
	if (!obj)
		return;

Quoted 36 lines · Daniel Vetter

Quoted 15 lines · Tomas Elf

In the past vmas have sometimes turned out to be NULL when capturing buffer objects during error state capture. To avoid NULL pointer exceptions throw a WARNING and exit early and be prepared for the error state not being fully accurate following this point.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index cc75ca4..ae24971 100644

drivers/gpu/drm/i915/i915_gpu_error.c unwind: · line 698

Quoted 9 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -1

@@ -691,9 +691,17 @@ unwind:
  static void capture_bo(struct drm_i915_error_buffer *err,
  		       struct i915_vma *vma)
  {
-	struct drm_i915_gem_object *obj = vma->obj;
+	struct drm_i915_gem_object *obj;
  	int i;
 
+	if (WARN_ON(!vma))
+	    return;

If we rcu-protect the vma lists and vmas themselves then this can't happen.

drivers/gpu/drm/i915/i915_gpu_error.c unwind: · line 701

Quoted 3 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -0

+
+	if (WARN_ON(!vma->obj))
+	    return;

Again we probably need to rcu-protect this, and more important we need to make sure the compiler doesn't do crap. So

Quoted 3 lines

	obj = rcu_derefence(vma->obj)
	if (!obj)
		return;

The other issue is that we don't actually want capture_bo() but capture_vma().

-Chris

Quoted 15 lines · Tomas Elf

In the past vmas have sometimes turned out to be NULL when capturing buffer objects during error state capture. To avoid NULL pointer exceptions throw a WARNING and exit early and be prepared for the error state not being fully accurate following this point.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gpu_error.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gpu_error.c +0 -0

index cc75ca4..ae24971 100644

drivers/gpu/drm/i915/i915_gpu_error.c unwind: · line 698

Quoted 9 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -1

@@ -691,9 +691,17 @@ unwind:
  static void capture_bo(struct drm_i915_error_buffer *err,
  		       struct i915_vma *vma)
  {
-	struct drm_i915_gem_object *obj = vma->obj;
+	struct drm_i915_gem_object *obj;
  	int i;
 
+	if (WARN_ON(!vma))
+	    return;

If we rcu-protect the vma lists and vmas themselves then this can't happen.

drivers/gpu/drm/i915/i915_gpu_error.c unwind: · line 701

Quoted 3 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -0

+
+	if (WARN_ON(!vma->obj))
+	    return;

Again we probably need to rcu-protect this, and more important we need to make sure the compiler doesn't do crap. So

	obj = rcu_derefence(vma->obj)
	if (!obj)
		return;

Ofc obj then can also only be released with kfree_rcu. -Daniel

Quoted 10 lines

drivers/gpu/drm/i915/i915_gpu_error.c +3 -1

+
+	obj = vma->obj;
+
  	err->size = obj->base.size;
  	err->name = obj->base.name;
  	for (i = 0; i < I915_NUM_RINGS; i++)
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Sometimes the iterated vma objects are NULL apparently. Be aware of this while iterating and do early exit instead of causing a NULL pointer exception.

Quoted 2 lines · Tomas Elf

Sometimes the iterated vma objects are NULL apparently. Be aware of this while iterating and do early exit instead of causing a NULL pointer exception.

Wrong.
-Chris

Quoted 7 lines · Chris Wilson

Quoted 2 lines · Tomas Elf

Sometimes the iterated vma objects are NULL apparently. Be aware of this while iterating and do early exit instead of causing a NULL pointer exception.

Wrong.
-Chris

So the NULL pointer exception that I ran into multiple times during several different test runs on the line that says "vma->pin_count" was not because the vma pointer was NULL. Would you mind sharing your explanation to how this might have happened? Is it because we're not synchronizing and there's no protection against the driver deallocating vmas while we're trying to capture them? If this all ties into the aforementioned RCU-based solution then maybe we should go with that then.

Quoted 9 lines · Tomas Elf

Quoted 7 lines · Chris Wilson

Quoted 2 lines · Tomas Elf

Sometimes the iterated vma objects are NULL apparently. Be aware of this while iterating and do early exit instead of causing a NULL pointer exception.

Wrong.
-Chris

Quoted 8 lines

So the NULL pointer exception that I ran into multiple times during several different test runs on the line that says "vma->pin_count" was not because the vma pointer was NULL. Would you mind sharing your explanation to how this might have happened? Is it because we're not synchronizing and there's no protection against the driver deallocating vmas while we're trying to capture them? If this all ties into the aforementioned RCU-based solution then maybe we should go with that then.

Correct. The driver is retiring requests whilst the hang check worker is running. And you chased a stale pointer, you could have equally chased a stale vma->obj, vma->ctx etc pointers.

What I have done in the past is to serialise the retirement and the hangcheck using a spinlock (as adding to the end of the list is safe), but there are still weak spots when walking the list of bound vma. What I would actually feel more comfortable with is to only record the request->batch_vma, at the cost of losing information about the currently bound buffers.

Or we could just stop_machine() whilst running the capture and have the machine unresponsive for a few 100ms. I don't think simply RCU the lists is enough (VM active_list, request list, bound list) as eventually we chase a pointer from obj itself (which means we need to RCU pretty much everything).

Sometimes the iterated vma objects are NULL apparently. Be aware of this while iterating and do early exit instead of causing a NULL pointer exception.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem.c +0 -0

index 52642af..8210ae7 100644

drivers/gpu/drm/i915/i915_gem.c +2 -0

+
 		if (vma->pin_count > 0)
 			return true;
+	}
 
 	return false;
 }

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 8 lines · Tomas Elf

Sometimes the iterated vma objects are NULL apparently. Be aware of this while iterating and do early exit instead of causing a NULL pointer exception.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem.c unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, · line 5127

Quoted 12 lines

drivers/gpu/drm/i915/i915_gem.c +3 -1

index 52642af..8210ae7 100644
@@ -5122,9 +5122,13 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
  bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
  {
  	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (!vma)
+			continue;

Again no need to check for that if we rcu-protect this list and vmas. -Daniel

Quoted 11 lines

drivers/gpu/drm/i915/i915_gem.c +2 -1

+
  		if (vma->pin_count > 0)
  			return true;
+	}
 
  	return false;
  }
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Error state capture is dependent on i915_gem_active_request() and i915_gem_obj_is_pinned(). Since there is no synchronization between error state capture and the driver state we at least need to use safe list iterators in these functions to alleviate the problem of request/vma lists changing during error state capture.

Quoted 5 lines · Tomas Elf

Error state capture is dependent on i915_gem_active_request() and i915_gem_obj_is_pinned(). Since there is no synchronization between error state capture and the driver state we at least need to use safe list iterators in these functions to alleviate the problem of request/vma lists changing during error state capture.

Does not make it safe.
-Chris

Quoted 10 lines · Chris Wilson

Quoted 5 lines · Tomas Elf

Error state capture is dependent on i915_gem_active_request() and i915_gem_obj_is_pinned(). Since there is no synchronization between error state capture and the driver state we at least need to use safe list iterators in these functions to alleviate the problem of request/vma lists changing during error state capture.

Does not make it safe.
-Chris

I know it doesn't make it safe, I didn't say this patch makes it safe.

Maybe I was unclear but I chose the word "alleviate" rather than "solve" to indicate that there are still problems but this change reduces the problem scope and makes crashes less likely to occur. I also used the formulation "at least" to indicate that we're not solving everything but we can do certain things to improve things somewhat.

The problems I've been seeing has been that the list state changes during iteration and that the error capture tries to read elements that are no longer part of the list - not that elements that the error capture code is dereferencing are deallocated by the driver or whatever. Using a safe iterator helps with that very particular problem. Or maybe I guess I've just been incredibly lucky for the last 2 months when I've been running this code as I've been able to get 12+ hours of stability during my tests instead of less than one hour in between crashes that was the case before I introduced these changes.

Quoted 21 lines · Tomas Elf

Quoted 10 lines · Chris Wilson

Quoted 5 lines · Tomas Elf

Error state capture is dependent on i915_gem_active_request() and i915_gem_obj_is_pinned(). Since there is no synchronization between error state capture and the driver state we at least need to use safe list iterators in these functions to alleviate the problem of request/vma lists changing during error state capture.

Does not make it safe.
-Chris

I know it doesn't make it safe, I didn't say this patch makes it safe.

Maybe I was unclear but I chose the word "alleviate" rather than "solve" to indicate that there are still problems but this change reduces the problem scope and makes crashes less likely to occur. I also used the formulation "at least" to indicate that we're not solving everything but we can do certain things to improve things somewhat.

Quoted 10 lines

The problems I've been seeing has been that the list state changes during iteration and that the error capture tries to read elements that are no longer part of the list - not that elements that the error capture code is dereferencing are deallocated by the driver or whatever. Using a safe iterator helps with that very particular problem. Or maybe I guess I've just been incredibly lucky for the last 2 months when I've been running this code as I've been able to get 12+ hours of stability during my tests instead of less than one hour in between crashes that was the case before I introduced these changes.

You have been incredibily lucky (probably due to how the requests are being cached now), but that the requests can be modified whilst error capture runs and oops is well known. Just pretending the list iterator is safe does nothing.
-Chris

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Quoted 3 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Hmm, my tree just does:

static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
struct intel_engine_cs *ring)

{
         /*
          * Clear the execlists queue up before freeing the requests, as those
          * are the ones that keep the context and ringbuffer backing objects
          * pinned in place.
          */
         if (i915.enable_execlists) {
                 spin_lock_irq(&ring->execlist_lock);
                 list_splice_tail_init(&ring->execlist_queue,
                                       &ring->execlist_completed);
                 memset(&ring->execlist_port, 0, sizeof(ring->execlist_port));
                 spin_unlock_irq(&ring->execlist_lock);

                 intel_execlists_retire_requests(ring);
         }

Oh, that's right another unreviewed patch from several months ago.

For your patch, you should not take the spinlock unconditionally as it is only initialised for execlists, and you can use the simpler form of irq locking as above.

-Chris

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/i915_gem.c +5 -0

index 1666499..50c2dcd 100644
@@ -2738,6 +2738,8 @@ static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv,
 static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 					struct intel_engine_cs *ring)
 {
+	unsigned long flags;
+
 	while (!list_empty(&ring->active_list)) {
 		struct drm_i915_gem_object *obj;
 

@@ -2753,6 +2755,8 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 	 * are the ones that keep the context and ringbuffer backing objects
 	 * pinned in place.
 	 */
+
+	spin_lock_irqsave(&ring->execlist_lock, flags);
 	while (!list_empty(&ring->execlist_queue)) {
 		struct drm_i915_gem_request *submit_req;
 

@@ -2766,6 +2770,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 
 		i915_gem_request_unreference(submit_req);
 	}
+	spin_unlock_irqrestore(&ring->execlist_lock, flags);
 
 	/*
 	 * We must free the requests after all the corresponding objects have

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 5 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Signed-off-by Tomas Elf <[email protected]>

Should we instead just stop any irqs from the GT completely while we do the reset (plus a synchronize_irq)? It's a bit heavy-weight, but probably safer. Well not the entire GT, but all the rings under reset (as prep for per-ring reset).

Quoted 11 lines · Daniel Vetter

Quoted 5 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Signed-off-by Tomas Elf <[email protected]>

Should we instead just stop any irqs from the GT completely while we do the reset (plus a synchronize_irq)? It's a bit heavy-weight, but probably safer. Well not the entire GT, but all the rings under reset (as prep for per-ring reset).

Bah, stopping IRQs is not enough for error state capture though since requests complete asynchronously just by polling a memory address. (If that is the goal here, this patch just makes execlist_queue access consistent and should only be run once the GPU has been reset and so is categorically idle.)

Quoted 13 lines · Chris Wilson

Quoted 11 lines · Daniel Vetter

Quoted 5 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Signed-off-by Tomas Elf <[email protected]>

Should we instead just stop any irqs from the GT completely while we do the reset (plus a synchronize_irq)? It's a bit heavy-weight, but probably safer. Well not the entire GT, but all the rings under reset (as prep for per-ring reset).

Quoted 5 lines

Bah, stopping IRQs is not enough for error state capture though since requests complete asynchronously just by polling a memory address. (If that is the goal here, this patch just makes execlist_queue access consistent and should only be run once the GPU has been reset and so is categorically idle.)

This is the execlist ELSP tracking, which is execlusively driven by the CTX_SWITCH interrupt signal from each engine.

At least that's been my assumption, and under that assumption I think stalling interrupts should be good enough.

Quoted 20 lines · Daniel Vetter

Quoted 18 lines · Chris Wilson

Quoted 11 lines · Daniel Vetter

Quoted 5 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Signed-off-by Tomas Elf <[email protected]>

Should we instead just stop any irqs from the GT completely while we do the reset (plus a synchronize_irq)? It's a bit heavy-weight, but probably safer. Well not the entire GT, but all the rings under reset (as prep for per-ring reset).

Bah, stopping IRQs is not enough for error state capture though since requests complete asynchronously just by polling a memory address. (If that is the goal here, this patch just makes execlist_queue access consistent and should only be run once the GPU has been reset and so is categorically idle.)

Quoted 5 lines

This is the execlist ELSP tracking, which is execlusively driven by the CTX_SWITCH interrupt signal from each engine.

At least that's been my assumption, and under that assumption I think stalling interrupts should be good enough.

No, because the requests and vma are not coupled to the interrupt in terms of when they can disappear.

Quoted 27 lines · Chris Wilson

Quoted 25 lines · Daniel Vetter

Quoted 13 lines · Chris Wilson

Quoted 11 lines · Daniel Vetter

Quoted 5 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

Signed-off-by Tomas Elf <[email protected]>

Should we instead just stop any irqs from the GT completely while we do the reset (plus a synchronize_irq)? It's a bit heavy-weight, but probably safer. Well not the entire GT, but all the rings under reset (as prep for per-ring reset).

Quoted 5 lines

Bah, stopping IRQs is not enough for error state capture though since requests complete asynchronously just by polling a memory address. (If that is the goal here, this patch just makes execlist_queue access consistent and should only be run once the GPU has been reset and so is categorically idle.)

This is the execlist ELSP tracking, which is execlusively driven by the CTX_SWITCH interrupt signal from each engine.

At least that's been my assumption, and under that assumption I think stalling interrupts should be good enough.

Quoted 2 lines

No, because the requests and vma are not coupled to the interrupt in terms of when they can disappear.

At least today execlist keeps its own reference on requests until the CTX_SWITCH stuff is done to exactly make sure this is the case. And even when we have that fixed up I think we do need to exclude this processing somehow, and the irqsave spinlock seems ok for that. disabling the interrupt itself plus synchronize_irq was really just an idea.

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
  1 file changed, 14 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +8 -8

index e57061a..2c7a0b7 100644
@@ -2753,18 +2753,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
  	 * are the ones that keep the context and ringbuffer backing objects
  	 * pinned in place.
  	 */
-	while (!list_empty(&ring->execlist_queue)) {
-		struct drm_i915_gem_request *submit_req;
 
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct drm_i915_gem_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
+	if (i915.enable_execlists) {
+		spin_lock_irq(&ring->execlist_lock);
+		while (!list_empty(&ring->execlist_queue)) {
+			struct drm_i915_gem_request *submit_req;
 
-		if (submit_req->ctx != ring->default_context)
-			intel_lr_context_unpin(submit_req);
+			submit_req = list_first_entry(&ring->execlist_queue,
+					struct drm_i915_gem_request,
+					execlist_link);
+			list_del(&submit_req->execlist_link);
 

Quoted 7 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

Quoted 4 lines

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

LGTM.

Reviewed-by Dave Gordon <[email protected]>

Quoted 29 lines

drivers/gpu/drm/i915/i915_gem.c +8 -8

index e57061a..2c7a0b7 100644
@@ -2753,18 +2753,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 	 * are the ones that keep the context and ringbuffer backing objects
 	 * pinned in place.
 	 */
-	while (!list_empty(&ring->execlist_queue)) {
-		struct drm_i915_gem_request *submit_req;
 
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct drm_i915_gem_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
+	if (i915.enable_execlists) {
+		spin_lock_irq(&ring->execlist_lock);
+		while (!list_empty(&ring->execlist_queue)) {
+			struct drm_i915_gem_request *submit_req;
 
-		if (submit_req->ctx != ring->default_context)
-			intel_lr_context_unpin(submit_req);
+			submit_req = list_first_entry(&ring->execlist_queue,
+					struct drm_i915_gem_request,
+					execlist_link);
+			list_del(&submit_req->execlist_link);
 

drivers/gpu/drm/i915/i915_gem.c static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, · line 2775

Quoted 10 lines

drivers/gpu/drm/i915/i915_gem.c +6 -1

-		i915_gem_request_unreference(submit_req);
+			if (submit_req->ctx != ring->default_context)
+				intel_lr_context_unpin(submit_req);
+
+			i915_gem_request_unreference(submit_req);
+		}
+		spin_unlock_irq(&ring->execlist_lock);
 	}
 
 	/*

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
  1 file changed, 14 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +6 -8

index e57061a..2c7a0b7 100644
@@ -2753,18 +2753,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
  	 * are the ones that keep the context and ringbuffer backing objects
  	 * pinned in place.
  	 */
-	while (!list_empty(&ring->execlist_queue)) {
-		struct drm_i915_gem_request *submit_req;
 
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct drm_i915_gem_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
+	if (i915.enable_execlists) {
+		spin_lock_irq(&ring->execlist_lock);
+		while (!list_empty(&ring->execlist_queue)) {
+			struct drm_i915_gem_request *submit_req;
 
-		if (submit_req->ctx != ring->default_context)
-			intel_lr_context_unpin(submit_req);
+			submit_req = list_first_entry(&ring->execlist_queue,
+					struct drm_i915_gem_request,

Quoted 36 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

Signed-off-by Tomas Elf <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +6 -8

index e57061a..2c7a0b7 100644
@@ -2753,18 +2753,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 	 * are the ones that keep the context and ringbuffer backing objects
 	 * pinned in place.
 	 */
-	while (!list_empty(&ring->execlist_queue)) {
-		struct drm_i915_gem_request *submit_req;
 
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct drm_i915_gem_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
+	if (i915.enable_execlists) {
+		spin_lock_irq(&ring->execlist_lock);
+		while (!list_empty(&ring->execlist_queue)) {
+			struct drm_i915_gem_request *submit_req;
 
-		if (submit_req->ctx != ring->default_context)
-			intel_lr_context_unpin(submit_req);
+			submit_req = list_first_entry(&ring->execlist_queue,
+					struct drm_i915_gem_request,

drivers/gpu/drm/i915/i915_gem.c static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, · line 2772

Quoted 10 lines

drivers/gpu/drm/i915/i915_gem.c +8 -1

+					execlist_link);
+			list_del(&submit_req->execlist_link);
 
-		i915_gem_request_unreference(submit_req);
+			if (submit_req->ctx != ring->default_context)
+				intel_lr_context_unpin(submit_req);
+
+			i915_gem_request_unreference(submit_req);
+		}
+		spin_unlock_irq(&ring->execlist_lock);

Can't this, in theory at least, end up calling drm_gem_object_unreference in a couple of code paths, which can sleep, while holding a spinlock?

If this cannot happen in practice for some reason it would probably be good to put a comment explaining it.

Or I am missing something?

Quoted 52 lines · Tvrtko Ursulin

Quoted 36 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
  1 file changed, 14 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +6 -8

index e57061a..2c7a0b7 100644
@@ -2753,18 +2753,23 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
  	 * are the ones that keep the context and ringbuffer backing objects
  	 * pinned in place.
  	 */
-	while (!list_empty(&ring->execlist_queue)) {
-		struct drm_i915_gem_request *submit_req;
 
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct drm_i915_gem_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
+	if (i915.enable_execlists) {
+		spin_lock_irq(&ring->execlist_lock);
+		while (!list_empty(&ring->execlist_queue)) {
+			struct drm_i915_gem_request *submit_req;
 
-		if (submit_req->ctx != ring->default_context)
-			intel_lr_context_unpin(submit_req);
+			submit_req = list_first_entry(&ring->execlist_queue,
+					struct drm_i915_gem_request,

drivers/gpu/drm/i915/i915_gem.c static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, · line 2772

Quoted 10 lines

drivers/gpu/drm/i915/i915_gem.c +8 -1

+					execlist_link);
+			list_del(&submit_req->execlist_link);
 
-		i915_gem_request_unreference(submit_req);
+			if (submit_req->ctx != ring->default_context)
+				intel_lr_context_unpin(submit_req);
+
+			i915_gem_request_unreference(submit_req);
+		}
+		spin_unlock_irq(&ring->execlist_lock);

Can't this, in theory at least, end up calling drm_gem_object_unreference in a couple of code paths, which can sleep, while holding a spinlock?

Quoted 4 lines

If this cannot happen in practice for some reason it would probably be good to put a comment explaining it.

Or I am missing something?

It can indeed I think. Tvrtko, since I'm off to KS next week and you have push access, can you pls push the revert if this checks out?

Thanks, Daniel

Quoted 59 lines · Daniel Vetter

Quoted 58 lines · Tvrtko Ursulin

Quoted 41 lines · Tomas Elf

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid
concurrency problems between the context event interrupt handler and the reset
path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

Signed-off-by Tomas Elf <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++++---------
  1 file changed, 14 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +0 -0

index e57061a..2c7a0b7 100644
@@ -2753,18 +2753,23 @@ static void
 i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
       * are the ones that keep the context and ringbuffer backing

objects
* pinned in place.
*/

drivers/gpu/drm/i915/i915_gem.c +6 -8

-    while (!list_empty(&ring->execlist_queue)) {
-        struct drm_i915_gem_request *submit_req;
 
-        submit_req = list_first_entry(&ring->execlist_queue,
-                struct drm_i915_gem_request,
-                execlist_link);
-        list_del(&submit_req->execlist_link);
+    if (i915.enable_execlists) {
+        spin_lock_irq(&ring->execlist_lock);
+        while (!list_empty(&ring->execlist_queue)) {
+            struct drm_i915_gem_request *submit_req;
 
-        if (submit_req->ctx != ring->default_context)
-            intel_lr_context_unpin(submit_req);
+            submit_req = list_first_entry(&ring->execlist_queue,
+                    struct drm_i915_gem_request,

drivers/gpu/drm/i915/i915_gem.c static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, · line 2772

Quoted 10 lines

drivers/gpu/drm/i915/i915_gem.c +8 -1

+                    execlist_link);
+            list_del(&submit_req->execlist_link);
 
-        i915_gem_request_unreference(submit_req);
+            if (submit_req->ctx != ring->default_context)
+                intel_lr_context_unpin(submit_req);
+
+            i915_gem_request_unreference(submit_req);
+        }
+        spin_unlock_irq(&ring->execlist_lock);

Can't this, in theory at least, end up calling drm_gem_object_unreference in
a couple of code paths, which can sleep, while holding a spinlock?

Quoted 8 lines

Quoted 5 lines

If this cannot happen in practice for some reason it would probably be good
to put a comment explaining it.

Or I am missing something?

It can indeed I think. Tvrtko, since I'm off to KS next week and you have push access, can you pls push the revert if this checks out?

I've discussed it with Tvrtko and one solution that seems reasonable is to hold the execlist lock while traversing the execlist queue and moving the elements over to a temporary list. After that we can drop the execlist lock, traverse the temporary list and unreference the elements while not holding lock. That way we can get rid of the element in a synchronized fashion and also unreference without holding the lock. I believe this pattern is used elsewhere (such as during request retirement)

Does that sound like a plan or does anyone have a problem with that solution?

Thanks,
Tomas

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Tomas Elf
Sent: Friday, October 23, 2015 2:09 PM
To: [email protected]
Subject: [Intel-gfx] [PATCH v3 7/8] drm/i915: Grab execlist spinlock to avoid post-reset concurrency issues.

Grab execlist lock when cleaning up execlist queues after GPU reset to avoid concurrency problems between the context event interrupt handler and the reset
path immediately following a GPU reset.

* v2 (Chris Wilson):
Do execlist check and use simpler form of spinlock functions.

* v3 (Tvrtko Ursulin):
Don't hold the execlist_lock while dereferencing the requests since that might lead to sleep while IRQs are disabled.

Signed-off-by Tomas Elf <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +7 -9

index e57061a..b492603 100644
@@ -2753,18 +2753,16 @@ static void i915_gem_reset_ring_cleanup(struct
 drm_i915_private *dev_priv,
 	 * are the ones that keep the context and ringbuffer backing objects
 	 * pinned in place.
 	 */
-	while (!list_empty(&ring->execlist_queue)) {
-		struct drm_i915_gem_request *submit_req;
 
-		submit_req = list_first_entry(&ring->execlist_queue,
-				struct drm_i915_gem_request,
-				execlist_link);
-		list_del(&submit_req->execlist_link);
+	if (i915.enable_execlists) {
+		spin_lock_irq(&ring->execlist_lock);
 
-		if (submit_req->ctx != ring->default_context)
-			intel_lr_context_unpin(submit_req);
+		/* list_splice_tail_init checks for empty lists */
+		list_splice_tail_init(&ring->execlist_queue,
+				      &ring->execlist_retired_req_list);
 
-		i915_gem_request_unreference(submit_req);
+		spin_unlock_irq(&ring->execlist_lock);
+		intel_execlists_retire_requests(ring);
 	}
 

/*
--
1.9.1

When clearing an execlist queue, instead of traversing it and unreferencing all requests while holding the spinlock (which might lead to thread sleeping with IRQs are turned off - bad news!), just move all requests to the retire request list while holding spinlock and then drop spinlock and invoke the execlists request retirement path, which already deals with the intricacies of purging/dereferencing execlist queue requests.

This patch can be considered v3 of:

commit b96db8b81c54ef30485ddb5992d63305d86ea8d3
Author: Tomas Elf <[email protected]>
drm/i915: Grab execlist spinlock to avoid post-reset concurrency issues

This patch assumes v2 of the above patch is part of the baseline, reverts v2 and adds changes on top to turn it into v3.

Signed-off-by Tomas Elf <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
  1 file changed, 4 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +3 -6

index 2c7a0b7..b492603 100644
@@ -2756,20 +2756,13 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 
  	if (i915.enable_execlists) {
  		spin_lock_irq(&ring->execlist_lock);
-		while (!list_empty(&ring->execlist_queue)) {
-			struct drm_i915_gem_request *submit_req;
 
-			submit_req = list_first_entry(&ring->execlist_queue,
-					struct drm_i915_gem_request,
-					execlist_link);
-			list_del(&submit_req->execlist_link);
+		/* list_splice_tail_init checks for empty lists */
+		list_splice_tail_init(&ring->execlist_queue,
+				      &ring->execlist_retired_req_list);
 

Quoted 59 lines · Tomas Elf

When clearing an execlist queue, instead of traversing it and unreferencing all
requests while holding the spinlock (which might lead to thread sleeping with
IRQs are turned off - bad news!), just move all requests to the retire request
list while holding spinlock and then drop spinlock and invoke the execlists
request retirement path, which already deals with the intricacies of purging/dereferencing execlist queue requests.

This patch can be considered v3 of:

commit b96db8b81c54ef30485ddb5992d63305d86ea8d3
Author: Tomas Elf <[email protected]>
drm/i915: Grab execlist spinlock to avoid post-reset concurrency
issues

This patch assumes v2 of the above patch is part of the baseline, reverts v2
and adds changes on top to turn it into v3.

Signed-off-by Tomas Elf <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
  1 file changed, 4 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +4 -11

index 2c7a0b7..b492603 100644
@@ -2756,20 +2756,13 @@ static void i915_gem_reset_ring_cleanup(struct
 drm_i915_private *dev_priv,
 
      if (i915.enable_execlists) {
          spin_lock_irq(&ring->execlist_lock);
-        while (!list_empty(&ring->execlist_queue)) {
-            struct drm_i915_gem_request *submit_req;
 
-            submit_req = list_first_entry(&ring->execlist_queue,
-                    struct drm_i915_gem_request,
-                    execlist_link);
-            list_del(&submit_req->execlist_link);
+        /* list_splice_tail_init checks for empty lists */
+        list_splice_tail_init(&ring->execlist_queue,
+                      &ring->execlist_retired_req_list);
 
-            if (submit_req->ctx != ring->default_context)
-                intel_lr_context_unpin(submit_req);
-
-            i915_gem_request_unreference(submit_req);
-        }
          spin_unlock_irq(&ring->execlist_lock);
+        intel_execlists_retire_requests(ring);
      }
 

Quoted 1 line

      /*

Fallen through the cracks..

This looks to be even more serious, since lockdep notices possible deadlock involving vmap_area_lock:

Possible interrupt unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(vmap_area_lock);
                                local_irq_disable();
                                lock(&(&ring->execlist_lock)->rlock);
                                lock(vmap_area_lock);
   <Interrupt>
     lock(&(&ring->execlist_lock)->rlock);

*** DEADLOCK ***

Because it unpins LRC context and ringbuffer which ends up in the VM code under the execlist_lock.

intel_execlists_retire_requests is slightly different from the code in the reset handler because it concerns itself with ctx_obj existence which the other one doesn't.

Could people more knowledgeable of this code check if it is OK and R-B?

Regards,

Tvrtko

Quoted 88 lines · Tvrtko Ursulin

Quoted 59 lines · Tomas Elf

When clearing an execlist queue, instead of traversing it and unreferencing all
requests while holding the spinlock (which might lead to thread sleeping with
IRQs are turned off - bad news!), just move all requests to the retire request
list while holding spinlock and then drop spinlock and invoke the execlists
request retirement path, which already deals with the intricacies of purging/dereferencing execlist queue requests.

This patch can be considered v3 of:

commit b96db8b81c54ef30485ddb5992d63305d86ea8d3
Author: Tomas Elf <[email protected]>
drm/i915: Grab execlist spinlock to avoid post-reset concurrency
issues

This patch assumes v2 of the above patch is part of the baseline, reverts v2
and adds changes on top to turn it into v3.

Signed-off-by Tomas Elf <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +4 -11

index 2c7a0b7..b492603 100644
@@ -2756,20 +2756,13 @@ static void i915_gem_reset_ring_cleanup(struct
 drm_i915_private *dev_priv,
 
     if (i915.enable_execlists) {
         spin_lock_irq(&ring->execlist_lock);
-        while (!list_empty(&ring->execlist_queue)) {
-            struct drm_i915_gem_request *submit_req;
 
-            submit_req = list_first_entry(&ring->execlist_queue,
-                    struct drm_i915_gem_request,
-                    execlist_link);
-            list_del(&submit_req->execlist_link);
+        /* list_splice_tail_init checks for empty lists */
+        list_splice_tail_init(&ring->execlist_queue,
+                      &ring->execlist_retired_req_list);
 
-            if (submit_req->ctx != ring->default_context)
-                intel_lr_context_unpin(submit_req);
-
-            i915_gem_request_unreference(submit_req);
-        }
         spin_unlock_irq(&ring->execlist_lock);
+        intel_execlists_retire_requests(ring);
     }
 

Quoted 1 line

     /*

Fallen through the cracks..

This looks to be even more serious, since lockdep notices possible deadlock involving vmap_area_lock:

Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(vmap_area_lock);
                               local_irq_disable();
                               lock(&(&ring->execlist_lock)->rlock);
                               lock(vmap_area_lock);
  <Interrupt>
    lock(&(&ring->execlist_lock)->rlock);

*** DEADLOCK ***

Because it unpins LRC context and ringbuffer which ends up in the VM code under the execlist_lock.

intel_execlists_retire_requests is slightly different from the code in the reset handler because it concerns itself with ctx_obj existence which the other one doesn't.

Quoted 5 lines

Could people more knowledgeable of this code check if it is OK and R-B?

Regards,

Tvrtko

Hi Tvrtko,

I didn't understand this message at first, I thought you'd found a problem with this ("v3") patch, but now I see what you actually meant is that there is indeed a problem with the (v2) that got merged, not the original question about unreferencing an object while holding a spinlock (because it can't be the last reference), but rather because of the unpin, which can indeed cause a problem with a non-i915-defined kernel lock.

So we should certainly update the current (v2) upstream with this. Thomas Daniel already R-B'd this code on 23rd October, when it was:

[PATCH v3 7/8] drm/i915: Grab execlist spinlock to avoid post-reset concurrency issues.

and it hasn't changed in substance since then, so you can carry his R-B over, plus I said on that same day that this was a better solution. So:

Reviewed-by Thomas Daniel <[email protected]>

Reviewed-by Dave Gordon <[email protected]>

Quoted 88 lines · Tvrtko Ursulin

Quoted 59 lines · Tomas Elf

When clearing an execlist queue, instead of traversing it and unreferencing all
requests while holding the spinlock (which might lead to thread sleeping with
IRQs are turned off - bad news!), just move all requests to the retire request
list while holding spinlock and then drop spinlock and invoke the execlists
request retirement path, which already deals with the intricacies of purging/dereferencing execlist queue requests.

This patch can be considered v3 of:

commit b96db8b81c54ef30485ddb5992d63305d86ea8d3
Author: Tomas Elf <[email protected]>
drm/i915: Grab execlist spinlock to avoid post-reset concurrency
issues

This patch assumes v2 of the above patch is part of the baseline, reverts v2
and adds changes on top to turn it into v3.

Signed-off-by Tomas Elf <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
  1 file changed, 4 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_gem.c +4 -11

index 2c7a0b7..b492603 100644
@@ -2756,20 +2756,13 @@ static void i915_gem_reset_ring_cleanup(struct
 drm_i915_private *dev_priv,
 
      if (i915.enable_execlists) {
          spin_lock_irq(&ring->execlist_lock);
-        while (!list_empty(&ring->execlist_queue)) {
-            struct drm_i915_gem_request *submit_req;
 
-            submit_req = list_first_entry(&ring->execlist_queue,
-                    struct drm_i915_gem_request,
-                    execlist_link);
-            list_del(&submit_req->execlist_link);
+        /* list_splice_tail_init checks for empty lists */
+        list_splice_tail_init(&ring->execlist_queue,
+                      &ring->execlist_retired_req_list);
 
-            if (submit_req->ctx != ring->default_context)
-                intel_lr_context_unpin(submit_req);
-
-            i915_gem_request_unreference(submit_req);
-        }
          spin_unlock_irq(&ring->execlist_lock);
+        intel_execlists_retire_requests(ring);
      }
 

Quoted 1 line

      /*

Fallen through the cracks..

This looks to be even more serious, since lockdep notices possible deadlock involving vmap_area_lock:

Possible interrupt unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(vmap_area_lock);
                                local_irq_disable();
                                lock(&(&ring->execlist_lock)->rlock);
                                lock(vmap_area_lock);
   <Interrupt>
     lock(&(&ring->execlist_lock)->rlock);

*** DEADLOCK ***

Because it unpins LRC context and ringbuffer which ends up in the VM code under the execlist_lock.

intel_execlists_retire_requests is slightly different from the code in the reset handler because it concerns itself with ctx_obj existence which the other one doesn't.

Quoted 5 lines

Could people more knowledgeable of this code check if it is OK and R-B?

Regards,

Tvrtko

Hi Tvrtko,

I didn't understand this message at first, I thought you'd found a problem with this ("v3") patch, but now I see what you actually meant is that there is indeed a problem with the (v2) that got merged, not the original question about unreferencing an object while holding a spinlock (because it can't be the last reference), but rather because of the unpin, which can indeed cause a problem with a non-i915-defined kernel lock.

So we should certainly update the current (v2) upstream with this. Thomas Daniel already R-B'd this code on 23rd October, when it was:

[PATCH v3 7/8] drm/i915: Grab execlist spinlock to avoid post-reset concurrency issues.

and it hasn't changed in substance since then, so you can carry his R-B over, plus I said on that same day that this was a better solution. So:

Reviewed-by Thomas Daniel <[email protected]>

Reviewed-by Dave Gordon <[email protected]>

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Quoted 2 lines · Tomas Elf

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Nope, the machine reached a point where it cannot possibly reach, we want the OOPS.

Quoted 4 lines · Chris Wilson

Quoted 2 lines · Tomas Elf

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Quoted 2 lines

Nope, the machine reached a point where it cannot possibly reach, we want the OOPS.

Yeah this looks funky ... how did you get to that state? If it indeed blows up in testing then we'd need at least a WARN_ON (and hopefully some ideas what's going wrong).
-Daniel

Quoted 2 lines · Tomas Elf

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Nope, the machine reached a point where it cannot possibly reach, we want the OOPS.
-Chris

Quoted 8 lines · Chris Wilson

Quoted 2 lines · Tomas Elf

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Nope, the machine reached a point where it cannot possibly reach, we want the OOPS.
-Chris

Really? Because if I have this in there I can actually run the long-duration stability tests for 12+ hours rather than just a few hours (it doesn't happen all the time but I've seen it at least once). But, hey, if you want to investigate the reason why we have a NULL there then go ahead. I guess I'll just have to carry this patch for as long as my machine crashes during my TDR testing.

Quoted 10 lines · Tomas Elf

Quoted 8 lines · Chris Wilson

Quoted 2 lines · Tomas Elf

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Nope, the machine reached a point where it cannot possibly reach, we want the OOPS.
-Chris

Quoted 6 lines

Really? Because if I have this in there I can actually run the long-duration stability tests for 12+ hours rather than just a few hours (it doesn't happen all the time but I've seen it at least once). But, hey, if you want to investigate the reason why we have a NULL there then go ahead. I guess I'll just have to carry this patch for as long as my machine crashes during my TDR testing.

You've sat on a critical bug for how long? There's been one recent report that appears to be fallout from Tvrtko's context cleanup, but nothing older, in public at least.
-Chris

Quoted 18 lines · Chris Wilson

Quoted 10 lines · Tomas Elf

Quoted 8 lines · Chris Wilson

Quoted 2 lines · Tomas Elf

Avoid NULL pointer exceptions in the display driver for certain critical cases when unpin_work has turned out to be NULL.

Nope, the machine reached a point where it cannot possibly reach, we want the OOPS.
-Chris

Quoted 6 lines

Really? Because if I have this in there I can actually run the long-duration stability tests for 12+ hours rather than just a few hours (it doesn't happen all the time but I've seen it at least once). But, hey, if you want to investigate the reason why we have a NULL there then go ahead. I guess I'll just have to carry this patch for as long as my machine crashes during my TDR testing.

Quoted 5 lines

You've sat on a critical bug for how long? There's been one recent report that appears to be fallout from Tvrtko's context cleanup, but nothing older, in public at least.
-Chris

Do people typically try to actively hang their machines several times a minute for a 12+ hours at a time? If not then maybe that's why they haven't seen this.

I haven't sat on anything, this has been part of my error state capture improvement patches which I've intended to upstream for several months now. I consider this part of the overall stability issue and grouped it together with all the other patches necessary to make the machine not crash for the duration of my test. I would've upstreamed this series a long time ago if I had actually been given time to work on it but that's another discussion.