Re: [PATCH 07/12] drm/i915: Revoke mmaps and prevent access to fence registers across reset

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/12] drm/i915: Revoke mmaps and prevent access to fence registers across reset
Date: Mon, 04 Feb 2019 13:47:17 +0000	[thread overview]
Message-ID: <154928803745.14784.6537096810109423790@skylake-alporthouse-com> (raw)
In-Reply-To: <8736p34r1a.fsf@gaia.fi.intel.com>

Quoting Mika Kuoppala (2019-02-04 13:33:21)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > @@ -272,6 +270,8 @@ struct i915_gpu_error {
> >        */
> >       wait_queue_head_t reset_queue;
> >  
> > +     struct srcu_struct srcu;
> > +
> 
> It is the only one in here so not causing confusion
> but could have been reset_backoff_srcu;

worksforme.

> > @@ -1274,9 +1272,12 @@ void i915_handle_error(struct drm_i915_private *i915,
> >               wait_event(i915->gpu_error.reset_queue,
> >                          !test_bit(I915_RESET_BACKOFF,
> >                                    &i915->gpu_error.flags));
> > -             goto out;
> > +             goto out; /* piggy-back on the other reset */
> >       }
> >  
> > +     /* Make sure i915_reset_trylock() sees the I915_RESET_BACKOFF */
> > +     synchronize_rcu_expedited();
> 
> Is the expedite here to minimize the time the faulted
> client can try to reaquire?

Simply to try and cap the amount of time it takes to issue a reset.
Without this we would regularly fail our assertion that userspace can do
a (full) reset in less than 250ms.

> >       /* Prevent any other reset-engine attempt. */
> >       for_each_engine(engine, i915, tmp) {
> >               while (test_and_set_bit(I915_RESET_ENGINE + engine->id,
> > @@ -1300,6 +1301,36 @@ void i915_handle_error(struct drm_i915_private *i915,
> >       intel_runtime_pm_put(i915, wakeref);
> >  }
> >  
> > +int i915_reset_trylock(struct drm_i915_private *i915)
> > +{
> > +     struct i915_gpu_error *error = &i915->gpu_error;
> > +     int srcu;
> > +
> > +     rcu_read_lock();
> > +     while (test_bit(I915_RESET_BACKOFF, &error->flags)) {
> > +             rcu_read_unlock();
> > +
> > +             if (wait_event_interruptible(error->reset_queue,
> > +                                          !test_bit(I915_RESET_BACKOFF,
> > +                                                    &error->flags)))
> > +                     return -EINTR;
> > +
> > +             rcu_read_lock();
> > +     }
> > +     srcu = srcu_read_lock(&error->srcu);
> 
> In here are we piggybacking the graze period from srcu
> into the rcu domain by nesting?
> 
> The srcu is ours, but the rcu is everyone. So this
> bothers me.

rcu serialises the update of the I915_RESET_BACKOFF bit. That
coordinates with the reset to say nothing is in use at this moment, and
the reset is not allowed to begin until all rcu reads are complete.

The srcu then coordinates with the actual reset; it is the
mutex/semaphore that prevents both from running at the same time.

We acquire it under the rcu read lock, while we know that
I915_RESET_BACKOFF is not set, and cannot be set. Then as we release the
rcu read lock and let reset start, it sets the I915_RESET_BACKOFF
putting the next faulter to sleep, but the reset is forced to sleep on
the active scru reader. As soon as they are all complete synchronize_srcu
returns and we can do the reset uncontended.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx