| Message ID | 20210609043613.102962-32-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: ioctl clean-ups (v6) | expand |
On Tue, Jun 08, 2021 at 11:36:13PM -0500, Jason Ekstrand wrote: > This instance now only happens during context creation so there's no way > we can race with a context close/destroy. We don't need to bother with > the RCU and can access the pointer directly. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> There's another one in execbuf.c, and the real crux is that we need to fully audit the lifetimes of everything. I think it's better to do this all together in a seperate patch series, which entirely removes the rcu barrier on the i915_address_space cleanup, and also removes the rcu protection from everywhere else. Otherwise we just have an inconsistent mess that happens to work, sometimes. -Daniel > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 5312142daa0c0..ffdfed536ce9a 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -787,15 +787,12 @@ static int intel_context_set_gem(struct intel_context *ce, > > ce->ring_size = SZ_16K; > > - if (rcu_access_pointer(ctx->vm)) { > - struct i915_address_space *vm; > - > - rcu_read_lock(); > - vm = context_get_vm_rcu(ctx); /* hmm */ > - rcu_read_unlock(); > - > + if (ctx->vm) { > + /* This only happens during context creation so no need to > + * bother with any RCU nonsense. > + */ > i915_vm_put(ce->vm); > - ce->vm = vm; > + ce->vm = i915_vm_get(ctx->vm); > } > > if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > -- > 2.31.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 9, 2021 at 6:41 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Jun 08, 2021 at 11:36:13PM -0500, Jason Ekstrand wrote: > > This instance now only happens during context creation so there's no way > > we can race with a context close/destroy. We don't need to bother with > > the RCU and can access the pointer directly. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > There's another one in execbuf.c, and the real crux is that we need to > fully audit the lifetimes of everything. I think it's better to do this > all together in a seperate patch series, which entirely removes the rcu > barrier on the i915_address_space cleanup, and also removes the rcu > protection from everywhere else. > > Otherwise we just have an inconsistent mess that happens to work, > sometimes. Ok, let's drop this for now. --Jason > -Daniel > > > --- > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 13 +++++-------- > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > index 5312142daa0c0..ffdfed536ce9a 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -787,15 +787,12 @@ static int intel_context_set_gem(struct intel_context *ce, > > > > ce->ring_size = SZ_16K; > > > > - if (rcu_access_pointer(ctx->vm)) { > > - struct i915_address_space *vm; > > - > > - rcu_read_lock(); > > - vm = context_get_vm_rcu(ctx); /* hmm */ > > - rcu_read_unlock(); > > - > > + if (ctx->vm) { > > + /* This only happens during context creation so no need to > > + * bother with any RCU nonsense. > > + */ > > i915_vm_put(ce->vm); > > - ce->vm = vm; > > + ce->vm = i915_vm_get(ctx->vm); > > } > > > > if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > > -- > > 2.31.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 5312142daa0c0..ffdfed536ce9a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -787,15 +787,12 @@ static int intel_context_set_gem(struct intel_context *ce, ce->ring_size = SZ_16K; - if (rcu_access_pointer(ctx->vm)) { - struct i915_address_space *vm; - - rcu_read_lock(); - vm = context_get_vm_rcu(ctx); /* hmm */ - rcu_read_unlock(); - + if (ctx->vm) { + /* This only happens during context creation so no need to + * bother with any RCU nonsense. + */ i915_vm_put(ce->vm); - ce->vm = vm; + ce->vm = i915_vm_get(ctx->vm); } if (ctx->sched.priority >= I915_PRIORITY_NORMAL &&
This instance now only happens during context creation so there's no way we can race with a context close/destroy. We don't need to bother with the RCU and can access the pointer directly. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)