[Intel-gfx] [RFC PATCH 2/2] Fix per client busyness locking

[Dixit, Ashutosh]

On Wed, 31 Aug 2022 15:45:49 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

I have updated my RFC patch based on your feedback so we can discuss again.

> On Wed, Aug 31, 2022 at 12:33:55PM -0700, Ashutosh Dixit wrote:
> > 1. Do all ce->stats updates and reads under guc->timestamp.lock
>
> Other than the question about READ_ONCE/WRITE_ONCE, I am not sure I
> understand what's broken in the locking logic here. Can you please add some
> description? thanks

Basically ce->stats.runtime.total and ce->stats.active are tied together so
should be accessed/updated atomically. Also just the update of
ce->stats.runtime.total (via lrc_update_runtime()) can have multiple
concurrent writers so even that has to be protected (since that update is
not atomic).

These was the reason for:
* Introducing lrc_update_runtime_locked
* Returning early from intel_context_get_total_runtime_ns otherwise we'll
  need to introduce the same locking there
* Enforcing locking in guc_context_update_stats (which was there in your
  original patch too)

(I think execlists code didn't have this issue because there
lrc_update_runtime is called from a tasklet (which iirc is like a single
thread/cpu). I am not sure how 'active' is updated in execlist mode so
there may or may not be a problem in intel_context_get_total_runtime_ns).

I have another question: what about that GPU vs. GuC race mentioned in the
comment? What is the sequence of events there between GPU updating the
timestamp in the context image vs. GuC setting engine_id to -1 (at least
what is the sequence in which GPU and GuC supposed to do these updates
though it may not matter to i915 due to scheduling delays)?

>
> > 2. Pin context image before reading
> > 3. Merge __guc_context_update_clks and guc_context_update_stats into a
> >   single function
> > 4. Call lrc_update_runtime() unconditionally in guc_context_update_stats
> > 5. Seems no need to update ce->stats.active with this approach
> >
> > Some of the above steps may not be correct or complete but the idea is to
> > discuss/improve the original patch.
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> > drivers/gpu/drm/i915/gt/intel_context.c       |  2 +-
> > drivers/gpu/drm/i915/gt/intel_context_types.h |  2 +-
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 41 ++++++++++---------
> > 3 files changed, 24 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index e2d70a9fdac0..c004f676de27 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -581,7 +581,7 @@ u64 intel_context_get_total_runtime_ns(struct intel_context *ce)
> >	u64 total, active;
> >
> >	if (ce->ops->update_stats)
> > -		ce->ops->update_stats(ce);
> > +		return ce->ops->update_stats(ce);
>
> This is an improvement that we can add and eventually, we can make this a
> GuC specific vfunc. Doing so may also remove the COPS_RUNTIME_ACTIVE_TOTAL
> option that I added to GuC specific context ops.

I went ahead and did this in v2 of the RFC patch.

> >
> >	total = ce->stats.runtime.total;
> >	if (ce->ops->flags & COPS_RUNTIME_CYCLES)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > index f7ff4c7d81c7..699435bfff99 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > @@ -59,7 +59,7 @@ struct intel_context_ops {
> >
> >	void (*sched_disable)(struct intel_context *ce);
> >
> > -	void (*update_stats)(struct intel_context *ce);
> > +	u64 (*update_stats)(struct intel_context *ce);
> >
> >	void (*reset)(struct intel_context *ce);
> >	void (*destroy)(struct kref *kref);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index bee8cf10f749..40d0edaf2e0a 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -1376,7 +1376,6 @@ static void __update_guc_busyness_stats(struct intel_guc *guc)
> >	spin_unlock_irqrestore(&guc->timestamp.lock, flags);
> > }
> >
> > -static void __guc_context_update_clks(struct intel_context *ce);
> > static void guc_timestamp_ping(struct work_struct *wrk)
> > {
> >	struct intel_guc *guc = container_of(wrk, typeof(*guc),
> > @@ -1401,7 +1400,8 @@ static void guc_timestamp_ping(struct work_struct *wrk)
> >
> >	/* adjust context stats for overflow */
> >	xa_for_each(&guc->context_lookup, index, ce)
> > -		__guc_context_update_clks(ce);
> > +		if (ce->ops->update_stats)
> > +			ce->ops->update_stats(ce);
>
> context pinning logic needs to be separated for this (ping) path vs. the
> query path - intel_context_get_total_runtime_ns().

Done in v2 of RFC patch.

> >
> >	intel_gt_reset_unlock(gt, srcu);
> >
> > @@ -1476,17 +1476,21 @@ void intel_guc_busyness_unpark(struct intel_gt *gt)
> >			 guc->timestamp.ping_delay);
> > }
> >
> > -static void __guc_context_update_clks(struct intel_context *ce)
> > +static u64 guc_context_update_stats(struct intel_context *ce)
> > {
> >	struct intel_guc *guc = ce_to_guc(ce);
> >	struct intel_gt *gt = ce->engine->gt;
> >	u32 *pphwsp, last_switch, engine_id;
> > -	u64 start_gt_clk, active;
> >	unsigned long flags;
> > +	u64 total, active = 0;
> >	ktime_t unused;
> >
> > +	intel_context_pin(ce);
>
> intel_context_pin can sleep and we are not allowed to sleep in this path -
> intel_context_get_total_runtime_ns(), however we can sleep in the ping
> worker path, so ideally we want to separate it out for the 2 paths.

Do we know which intel_context_get_total_runtime_ns() call is not allowed
to sleep? E.g. the code path from i915_drm_client_fdinfo() is allowed to
sleep. As mentioned I have done this is v2 of RFC patch which I think is
sufficient, but a more complicated scheme (which I think we can avoid for
now) would be to pin in code paths when sleeping is allowed.

> >	spin_lock_irqsave(&guc->timestamp.lock, flags);
> >
> > +	lrc_update_runtime(ce);
> > +	total = ce->stats.runtime.total;
>
> That would get called every second (default intel_gpu_top query internal)
> for a long running workload. multiply that with all active contexts.
>
> For long running contexts and frequenct queries, calling this ^ when a
> context is active does not add value. I would just call it in the else like
> before.

I have done this in v2 but are we certain that when a context is active
it's ce->stats.runtime.total has been updated? Is it updated on each
context save or only when scheduling is disabled (which would not happen if
the context is active). That was the reason for my keeping it out of the
else.

> > +
> >	/*
> >	 * GPU updates ce->lrc_reg_state[CTX_TIMESTAMP] when context is switched
> >	 * out, however GuC updates PPHWSP offsets below. Hence KMD (CPU)
> > @@ -1502,27 +1506,26 @@ static void __guc_context_update_clks(struct intel_context *ce)
> >	guc_update_pm_timestamp(guc, &unused);
> >
> >	if (engine_id != 0xffffffff && last_switch) {
> > -		start_gt_clk = READ_ONCE(ce->stats.runtime.start_gt_clk);
> > -		__extend_last_switch(guc, &start_gt_clk, last_switch);
> > -		active = intel_gt_clock_interval_to_ns(gt, guc->timestamp.gt_stamp - start_gt_clk);
> > -		WRITE_ONCE(ce->stats.runtime.start_gt_clk, start_gt_clk);
> > -		WRITE_ONCE(ce->stats.active, active);
> > -	} else {
> > -		lrc_update_runtime(ce);
> > +		__extend_last_switch(guc, &ce->stats.runtime.start_gt_clk, last_switch);
> > +		active = intel_gt_clock_interval_to_ns(gt,
> > +			      guc->timestamp.gt_stamp - ce->stats.runtime.start_gt_clk);
>
> Makes sense. Earlier it was a rmw, now it's just a write to
> ce->stats.runtime.start_gt_clk.

It is still a rmw, just that it is not as explicitly seen in the code as
was the case earlier.

> >	}
> >
> >	spin_unlock_irqrestore(&guc->timestamp.lock, flags);
> > +	intel_context_unpin(ce);
> > +
> > +	return total + active;
>
> return ce->stats.runtime.total + active;
>
> and then drop the local copy of total by bringing back the else{}.

Some changes here in v2, ce->stats.active had to be revived since we need
to return previously computed value when we cannot pin.

> > }
> >
> > -static void guc_context_update_stats(struct intel_context *ce)
> > +void lrc_update_runtime_locked(struct intel_context *ce)
> > {
> > -	if (!intel_context_pin_if_active(ce)) {
> > -		WRITE_ONCE(ce->stats.runtime.start_gt_clk, 0);
> > -		WRITE_ONCE(ce->stats.active, 0);
> > -		return;
> > -	}
> > +	struct intel_guc *guc = ce_to_guc(ce);
> > +	unsigned long flags;
> >
> > -	__guc_context_update_clks(ce);
> > +	intel_context_pin(ce);
> > +	spin_lock_irqsave(&guc->timestamp.lock, flags);
> > +	lrc_update_runtime(ce);
> > +	spin_unlock_irqrestore(&guc->timestamp.lock, flags);
> >	intel_context_unpin(ce);
>
> From where lrc_update_runtime_locked is being called, I would assume that
> the context is already pinned (until unpin_guc_id is called).

Fixed in v2.

From the other email:

> From your rfc patch, I like
> - the idea of not touching ce->stats.active
> - having the update_stats return u64
> - not doing a rmw for start_gt_clk
>
> With those changes, we are only accessing total in ce->stats, so we don't
> really need a lrc_update_runtime_locked.

Strictly speaking, (as explained above) because the computation of
ce->stats.runtime.total in lrc_update_runtime is not atomic and there are
potentially multiple concurrent callers of lrc_update_runtime,
lrc_update_runtime_locked is needed and I have retained it in v2.

When you say we don't need it, you probably mean that because we are
dealing with something like busyness, we can tolerate occasional errors in
busyness if we don't have locking (and we can't verify busyness values
anyway though I think they are expected to be monotonically
increasing). But my question is why not keep it simple and retain the
locking?

Thanks.
--
Ashutosh

> > }
> >
> > @@ -2780,7 +2783,7 @@ static void guc_context_unpin(struct intel_context *ce)
> > {
> >	struct intel_guc *guc = ce_to_guc(ce);
> >
> > -	lrc_update_runtime(ce);
> > +	lrc_update_runtime_locked(ce);
> >	unpin_guc_id(guc, ce);
> >	lrc_unpin(ce);
> >
> > --
> > 2.34.1
> >