[Intel-gfx] [PATCH v3] drm/i915/pmu: Reconstruct active state on starting busy-stats

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-12 13:19:30)
> 
> On 12/01/2018 13:03, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-01-12 11:43:11)
> >>
> >> On 12/01/2018 10:35, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-01-12 10:30:26)
> >>>>
> >>>>
> >>>> On 12/01/2018 09:51, Chris Wilson wrote:
> >>>>> Quoting Tvrtko Ursulin (2018-01-12 09:40:40)
> >>>>>> So submit side doesn't work in either case, unless I am missing
> >>>>>> something. Would need the pair of port manipulation and context_in to be
> >>>>>> atomic.
> >>>>>
> >>>>> Sure, there is a small window with the result that we either never turn
> >>>>> off the stats, or turn them off one too early (and then recover if the
> >>>>> my understanding of the underflow protection holds). The same problem as
> >>>>> existed before the reconstruction, except now the window is much
> >>>>> smaller. I'm not that scared by this (it should not explode, nor result
> >>>>> in hopelessly wrong stats) so it can wait until stats enabling doesn't
> >>>>> need to peek at execlists. I think you will need to postpone enabling
> >>>>> until the next context-switch if we wanted to avoid the atomics; except
> >>>>> that poses a problem with the igt expecting busy-start to be accurate. A
> >>>>> dilemma for later.
> >>>>
> >>>> My analysis was partially incorrect, yes, there is underflow protection
> >>>> already.
> >>>>
> >>>> But I don't see that there is a race window where we end up with
> >>>> permanent 100% busyness before the reconstruction patch. Where do you
> >>>> see that?
> >>>>
> >>>> The worst I see without the reconstruction is to miss the accounting of
> >>>> the batch currently in progress when stats get enabled. Which is a much
> >>>> less serious, totally ignorable event.
> >>>
> >>> One is observable via pmu, the other not. If we fail to turn off
> >>> busy-stats accounting, nothing is lost except for a few wasted cycles.
> >>> Except on the next pmu, it starts from the previous cs instead of the
> >>> enabling -- but that is a problem that also exists with the second user.
> >>
> >> I don't follow, I am talking about permanent 100%. Let me copy what I
> >> wrote earlier:
> >>
> >> port0 context complete
> >> context_out - not enabled, no-op
> >> stats enable - port0 busy, active = 1
> >> port0 clear
> >> submit
> >> context_in - active = 2 !!! BAD
> >> port0 set
> >>
> >> That leads to permanent 100% until busy stats are disabled. I think that
> >> is hugely less desirable than just failing to account for the currently
> >> running batch, which was the case before reconstruction on enable, and
> >> is 99.9% only a problem for IGTs.
> >>
> >> Do you think there is a flaw in this analysis or something else?
> > 
> > No, I don't think it's a huge problem, an improbable race for which the
> > quick-and-dirty cure may worse than the disease:
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 25360ce0353f..62d9ee9d45a6 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1980,16 +1980,22 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
> >    */
> >   int intel_enable_engine_stats(struct intel_engine_cs *engine)
> >   {
> > +       struct intel_engine_execlists *execlists = &engine->execlists;
> >          unsigned long flags;
> > +       int err = 0;
> >   
> >          if (!intel_engine_supports_stats(engine))
> >                  return -ENODEV;
> >   
> > +       tasklet_disable(&execlists->tasklet);
> >          spin_lock_irqsave(&engine->stats.lock, flags);
> > -       if (engine->stats.enabled == ~0)
> > -               goto busy;
> > +
> > +       if (unlikely(engine->stats.enabled == ~0)) {
> > +               err = -EBUSY;
> > +               goto unlock;
> > +       }
> > +
> >          if (engine->stats.enabled++ == 0) {
> > -               struct intel_engine_execlists *execlists = &engine->execlists;
> >                  const struct execlist_port *port = execlists->port;
> >                  unsigned int num_ports = execlists_num_ports(execlists);
> >   
> > @@ -2004,14 +2010,12 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
> >                  if (engine->stats.active)
> >                          engine->stats.start = engine->stats.enabled_at;
> >          }
> > -       spin_unlock_irqrestore(&engine->stats.lock, flags);
> >   
> > -       return 0;
> > -
> > -busy:
> > +unlock:
> >          spin_unlock_irqrestore(&engine->stats.lock, flags);
> > +       tasklet_enable(&execlists->tasklet);
> >   
> > -       return -EBUSY;
> > +       return err;
> >   }
> >   
> >   static ktime_t __intel_engine_get_busy_time(struct intel_engine_cs *engine)
> > 
> > using the tasklet control as a spinlock. Whereas the previous race did
> > not take much effort to observe.
> 
> By the previous race you are referring to the missed currently running 
> batch, or the permanent 100% with active state reconstruction?
> 
> You are against reverting the reconstruction? Even if it is unlikely, I 
> see it as very severe, while the missed current batch is more likely but 
> not at all severe. So I would be really for the revert. Unfortunately I 
> did not spot this issue during review. :(

I am against it since we can demonstrate one race very easily in igt,
and the other requires a stress test to be written, when I expect it to
be fixed. We both saw it as a temporary measure as it is poking in places
it should, and the above patch would provide the protection it needs.
-Chris