All of lore.kernel.org
 help / color / mirror / Atom feed
* [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  7:02 ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22  7:02 UTC (permalink / raw)
  To: intel-gfx

As we start peeking into requests for longer and longer, e.g.
incorporating use of spinlocks when only protected by an
rcu_read_lock(), we need to be careful in how we reset the request when
recycling and need to preserve any barriers that may still be in use as
the request is reset for reuse.

Quoting Linus Torvalds:

> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?

  .. because the object can be accessed (by RCU) after the refcount has
  gone down to zero, and the thing has been released.

  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.

  That flag basically says:

  "I may end up accessing this object *after* it has been free'd,
  because there may be RCU lookups in flight"

  This has nothing to do with constructors. It's ok if the object gets
  reused as an object of the same type and does *not* get
  re-initialized, because we're perfectly fine seeing old stale data.

  What it guarantees is that the slab isn't shared with any other kind
  of object, _and_ that the underlying pages are free'd after an RCU
  quiescent period (so the pages aren't shared with another kind of
  object either during an RCU walk).

  And it doesn't necessarily have to have a constructor, because the
  thing that a RCU walk will care about is

    (a) guaranteed to be an object that *has* been on some RCU list (so
    it's not a "new" object)

    (b) the RCU walk needs to have logic to verify that it's still the
    *same* object and hasn't been re-used as something else.

  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
  immediately, but because it gets reused as the same kind of object,
  the RCU walker can "know" what parts have meaning for re-use, in a way
  it couidn't if the re-use was random.

  That said, it *is* subtle, and people should be careful.

> So the re-use might initialize the fields lazily, not necessarily using a ctor.

  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
  to guard the speculative RCU access section, and use
  "atomic_dec_and_test()" in the freeing section, then you should be
  safe wrt new allocations.

  If you have a completely new allocation that has "random stale
  content", you know that it cannot be on the RCU list, so there is no
  speculative access that can ever see that random content.

  So the only case you need to worry about is a re-use allocation, and
  you know that the refcount will start out as zero even if you don't
  have a constructor.

  So you can think of the refcount itself as always having a zero
  constructor, *BUT* you need to be careful with ordering.

  In particular, whoever does the allocation needs to then set the
  refcount to a non-zero value *after* it has initialized all the other
  fields. And in particular, it needs to make sure that it uses the
  proper memory ordering to do so.

  NOTE! One thing to be very worried about is that re-initializing
  whatever RCU lists means that now the RCU walker may be walking on the
  wrong list so the walker may do the right thing for this particular
  entry, but it may miss walking *other* entries. So then you can get
  spurious lookup failures, because the RCU walker never walked all the
  way to the end of the right list. That ends up being a much more
  subtle bug.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
 drivers/gpu/drm/i915/i915_scheduler.h |  1 +
 drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
 drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
 5 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 00011f9533b6..a558f64186fa 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request *request)
 {
 	struct i915_capture_list *capture;
 
-	capture = request->capture_list;
+	capture = fetch_and_zero(&request->capture_list);
 	while (capture) {
 		struct i915_capture_list *next = capture->next;
 
@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
 		spin_lock(&engine->active.lock);
 		locked = engine;
 	}
-	list_del(&rq->sched.link);
+	list_del_init(&rq->sched.link);
 	spin_unlock_irq(&locked->active.lock);
 }
 
@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
 	return kmem_cache_alloc(global.slab_requests, gfp);
 }
 
+static void __i915_request_ctor(void *arg)
+{
+	struct i915_request *rq = arg;
+
+	spin_lock_init(&rq->lock);
+	i915_sched_node_init(&rq->sched);
+	i915_sw_fence_init(&rq->submit, submit_notify);
+	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
+
+	rq->file_priv = NULL;
+	rq->capture_list = NULL;
+
+	INIT_LIST_HEAD(&rq->execute_cb);
+}
+
 struct i915_request *
 __i915_request_create(struct intel_context *ce, gfp_t gfp)
 {
@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 	rq->engine = ce->engine;
 	rq->ring = ce->ring;
 	rq->execution_mask = ce->engine->mask;
+	rq->flags = 0;
 
 	rcu_assign_pointer(rq->timeline, tl);
 	rq->hwsp_seqno = tl->hwsp_seqno;
@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 
 	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
 
-	spin_lock_init(&rq->lock);
 	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
 		       tl->fence_context, seqno);
 
 	/* We bump the ref for the fence chain */
-	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
-	i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
+	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
+	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
 
-	i915_sched_node_init(&rq->sched);
+	i915_sched_node_reinit(&rq->sched);
 
-	/* No zalloc, must clear what we need by hand */
-	rq->file_priv = NULL;
+	/* No zalloc, everything must be cleared after use */
 	rq->batch = NULL;
-	rq->capture_list = NULL;
-	rq->flags = 0;
-
-	INIT_LIST_HEAD(&rq->execute_cb);
+	GEM_BUG_ON(rq->file_priv);
+	GEM_BUG_ON(rq->capture_list);
+	GEM_BUG_ON(!list_empty(&rq->execute_cb));
 
 	/*
 	 * Reserve space in the ring buffer for all the commands required to
@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
 
 int __init i915_global_request_init(void)
 {
-	global.slab_requests = KMEM_CACHE(i915_request,
-					  SLAB_HWCACHE_ALIGN |
-					  SLAB_RECLAIM_ACCOUNT |
-					  SLAB_TYPESAFE_BY_RCU);
+	global.slab_requests =
+		kmem_cache_create("i915_request",
+				  sizeof(struct i915_request),
+				  __alignof__(struct i915_request),
+				  SLAB_HWCACHE_ALIGN |
+				  SLAB_RECLAIM_ACCOUNT |
+				  SLAB_TYPESAFE_BY_RCU,
+				  __i915_request_ctor);
 	if (!global.slab_requests)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 194246548c4d..a5a6dbe6a53c 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node *node)
 	INIT_LIST_HEAD(&node->signalers_list);
 	INIT_LIST_HEAD(&node->waiters_list);
 	INIT_LIST_HEAD(&node->link);
+}
+
+void i915_sched_node_reinit(struct i915_sched_node *node)
+{
 	node->attr.priority = I915_PRIORITY_INVALID;
 	node->semaphores = 0;
 	node->flags = 0;
@@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->signalers_list);
 
 	/* Remove ourselves from everyone who depends upon us */
 	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
@@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->waiters_list);
 
 	spin_unlock_irq(&schedule_lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
index 07d243acf553..d1dc4efef77b 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -26,6 +26,7 @@
 					 sched.link)
 
 void i915_sched_node_init(struct i915_sched_node *node);
+void i915_sched_node_reinit(struct i915_sched_node *node);
 
 bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
 				      struct i915_sched_node *signal,
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 6a88db291252..eacc6c5ce0fd 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
 	fence->flags = (unsigned long)fn;
 }
 
+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
+{
+	debug_fence_init(fence);
+
+	atomic_set(&fence->pending, 1);
+	fence->error = 0;
+}
+
 void i915_sw_fence_commit(struct i915_sw_fence *fence)
 {
 	debug_fence_activate(fence);
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index ab7d58bd0b9d..1e90d9a51bd2 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -54,6 +54,8 @@ do {								\
 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
 #endif
 
+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
+
 #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
 void i915_sw_fence_fini(struct i915_sw_fence *fence);
 #else
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  7:02 ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22  7:02 UTC (permalink / raw)
  To: intel-gfx

As we start peeking into requests for longer and longer, e.g.
incorporating use of spinlocks when only protected by an
rcu_read_lock(), we need to be careful in how we reset the request when
recycling and need to preserve any barriers that may still be in use as
the request is reset for reuse.

Quoting Linus Torvalds:

> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?

  .. because the object can be accessed (by RCU) after the refcount has
  gone down to zero, and the thing has been released.

  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.

  That flag basically says:

  "I may end up accessing this object *after* it has been free'd,
  because there may be RCU lookups in flight"

  This has nothing to do with constructors. It's ok if the object gets
  reused as an object of the same type and does *not* get
  re-initialized, because we're perfectly fine seeing old stale data.

  What it guarantees is that the slab isn't shared with any other kind
  of object, _and_ that the underlying pages are free'd after an RCU
  quiescent period (so the pages aren't shared with another kind of
  object either during an RCU walk).

  And it doesn't necessarily have to have a constructor, because the
  thing that a RCU walk will care about is

    (a) guaranteed to be an object that *has* been on some RCU list (so
    it's not a "new" object)

    (b) the RCU walk needs to have logic to verify that it's still the
    *same* object and hasn't been re-used as something else.

  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
  immediately, but because it gets reused as the same kind of object,
  the RCU walker can "know" what parts have meaning for re-use, in a way
  it couidn't if the re-use was random.

  That said, it *is* subtle, and people should be careful.

> So the re-use might initialize the fields lazily, not necessarily using a ctor.

  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
  to guard the speculative RCU access section, and use
  "atomic_dec_and_test()" in the freeing section, then you should be
  safe wrt new allocations.

  If you have a completely new allocation that has "random stale
  content", you know that it cannot be on the RCU list, so there is no
  speculative access that can ever see that random content.

  So the only case you need to worry about is a re-use allocation, and
  you know that the refcount will start out as zero even if you don't
  have a constructor.

  So you can think of the refcount itself as always having a zero
  constructor, *BUT* you need to be careful with ordering.

  In particular, whoever does the allocation needs to then set the
  refcount to a non-zero value *after* it has initialized all the other
  fields. And in particular, it needs to make sure that it uses the
  proper memory ordering to do so.

  NOTE! One thing to be very worried about is that re-initializing
  whatever RCU lists means that now the RCU walker may be walking on the
  wrong list so the walker may do the right thing for this particular
  entry, but it may miss walking *other* entries. So then you can get
  spurious lookup failures, because the RCU walker never walked all the
  way to the end of the right list. That ends up being a much more
  subtle bug.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
 drivers/gpu/drm/i915/i915_scheduler.h |  1 +
 drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
 drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
 5 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 00011f9533b6..a558f64186fa 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request *request)
 {
 	struct i915_capture_list *capture;
 
-	capture = request->capture_list;
+	capture = fetch_and_zero(&request->capture_list);
 	while (capture) {
 		struct i915_capture_list *next = capture->next;
 
@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
 		spin_lock(&engine->active.lock);
 		locked = engine;
 	}
-	list_del(&rq->sched.link);
+	list_del_init(&rq->sched.link);
 	spin_unlock_irq(&locked->active.lock);
 }
 
@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
 	return kmem_cache_alloc(global.slab_requests, gfp);
 }
 
+static void __i915_request_ctor(void *arg)
+{
+	struct i915_request *rq = arg;
+
+	spin_lock_init(&rq->lock);
+	i915_sched_node_init(&rq->sched);
+	i915_sw_fence_init(&rq->submit, submit_notify);
+	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
+
+	rq->file_priv = NULL;
+	rq->capture_list = NULL;
+
+	INIT_LIST_HEAD(&rq->execute_cb);
+}
+
 struct i915_request *
 __i915_request_create(struct intel_context *ce, gfp_t gfp)
 {
@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 	rq->engine = ce->engine;
 	rq->ring = ce->ring;
 	rq->execution_mask = ce->engine->mask;
+	rq->flags = 0;
 
 	rcu_assign_pointer(rq->timeline, tl);
 	rq->hwsp_seqno = tl->hwsp_seqno;
@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 
 	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
 
-	spin_lock_init(&rq->lock);
 	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
 		       tl->fence_context, seqno);
 
 	/* We bump the ref for the fence chain */
-	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
-	i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
+	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
+	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
 
-	i915_sched_node_init(&rq->sched);
+	i915_sched_node_reinit(&rq->sched);
 
-	/* No zalloc, must clear what we need by hand */
-	rq->file_priv = NULL;
+	/* No zalloc, everything must be cleared after use */
 	rq->batch = NULL;
-	rq->capture_list = NULL;
-	rq->flags = 0;
-
-	INIT_LIST_HEAD(&rq->execute_cb);
+	GEM_BUG_ON(rq->file_priv);
+	GEM_BUG_ON(rq->capture_list);
+	GEM_BUG_ON(!list_empty(&rq->execute_cb));
 
 	/*
 	 * Reserve space in the ring buffer for all the commands required to
@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
 
 int __init i915_global_request_init(void)
 {
-	global.slab_requests = KMEM_CACHE(i915_request,
-					  SLAB_HWCACHE_ALIGN |
-					  SLAB_RECLAIM_ACCOUNT |
-					  SLAB_TYPESAFE_BY_RCU);
+	global.slab_requests =
+		kmem_cache_create("i915_request",
+				  sizeof(struct i915_request),
+				  __alignof__(struct i915_request),
+				  SLAB_HWCACHE_ALIGN |
+				  SLAB_RECLAIM_ACCOUNT |
+				  SLAB_TYPESAFE_BY_RCU,
+				  __i915_request_ctor);
 	if (!global.slab_requests)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 194246548c4d..a5a6dbe6a53c 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node *node)
 	INIT_LIST_HEAD(&node->signalers_list);
 	INIT_LIST_HEAD(&node->waiters_list);
 	INIT_LIST_HEAD(&node->link);
+}
+
+void i915_sched_node_reinit(struct i915_sched_node *node)
+{
 	node->attr.priority = I915_PRIORITY_INVALID;
 	node->semaphores = 0;
 	node->flags = 0;
@@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->signalers_list);
 
 	/* Remove ourselves from everyone who depends upon us */
 	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
@@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->waiters_list);
 
 	spin_unlock_irq(&schedule_lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
index 07d243acf553..d1dc4efef77b 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -26,6 +26,7 @@
 					 sched.link)
 
 void i915_sched_node_init(struct i915_sched_node *node);
+void i915_sched_node_reinit(struct i915_sched_node *node);
 
 bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
 				      struct i915_sched_node *signal,
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 6a88db291252..eacc6c5ce0fd 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
 	fence->flags = (unsigned long)fn;
 }
 
+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
+{
+	debug_fence_init(fence);
+
+	atomic_set(&fence->pending, 1);
+	fence->error = 0;
+}
+
 void i915_sw_fence_commit(struct i915_sw_fence *fence)
 {
 	debug_fence_activate(fence);
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index ab7d58bd0b9d..1e90d9a51bd2 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -54,6 +54,8 @@ do {								\
 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
 #endif
 
+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
+
 #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
 void i915_sw_fence_fini(struct i915_sw_fence *fence);
 #else
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
@ 2019-11-22  7:46   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22  7:46 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
URL   : https://patchwork.freedesktop.org/series/69824/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7aea4369d8b2 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
-:51: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#51: 
> So the re-use might initialize the fields lazily, not necessarily using a ctor.

total: 0 errors, 1 warnings, 0 checks, 145 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
@ 2019-11-22  7:46   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22  7:46 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
URL   : https://patchwork.freedesktop.org/series/69824/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7aea4369d8b2 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
-:51: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#51: 
> So the re-use might initialize the fields lazily, not necessarily using a ctor.

total: 0 errors, 1 warnings, 0 checks, 145 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
@ 2019-11-22  8:07   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22  8:07 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
URL   : https://patchwork.freedesktop.org/series/69824/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7402 -> Patchwork_15389
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/index.html

Known issues
------------

  Here are the changes found in Patchwork_15389 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-bsw-nick:        [PASS][1] -> [INCOMPLETE][2] ([fdo#105876])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][3] -> [FAIL][4] ([fdo#111045] / [fdo#111096])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload-with-fault-injection:
    - {fi-kbl-7560u}:     [INCOMPLETE][5] ([fdo#109964] / [fdo#112298]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-kbl-7560u/igt@i915_module_load@reload-with-fault-injection.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-kbl-7560u/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [FAIL][7] ([fdo#103167]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#105876]: https://bugs.freedesktop.org/show_bug.cgi?id=105876
  [fdo#109964]: https://bugs.freedesktop.org/show_bug.cgi?id=109964
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#112298]: https://bugs.freedesktop.org/show_bug.cgi?id=112298


Participating hosts (50 -> 44)
------------------------------

  Additional (1): fi-bsw-n3050 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7402 -> Patchwork_15389

  CI-20190529: 20190529
  CI_DRM_7402: d5f0845c4b92c5826d63b76f89866492e0935c1b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5299: 65fed6a79adea14f7bef6d55530da47d7731d370 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15389: 7aea4369d8b295b6280d3327e7c61244fe6b7bbe @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7aea4369d8b2 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
@ 2019-11-22  8:07   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22  8:07 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2)
URL   : https://patchwork.freedesktop.org/series/69824/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7402 -> Patchwork_15389
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/index.html

Known issues
------------

  Here are the changes found in Patchwork_15389 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-bsw-nick:        [PASS][1] -> [INCOMPLETE][2] ([fdo#105876])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-bsw-nick/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][3] -> [FAIL][4] ([fdo#111045] / [fdo#111096])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload-with-fault-injection:
    - {fi-kbl-7560u}:     [INCOMPLETE][5] ([fdo#109964] / [fdo#112298]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-kbl-7560u/igt@i915_module_load@reload-with-fault-injection.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-kbl-7560u/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [FAIL][7] ([fdo#103167]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7402/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#105876]: https://bugs.freedesktop.org/show_bug.cgi?id=105876
  [fdo#109964]: https://bugs.freedesktop.org/show_bug.cgi?id=109964
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#112298]: https://bugs.freedesktop.org/show_bug.cgi?id=112298


Participating hosts (50 -> 44)
------------------------------

  Additional (1): fi-bsw-n3050 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7402 -> Patchwork_15389

  CI-20190529: 20190529
  CI_DRM_7402: d5f0845c4b92c5826d63b76f89866492e0935c1b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5299: 65fed6a79adea14f7bef6d55530da47d7731d370 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15389: 7aea4369d8b295b6280d3327e7c61244fe6b7bbe @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7aea4369d8b2 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15389/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  9:24   ` Tvrtko Ursulin
  0 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2019-11-22  9:24 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 22/11/2019 07:02, Chris Wilson wrote:
> As we start peeking into requests for longer and longer, e.g.
> incorporating use of spinlocks when only protected by an
> rcu_read_lock(), we need to be careful in how we reset the request when
> recycling and need to preserve any barriers that may still be in use as
> the request is reset for reuse.
> 
> Quoting Linus Torvalds:
> 
>> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
> 
>    .. because the object can be accessed (by RCU) after the refcount has
>    gone down to zero, and the thing has been released.
> 
>    That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
> 
>    That flag basically says:
> 
>    "I may end up accessing this object *after* it has been free'd,
>    because there may be RCU lookups in flight"
> 
>    This has nothing to do with constructors. It's ok if the object gets
>    reused as an object of the same type and does *not* get
>    re-initialized, because we're perfectly fine seeing old stale data.
> 
>    What it guarantees is that the slab isn't shared with any other kind
>    of object, _and_ that the underlying pages are free'd after an RCU
>    quiescent period (so the pages aren't shared with another kind of
>    object either during an RCU walk).
> 
>    And it doesn't necessarily have to have a constructor, because the
>    thing that a RCU walk will care about is
> 
>      (a) guaranteed to be an object that *has* been on some RCU list (so
>      it's not a "new" object)
> 
>      (b) the RCU walk needs to have logic to verify that it's still the
>      *same* object and hasn't been re-used as something else.
> 
>    In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
>    immediately, but because it gets reused as the same kind of object,
>    the RCU walker can "know" what parts have meaning for re-use, in a way
>    it couidn't if the re-use was random.
> 
>    That said, it *is* subtle, and people should be careful.
> 
>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> 
>    If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>    to guard the speculative RCU access section, and use
>    "atomic_dec_and_test()" in the freeing section, then you should be
>    safe wrt new allocations.
> 
>    If you have a completely new allocation that has "random stale
>    content", you know that it cannot be on the RCU list, so there is no
>    speculative access that can ever see that random content.
> 
>    So the only case you need to worry about is a re-use allocation, and
>    you know that the refcount will start out as zero even if you don't
>    have a constructor.
> 
>    So you can think of the refcount itself as always having a zero
>    constructor, *BUT* you need to be careful with ordering.
> 
>    In particular, whoever does the allocation needs to then set the
>    refcount to a non-zero value *after* it has initialized all the other
>    fields. And in particular, it needs to make sure that it uses the
>    proper memory ordering to do so.
> 
>    NOTE! One thing to be very worried about is that re-initializing
>    whatever RCU lists means that now the RCU walker may be walking on the
>    wrong list so the walker may do the right thing for this particular
>    entry, but it may miss walking *other* entries. So then you can get
>    spurious lookup failures, because the RCU walker never walked all the
>    way to the end of the right list. That ends up being a much more
>    subtle bug.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Now that you made the commit message so pretty I have not choice but to:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Seriously, I think it looks fine. Am I missing something? Could be I 
guess. Anything more could be done for instance in sw_fence_reinit 
checking the wq head?

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
>   drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
>   drivers/gpu/drm/i915/i915_scheduler.h |  1 +
>   drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
>   drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
>   5 files changed, 50 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 00011f9533b6..a558f64186fa 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request *request)
>   {
>   	struct i915_capture_list *capture;
>   
> -	capture = request->capture_list;
> +	capture = fetch_and_zero(&request->capture_list);
>   	while (capture) {
>   		struct i915_capture_list *next = capture->next;
>   
> @@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
>   		spin_lock(&engine->active.lock);
>   		locked = engine;
>   	}
> -	list_del(&rq->sched.link);
> +	list_del_init(&rq->sched.link);
>   	spin_unlock_irq(&locked->active.lock);
>   }
>   
> @@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
>   	return kmem_cache_alloc(global.slab_requests, gfp);
>   }
>   
> +static void __i915_request_ctor(void *arg)
> +{
> +	struct i915_request *rq = arg;
> +
> +	spin_lock_init(&rq->lock);
> +	i915_sched_node_init(&rq->sched);
> +	i915_sw_fence_init(&rq->submit, submit_notify);
> +	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
> +
> +	rq->file_priv = NULL;
> +	rq->capture_list = NULL;
> +
> +	INIT_LIST_HEAD(&rq->execute_cb);
> +}
> +
>   struct i915_request *
>   __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   {
> @@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   	rq->engine = ce->engine;
>   	rq->ring = ce->ring;
>   	rq->execution_mask = ce->engine->mask;
> +	rq->flags = 0;
>   
>   	rcu_assign_pointer(rq->timeline, tl);
>   	rq->hwsp_seqno = tl->hwsp_seqno;
> @@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   
>   	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>   
> -	spin_lock_init(&rq->lock);
>   	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
>   		       tl->fence_context, seqno);
>   
>   	/* We bump the ref for the fence chain */
> -	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
> -	i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
> +	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
> +	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
>   
> -	i915_sched_node_init(&rq->sched);
> +	i915_sched_node_reinit(&rq->sched);
>   
> -	/* No zalloc, must clear what we need by hand */
> -	rq->file_priv = NULL;
> +	/* No zalloc, everything must be cleared after use */
>   	rq->batch = NULL;
> -	rq->capture_list = NULL;
> -	rq->flags = 0;
> -
> -	INIT_LIST_HEAD(&rq->execute_cb);
> +	GEM_BUG_ON(rq->file_priv);
> +	GEM_BUG_ON(rq->capture_list);
> +	GEM_BUG_ON(!list_empty(&rq->execute_cb));
>   
>   	/*
>   	 * Reserve space in the ring buffer for all the commands required to
> @@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
>   
>   int __init i915_global_request_init(void)
>   {
> -	global.slab_requests = KMEM_CACHE(i915_request,
> -					  SLAB_HWCACHE_ALIGN |
> -					  SLAB_RECLAIM_ACCOUNT |
> -					  SLAB_TYPESAFE_BY_RCU);
> +	global.slab_requests =
> +		kmem_cache_create("i915_request",
> +				  sizeof(struct i915_request),
> +				  __alignof__(struct i915_request),
> +				  SLAB_HWCACHE_ALIGN |
> +				  SLAB_RECLAIM_ACCOUNT |
> +				  SLAB_TYPESAFE_BY_RCU,
> +				  __i915_request_ctor);
>   	if (!global.slab_requests)
>   		return -ENOMEM;
>   
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 194246548c4d..a5a6dbe6a53c 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node *node)
>   	INIT_LIST_HEAD(&node->signalers_list);
>   	INIT_LIST_HEAD(&node->waiters_list);
>   	INIT_LIST_HEAD(&node->link);
> +}
> +
> +void i915_sched_node_reinit(struct i915_sched_node *node)
> +{
>   	node->attr.priority = I915_PRIORITY_INVALID;
>   	node->semaphores = 0;
>   	node->flags = 0;
> @@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
>   		if (dep->flags & I915_DEPENDENCY_ALLOC)
>   			i915_dependency_free(dep);
>   	}
> +	INIT_LIST_HEAD(&node->signalers_list);
>   
>   	/* Remove ourselves from everyone who depends upon us */
>   	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
> @@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
>   		if (dep->flags & I915_DEPENDENCY_ALLOC)
>   			i915_dependency_free(dep);
>   	}
> +	INIT_LIST_HEAD(&node->waiters_list);
>   
>   	spin_unlock_irq(&schedule_lock);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> index 07d243acf553..d1dc4efef77b 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> @@ -26,6 +26,7 @@
>   					 sched.link)
>   
>   void i915_sched_node_init(struct i915_sched_node *node);
> +void i915_sched_node_reinit(struct i915_sched_node *node);
>   
>   bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
>   				      struct i915_sched_node *signal,
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 6a88db291252..eacc6c5ce0fd 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
>   	fence->flags = (unsigned long)fn;
>   }
>   
> +void i915_sw_fence_reinit(struct i915_sw_fence *fence)
> +{
> +	debug_fence_init(fence);
> +
> +	atomic_set(&fence->pending, 1);
> +	fence->error = 0;
> +}
> +
>   void i915_sw_fence_commit(struct i915_sw_fence *fence)
>   {
>   	debug_fence_activate(fence);
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index ab7d58bd0b9d..1e90d9a51bd2 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -54,6 +54,8 @@ do {								\
>   	__i915_sw_fence_init((fence), (fn), NULL, NULL)
>   #endif
>   
> +void i915_sw_fence_reinit(struct i915_sw_fence *fence);
> +
>   #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
>   void i915_sw_fence_fini(struct i915_sw_fence *fence);
>   #else
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  9:24   ` Tvrtko Ursulin
  0 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2019-11-22  9:24 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 22/11/2019 07:02, Chris Wilson wrote:
> As we start peeking into requests for longer and longer, e.g.
> incorporating use of spinlocks when only protected by an
> rcu_read_lock(), we need to be careful in how we reset the request when
> recycling and need to preserve any barriers that may still be in use as
> the request is reset for reuse.
> 
> Quoting Linus Torvalds:
> 
>> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
> 
>    .. because the object can be accessed (by RCU) after the refcount has
>    gone down to zero, and the thing has been released.
> 
>    That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
> 
>    That flag basically says:
> 
>    "I may end up accessing this object *after* it has been free'd,
>    because there may be RCU lookups in flight"
> 
>    This has nothing to do with constructors. It's ok if the object gets
>    reused as an object of the same type and does *not* get
>    re-initialized, because we're perfectly fine seeing old stale data.
> 
>    What it guarantees is that the slab isn't shared with any other kind
>    of object, _and_ that the underlying pages are free'd after an RCU
>    quiescent period (so the pages aren't shared with another kind of
>    object either during an RCU walk).
> 
>    And it doesn't necessarily have to have a constructor, because the
>    thing that a RCU walk will care about is
> 
>      (a) guaranteed to be an object that *has* been on some RCU list (so
>      it's not a "new" object)
> 
>      (b) the RCU walk needs to have logic to verify that it's still the
>      *same* object and hasn't been re-used as something else.
> 
>    In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
>    immediately, but because it gets reused as the same kind of object,
>    the RCU walker can "know" what parts have meaning for re-use, in a way
>    it couidn't if the re-use was random.
> 
>    That said, it *is* subtle, and people should be careful.
> 
>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> 
>    If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>    to guard the speculative RCU access section, and use
>    "atomic_dec_and_test()" in the freeing section, then you should be
>    safe wrt new allocations.
> 
>    If you have a completely new allocation that has "random stale
>    content", you know that it cannot be on the RCU list, so there is no
>    speculative access that can ever see that random content.
> 
>    So the only case you need to worry about is a re-use allocation, and
>    you know that the refcount will start out as zero even if you don't
>    have a constructor.
> 
>    So you can think of the refcount itself as always having a zero
>    constructor, *BUT* you need to be careful with ordering.
> 
>    In particular, whoever does the allocation needs to then set the
>    refcount to a non-zero value *after* it has initialized all the other
>    fields. And in particular, it needs to make sure that it uses the
>    proper memory ordering to do so.
> 
>    NOTE! One thing to be very worried about is that re-initializing
>    whatever RCU lists means that now the RCU walker may be walking on the
>    wrong list so the walker may do the right thing for this particular
>    entry, but it may miss walking *other* entries. So then you can get
>    spurious lookup failures, because the RCU walker never walked all the
>    way to the end of the right list. That ends up being a much more
>    subtle bug.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Now that you made the commit message so pretty I have not choice but to:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Seriously, I think it looks fine. Am I missing something? Could be I 
guess. Anything more could be done for instance in sw_fence_reinit 
checking the wq head?

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
>   drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
>   drivers/gpu/drm/i915/i915_scheduler.h |  1 +
>   drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
>   drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
>   5 files changed, 50 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 00011f9533b6..a558f64186fa 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request *request)
>   {
>   	struct i915_capture_list *capture;
>   
> -	capture = request->capture_list;
> +	capture = fetch_and_zero(&request->capture_list);
>   	while (capture) {
>   		struct i915_capture_list *next = capture->next;
>   
> @@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
>   		spin_lock(&engine->active.lock);
>   		locked = engine;
>   	}
> -	list_del(&rq->sched.link);
> +	list_del_init(&rq->sched.link);
>   	spin_unlock_irq(&locked->active.lock);
>   }
>   
> @@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
>   	return kmem_cache_alloc(global.slab_requests, gfp);
>   }
>   
> +static void __i915_request_ctor(void *arg)
> +{
> +	struct i915_request *rq = arg;
> +
> +	spin_lock_init(&rq->lock);
> +	i915_sched_node_init(&rq->sched);
> +	i915_sw_fence_init(&rq->submit, submit_notify);
> +	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
> +
> +	rq->file_priv = NULL;
> +	rq->capture_list = NULL;
> +
> +	INIT_LIST_HEAD(&rq->execute_cb);
> +}
> +
>   struct i915_request *
>   __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   {
> @@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   	rq->engine = ce->engine;
>   	rq->ring = ce->ring;
>   	rq->execution_mask = ce->engine->mask;
> +	rq->flags = 0;
>   
>   	rcu_assign_pointer(rq->timeline, tl);
>   	rq->hwsp_seqno = tl->hwsp_seqno;
> @@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
>   
>   	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>   
> -	spin_lock_init(&rq->lock);
>   	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
>   		       tl->fence_context, seqno);
>   
>   	/* We bump the ref for the fence chain */
> -	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
> -	i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
> +	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
> +	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
>   
> -	i915_sched_node_init(&rq->sched);
> +	i915_sched_node_reinit(&rq->sched);
>   
> -	/* No zalloc, must clear what we need by hand */
> -	rq->file_priv = NULL;
> +	/* No zalloc, everything must be cleared after use */
>   	rq->batch = NULL;
> -	rq->capture_list = NULL;
> -	rq->flags = 0;
> -
> -	INIT_LIST_HEAD(&rq->execute_cb);
> +	GEM_BUG_ON(rq->file_priv);
> +	GEM_BUG_ON(rq->capture_list);
> +	GEM_BUG_ON(!list_empty(&rq->execute_cb));
>   
>   	/*
>   	 * Reserve space in the ring buffer for all the commands required to
> @@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
>   
>   int __init i915_global_request_init(void)
>   {
> -	global.slab_requests = KMEM_CACHE(i915_request,
> -					  SLAB_HWCACHE_ALIGN |
> -					  SLAB_RECLAIM_ACCOUNT |
> -					  SLAB_TYPESAFE_BY_RCU);
> +	global.slab_requests =
> +		kmem_cache_create("i915_request",
> +				  sizeof(struct i915_request),
> +				  __alignof__(struct i915_request),
> +				  SLAB_HWCACHE_ALIGN |
> +				  SLAB_RECLAIM_ACCOUNT |
> +				  SLAB_TYPESAFE_BY_RCU,
> +				  __i915_request_ctor);
>   	if (!global.slab_requests)
>   		return -ENOMEM;
>   
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 194246548c4d..a5a6dbe6a53c 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node *node)
>   	INIT_LIST_HEAD(&node->signalers_list);
>   	INIT_LIST_HEAD(&node->waiters_list);
>   	INIT_LIST_HEAD(&node->link);
> +}
> +
> +void i915_sched_node_reinit(struct i915_sched_node *node)
> +{
>   	node->attr.priority = I915_PRIORITY_INVALID;
>   	node->semaphores = 0;
>   	node->flags = 0;
> @@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
>   		if (dep->flags & I915_DEPENDENCY_ALLOC)
>   			i915_dependency_free(dep);
>   	}
> +	INIT_LIST_HEAD(&node->signalers_list);
>   
>   	/* Remove ourselves from everyone who depends upon us */
>   	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
> @@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
>   		if (dep->flags & I915_DEPENDENCY_ALLOC)
>   			i915_dependency_free(dep);
>   	}
> +	INIT_LIST_HEAD(&node->waiters_list);
>   
>   	spin_unlock_irq(&schedule_lock);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> index 07d243acf553..d1dc4efef77b 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> @@ -26,6 +26,7 @@
>   					 sched.link)
>   
>   void i915_sched_node_init(struct i915_sched_node *node);
> +void i915_sched_node_reinit(struct i915_sched_node *node);
>   
>   bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
>   				      struct i915_sched_node *signal,
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 6a88db291252..eacc6c5ce0fd 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
>   	fence->flags = (unsigned long)fn;
>   }
>   
> +void i915_sw_fence_reinit(struct i915_sw_fence *fence)
> +{
> +	debug_fence_init(fence);
> +
> +	atomic_set(&fence->pending, 1);
> +	fence->error = 0;
> +}
> +
>   void i915_sw_fence_commit(struct i915_sw_fence *fence)
>   {
>   	debug_fence_activate(fence);
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index ab7d58bd0b9d..1e90d9a51bd2 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -54,6 +54,8 @@ do {								\
>   	__i915_sw_fence_init((fence), (fn), NULL, NULL)
>   #endif
>   
> +void i915_sw_fence_reinit(struct i915_sw_fence *fence);
> +
>   #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
>   void i915_sw_fence_fini(struct i915_sw_fence *fence);
>   #else
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  9:33     ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22  9:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2019-11-22 09:24:56)
> 
> On 22/11/2019 07:02, Chris Wilson wrote:
> > As we start peeking into requests for longer and longer, e.g.
> > incorporating use of spinlocks when only protected by an
> > rcu_read_lock(), we need to be careful in how we reset the request when
> > recycling and need to preserve any barriers that may still be in use as
> > the request is reset for reuse.
> > 
> > Quoting Linus Torvalds:
> > 
> >> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
> > 
> >    .. because the object can be accessed (by RCU) after the refcount has
> >    gone down to zero, and the thing has been released.
> > 
> >    That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
> > 
> >    That flag basically says:
> > 
> >    "I may end up accessing this object *after* it has been free'd,
> >    because there may be RCU lookups in flight"
> > 
> >    This has nothing to do with constructors. It's ok if the object gets
> >    reused as an object of the same type and does *not* get
> >    re-initialized, because we're perfectly fine seeing old stale data.
> > 
> >    What it guarantees is that the slab isn't shared with any other kind
> >    of object, _and_ that the underlying pages are free'd after an RCU
> >    quiescent period (so the pages aren't shared with another kind of
> >    object either during an RCU walk).
> > 
> >    And it doesn't necessarily have to have a constructor, because the
> >    thing that a RCU walk will care about is
> > 
> >      (a) guaranteed to be an object that *has* been on some RCU list (so
> >      it's not a "new" object)
> > 
> >      (b) the RCU walk needs to have logic to verify that it's still the
> >      *same* object and hasn't been re-used as something else.
> > 
> >    In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
> >    immediately, but because it gets reused as the same kind of object,
> >    the RCU walker can "know" what parts have meaning for re-use, in a way
> >    it couidn't if the re-use was random.
> > 
> >    That said, it *is* subtle, and people should be careful.
> > 
> >> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> > 
> >    If you have a well-defined refcount, and use "atomic_inc_not_zero()"
> >    to guard the speculative RCU access section, and use
> >    "atomic_dec_and_test()" in the freeing section, then you should be
> >    safe wrt new allocations.
> > 
> >    If you have a completely new allocation that has "random stale
> >    content", you know that it cannot be on the RCU list, so there is no
> >    speculative access that can ever see that random content.
> > 
> >    So the only case you need to worry about is a re-use allocation, and
> >    you know that the refcount will start out as zero even if you don't
> >    have a constructor.
> > 
> >    So you can think of the refcount itself as always having a zero
> >    constructor, *BUT* you need to be careful with ordering.
> > 
> >    In particular, whoever does the allocation needs to then set the
> >    refcount to a non-zero value *after* it has initialized all the other
> >    fields. And in particular, it needs to make sure that it uses the
> >    proper memory ordering to do so.
> > 
> >    NOTE! One thing to be very worried about is that re-initializing
> >    whatever RCU lists means that now the RCU walker may be walking on the
> >    wrong list so the walker may do the right thing for this particular
> >    entry, but it may miss walking *other* entries. So then you can get
> >    spurious lookup failures, because the RCU walker never walked all the
> >    way to the end of the right list. That ends up being a much more
> >    subtle bug.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Now that you made the commit message so pretty I have not choice but to:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Seriously, I think it looks fine. Am I missing something? Could be I 
> guess. Anything more could be done for instance in sw_fence_reinit 
> checking the wq head?

wq head is a good call. I was looking at pending, decided that was
untestable and stopped there.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  9:33     ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22  9:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2019-11-22 09:24:56)
> 
> On 22/11/2019 07:02, Chris Wilson wrote:
> > As we start peeking into requests for longer and longer, e.g.
> > incorporating use of spinlocks when only protected by an
> > rcu_read_lock(), we need to be careful in how we reset the request when
> > recycling and need to preserve any barriers that may still be in use as
> > the request is reset for reuse.
> > 
> > Quoting Linus Torvalds:
> > 
> >> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
> > 
> >    .. because the object can be accessed (by RCU) after the refcount has
> >    gone down to zero, and the thing has been released.
> > 
> >    That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
> > 
> >    That flag basically says:
> > 
> >    "I may end up accessing this object *after* it has been free'd,
> >    because there may be RCU lookups in flight"
> > 
> >    This has nothing to do with constructors. It's ok if the object gets
> >    reused as an object of the same type and does *not* get
> >    re-initialized, because we're perfectly fine seeing old stale data.
> > 
> >    What it guarantees is that the slab isn't shared with any other kind
> >    of object, _and_ that the underlying pages are free'd after an RCU
> >    quiescent period (so the pages aren't shared with another kind of
> >    object either during an RCU walk).
> > 
> >    And it doesn't necessarily have to have a constructor, because the
> >    thing that a RCU walk will care about is
> > 
> >      (a) guaranteed to be an object that *has* been on some RCU list (so
> >      it's not a "new" object)
> > 
> >      (b) the RCU walk needs to have logic to verify that it's still the
> >      *same* object and hasn't been re-used as something else.
> > 
> >    In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
> >    immediately, but because it gets reused as the same kind of object,
> >    the RCU walker can "know" what parts have meaning for re-use, in a way
> >    it couidn't if the re-use was random.
> > 
> >    That said, it *is* subtle, and people should be careful.
> > 
> >> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> > 
> >    If you have a well-defined refcount, and use "atomic_inc_not_zero()"
> >    to guard the speculative RCU access section, and use
> >    "atomic_dec_and_test()" in the freeing section, then you should be
> >    safe wrt new allocations.
> > 
> >    If you have a completely new allocation that has "random stale
> >    content", you know that it cannot be on the RCU list, so there is no
> >    speculative access that can ever see that random content.
> > 
> >    So the only case you need to worry about is a re-use allocation, and
> >    you know that the refcount will start out as zero even if you don't
> >    have a constructor.
> > 
> >    So you can think of the refcount itself as always having a zero
> >    constructor, *BUT* you need to be careful with ordering.
> > 
> >    In particular, whoever does the allocation needs to then set the
> >    refcount to a non-zero value *after* it has initialized all the other
> >    fields. And in particular, it needs to make sure that it uses the
> >    proper memory ordering to do so.
> > 
> >    NOTE! One thing to be very worried about is that re-initializing
> >    whatever RCU lists means that now the RCU walker may be walking on the
> >    wrong list so the walker may do the right thing for this particular
> >    entry, but it may miss walking *other* entries. So then you can get
> >    spurious lookup failures, because the RCU walker never walked all the
> >    way to the end of the right list. That ends up being a much more
> >    subtle bug.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Now that you made the commit message so pretty I have not choice but to:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Seriously, I think it looks fine. Am I missing something? Could be I 
> guess. Anything more could be done for instance in sw_fence_reinit 
> checking the wq head?

wq head is a good call. I was looking at pending, decided that was
untestable and stopped there.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  9:49   ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22  9:49 UTC (permalink / raw)
  To: intel-gfx

As we start peeking into requests for longer and longer, e.g.
incorporating use of spinlocks when only protected by an
rcu_read_lock(), we need to be careful in how we reset the request when
recycling and need to preserve any barriers that may still be in use as
the request is reset for reuse.

Quoting Linus Torvalds:

> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?

  .. because the object can be accessed (by RCU) after the refcount has
  gone down to zero, and the thing has been released.

  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.

  That flag basically says:

  "I may end up accessing this object *after* it has been free'd,
  because there may be RCU lookups in flight"

  This has nothing to do with constructors. It's ok if the object gets
  reused as an object of the same type and does *not* get
  re-initialized, because we're perfectly fine seeing old stale data.

  What it guarantees is that the slab isn't shared with any other kind
  of object, _and_ that the underlying pages are free'd after an RCU
  quiescent period (so the pages aren't shared with another kind of
  object either during an RCU walk).

  And it doesn't necessarily have to have a constructor, because the
  thing that a RCU walk will care about is

    (a) guaranteed to be an object that *has* been on some RCU list (so
    it's not a "new" object)

    (b) the RCU walk needs to have logic to verify that it's still the
    *same* object and hasn't been re-used as something else.

  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
  immediately, but because it gets reused as the same kind of object,
  the RCU walker can "know" what parts have meaning for re-use, in a way
  it couidn't if the re-use was random.

  That said, it *is* subtle, and people should be careful.

> So the re-use might initialize the fields lazily, not necessarily using a ctor.

  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
  to guard the speculative RCU access section, and use
  "atomic_dec_and_test()" in the freeing section, then you should be
  safe wrt new allocations.

  If you have a completely new allocation that has "random stale
  content", you know that it cannot be on the RCU list, so there is no
  speculative access that can ever see that random content.

  So the only case you need to worry about is a re-use allocation, and
  you know that the refcount will start out as zero even if you don't
  have a constructor.

  So you can think of the refcount itself as always having a zero
  constructor, *BUT* you need to be careful with ordering.

  In particular, whoever does the allocation needs to then set the
  refcount to a non-zero value *after* it has initialized all the other
  fields. And in particular, it needs to make sure that it uses the
  proper memory ordering to do so.

  NOTE! One thing to be very worried about is that re-initializing
  whatever RCU lists means that now the RCU walker may be walking on the
  wrong list so the walker may do the right thing for this particular
  entry, but it may miss walking *other* entries. So then you can get
  spurious lookup failures, because the RCU walker never walked all the
  way to the end of the right list. That ends up being a much more
  subtle bug.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_scheduler.c | 12 +++++++
 drivers/gpu/drm/i915/i915_scheduler.h |  1 +
 drivers/gpu/drm/i915/i915_sw_fence.c  | 18 ++++++++--
 drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
 5 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 00011f9533b6..a558f64186fa 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request *request)
 {
 	struct i915_capture_list *capture;
 
-	capture = request->capture_list;
+	capture = fetch_and_zero(&request->capture_list);
 	while (capture) {
 		struct i915_capture_list *next = capture->next;
 
@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
 		spin_lock(&engine->active.lock);
 		locked = engine;
 	}
-	list_del(&rq->sched.link);
+	list_del_init(&rq->sched.link);
 	spin_unlock_irq(&locked->active.lock);
 }
 
@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
 	return kmem_cache_alloc(global.slab_requests, gfp);
 }
 
+static void __i915_request_ctor(void *arg)
+{
+	struct i915_request *rq = arg;
+
+	spin_lock_init(&rq->lock);
+	i915_sched_node_init(&rq->sched);
+	i915_sw_fence_init(&rq->submit, submit_notify);
+	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
+
+	rq->file_priv = NULL;
+	rq->capture_list = NULL;
+
+	INIT_LIST_HEAD(&rq->execute_cb);
+}
+
 struct i915_request *
 __i915_request_create(struct intel_context *ce, gfp_t gfp)
 {
@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 	rq->engine = ce->engine;
 	rq->ring = ce->ring;
 	rq->execution_mask = ce->engine->mask;
+	rq->flags = 0;
 
 	rcu_assign_pointer(rq->timeline, tl);
 	rq->hwsp_seqno = tl->hwsp_seqno;
@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 
 	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
 
-	spin_lock_init(&rq->lock);
 	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
 		       tl->fence_context, seqno);
 
 	/* We bump the ref for the fence chain */
-	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
-	i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
+	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
+	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
 
-	i915_sched_node_init(&rq->sched);
+	i915_sched_node_reinit(&rq->sched);
 
-	/* No zalloc, must clear what we need by hand */
-	rq->file_priv = NULL;
+	/* No zalloc, everything must be cleared after use */
 	rq->batch = NULL;
-	rq->capture_list = NULL;
-	rq->flags = 0;
-
-	INIT_LIST_HEAD(&rq->execute_cb);
+	GEM_BUG_ON(rq->file_priv);
+	GEM_BUG_ON(rq->capture_list);
+	GEM_BUG_ON(!list_empty(&rq->execute_cb));
 
 	/*
 	 * Reserve space in the ring buffer for all the commands required to
@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
 
 int __init i915_global_request_init(void)
 {
-	global.slab_requests = KMEM_CACHE(i915_request,
-					  SLAB_HWCACHE_ALIGN |
-					  SLAB_RECLAIM_ACCOUNT |
-					  SLAB_TYPESAFE_BY_RCU);
+	global.slab_requests =
+		kmem_cache_create("i915_request",
+				  sizeof(struct i915_request),
+				  __alignof__(struct i915_request),
+				  SLAB_HWCACHE_ALIGN |
+				  SLAB_RECLAIM_ACCOUNT |
+				  SLAB_TYPESAFE_BY_RCU,
+				  __i915_request_ctor);
 	if (!global.slab_requests)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 194246548c4d..e3b0a7c4a741 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -387,9 +387,19 @@ void i915_sched_node_init(struct i915_sched_node *node)
 	INIT_LIST_HEAD(&node->signalers_list);
 	INIT_LIST_HEAD(&node->waiters_list);
 	INIT_LIST_HEAD(&node->link);
+
+	i915_sched_node_reinit(node);
+}
+
+void i915_sched_node_reinit(struct i915_sched_node *node)
+{
 	node->attr.priority = I915_PRIORITY_INVALID;
 	node->semaphores = 0;
 	node->flags = 0;
+
+	GEM_BUG_ON(!list_empty(&node->signalers_list));
+	GEM_BUG_ON(!list_empty(&node->waiters_list));
+	GEM_BUG_ON(!list_empty(&node->link));
 }
 
 static struct i915_dependency *
@@ -481,6 +491,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->signalers_list);
 
 	/* Remove ourselves from everyone who depends upon us */
 	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
@@ -491,6 +502,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->waiters_list);
 
 	spin_unlock_irq(&schedule_lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
index 07d243acf553..d1dc4efef77b 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -26,6 +26,7 @@
 					 sched.link)
 
 void i915_sched_node_init(struct i915_sched_node *node);
+void i915_sched_node_reinit(struct i915_sched_node *node);
 
 bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
 				      struct i915_sched_node *signal,
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 6a88db291252..1584f34a6bf9 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -12,6 +12,12 @@
 #include "i915_sw_fence.h"
 #include "i915_selftest.h"
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
+#define I915_SW_FENCE_BUG_ON(expr) BUG_ON(expr)
+#else
+#define I915_SW_FENCE_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
+#endif
+
 #define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for safety */
 
 static DEFINE_SPINLOCK(i915_sw_fence_lock);
@@ -218,13 +224,21 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
 {
 	BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK);
 
+	__init_waitqueue_head(&fence->wait, name, key);
+	fence->flags = (unsigned long)fn;
+
+	i915_sw_fence_reinit(fence);
+}
+
+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
+{
 	debug_fence_init(fence);
 
-	__init_waitqueue_head(&fence->wait, name, key);
 	atomic_set(&fence->pending, 1);
 	fence->error = 0;
 
-	fence->flags = (unsigned long)fn;
+	I915_SW_FENCE_BUG_ON(!fence->flags);
+	I915_SW_FENCE_BUG_ON(!list_empty(&fence->wait.head));
 }
 
 void i915_sw_fence_commit(struct i915_sw_fence *fence)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index ab7d58bd0b9d..1e90d9a51bd2 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -54,6 +54,8 @@ do {								\
 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
 #endif
 
+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
+
 #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
 void i915_sw_fence_fini(struct i915_sw_fence *fence);
 #else
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [Intel-gfx] [PATCH] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22  9:49   ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22  9:49 UTC (permalink / raw)
  To: intel-gfx

As we start peeking into requests for longer and longer, e.g.
incorporating use of spinlocks when only protected by an
rcu_read_lock(), we need to be careful in how we reset the request when
recycling and need to preserve any barriers that may still be in use as
the request is reset for reuse.

Quoting Linus Torvalds:

> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?

  .. because the object can be accessed (by RCU) after the refcount has
  gone down to zero, and the thing has been released.

  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.

  That flag basically says:

  "I may end up accessing this object *after* it has been free'd,
  because there may be RCU lookups in flight"

  This has nothing to do with constructors. It's ok if the object gets
  reused as an object of the same type and does *not* get
  re-initialized, because we're perfectly fine seeing old stale data.

  What it guarantees is that the slab isn't shared with any other kind
  of object, _and_ that the underlying pages are free'd after an RCU
  quiescent period (so the pages aren't shared with another kind of
  object either during an RCU walk).

  And it doesn't necessarily have to have a constructor, because the
  thing that a RCU walk will care about is

    (a) guaranteed to be an object that *has* been on some RCU list (so
    it's not a "new" object)

    (b) the RCU walk needs to have logic to verify that it's still the
    *same* object and hasn't been re-used as something else.

  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
  immediately, but because it gets reused as the same kind of object,
  the RCU walker can "know" what parts have meaning for re-use, in a way
  it couidn't if the re-use was random.

  That said, it *is* subtle, and people should be careful.

> So the re-use might initialize the fields lazily, not necessarily using a ctor.

  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
  to guard the speculative RCU access section, and use
  "atomic_dec_and_test()" in the freeing section, then you should be
  safe wrt new allocations.

  If you have a completely new allocation that has "random stale
  content", you know that it cannot be on the RCU list, so there is no
  speculative access that can ever see that random content.

  So the only case you need to worry about is a re-use allocation, and
  you know that the refcount will start out as zero even if you don't
  have a constructor.

  So you can think of the refcount itself as always having a zero
  constructor, *BUT* you need to be careful with ordering.

  In particular, whoever does the allocation needs to then set the
  refcount to a non-zero value *after* it has initialized all the other
  fields. And in particular, it needs to make sure that it uses the
  proper memory ordering to do so.

  NOTE! One thing to be very worried about is that re-initializing
  whatever RCU lists means that now the RCU walker may be walking on the
  wrong list so the walker may do the right thing for this particular
  entry, but it may miss walking *other* entries. So then you can get
  spurious lookup failures, because the RCU walker never walked all the
  way to the end of the right list. That ends up being a much more
  subtle bug.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_scheduler.c | 12 +++++++
 drivers/gpu/drm/i915/i915_scheduler.h |  1 +
 drivers/gpu/drm/i915/i915_sw_fence.c  | 18 ++++++++--
 drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
 5 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 00011f9533b6..a558f64186fa 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request *request)
 {
 	struct i915_capture_list *capture;
 
-	capture = request->capture_list;
+	capture = fetch_and_zero(&request->capture_list);
 	while (capture) {
 		struct i915_capture_list *next = capture->next;
 
@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
 		spin_lock(&engine->active.lock);
 		locked = engine;
 	}
-	list_del(&rq->sched.link);
+	list_del_init(&rq->sched.link);
 	spin_unlock_irq(&locked->active.lock);
 }
 
@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
 	return kmem_cache_alloc(global.slab_requests, gfp);
 }
 
+static void __i915_request_ctor(void *arg)
+{
+	struct i915_request *rq = arg;
+
+	spin_lock_init(&rq->lock);
+	i915_sched_node_init(&rq->sched);
+	i915_sw_fence_init(&rq->submit, submit_notify);
+	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
+
+	rq->file_priv = NULL;
+	rq->capture_list = NULL;
+
+	INIT_LIST_HEAD(&rq->execute_cb);
+}
+
 struct i915_request *
 __i915_request_create(struct intel_context *ce, gfp_t gfp)
 {
@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 	rq->engine = ce->engine;
 	rq->ring = ce->ring;
 	rq->execution_mask = ce->engine->mask;
+	rq->flags = 0;
 
 	rcu_assign_pointer(rq->timeline, tl);
 	rq->hwsp_seqno = tl->hwsp_seqno;
@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
 
 	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
 
-	spin_lock_init(&rq->lock);
 	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
 		       tl->fence_context, seqno);
 
 	/* We bump the ref for the fence chain */
-	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
-	i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
+	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
+	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
 
-	i915_sched_node_init(&rq->sched);
+	i915_sched_node_reinit(&rq->sched);
 
-	/* No zalloc, must clear what we need by hand */
-	rq->file_priv = NULL;
+	/* No zalloc, everything must be cleared after use */
 	rq->batch = NULL;
-	rq->capture_list = NULL;
-	rq->flags = 0;
-
-	INIT_LIST_HEAD(&rq->execute_cb);
+	GEM_BUG_ON(rq->file_priv);
+	GEM_BUG_ON(rq->capture_list);
+	GEM_BUG_ON(!list_empty(&rq->execute_cb));
 
 	/*
 	 * Reserve space in the ring buffer for all the commands required to
@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
 
 int __init i915_global_request_init(void)
 {
-	global.slab_requests = KMEM_CACHE(i915_request,
-					  SLAB_HWCACHE_ALIGN |
-					  SLAB_RECLAIM_ACCOUNT |
-					  SLAB_TYPESAFE_BY_RCU);
+	global.slab_requests =
+		kmem_cache_create("i915_request",
+				  sizeof(struct i915_request),
+				  __alignof__(struct i915_request),
+				  SLAB_HWCACHE_ALIGN |
+				  SLAB_RECLAIM_ACCOUNT |
+				  SLAB_TYPESAFE_BY_RCU,
+				  __i915_request_ctor);
 	if (!global.slab_requests)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 194246548c4d..e3b0a7c4a741 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -387,9 +387,19 @@ void i915_sched_node_init(struct i915_sched_node *node)
 	INIT_LIST_HEAD(&node->signalers_list);
 	INIT_LIST_HEAD(&node->waiters_list);
 	INIT_LIST_HEAD(&node->link);
+
+	i915_sched_node_reinit(node);
+}
+
+void i915_sched_node_reinit(struct i915_sched_node *node)
+{
 	node->attr.priority = I915_PRIORITY_INVALID;
 	node->semaphores = 0;
 	node->flags = 0;
+
+	GEM_BUG_ON(!list_empty(&node->signalers_list));
+	GEM_BUG_ON(!list_empty(&node->waiters_list));
+	GEM_BUG_ON(!list_empty(&node->link));
 }
 
 static struct i915_dependency *
@@ -481,6 +491,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->signalers_list);
 
 	/* Remove ourselves from everyone who depends upon us */
 	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
@@ -491,6 +502,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
 		if (dep->flags & I915_DEPENDENCY_ALLOC)
 			i915_dependency_free(dep);
 	}
+	INIT_LIST_HEAD(&node->waiters_list);
 
 	spin_unlock_irq(&schedule_lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
index 07d243acf553..d1dc4efef77b 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -26,6 +26,7 @@
 					 sched.link)
 
 void i915_sched_node_init(struct i915_sched_node *node);
+void i915_sched_node_reinit(struct i915_sched_node *node);
 
 bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
 				      struct i915_sched_node *signal,
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 6a88db291252..1584f34a6bf9 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -12,6 +12,12 @@
 #include "i915_sw_fence.h"
 #include "i915_selftest.h"
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
+#define I915_SW_FENCE_BUG_ON(expr) BUG_ON(expr)
+#else
+#define I915_SW_FENCE_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
+#endif
+
 #define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for safety */
 
 static DEFINE_SPINLOCK(i915_sw_fence_lock);
@@ -218,13 +224,21 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
 {
 	BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK);
 
+	__init_waitqueue_head(&fence->wait, name, key);
+	fence->flags = (unsigned long)fn;
+
+	i915_sw_fence_reinit(fence);
+}
+
+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
+{
 	debug_fence_init(fence);
 
-	__init_waitqueue_head(&fence->wait, name, key);
 	atomic_set(&fence->pending, 1);
 	fence->error = 0;
 
-	fence->flags = (unsigned long)fn;
+	I915_SW_FENCE_BUG_ON(!fence->flags);
+	I915_SW_FENCE_BUG_ON(!list_empty(&fence->wait.head));
 }
 
 void i915_sw_fence_commit(struct i915_sw_fence *fence)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index ab7d58bd0b9d..1e90d9a51bd2 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -54,6 +54,8 @@ do {								\
 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
 #endif
 
+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
+
 #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
 void i915_sw_fence_fini(struct i915_sw_fence *fence);
 #else
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
@ 2019-11-22 10:12   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22 10:12 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
URL   : https://patchwork.freedesktop.org/series/69824/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c125a9e24761 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
-:51: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#51: 
> So the re-use might initialize the fields lazily, not necessarily using a ctor.

-:249: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#249: FILE: drivers/gpu/drm/i915/i915_sw_fence.c:16:
+#define I915_SW_FENCE_BUG_ON(expr) BUG_ON(expr)

total: 0 errors, 2 warnings, 0 checks, 175 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
@ 2019-11-22 10:12   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22 10:12 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
URL   : https://patchwork.freedesktop.org/series/69824/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c125a9e24761 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
-:51: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#51: 
> So the re-use might initialize the fields lazily, not necessarily using a ctor.

-:249: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#249: FILE: drivers/gpu/drm/i915/i915_sw_fence.c:16:
+#define I915_SW_FENCE_BUG_ON(expr) BUG_ON(expr)

total: 0 errors, 2 warnings, 0 checks, 175 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
@ 2019-11-22 10:33   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22 10:33 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
URL   : https://patchwork.freedesktop.org/series/69824/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7403 -> Patchwork_15390
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/index.html

Known issues
------------

  Here are the changes found in Patchwork_15390 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-bsw-nick:        [PASS][1] -> [INCOMPLETE][2] ([fdo# 111542])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-bsw-nick/igt@i915_selftest@live_gem_contexts.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-bsw-nick/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-icl-u2:          [PASS][3] -> [FAIL][4] ([fdo#109483])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-icl-u2/igt@kms_chamelium@hdmi-edid-read.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-icl-u2/igt@kms_chamelium@hdmi-edid-read.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-bsw-n3050:       [INCOMPLETE][5] ([fdo# 111542]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-bsw-n3050/igt@i915_selftest@live_gem_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-bsw-n3050/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][7] ([fdo#111045] / [fdo#111096]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo# 111542]: https://bugs.freedesktop.org/show_bug.cgi?id= 111542
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096


Participating hosts (51 -> 44)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7403 -> Patchwork_15390

  CI-20190529: 20190529
  CI_DRM_7403: 4f8ab08e9231940b8fb83c56e83f8e31572c64be @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5299: 65fed6a79adea14f7bef6d55530da47d7731d370 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15390: c125a9e2476148b489a349f3947c4b327bf29bbc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c125a9e24761 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
@ 2019-11-22 10:33   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-22 10:33 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
URL   : https://patchwork.freedesktop.org/series/69824/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7403 -> Patchwork_15390
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/index.html

Known issues
------------

  Here are the changes found in Patchwork_15390 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-bsw-nick:        [PASS][1] -> [INCOMPLETE][2] ([fdo# 111542])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-bsw-nick/igt@i915_selftest@live_gem_contexts.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-bsw-nick/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-icl-u2:          [PASS][3] -> [FAIL][4] ([fdo#109483])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-icl-u2/igt@kms_chamelium@hdmi-edid-read.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-icl-u2/igt@kms_chamelium@hdmi-edid-read.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-bsw-n3050:       [INCOMPLETE][5] ([fdo# 111542]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-bsw-n3050/igt@i915_selftest@live_gem_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-bsw-n3050/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][7] ([fdo#111045] / [fdo#111096]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo# 111542]: https://bugs.freedesktop.org/show_bug.cgi?id= 111542
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096


Participating hosts (51 -> 44)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7403 -> Patchwork_15390

  CI-20190529: 20190529
  CI_DRM_7403: 4f8ab08e9231940b8fb83c56e83f8e31572c64be @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5299: 65fed6a79adea14f7bef6d55530da47d7731d370 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15390: c125a9e2476148b489a349f3947c4b327bf29bbc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c125a9e24761 drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22 20:37   ` Ruhl, Michael J
  0 siblings, 0 replies; 24+ messages in thread
From: Ruhl, Michael J @ 2019-11-22 20:37 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

>-----Original Message-----
>From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Chris
>Wilson
>Sent: Friday, November 22, 2019 2:03 AM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>i915_request
>
>As we start peeking into requests for longer and longer, e.g.
>incorporating use of spinlocks when only protected by an
>rcu_read_lock(), we need to be careful in how we reset the request when
>recycling and need to preserve any barriers that may still be in use as
>the request is reset for reuse.
>
>Quoting Linus Torvalds:
>
>> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
>
>  .. because the object can be accessed (by RCU) after the refcount has
>  gone down to zero, and the thing has been released.
>
>  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
>
>  That flag basically says:
>
>  "I may end up accessing this object *after* it has been free'd,
>  because there may be RCU lookups in flight"
>
>  This has nothing to do with constructors. It's ok if the object gets
>  reused as an object of the same type and does *not* get
>  re-initialized, because we're perfectly fine seeing old stale data.
>
>  What it guarantees is that the slab isn't shared with any other kind
>  of object, _and_ that the underlying pages are free'd after an RCU
>  quiescent period (so the pages aren't shared with another kind of
>  object either during an RCU walk).
>
>  And it doesn't necessarily have to have a constructor, because the
>  thing that a RCU walk will care about is
>
>    (a) guaranteed to be an object that *has* been on some RCU list (so
>    it's not a "new" object)
>
>    (b) the RCU walk needs to have logic to verify that it's still the
>    *same* object and hasn't been re-used as something else.
>
>  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
>  immediately, but because it gets reused as the same kind of object,
>  the RCU walker can "know" what parts have meaning for re-use, in a way
>  it couidn't if the re-use was random.
>
>  That said, it *is* subtle, and people should be careful.
>
>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
>
>  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>  to guard the speculative RCU access section, and use
>  "atomic_dec_and_test()" in the freeing section, then you should be
>  safe wrt new allocations.
>
>  If you have a completely new allocation that has "random stale
>  content", you know that it cannot be on the RCU list, so there is no
>  speculative access that can ever see that random content.
>
>  So the only case you need to worry about is a re-use allocation, and
>  you know that the refcount will start out as zero even if you don't
>  have a constructor.
>
>  So you can think of the refcount itself as always having a zero
>  constructor, *BUT* you need to be careful with ordering.
>
>  In particular, whoever does the allocation needs to then set the
>  refcount to a non-zero value *after* it has initialized all the other
>  fields. And in particular, it needs to make sure that it uses the
>  proper memory ordering to do so.
>
>  NOTE! One thing to be very worried about is that re-initializing
>  whatever RCU lists means that now the RCU walker may be walking on the
>  wrong list so the walker may do the right thing for this particular
>  entry, but it may miss walking *other* entries. So then you can get
>  spurious lookup failures, because the RCU walker never walked all the
>  way to the end of the right list. That ends up being a much more
>  subtle bug.
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>---
> drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
> drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
> drivers/gpu/drm/i915/i915_scheduler.h |  1 +
> drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
> drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
> 5 files changed, 50 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_request.c
>b/drivers/gpu/drm/i915/i915_request.c
>index 00011f9533b6..a558f64186fa 100644
>--- a/drivers/gpu/drm/i915/i915_request.c
>+++ b/drivers/gpu/drm/i915/i915_request.c
>@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
>*request)
> {
> 	struct i915_capture_list *capture;
>
>-	capture = request->capture_list;
>+	capture = fetch_and_zero(&request->capture_list);
> 	while (capture) {
> 		struct i915_capture_list *next = capture->next;
>
>@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request
>*rq)
> 		spin_lock(&engine->active.lock);
> 		locked = engine;
> 	}
>-	list_del(&rq->sched.link);
>+	list_del_init(&rq->sched.link);
> 	spin_unlock_irq(&locked->active.lock);
> }
>
>@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
>gfp)
> 	return kmem_cache_alloc(global.slab_requests, gfp);
> }
>
>+static void __i915_request_ctor(void *arg)
>+{
>+	struct i915_request *rq = arg;
>+
>+	spin_lock_init(&rq->lock);
>+	i915_sched_node_init(&rq->sched);
>+	i915_sw_fence_init(&rq->submit, submit_notify);
>+	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
>+
>+	rq->file_priv = NULL;
>+	rq->capture_list = NULL;
>+
>+	INIT_LIST_HEAD(&rq->execute_cb);
>+}

My understanding of the cache ctor is that it will only get invoked when
the cache is expanded.  It does not get called on each kmem_cache_alloc().

I.e. if you do an _alloc() and there is nothing available in the cache, a new
slab is created, and the ctor is called.

Is that the subtly that you are referring to?

Is doing this ctor only when new cache slabs are created sufficient?

Mike


> struct i915_request *
> __i915_request_create(struct intel_context *ce, gfp_t gfp)
> {
>@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t
>gfp)
> 	rq->engine = ce->engine;
> 	rq->ring = ce->ring;
> 	rq->execution_mask = ce->engine->mask;
>+	rq->flags = 0;
>
> 	rcu_assign_pointer(rq->timeline, tl);
> 	rq->hwsp_seqno = tl->hwsp_seqno;
>@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce,
>gfp_t gfp)
>
> 	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>
>-	spin_lock_init(&rq->lock);
> 	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
> 		       tl->fence_context, seqno);
>
> 	/* We bump the ref for the fence chain */
>-	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
>-	i915_sw_fence_init(&i915_request_get(rq)->semaphore,
>semaphore_notify);
>+	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
>+	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
>
>-	i915_sched_node_init(&rq->sched);
>+	i915_sched_node_reinit(&rq->sched);
>
>-	/* No zalloc, must clear what we need by hand */
>-	rq->file_priv = NULL;
>+	/* No zalloc, everything must be cleared after use */
> 	rq->batch = NULL;
>-	rq->capture_list = NULL;
>-	rq->flags = 0;
>-
>-	INIT_LIST_HEAD(&rq->execute_cb);
>+	GEM_BUG_ON(rq->file_priv);
>+	GEM_BUG_ON(rq->capture_list);
>+	GEM_BUG_ON(!list_empty(&rq->execute_cb));
>
> 	/*
> 	 * Reserve space in the ring buffer for all the commands required to
>@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
>
> int __init i915_global_request_init(void)
> {
>-	global.slab_requests = KMEM_CACHE(i915_request,
>-					  SLAB_HWCACHE_ALIGN |
>-					  SLAB_RECLAIM_ACCOUNT |
>-					  SLAB_TYPESAFE_BY_RCU);
>+	global.slab_requests =
>+		kmem_cache_create("i915_request",
>+				  sizeof(struct i915_request),
>+				  __alignof__(struct i915_request),
>+				  SLAB_HWCACHE_ALIGN |
>+				  SLAB_RECLAIM_ACCOUNT |
>+				  SLAB_TYPESAFE_BY_RCU,
>+				  __i915_request_ctor);
> 	if (!global.slab_requests)
> 		return -ENOMEM;
>
>diff --git a/drivers/gpu/drm/i915/i915_scheduler.c
>b/drivers/gpu/drm/i915/i915_scheduler.c
>index 194246548c4d..a5a6dbe6a53c 100644
>--- a/drivers/gpu/drm/i915/i915_scheduler.c
>+++ b/drivers/gpu/drm/i915/i915_scheduler.c
>@@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node
>*node)
> 	INIT_LIST_HEAD(&node->signalers_list);
> 	INIT_LIST_HEAD(&node->waiters_list);
> 	INIT_LIST_HEAD(&node->link);
>+}
>+
>+void i915_sched_node_reinit(struct i915_sched_node *node)
>+{
> 	node->attr.priority = I915_PRIORITY_INVALID;
> 	node->semaphores = 0;
> 	node->flags = 0;
>@@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node
>*node)
> 		if (dep->flags & I915_DEPENDENCY_ALLOC)
> 			i915_dependency_free(dep);
> 	}
>+	INIT_LIST_HEAD(&node->signalers_list);
>
> 	/* Remove ourselves from everyone who depends upon us */
> 	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
>@@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node
>*node)
> 		if (dep->flags & I915_DEPENDENCY_ALLOC)
> 			i915_dependency_free(dep);
> 	}
>+	INIT_LIST_HEAD(&node->waiters_list);
>
> 	spin_unlock_irq(&schedule_lock);
> }
>diff --git a/drivers/gpu/drm/i915/i915_scheduler.h
>b/drivers/gpu/drm/i915/i915_scheduler.h
>index 07d243acf553..d1dc4efef77b 100644
>--- a/drivers/gpu/drm/i915/i915_scheduler.h
>+++ b/drivers/gpu/drm/i915/i915_scheduler.h
>@@ -26,6 +26,7 @@
> 					 sched.link)
>
> void i915_sched_node_init(struct i915_sched_node *node);
>+void i915_sched_node_reinit(struct i915_sched_node *node);
>
> bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
> 				      struct i915_sched_node *signal,
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
>b/drivers/gpu/drm/i915/i915_sw_fence.c
>index 6a88db291252..eacc6c5ce0fd 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.c
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>@@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence
>*fence,
> 	fence->flags = (unsigned long)fn;
> }
>
>+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
>+{
>+	debug_fence_init(fence);
>+
>+	atomic_set(&fence->pending, 1);
>+	fence->error = 0;
>+}
>+
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
> {
> 	debug_fence_activate(fence);
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h
>b/drivers/gpu/drm/i915/i915_sw_fence.h
>index ab7d58bd0b9d..1e90d9a51bd2 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.h
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
>@@ -54,6 +54,8 @@ do {
>		\
> 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
> #endif
>
>+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
>+
> #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
> void i915_sw_fence_fini(struct i915_sw_fence *fence);
> #else
>--
>2.24.0
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22 20:37   ` Ruhl, Michael J
  0 siblings, 0 replies; 24+ messages in thread
From: Ruhl, Michael J @ 2019-11-22 20:37 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

>-----Original Message-----
>From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Chris
>Wilson
>Sent: Friday, November 22, 2019 2:03 AM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>i915_request
>
>As we start peeking into requests for longer and longer, e.g.
>incorporating use of spinlocks when only protected by an
>rcu_read_lock(), we need to be careful in how we reset the request when
>recycling and need to preserve any barriers that may still be in use as
>the request is reset for reuse.
>
>Quoting Linus Torvalds:
>
>> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
>
>  .. because the object can be accessed (by RCU) after the refcount has
>  gone down to zero, and the thing has been released.
>
>  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
>
>  That flag basically says:
>
>  "I may end up accessing this object *after* it has been free'd,
>  because there may be RCU lookups in flight"
>
>  This has nothing to do with constructors. It's ok if the object gets
>  reused as an object of the same type and does *not* get
>  re-initialized, because we're perfectly fine seeing old stale data.
>
>  What it guarantees is that the slab isn't shared with any other kind
>  of object, _and_ that the underlying pages are free'd after an RCU
>  quiescent period (so the pages aren't shared with another kind of
>  object either during an RCU walk).
>
>  And it doesn't necessarily have to have a constructor, because the
>  thing that a RCU walk will care about is
>
>    (a) guaranteed to be an object that *has* been on some RCU list (so
>    it's not a "new" object)
>
>    (b) the RCU walk needs to have logic to verify that it's still the
>    *same* object and hasn't been re-used as something else.
>
>  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
>  immediately, but because it gets reused as the same kind of object,
>  the RCU walker can "know" what parts have meaning for re-use, in a way
>  it couidn't if the re-use was random.
>
>  That said, it *is* subtle, and people should be careful.
>
>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
>
>  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>  to guard the speculative RCU access section, and use
>  "atomic_dec_and_test()" in the freeing section, then you should be
>  safe wrt new allocations.
>
>  If you have a completely new allocation that has "random stale
>  content", you know that it cannot be on the RCU list, so there is no
>  speculative access that can ever see that random content.
>
>  So the only case you need to worry about is a re-use allocation, and
>  you know that the refcount will start out as zero even if you don't
>  have a constructor.
>
>  So you can think of the refcount itself as always having a zero
>  constructor, *BUT* you need to be careful with ordering.
>
>  In particular, whoever does the allocation needs to then set the
>  refcount to a non-zero value *after* it has initialized all the other
>  fields. And in particular, it needs to make sure that it uses the
>  proper memory ordering to do so.
>
>  NOTE! One thing to be very worried about is that re-initializing
>  whatever RCU lists means that now the RCU walker may be walking on the
>  wrong list so the walker may do the right thing for this particular
>  entry, but it may miss walking *other* entries. So then you can get
>  spurious lookup failures, because the RCU walker never walked all the
>  way to the end of the right list. That ends up being a much more
>  subtle bug.
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>---
> drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
> drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
> drivers/gpu/drm/i915/i915_scheduler.h |  1 +
> drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
> drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
> 5 files changed, 50 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_request.c
>b/drivers/gpu/drm/i915/i915_request.c
>index 00011f9533b6..a558f64186fa 100644
>--- a/drivers/gpu/drm/i915/i915_request.c
>+++ b/drivers/gpu/drm/i915/i915_request.c
>@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
>*request)
> {
> 	struct i915_capture_list *capture;
>
>-	capture = request->capture_list;
>+	capture = fetch_and_zero(&request->capture_list);
> 	while (capture) {
> 		struct i915_capture_list *next = capture->next;
>
>@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request
>*rq)
> 		spin_lock(&engine->active.lock);
> 		locked = engine;
> 	}
>-	list_del(&rq->sched.link);
>+	list_del_init(&rq->sched.link);
> 	spin_unlock_irq(&locked->active.lock);
> }
>
>@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
>gfp)
> 	return kmem_cache_alloc(global.slab_requests, gfp);
> }
>
>+static void __i915_request_ctor(void *arg)
>+{
>+	struct i915_request *rq = arg;
>+
>+	spin_lock_init(&rq->lock);
>+	i915_sched_node_init(&rq->sched);
>+	i915_sw_fence_init(&rq->submit, submit_notify);
>+	i915_sw_fence_init(&rq->semaphore, semaphore_notify);
>+
>+	rq->file_priv = NULL;
>+	rq->capture_list = NULL;
>+
>+	INIT_LIST_HEAD(&rq->execute_cb);
>+}

My understanding of the cache ctor is that it will only get invoked when
the cache is expanded.  It does not get called on each kmem_cache_alloc().

I.e. if you do an _alloc() and there is nothing available in the cache, a new
slab is created, and the ctor is called.

Is that the subtly that you are referring to?

Is doing this ctor only when new cache slabs are created sufficient?

Mike


> struct i915_request *
> __i915_request_create(struct intel_context *ce, gfp_t gfp)
> {
>@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t
>gfp)
> 	rq->engine = ce->engine;
> 	rq->ring = ce->ring;
> 	rq->execution_mask = ce->engine->mask;
>+	rq->flags = 0;
>
> 	rcu_assign_pointer(rq->timeline, tl);
> 	rq->hwsp_seqno = tl->hwsp_seqno;
>@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce,
>gfp_t gfp)
>
> 	rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>
>-	spin_lock_init(&rq->lock);
> 	dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
> 		       tl->fence_context, seqno);
>
> 	/* We bump the ref for the fence chain */
>-	i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
>-	i915_sw_fence_init(&i915_request_get(rq)->semaphore,
>semaphore_notify);
>+	i915_sw_fence_reinit(&i915_request_get(rq)->submit);
>+	i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
>
>-	i915_sched_node_init(&rq->sched);
>+	i915_sched_node_reinit(&rq->sched);
>
>-	/* No zalloc, must clear what we need by hand */
>-	rq->file_priv = NULL;
>+	/* No zalloc, everything must be cleared after use */
> 	rq->batch = NULL;
>-	rq->capture_list = NULL;
>-	rq->flags = 0;
>-
>-	INIT_LIST_HEAD(&rq->execute_cb);
>+	GEM_BUG_ON(rq->file_priv);
>+	GEM_BUG_ON(rq->capture_list);
>+	GEM_BUG_ON(!list_empty(&rq->execute_cb));
>
> 	/*
> 	 * Reserve space in the ring buffer for all the commands required to
>@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
>
> int __init i915_global_request_init(void)
> {
>-	global.slab_requests = KMEM_CACHE(i915_request,
>-					  SLAB_HWCACHE_ALIGN |
>-					  SLAB_RECLAIM_ACCOUNT |
>-					  SLAB_TYPESAFE_BY_RCU);
>+	global.slab_requests =
>+		kmem_cache_create("i915_request",
>+				  sizeof(struct i915_request),
>+				  __alignof__(struct i915_request),
>+				  SLAB_HWCACHE_ALIGN |
>+				  SLAB_RECLAIM_ACCOUNT |
>+				  SLAB_TYPESAFE_BY_RCU,
>+				  __i915_request_ctor);
> 	if (!global.slab_requests)
> 		return -ENOMEM;
>
>diff --git a/drivers/gpu/drm/i915/i915_scheduler.c
>b/drivers/gpu/drm/i915/i915_scheduler.c
>index 194246548c4d..a5a6dbe6a53c 100644
>--- a/drivers/gpu/drm/i915/i915_scheduler.c
>+++ b/drivers/gpu/drm/i915/i915_scheduler.c
>@@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node
>*node)
> 	INIT_LIST_HEAD(&node->signalers_list);
> 	INIT_LIST_HEAD(&node->waiters_list);
> 	INIT_LIST_HEAD(&node->link);
>+}
>+
>+void i915_sched_node_reinit(struct i915_sched_node *node)
>+{
> 	node->attr.priority = I915_PRIORITY_INVALID;
> 	node->semaphores = 0;
> 	node->flags = 0;
>@@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node
>*node)
> 		if (dep->flags & I915_DEPENDENCY_ALLOC)
> 			i915_dependency_free(dep);
> 	}
>+	INIT_LIST_HEAD(&node->signalers_list);
>
> 	/* Remove ourselves from everyone who depends upon us */
> 	list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
>@@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node
>*node)
> 		if (dep->flags & I915_DEPENDENCY_ALLOC)
> 			i915_dependency_free(dep);
> 	}
>+	INIT_LIST_HEAD(&node->waiters_list);
>
> 	spin_unlock_irq(&schedule_lock);
> }
>diff --git a/drivers/gpu/drm/i915/i915_scheduler.h
>b/drivers/gpu/drm/i915/i915_scheduler.h
>index 07d243acf553..d1dc4efef77b 100644
>--- a/drivers/gpu/drm/i915/i915_scheduler.h
>+++ b/drivers/gpu/drm/i915/i915_scheduler.h
>@@ -26,6 +26,7 @@
> 					 sched.link)
>
> void i915_sched_node_init(struct i915_sched_node *node);
>+void i915_sched_node_reinit(struct i915_sched_node *node);
>
> bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
> 				      struct i915_sched_node *signal,
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
>b/drivers/gpu/drm/i915/i915_sw_fence.c
>index 6a88db291252..eacc6c5ce0fd 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.c
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>@@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence
>*fence,
> 	fence->flags = (unsigned long)fn;
> }
>
>+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
>+{
>+	debug_fence_init(fence);
>+
>+	atomic_set(&fence->pending, 1);
>+	fence->error = 0;
>+}
>+
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
> {
> 	debug_fence_activate(fence);
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h
>b/drivers/gpu/drm/i915/i915_sw_fence.h
>index ab7d58bd0b9d..1e90d9a51bd2 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.h
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
>@@ -54,6 +54,8 @@ do {
>		\
> 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
> #endif
>
>+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
>+
> #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
> void i915_sw_fence_fini(struct i915_sw_fence *fence);
> #else
>--
>2.24.0
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22 20:50     ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22 20:50 UTC (permalink / raw)
  To: Ruhl, Michael J, intel-gfx

Quoting Ruhl, Michael J (2019-11-22 20:37:59)
> >-----Original Message-----
> >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Chris
> >Wilson
> >Sent: Friday, November 22, 2019 2:03 AM
> >To: intel-gfx@lists.freedesktop.org
> >Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
> >i915_request
> >
> >As we start peeking into requests for longer and longer, e.g.
> >incorporating use of spinlocks when only protected by an
> >rcu_read_lock(), we need to be careful in how we reset the request when
> >recycling and need to preserve any barriers that may still be in use as
> >the request is reset for reuse.
> >
> >Quoting Linus Torvalds:
> >
> >> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
> >
> >  .. because the object can be accessed (by RCU) after the refcount has
> >  gone down to zero, and the thing has been released.
> >
> >  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
> >
> >  That flag basically says:
> >
> >  "I may end up accessing this object *after* it has been free'd,
> >  because there may be RCU lookups in flight"
> >
> >  This has nothing to do with constructors. It's ok if the object gets
> >  reused as an object of the same type and does *not* get
> >  re-initialized, because we're perfectly fine seeing old stale data.
> >
> >  What it guarantees is that the slab isn't shared with any other kind
> >  of object, _and_ that the underlying pages are free'd after an RCU
> >  quiescent period (so the pages aren't shared with another kind of
> >  object either during an RCU walk).
> >
> >  And it doesn't necessarily have to have a constructor, because the
> >  thing that a RCU walk will care about is
> >
> >    (a) guaranteed to be an object that *has* been on some RCU list (so
> >    it's not a "new" object)
> >
> >    (b) the RCU walk needs to have logic to verify that it's still the
> >    *same* object and hasn't been re-used as something else.
> >
> >  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
> >  immediately, but because it gets reused as the same kind of object,
> >  the RCU walker can "know" what parts have meaning for re-use, in a way
> >  it couidn't if the re-use was random.
> >
> >  That said, it *is* subtle, and people should be careful.
> >
> >> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> >
> >  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
> >  to guard the speculative RCU access section, and use
> >  "atomic_dec_and_test()" in the freeing section, then you should be
> >  safe wrt new allocations.
> >
> >  If you have a completely new allocation that has "random stale
> >  content", you know that it cannot be on the RCU list, so there is no
> >  speculative access that can ever see that random content.
> >
> >  So the only case you need to worry about is a re-use allocation, and
> >  you know that the refcount will start out as zero even if you don't
> >  have a constructor.
> >
> >  So you can think of the refcount itself as always having a zero
> >  constructor, *BUT* you need to be careful with ordering.
> >
> >  In particular, whoever does the allocation needs to then set the
> >  refcount to a non-zero value *after* it has initialized all the other
> >  fields. And in particular, it needs to make sure that it uses the
> >  proper memory ordering to do so.
> >
> >  NOTE! One thing to be very worried about is that re-initializing
> >  whatever RCU lists means that now the RCU walker may be walking on the
> >  wrong list so the walker may do the right thing for this particular
> >  entry, but it may miss walking *other* entries. So then you can get
> >  spurious lookup failures, because the RCU walker never walked all the
> >  way to the end of the right list. That ends up being a much more
> >  subtle bug.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
> > drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
> > drivers/gpu/drm/i915/i915_scheduler.h |  1 +
> > drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
> > drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
> > 5 files changed, 50 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_request.c
> >b/drivers/gpu/drm/i915/i915_request.c
> >index 00011f9533b6..a558f64186fa 100644
> >--- a/drivers/gpu/drm/i915/i915_request.c
> >+++ b/drivers/gpu/drm/i915/i915_request.c
> >@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
> >*request)
> > {
> >       struct i915_capture_list *capture;
> >
> >-      capture = request->capture_list;
> >+      capture = fetch_and_zero(&request->capture_list);
> >       while (capture) {
> >               struct i915_capture_list *next = capture->next;
> >
> >@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request
> >*rq)
> >               spin_lock(&engine->active.lock);
> >               locked = engine;
> >       }
> >-      list_del(&rq->sched.link);
> >+      list_del_init(&rq->sched.link);
> >       spin_unlock_irq(&locked->active.lock);
> > }
> >
> >@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
> >gfp)
> >       return kmem_cache_alloc(global.slab_requests, gfp);
> > }
> >
> >+static void __i915_request_ctor(void *arg)
> >+{
> >+      struct i915_request *rq = arg;
> >+
> >+      spin_lock_init(&rq->lock);
> >+      i915_sched_node_init(&rq->sched);
> >+      i915_sw_fence_init(&rq->submit, submit_notify);
> >+      i915_sw_fence_init(&rq->semaphore, semaphore_notify);
> >+
> >+      rq->file_priv = NULL;
> >+      rq->capture_list = NULL;
> >+
> >+      INIT_LIST_HEAD(&rq->execute_cb);
> >+}
> 
> My understanding of the cache ctor is that it will only get invoked when
> the cache is expanded.  It does not get called on each kmem_cache_alloc().
> 
> I.e. if you do an _alloc() and there is nothing available in the cache, a new
> slab is created, and the ctor is called.

Correct.

> Is that the subtly that you are referring to?
> 
> Is doing this ctor only when new cache slabs are created sufficient?

That is precisely the point. As we only protect some accesses with RCU,
the request we are peeking at may be reused while we access it. The
challenge is to keep all such access valid. E.g. consider that the whole
of i915_gem_busy_ioctl() is performed only protected by the
rcu_read_lock(), and yet we need the results to be accurate even though
the requests be reallocated in the middle of it.

The situation that spurred the ctor was taking a spinlock in an rcu path
and finding it was re-initialised in the middle of that.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22 20:50     ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2019-11-22 20:50 UTC (permalink / raw)
  To: Ruhl, Michael J, intel-gfx

Quoting Ruhl, Michael J (2019-11-22 20:37:59)
> >-----Original Message-----
> >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Chris
> >Wilson
> >Sent: Friday, November 22, 2019 2:03 AM
> >To: intel-gfx@lists.freedesktop.org
> >Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
> >i915_request
> >
> >As we start peeking into requests for longer and longer, e.g.
> >incorporating use of spinlocks when only protected by an
> >rcu_read_lock(), we need to be careful in how we reset the request when
> >recycling and need to preserve any barriers that may still be in use as
> >the request is reset for reuse.
> >
> >Quoting Linus Torvalds:
> >
> >> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
> >
> >  .. because the object can be accessed (by RCU) after the refcount has
> >  gone down to zero, and the thing has been released.
> >
> >  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
> >
> >  That flag basically says:
> >
> >  "I may end up accessing this object *after* it has been free'd,
> >  because there may be RCU lookups in flight"
> >
> >  This has nothing to do with constructors. It's ok if the object gets
> >  reused as an object of the same type and does *not* get
> >  re-initialized, because we're perfectly fine seeing old stale data.
> >
> >  What it guarantees is that the slab isn't shared with any other kind
> >  of object, _and_ that the underlying pages are free'd after an RCU
> >  quiescent period (so the pages aren't shared with another kind of
> >  object either during an RCU walk).
> >
> >  And it doesn't necessarily have to have a constructor, because the
> >  thing that a RCU walk will care about is
> >
> >    (a) guaranteed to be an object that *has* been on some RCU list (so
> >    it's not a "new" object)
> >
> >    (b) the RCU walk needs to have logic to verify that it's still the
> >    *same* object and hasn't been re-used as something else.
> >
> >  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
> >  immediately, but because it gets reused as the same kind of object,
> >  the RCU walker can "know" what parts have meaning for re-use, in a way
> >  it couidn't if the re-use was random.
> >
> >  That said, it *is* subtle, and people should be careful.
> >
> >> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> >
> >  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
> >  to guard the speculative RCU access section, and use
> >  "atomic_dec_and_test()" in the freeing section, then you should be
> >  safe wrt new allocations.
> >
> >  If you have a completely new allocation that has "random stale
> >  content", you know that it cannot be on the RCU list, so there is no
> >  speculative access that can ever see that random content.
> >
> >  So the only case you need to worry about is a re-use allocation, and
> >  you know that the refcount will start out as zero even if you don't
> >  have a constructor.
> >
> >  So you can think of the refcount itself as always having a zero
> >  constructor, *BUT* you need to be careful with ordering.
> >
> >  In particular, whoever does the allocation needs to then set the
> >  refcount to a non-zero value *after* it has initialized all the other
> >  fields. And in particular, it needs to make sure that it uses the
> >  proper memory ordering to do so.
> >
> >  NOTE! One thing to be very worried about is that re-initializing
> >  whatever RCU lists means that now the RCU walker may be walking on the
> >  wrong list so the walker may do the right thing for this particular
> >  entry, but it may miss walking *other* entries. So then you can get
> >  spurious lookup failures, because the RCU walker never walked all the
> >  way to the end of the right list. That ends up being a much more
> >  subtle bug.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
> > drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
> > drivers/gpu/drm/i915/i915_scheduler.h |  1 +
> > drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
> > drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
> > 5 files changed, 50 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_request.c
> >b/drivers/gpu/drm/i915/i915_request.c
> >index 00011f9533b6..a558f64186fa 100644
> >--- a/drivers/gpu/drm/i915/i915_request.c
> >+++ b/drivers/gpu/drm/i915/i915_request.c
> >@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
> >*request)
> > {
> >       struct i915_capture_list *capture;
> >
> >-      capture = request->capture_list;
> >+      capture = fetch_and_zero(&request->capture_list);
> >       while (capture) {
> >               struct i915_capture_list *next = capture->next;
> >
> >@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request
> >*rq)
> >               spin_lock(&engine->active.lock);
> >               locked = engine;
> >       }
> >-      list_del(&rq->sched.link);
> >+      list_del_init(&rq->sched.link);
> >       spin_unlock_irq(&locked->active.lock);
> > }
> >
> >@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
> >gfp)
> >       return kmem_cache_alloc(global.slab_requests, gfp);
> > }
> >
> >+static void __i915_request_ctor(void *arg)
> >+{
> >+      struct i915_request *rq = arg;
> >+
> >+      spin_lock_init(&rq->lock);
> >+      i915_sched_node_init(&rq->sched);
> >+      i915_sw_fence_init(&rq->submit, submit_notify);
> >+      i915_sw_fence_init(&rq->semaphore, semaphore_notify);
> >+
> >+      rq->file_priv = NULL;
> >+      rq->capture_list = NULL;
> >+
> >+      INIT_LIST_HEAD(&rq->execute_cb);
> >+}
> 
> My understanding of the cache ctor is that it will only get invoked when
> the cache is expanded.  It does not get called on each kmem_cache_alloc().
> 
> I.e. if you do an _alloc() and there is nothing available in the cache, a new
> slab is created, and the ctor is called.

Correct.

> Is that the subtly that you are referring to?
> 
> Is doing this ctor only when new cache slabs are created sufficient?

That is precisely the point. As we only protect some accesses with RCU,
the request we are peeking at may be reused while we access it. The
challenge is to keep all such access valid. E.g. consider that the whole
of i915_gem_busy_ioctl() is performed only protected by the
rcu_read_lock(), and yet we need the results to be accurate even though
the requests be reallocated in the middle of it.

The situation that spurred the ctor was taking a spinlock in an rcu path
and finding it was re-initialised in the middle of that.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22 21:08       ` Ruhl, Michael J
  0 siblings, 0 replies; 24+ messages in thread
From: Ruhl, Michael J @ 2019-11-22 21:08 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

>-----Original Message-----
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Sent: Friday, November 22, 2019 3:50 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel-
>gfx@lists.freedesktop.org
>Subject: RE: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>i915_request
>
>Quoting Ruhl, Michael J (2019-11-22 20:37:59)
>> >-----Original Message-----
>> >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>Chris
>> >Wilson
>> >Sent: Friday, November 22, 2019 2:03 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>> >i915_request
>> >
>> >As we start peeking into requests for longer and longer, e.g.
>> >incorporating use of spinlocks when only protected by an
>> >rcu_read_lock(), we need to be careful in how we reset the request when
>> >recycling and need to preserve any barriers that may still be in use as
>> >the request is reset for reuse.
>> >
>> >Quoting Linus Torvalds:
>> >
>> >> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
>> >
>> >  .. because the object can be accessed (by RCU) after the refcount has
>> >  gone down to zero, and the thing has been released.
>> >
>> >  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
>> >
>> >  That flag basically says:
>> >
>> >  "I may end up accessing this object *after* it has been free'd,
>> >  because there may be RCU lookups in flight"
>> >
>> >  This has nothing to do with constructors. It's ok if the object gets
>> >  reused as an object of the same type and does *not* get
>> >  re-initialized, because we're perfectly fine seeing old stale data.
>> >
>> >  What it guarantees is that the slab isn't shared with any other kind
>> >  of object, _and_ that the underlying pages are free'd after an RCU
>> >  quiescent period (so the pages aren't shared with another kind of
>> >  object either during an RCU walk).
>> >
>> >  And it doesn't necessarily have to have a constructor, because the
>> >  thing that a RCU walk will care about is
>> >
>> >    (a) guaranteed to be an object that *has* been on some RCU list (so
>> >    it's not a "new" object)
>> >
>> >    (b) the RCU walk needs to have logic to verify that it's still the
>> >    *same* object and hasn't been re-used as something else.
>> >
>> >  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
>> >  immediately, but because it gets reused as the same kind of object,
>> >  the RCU walker can "know" what parts have meaning for re-use, in a way
>> >  it couidn't if the re-use was random.
>> >
>> >  That said, it *is* subtle, and people should be careful.
>> >
>> >> So the re-use might initialize the fields lazily, not necessarily using a ctor.
>> >
>> >  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>> >  to guard the speculative RCU access section, and use
>> >  "atomic_dec_and_test()" in the freeing section, then you should be
>> >  safe wrt new allocations.
>> >
>> >  If you have a completely new allocation that has "random stale
>> >  content", you know that it cannot be on the RCU list, so there is no
>> >  speculative access that can ever see that random content.
>> >
>> >  So the only case you need to worry about is a re-use allocation, and
>> >  you know that the refcount will start out as zero even if you don't
>> >  have a constructor.
>> >
>> >  So you can think of the refcount itself as always having a zero
>> >  constructor, *BUT* you need to be careful with ordering.
>> >
>> >  In particular, whoever does the allocation needs to then set the
>> >  refcount to a non-zero value *after* it has initialized all the other
>> >  fields. And in particular, it needs to make sure that it uses the
>> >  proper memory ordering to do so.
>> >
>> >  NOTE! One thing to be very worried about is that re-initializing
>> >  whatever RCU lists means that now the RCU walker may be walking on
>the
>> >  wrong list so the walker may do the right thing for this particular
>> >  entry, but it may miss walking *other* entries. So then you can get
>> >  spurious lookup failures, because the RCU walker never walked all the
>> >  way to the end of the right list. That ends up being a much more
>> >  subtle bug.
>> >
>> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >---
>> > drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
>> > drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
>> > drivers/gpu/drm/i915/i915_scheduler.h |  1 +
>> > drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
>> > drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
>> > 5 files changed, 50 insertions(+), 16 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/i915_request.c
>> >b/drivers/gpu/drm/i915/i915_request.c
>> >index 00011f9533b6..a558f64186fa 100644
>> >--- a/drivers/gpu/drm/i915/i915_request.c
>> >+++ b/drivers/gpu/drm/i915/i915_request.c
>> >@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
>> >*request)
>> > {
>> >       struct i915_capture_list *capture;
>> >
>> >-      capture = request->capture_list;
>> >+      capture = fetch_and_zero(&request->capture_list);
>> >       while (capture) {
>> >               struct i915_capture_list *next = capture->next;
>> >
>> >@@ -214,7 +214,7 @@ static void remove_from_engine(struct
>i915_request
>> >*rq)
>> >               spin_lock(&engine->active.lock);
>> >               locked = engine;
>> >       }
>> >-      list_del(&rq->sched.link);
>> >+      list_del_init(&rq->sched.link);
>> >       spin_unlock_irq(&locked->active.lock);
>> > }
>> >
>> >@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
>> >gfp)
>> >       return kmem_cache_alloc(global.slab_requests, gfp);
>> > }
>> >
>> >+static void __i915_request_ctor(void *arg)
>> >+{
>> >+      struct i915_request *rq = arg;
>> >+
>> >+      spin_lock_init(&rq->lock);
>> >+      i915_sched_node_init(&rq->sched);
>> >+      i915_sw_fence_init(&rq->submit, submit_notify);
>> >+      i915_sw_fence_init(&rq->semaphore, semaphore_notify);
>> >+
>> >+      rq->file_priv = NULL;
>> >+      rq->capture_list = NULL;
>> >+
>> >+      INIT_LIST_HEAD(&rq->execute_cb);
>> >+}
>>
>> My understanding of the cache ctor is that it will only get invoked when
>> the cache is expanded.  It does not get called on each kmem_cache_alloc().
>>
>> I.e. if you do an _alloc() and there is nothing available in the cache, a new
>> slab is created, and the ctor is called.
>
>Correct.
>
>> Is that the subtly that you are referring to?
>>
>> Is doing this ctor only when new cache slabs are created sufficient?
>
>That is precisely the point. As we only protect some accesses with RCU,
>the request we are peeking at may be reused while we access it. The
>challenge is to keep all such access valid. E.g. consider that the whole
>of i915_gem_busy_ioctl() is performed only protected by the
>rcu_read_lock(), and yet we need the results to be accurate even though
>the requests be reallocated in the middle of it.
>
>The situation that spurred the ctor was taking a spinlock in an rcu path
>and finding it was re-initialised in the middle of that.

To requote:

> > This has nothing to do with constructors. It's ok if the object gets
> >  reused as an object of the same type and does *not* get
> >  re-initialized, because we're perfectly fine seeing old stale data.

Ok.  I think I am following this.  You are using the side effect of the ctor
to protect against old data from being re-initialized until after RCU is
done with it.

Thanks for the clarification.

Mike

>-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
@ 2019-11-22 21:08       ` Ruhl, Michael J
  0 siblings, 0 replies; 24+ messages in thread
From: Ruhl, Michael J @ 2019-11-22 21:08 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

>-----Original Message-----
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Sent: Friday, November 22, 2019 3:50 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel-
>gfx@lists.freedesktop.org
>Subject: RE: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>i915_request
>
>Quoting Ruhl, Michael J (2019-11-22 20:37:59)
>> >-----Original Message-----
>> >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>Chris
>> >Wilson
>> >Sent: Friday, November 22, 2019 2:03 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>> >i915_request
>> >
>> >As we start peeking into requests for longer and longer, e.g.
>> >incorporating use of spinlocks when only protected by an
>> >rcu_read_lock(), we need to be careful in how we reset the request when
>> >recycling and need to preserve any barriers that may still be in use as
>> >the request is reset for reuse.
>> >
>> >Quoting Linus Torvalds:
>> >
>> >> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
>> >
>> >  .. because the object can be accessed (by RCU) after the refcount has
>> >  gone down to zero, and the thing has been released.
>> >
>> >  That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
>> >
>> >  That flag basically says:
>> >
>> >  "I may end up accessing this object *after* it has been free'd,
>> >  because there may be RCU lookups in flight"
>> >
>> >  This has nothing to do with constructors. It's ok if the object gets
>> >  reused as an object of the same type and does *not* get
>> >  re-initialized, because we're perfectly fine seeing old stale data.
>> >
>> >  What it guarantees is that the slab isn't shared with any other kind
>> >  of object, _and_ that the underlying pages are free'd after an RCU
>> >  quiescent period (so the pages aren't shared with another kind of
>> >  object either during an RCU walk).
>> >
>> >  And it doesn't necessarily have to have a constructor, because the
>> >  thing that a RCU walk will care about is
>> >
>> >    (a) guaranteed to be an object that *has* been on some RCU list (so
>> >    it's not a "new" object)
>> >
>> >    (b) the RCU walk needs to have logic to verify that it's still the
>> >    *same* object and hasn't been re-used as something else.
>> >
>> >  In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
>> >  immediately, but because it gets reused as the same kind of object,
>> >  the RCU walker can "know" what parts have meaning for re-use, in a way
>> >  it couidn't if the re-use was random.
>> >
>> >  That said, it *is* subtle, and people should be careful.
>> >
>> >> So the re-use might initialize the fields lazily, not necessarily using a ctor.
>> >
>> >  If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>> >  to guard the speculative RCU access section, and use
>> >  "atomic_dec_and_test()" in the freeing section, then you should be
>> >  safe wrt new allocations.
>> >
>> >  If you have a completely new allocation that has "random stale
>> >  content", you know that it cannot be on the RCU list, so there is no
>> >  speculative access that can ever see that random content.
>> >
>> >  So the only case you need to worry about is a re-use allocation, and
>> >  you know that the refcount will start out as zero even if you don't
>> >  have a constructor.
>> >
>> >  So you can think of the refcount itself as always having a zero
>> >  constructor, *BUT* you need to be careful with ordering.
>> >
>> >  In particular, whoever does the allocation needs to then set the
>> >  refcount to a non-zero value *after* it has initialized all the other
>> >  fields. And in particular, it needs to make sure that it uses the
>> >  proper memory ordering to do so.
>> >
>> >  NOTE! One thing to be very worried about is that re-initializing
>> >  whatever RCU lists means that now the RCU walker may be walking on
>the
>> >  wrong list so the walker may do the right thing for this particular
>> >  entry, but it may miss walking *other* entries. So then you can get
>> >  spurious lookup failures, because the RCU walker never walked all the
>> >  way to the end of the right list. That ends up being a much more
>> >  subtle bug.
>> >
>> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> >---
>> > drivers/gpu/drm/i915/i915_request.c   | 49 ++++++++++++++++++---------
>> > drivers/gpu/drm/i915/i915_scheduler.c |  6 ++++
>> > drivers/gpu/drm/i915/i915_scheduler.h |  1 +
>> > drivers/gpu/drm/i915/i915_sw_fence.c  |  8 +++++
>> > drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
>> > 5 files changed, 50 insertions(+), 16 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/i915_request.c
>> >b/drivers/gpu/drm/i915/i915_request.c
>> >index 00011f9533b6..a558f64186fa 100644
>> >--- a/drivers/gpu/drm/i915/i915_request.c
>> >+++ b/drivers/gpu/drm/i915/i915_request.c
>> >@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
>> >*request)
>> > {
>> >       struct i915_capture_list *capture;
>> >
>> >-      capture = request->capture_list;
>> >+      capture = fetch_and_zero(&request->capture_list);
>> >       while (capture) {
>> >               struct i915_capture_list *next = capture->next;
>> >
>> >@@ -214,7 +214,7 @@ static void remove_from_engine(struct
>i915_request
>> >*rq)
>> >               spin_lock(&engine->active.lock);
>> >               locked = engine;
>> >       }
>> >-      list_del(&rq->sched.link);
>> >+      list_del_init(&rq->sched.link);
>> >       spin_unlock_irq(&locked->active.lock);
>> > }
>> >
>> >@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
>> >gfp)
>> >       return kmem_cache_alloc(global.slab_requests, gfp);
>> > }
>> >
>> >+static void __i915_request_ctor(void *arg)
>> >+{
>> >+      struct i915_request *rq = arg;
>> >+
>> >+      spin_lock_init(&rq->lock);
>> >+      i915_sched_node_init(&rq->sched);
>> >+      i915_sw_fence_init(&rq->submit, submit_notify);
>> >+      i915_sw_fence_init(&rq->semaphore, semaphore_notify);
>> >+
>> >+      rq->file_priv = NULL;
>> >+      rq->capture_list = NULL;
>> >+
>> >+      INIT_LIST_HEAD(&rq->execute_cb);
>> >+}
>>
>> My understanding of the cache ctor is that it will only get invoked when
>> the cache is expanded.  It does not get called on each kmem_cache_alloc().
>>
>> I.e. if you do an _alloc() and there is nothing available in the cache, a new
>> slab is created, and the ctor is called.
>
>Correct.
>
>> Is that the subtly that you are referring to?
>>
>> Is doing this ctor only when new cache slabs are created sufficient?
>
>That is precisely the point. As we only protect some accesses with RCU,
>the request we are peeking at may be reused while we access it. The
>challenge is to keep all such access valid. E.g. consider that the whole
>of i915_gem_busy_ioctl() is performed only protected by the
>rcu_read_lock(), and yet we need the results to be accurate even though
>the requests be reallocated in the middle of it.
>
>The situation that spurred the ctor was taking a spinlock in an rcu path
>and finding it was re-initialised in the middle of that.

