[PATCH 1/2] drm/i915: Check new context against kernel_context after reporting an error

[PATCH 1/2] drm/i915: Check new context against kernel_context after reporting an error

[Chris Wilson]

Avoid any pointer dereference in inspecting a potential PTR_ERR by
checking for the error pointer before checking for an invalid context.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 2eb5d8203999..98d2ce98f467 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -571,14 +571,13 @@ int i915_gem_context_open(struct drm_i915_private *i915,
 	mutex_lock(&i915->drm.struct_mutex);
 	ctx = i915_gem_create_context(i915, file_priv);
 	mutex_unlock(&i915->drm.struct_mutex);
-
-	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
-
 	if (IS_ERR(ctx)) {
 		idr_destroy(&file_priv->context_idr);
 		return PTR_ERR(ctx);
 	}
 
+	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
+
 	return 0;
 }
 
-- 
2.13.2

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

[PATCH 2/2] drm/i915: Move stale context reaping to common i915_gem_context_create

[Chris Wilson]

We need to reap the stale contexts for all new contexts, be they created
by user in i915_gem_context_ioctl or from opening a new file in
i915_gem_context_open. Both paths may be called very frequently
accumulating many stale contexts before any worker has a chance to run
and free their memory.

Fixes: 1acfc104cdf8 ("drm/i915: Enable rcu-only context lookups")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 98d2ce98f467..c58a95c33c25 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -383,6 +383,10 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
+	/* Reap stale contexts */
+	i915_gem_retire_requests(dev_priv);
+	contexts_free(dev_priv);
+
 	ctx = __create_hw_context(dev_priv, file_priv);
 	if (IS_ERR(ctx))
 		return ctx;
@@ -989,10 +993,6 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	/* Reap stale contexts */
-	i915_gem_retire_requests(dev_priv);
-	contexts_free(dev_priv);
-
 	ctx = i915_gem_create_context(dev_priv, file_priv);
 	mutex_unlock(&dev->struct_mutex);
 	if (IS_ERR(ctx))
-- 
2.13.2

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Check new context against kernel_context after reporting an error

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Check new context against kernel_context after reporting an error
URL   : https://patchwork.freedesktop.org/series/26851/
State : success

== Summary ==

Series 26851v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/26851/revisions/1/mbox/

Test kms_busy:
        Subgroup basic-flip-default-b:
                dmesg-warn -> PASS       (fi-skl-6700hq) fdo#101144
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597

fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:440s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:429s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:360s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:526s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:510s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:492s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:480s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:592s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:435s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:411s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:493s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:571s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:578s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:556s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:279  pass:262  dwarn:0   dfail:0   fail:0   skip:17  time:585s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:463s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:475s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:408s

24346e831017070c18f3c33b74a7b098682e20f7 drm-tip: 2017y-07m-04d-15h-39m-34s UTC integration manifest
cb408f2 drm/i915: Move stale context reaping to common i915_gem_context_create
d62d437 drm/i915: Check new context against kernel_context after reporting an error

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5113/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Check new context against kernel_context after reporting an error

[Tvrtko Ursulin]

On 05/07/2017 14:18, Chris Wilson wrote:
> Avoid any pointer dereference in inspecting a potential PTR_ERR by
> checking for the error pointer before checking for an invalid context.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2eb5d8203999..98d2ce98f467 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -571,14 +571,13 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>   	mutex_lock(&i915->drm.struct_mutex);
>   	ctx = i915_gem_create_context(i915, file_priv);
>   	mutex_unlock(&i915->drm.struct_mutex);
> -
> -	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> -
>   	if (IS_ERR(ctx)) {
>   		idr_destroy(&file_priv->context_idr);
>   		return PTR_ERR(ctx);
>   	}
>   
> +	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> +
>   	return 0;
>   }
>   
> 

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

Regards,

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

Re: [PATCH 2/2] drm/i915: Move stale context reaping to common i915_gem_context_create

[Tvrtko Ursulin]