To requote:

> > This has nothing to do with constructors. It's ok if the object gets
> >  reused as an object of the same type and does *not* get
> >  re-initialized, because we're perfectly fine seeing old stale data.

Ok.  I think I am following this.  You are using the side effect of the ctor
to protect against old data from being re-initialized until after RCU is
done with it.

Thanks for the clarification.

Mike

>-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
@ 2019-11-23 15:34   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-23 15:34 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
URL   : https://patchwork.freedesktop.org/series/69824/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7403_full -> Patchwork_15390_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15390_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15390_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_15390_full:

### IGT changes ###

#### Possible regressions ####

  * igt@perf@gen8-unprivileged-single-ctx-counters:
    - shard-glk:          [PASS][1] -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-glk2/igt@perf@gen8-unprivileged-single-ctx-counters.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-glk2/igt@perf@gen8-unprivileged-single-ctx-counters.html

  
Known issues
------------

  Here are the changes found in Patchwork_15390_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108566]) +7 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-kbl2/igt@gem_ctx_isolation@rcs0-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-kbl2/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_ctx_isolation@vecs0-s3:
    - shard-tglb:         [PASS][5] -> [INCOMPLETE][6] ([fdo#111832])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb9/igt@gem_ctx_isolation@vecs0-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb8/igt@gem_ctx_isolation@vecs0-s3.html

  * igt@gem_ctx_persistence@vcs1-queued:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109276] / [fdo#112080]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@gem_ctx_persistence@vcs1-queued.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb8/igt@gem_ctx_persistence@vcs1-queued.html

  * igt@gem_ctx_shared@q-smoketest-all:
    - shard-tglb:         [PASS][9] -> [INCOMPLETE][10] ([fdo#111735])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb7/igt@gem_ctx_shared@q-smoketest-all.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb6/igt@gem_ctx_shared@q-smoketest-all.html

  * igt@gem_ctx_switch@queue-heavy:
    - shard-tglb:         [PASS][11] -> [INCOMPLETE][12] ([fdo#111747])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb8/igt@gem_ctx_switch@queue-heavy.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb6/igt@gem_ctx_switch@queue-heavy.html

  * igt@gem_ctx_switch@vcs1-heavy-queue:
    - shard-iclb:         [PASS][13] -> [SKIP][14] ([fdo#112080]) +4 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb4/igt@gem_ctx_switch@vcs1-heavy-queue.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb8/igt@gem_ctx_switch@vcs1-heavy-queue.html

  * igt@gem_exec_schedule@in-order-bsd2:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109276]) +12 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@gem_exec_schedule@in-order-bsd2.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb5/igt@gem_exec_schedule@in-order-bsd2.html

  * igt@gem_exec_schedule@out-order-bsd:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#112146])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_exec_schedule@out-order-bsd.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb1/igt@gem_exec_schedule@out-order-bsd.html

  * igt@gem_persistent_relocs@forked-thrash-inactive:
    - shard-snb:          [PASS][19] -> [TIMEOUT][20] ([fdo#112068 ])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-snb7/igt@gem_persistent_relocs@forked-thrash-inactive.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-snb6/igt@gem_persistent_relocs@forked-thrash-inactive.html

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [PASS][21] -> [DMESG-WARN][22] ([fdo#108566]) +3 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-apl3/igt@gem_softpin@noreloc-s3.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-apl6/igt@gem_softpin@noreloc-s3.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-hsw:          [PASS][23] -> [DMESG-WARN][24] ([fdo#111870]) +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw4/igt@gem_userptr_blits@sync-unmap-cycles.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw6/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@i915_selftest@live_hangcheck:
    - shard-hsw:          [PASS][25] -> [DMESG-FAIL][26] ([fdo#111991])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw7/igt@i915_selftest@live_hangcheck.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw7/igt@i915_selftest@live_hangcheck.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff:
    - shard-tglb:         [PASS][27] -> [FAIL][28] ([fdo#103167]) +5 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][29] -> [FAIL][30] ([fdo#103167])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbcpsr-suspend:
    - shard-tglb:         [PASS][31] -> [INCOMPLETE][32] ([fdo#111832] / [fdo#111850] / [fdo#111884])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb6/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb1/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-tglb:         [PASS][33] -> [INCOMPLETE][34] ([fdo#111832] / [fdo#111850]) +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb6/igt@kms_frontbuffer_tracking@psr-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb1/igt@kms_frontbuffer_tracking@psr-suspend.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][35] -> [FAIL][36] ([fdo#108145] / [fdo#110403])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl9/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][37] -> [FAIL][38] ([fdo#108341])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb6/igt@kms_psr@no_drrs.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_primary_blt:
    - shard-iclb:         [PASS][39] -> [SKIP][40] ([fdo#109441])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@kms_psr@psr2_primary_blt.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb5/igt@kms_psr@psr2_primary_blt.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-kbl:          [PASS][41] -> [INCOMPLETE][42] ([fdo#103665]) +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-kbl7/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-kbl3/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-iclb:         [SKIP][43] ([fdo#109276] / [fdo#112080]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb8/igt@gem_ctx_isolation@vcs1-s3.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb4/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][45] ([fdo#110854]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_exec_balancer@smoke.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_gttfill@basic:
    - shard-tglb:         [INCOMPLETE][47] ([fdo#111593]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb9/igt@gem_exec_gttfill@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_parallel@vcs1-fds:
    - shard-iclb:         [SKIP][49] ([fdo#112080]) -> [PASS][50] +12 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_exec_parallel@vcs1-fds.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@gem_exec_parallel@vcs1-fds.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [SKIP][51] ([fdo#112146]) -> [PASS][52] +4 similar issues
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@gem_exec_schedule@in-order-bsd.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb8/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@preempt-queue-chain-render:
    - shard-tglb:         [INCOMPLETE][53] ([fdo#111606] / [fdo#111677]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb6/igt@gem_exec_schedule@preempt-queue-chain-render.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@gem_exec_schedule@preempt-queue-chain-render.html

  * igt@gem_userptr_blits@dmabuf-sync:
    - shard-snb:          [DMESG-WARN][55] ([fdo#111870]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-snb5/igt@gem_userptr_blits@dmabuf-sync.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-snb7/igt@gem_userptr_blits@dmabuf-sync.html

  * igt@gem_userptr_blits@sync-unmap:
    - shard-hsw:          [DMESG-WARN][57] ([fdo#111870]) -> [PASS][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw6/igt@gem_userptr_blits@sync-unmap.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw4/igt@gem_userptr_blits@sync-unmap.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][59] ([fdo#108566]) -> [PASS][60] +5 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-kbl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-untiled:
    - shard-skl:          [FAIL][61] ([fdo#103184] / [fdo#103232] / [fdo#108472]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl6/igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-untiled.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl2/igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-untiled.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-ytiled:
    - shard-skl:          [FAIL][63] ([fdo#103184] / [fdo#103232] / [fdo#108145]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl6/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-ytiled.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl2/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-ytiled.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][65] ([fdo#105363]) -> [PASS][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl4/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-hsw:          [INCOMPLETE][67] ([fdo#103540]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw5/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt:
    - shard-tglb:         [FAIL][69] ([fdo#103167]) -> [PASS][70] +3 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-tglb:         [INCOMPLETE][71] ([fdo#111832] / [fdo#111850] / [fdo#111884]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb3/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb6/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
    - shard-iclb:         [FAIL][73] ([fdo#103167]) -> [PASS][74] +6 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-gtt:
    - shard-skl:          [FAIL][75] ([fdo#103167]) -> [PASS][76] +1 similar issue
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-gtt.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl10/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-gtt.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [FAIL][77] ([fdo#108145]) -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_psr@psr2_no_drrs:
    - shard-iclb:         [SKIP][79] ([fdo#109441]) -> [PASS][80] +1 similar issue
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@kms_psr@psr2_no_drrs.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@kms_psr@psr2_no_drrs.html

  * igt@kms_psr@suspend:
    - shard-tglb:         [INCOMPLETE][81] ([fdo#111832] / [fdo#111850]) -> [PASS][82] +4 similar issues
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb4/igt@kms_psr@suspend.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@kms_psr@suspend.html

  * igt@kms_vblank@pipe-d-ts-continuation-suspend:
    - shard-tglb:         [INCOMPLETE][83] ([fdo#111850]) -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb3/igt@kms_vblank@pipe-d-ts-continuation-suspend.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@kms_vblank@pipe-d-ts-continuation-suspend.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][85] ([fdo#109276]) -> [PASS][86] +9 similar issues
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@prime_busy@hang-bsd2.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb1/igt@prime_busy@hang-bsd2.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv-switch:
    - shard-iclb:         [SKIP][87] ([fdo#109276] / [fdo#112080]) -> [FAIL][88] ([fdo#111329])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html

  * igt@gem_ctx_isolation@vcs2-nonpriv-switch:
    - shard-tglb:         [SKIP][89] ([fdo#111912] / [fdo#112080]) -> [SKIP][90] ([fdo#112080])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb3/igt@gem_ctx_isolation@vcs2-nonpriv-switch.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb9/igt@gem_ctx_isolation@vcs2-nonpriv-switch.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-tglb:         [INCOMPLETE][91] ([fdo#111832] / [fdo#111850]) -> [INCOMPLETE][92] ([fdo#111850])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb5/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb7/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108472]: https://bugs.freedesktop.org/show_bug.cgi?id=108472
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111593]: https://bugs.freedesktop.org/show_bug.cgi?id=111593
  [fdo#111606]: https://bugs.freedesktop.org/show_bug.cgi?id=111606
  [fdo#111677]: https://bugs.freedesktop.org/show_bug.cgi?id=111677
  [fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735
  [fdo#111747]: https://bugs.freedesktop.org/show_bug.cgi?id=111747
  [fdo#111832]: https://bugs.freedesktop.org/show_bug.cgi?id=111832
  [fdo#111850]: https://bugs.freedesktop.org/show_bug.cgi?id=111850
  [fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870
  [fdo#111884]: https://bugs.freedesktop.org/show_bug.cgi?id=111884
  [fdo#111912]: https://bugs.freedesktop.org/show_bug.cgi?id=111912
  [fdo#111991]: https://bugs.freedesktop.org/show_bug.cgi?id=111991
  [fdo#112068 ]: https://bugs.freedesktop.org/show_bug.cgi?id=112068 
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
@ 2019-11-23 15:34   ` Patchwork
  0 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2019-11-23 15:34 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3)
URL   : https://patchwork.freedesktop.org/series/69824/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7403_full -> Patchwork_15390_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15390_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15390_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_15390_full:

### IGT changes ###

#### Possible regressions ####

  * igt@perf@gen8-unprivileged-single-ctx-counters:
    - shard-glk:          [PASS][1] -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-glk2/igt@perf@gen8-unprivileged-single-ctx-counters.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-glk2/igt@perf@gen8-unprivileged-single-ctx-counters.html

  
Known issues
------------

  Here are the changes found in Patchwork_15390_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108566]) +7 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-kbl2/igt@gem_ctx_isolation@rcs0-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-kbl2/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_ctx_isolation@vecs0-s3:
    - shard-tglb:         [PASS][5] -> [INCOMPLETE][6] ([fdo#111832])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb9/igt@gem_ctx_isolation@vecs0-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb8/igt@gem_ctx_isolation@vecs0-s3.html

  * igt@gem_ctx_persistence@vcs1-queued:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109276] / [fdo#112080]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@gem_ctx_persistence@vcs1-queued.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb8/igt@gem_ctx_persistence@vcs1-queued.html

  * igt@gem_ctx_shared@q-smoketest-all:
    - shard-tglb:         [PASS][9] -> [INCOMPLETE][10] ([fdo#111735])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb7/igt@gem_ctx_shared@q-smoketest-all.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb6/igt@gem_ctx_shared@q-smoketest-all.html

  * igt@gem_ctx_switch@queue-heavy:
    - shard-tglb:         [PASS][11] -> [INCOMPLETE][12] ([fdo#111747])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb8/igt@gem_ctx_switch@queue-heavy.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb6/igt@gem_ctx_switch@queue-heavy.html

  * igt@gem_ctx_switch@vcs1-heavy-queue:
    - shard-iclb:         [PASS][13] -> [SKIP][14] ([fdo#112080]) +4 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb4/igt@gem_ctx_switch@vcs1-heavy-queue.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb8/igt@gem_ctx_switch@vcs1-heavy-queue.html

  * igt@gem_exec_schedule@in-order-bsd2:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109276]) +12 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@gem_exec_schedule@in-order-bsd2.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb5/igt@gem_exec_schedule@in-order-bsd2.html

  * igt@gem_exec_schedule@out-order-bsd:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#112146])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_exec_schedule@out-order-bsd.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb1/igt@gem_exec_schedule@out-order-bsd.html

  * igt@gem_persistent_relocs@forked-thrash-inactive:
    - shard-snb:          [PASS][19] -> [TIMEOUT][20] ([fdo#112068 ])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-snb7/igt@gem_persistent_relocs@forked-thrash-inactive.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-snb6/igt@gem_persistent_relocs@forked-thrash-inactive.html

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [PASS][21] -> [DMESG-WARN][22] ([fdo#108566]) +3 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-apl3/igt@gem_softpin@noreloc-s3.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-apl6/igt@gem_softpin@noreloc-s3.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-hsw:          [PASS][23] -> [DMESG-WARN][24] ([fdo#111870]) +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw4/igt@gem_userptr_blits@sync-unmap-cycles.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw6/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@i915_selftest@live_hangcheck:
    - shard-hsw:          [PASS][25] -> [DMESG-FAIL][26] ([fdo#111991])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw7/igt@i915_selftest@live_hangcheck.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw7/igt@i915_selftest@live_hangcheck.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff:
    - shard-tglb:         [PASS][27] -> [FAIL][28] ([fdo#103167]) +5 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][29] -> [FAIL][30] ([fdo#103167])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbcpsr-suspend:
    - shard-tglb:         [PASS][31] -> [INCOMPLETE][32] ([fdo#111832] / [fdo#111850] / [fdo#111884])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb6/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb1/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-tglb:         [PASS][33] -> [INCOMPLETE][34] ([fdo#111832] / [fdo#111850]) +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb6/igt@kms_frontbuffer_tracking@psr-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb1/igt@kms_frontbuffer_tracking@psr-suspend.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][35] -> [FAIL][36] ([fdo#108145] / [fdo#110403])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl9/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][37] -> [FAIL][38] ([fdo#108341])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb6/igt@kms_psr@no_drrs.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_primary_blt:
    - shard-iclb:         [PASS][39] -> [SKIP][40] ([fdo#109441])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@kms_psr@psr2_primary_blt.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb5/igt@kms_psr@psr2_primary_blt.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-kbl:          [PASS][41] -> [INCOMPLETE][42] ([fdo#103665]) +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-kbl7/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-kbl3/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-iclb:         [SKIP][43] ([fdo#109276] / [fdo#112080]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb8/igt@gem_ctx_isolation@vcs1-s3.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb4/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][45] ([fdo#110854]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_exec_balancer@smoke.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_gttfill@basic:
    - shard-tglb:         [INCOMPLETE][47] ([fdo#111593]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb9/igt@gem_exec_gttfill@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_parallel@vcs1-fds:
    - shard-iclb:         [SKIP][49] ([fdo#112080]) -> [PASS][50] +12 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_exec_parallel@vcs1-fds.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@gem_exec_parallel@vcs1-fds.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [SKIP][51] ([fdo#112146]) -> [PASS][52] +4 similar issues
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb2/igt@gem_exec_schedule@in-order-bsd.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb8/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@preempt-queue-chain-render:
    - shard-tglb:         [INCOMPLETE][53] ([fdo#111606] / [fdo#111677]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb6/igt@gem_exec_schedule@preempt-queue-chain-render.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@gem_exec_schedule@preempt-queue-chain-render.html

  * igt@gem_userptr_blits@dmabuf-sync:
    - shard-snb:          [DMESG-WARN][55] ([fdo#111870]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-snb5/igt@gem_userptr_blits@dmabuf-sync.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-snb7/igt@gem_userptr_blits@dmabuf-sync.html

  * igt@gem_userptr_blits@sync-unmap:
    - shard-hsw:          [DMESG-WARN][57] ([fdo#111870]) -> [PASS][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw6/igt@gem_userptr_blits@sync-unmap.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw4/igt@gem_userptr_blits@sync-unmap.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][59] ([fdo#108566]) -> [PASS][60] +5 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-kbl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-untiled:
    - shard-skl:          [FAIL][61] ([fdo#103184] / [fdo#103232] / [fdo#108472]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl6/igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-untiled.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl2/igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-untiled.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-ytiled:
    - shard-skl:          [FAIL][63] ([fdo#103184] / [fdo#103232] / [fdo#108145]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl6/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-ytiled.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl2/igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-ytiled.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][65] ([fdo#105363]) -> [PASS][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl4/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-hsw:          [INCOMPLETE][67] ([fdo#103540]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-hsw1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-hsw5/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt:
    - shard-tglb:         [FAIL][69] ([fdo#103167]) -> [PASS][70] +3 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-tglb:         [INCOMPLETE][71] ([fdo#111832] / [fdo#111850] / [fdo#111884]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb3/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb6/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt:
    - shard-iclb:         [FAIL][73] ([fdo#103167]) -> [PASS][74] +6 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-gtt.html

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-gtt:
    - shard-skl:          [FAIL][75] ([fdo#103167]) -> [PASS][76] +1 similar issue
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-gtt.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl10/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-gtt.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [FAIL][77] ([fdo#108145]) -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-skl2/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_psr@psr2_no_drrs:
    - shard-iclb:         [SKIP][79] ([fdo#109441]) -> [PASS][80] +1 similar issue
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@kms_psr@psr2_no_drrs.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@kms_psr@psr2_no_drrs.html

  * igt@kms_psr@suspend:
    - shard-tglb:         [INCOMPLETE][81] ([fdo#111832] / [fdo#111850]) -> [PASS][82] +4 similar issues
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb4/igt@kms_psr@suspend.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@kms_psr@suspend.html

  * igt@kms_vblank@pipe-d-ts-continuation-suspend:
    - shard-tglb:         [INCOMPLETE][83] ([fdo#111850]) -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb3/igt@kms_vblank@pipe-d-ts-continuation-suspend.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb2/igt@kms_vblank@pipe-d-ts-continuation-suspend.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][85] ([fdo#109276]) -> [PASS][86] +9 similar issues
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@prime_busy@hang-bsd2.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb1/igt@prime_busy@hang-bsd2.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv-switch:
    - shard-iclb:         [SKIP][87] ([fdo#109276] / [fdo#112080]) -> [FAIL][88] ([fdo#111329])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-iclb5/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html

  * igt@gem_ctx_isolation@vcs2-nonpriv-switch:
    - shard-tglb:         [SKIP][89] ([fdo#111912] / [fdo#112080]) -> [SKIP][90] ([fdo#112080])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb3/igt@gem_ctx_isolation@vcs2-nonpriv-switch.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb9/igt@gem_ctx_isolation@vcs2-nonpriv-switch.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-tglb:         [INCOMPLETE][91] ([fdo#111832] / [fdo#111850]) -> [INCOMPLETE][92] ([fdo#111850])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7403/shard-tglb5/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/shard-tglb7/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108472]: https://bugs.freedesktop.org/show_bug.cgi?id=108472
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111593]: https://bugs.freedesktop.org/show_bug.cgi?id=111593
  [fdo#111606]: https://bugs.freedesktop.org/show_bug.cgi?id=111606
  [fdo#111677]: https://bugs.freedesktop.org/show_bug.cgi?id=111677
  [fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735
  [fdo#111747]: https://bugs.freedesktop.org/show_bug.cgi?id=111747
  [fdo#111832]: https://bugs.freedesktop.org/show_bug.cgi?id=111832
  [fdo#111850]: https://bugs.freedesktop.org/show_bug.cgi?id=111850
  [fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870
  [fdo#111884]: https://bugs.freedesktop.org/show_bug.cgi?id=111884
  [fdo#111912]: https://bugs.freedesktop.org/show_bug.cgi?id=111912
  [fdo#111991]: https://bugs.freedesktop.org/show_bug.cgi?id=111991
  [fdo#112068 ]: https://bugs.freedesktop.org/show_bug.cgi?id=112068 
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15390/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-11-23 15:34 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-22  7:02 [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request Chris Wilson
2019-11-22  7:02 ` [Intel-gfx] " Chris Wilson
2019-11-22  7:46 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev2) Patchwork
2019-11-22  7:46   ` [Intel-gfx] " Patchwork
2019-11-22  8:07 ` ✓ Fi.CI.BAT: success " Patchwork
2019-11-22  8:07   ` [Intel-gfx] " Patchwork
2019-11-22  9:24 ` [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request Tvrtko Ursulin
2019-11-22  9:24   ` [Intel-gfx] " Tvrtko Ursulin
2019-11-22  9:33   ` Chris Wilson
2019-11-22  9:33     ` [Intel-gfx] " Chris Wilson
2019-11-22  9:49 ` [PATCH] " Chris Wilson
2019-11-22  9:49   ` [Intel-gfx] " Chris Wilson
2019-11-22 10:12 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3) Patchwork
2019-11-22 10:12   ` [Intel-gfx] " Patchwork
2019-11-22 10:33 ` ✓ Fi.CI.BAT: success " Patchwork
2019-11-22 10:33   ` [Intel-gfx] " Patchwork
2019-11-22 20:37 ` [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request Ruhl, Michael J
2019-11-22 20:37   ` [Intel-gfx] " Ruhl, Michael J
2019-11-22 20:50   ` Chris Wilson
2019-11-22 20:50     ` [Intel-gfx] " Chris Wilson
2019-11-22 21:08     ` Ruhl, Michael J
2019-11-22 21:08       ` [Intel-gfx] " Ruhl, Michael J
2019-11-23 15:34 ` ✗ Fi.CI.IGT: failure for drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request (rev3) Patchwork
2019-11-23 15:34   ` [Intel-gfx] " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.