On 05/07/2017 14:18, Chris Wilson wrote:
> We need to reap the stale contexts for all new contexts, be they created
> by user in i915_gem_context_ioctl or from opening a new file in
> i915_gem_context_open. Both paths may be called very frequently
> accumulating many stale contexts before any worker has a chance to run
> and free their memory.
> 
> Fixes: 1acfc104cdf8 ("drm/i915: Enable rcu-only context lookups")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 98d2ce98f467..c58a95c33c25 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -383,6 +383,10 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
>   
>   	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>   
> +	/* Reap stale contexts */
> +	i915_gem_retire_requests(dev_priv);
> +	contexts_free(dev_priv);
> +
>   	ctx = __create_hw_context(dev_priv, file_priv);
>   	if (IS_ERR(ctx))
>   		return ctx;
> @@ -989,10 +993,6 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>   	if (ret)
>   		return ret;
>   
> -	/* Reap stale contexts */
> -	i915_gem_retire_requests(dev_priv);
> -	contexts_free(dev_priv);
> -
>   	ctx = i915_gem_create_context(dev_priv, file_priv);
>   	mutex_unlock(&dev->struct_mutex);
>   	if (IS_ERR(ctx))
> 

Can't make myself concerned about the context creation slowdown with 
this solution. I guess it highlights the same old RCU/delayed worker 
issues, but it could be an option to somehow generalize these things 
based on some threshold, like:

maybe_pruge_deleted_contexts()
{
	if (nr_contexts_on_free_list > some_threshold) {
		retire...
		free...
	}
}

Anyway:

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

Regards,

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

Re: [PATCH 2/2] drm/i915: Move stale context reaping to common i915_gem_context_create

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-07-05 14:50:57)
> 
> On 05/07/2017 14:18, Chris Wilson wrote:
> > We need to reap the stale contexts for all new contexts, be they created
> > by user in i915_gem_context_ioctl or from opening a new file in
> > i915_gem_context_open. Both paths may be called very frequently
> > accumulating many stale contexts before any worker has a chance to run
> > and free their memory.
> > 
> > Fixes: 1acfc104cdf8 ("drm/i915: Enable rcu-only context lookups")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_context.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 98d2ce98f467..c58a95c33c25 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -383,6 +383,10 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
> >   
> >       lockdep_assert_held(&dev_priv->drm.struct_mutex);
> >   
> > +     /* Reap stale contexts */
> > +     i915_gem_retire_requests(dev_priv);
> > +     contexts_free(dev_priv);
> > +
> >       ctx = __create_hw_context(dev_priv, file_priv);
> >       if (IS_ERR(ctx))
> >               return ctx;
> > @@ -989,10 +993,6 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> >       if (ret)
> >               return ret;
> >   
> > -     /* Reap stale contexts */
> > -     i915_gem_retire_requests(dev_priv);
> > -     contexts_free(dev_priv);
> > -
> >       ctx = i915_gem_create_context(dev_priv, file_priv);
> >       mutex_unlock(&dev->struct_mutex);
> >       if (IS_ERR(ctx))
> > 
> 
> Can't make myself concerned about the context creation slowdown with 
> this solution. I guess it highlights the same old RCU/delayed worker 
> issues, but it could be an option to somehow generalize these things 
> based on some threshold, like:

We are just moving the cost from delete (where we don't care that much)
to create (where we do care a bit more). However, the cost of context
destruction isn't that great and the likelihood of there being contexts
to destroy is marginal.

Retiring requests is overkill really, doing so won't immediately give us
contexts to free (since they will instead go onto the RCU list) so that
is the first thing to delete if we are concerned.

> maybe_pruge_deleted_contexts()
> {
>         if (nr_contexts_on_free_list > some_threshold) {
>                 retire...
>                 free...
>         }
> }

If pushed, I think something like where we only free the first in the
list just to keep a cap (one in, one out) on the list. Let's give that
a try... But first I'm going to see if Tomi is finally happy again.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx