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

Tue Sep 6 18:29:15 UTC 2022

On Thu, Sep 01, 2022 at 04:55:22PM -0700, Dixit, Ashutosh wrote:
>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)?

With GPU, KMD and GuC running concurrently, after a context is done, 
this is the sequence.

GPU) updates context image (total_runtime)
KMD) reads total_runtime.
KMD) sees engine_id is valid. KMD uses start_time from pphwsp to 
calculate active_time.
KMD) returns total_runtime + active_time (double accounted busyness!!)
GuC) gets ctxt switchout event and sets engine_id and start_time to ~0

This is not rare when running the IGT tests, so the total runtime is 
updated in the query only if engine_id is idle. continued below ...

>
>>
>> > 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.
>

Hmm, maybe intel_context_get_total_runtime_ns can sleep, not sure why I 
was assuming that this falls in the perf_pmu path. This is now in the 
drm_fdinfo query path. + Tvrtko.

@Tvrtko, any idea if intel_context_get_total_runtime_ns path can sleep?

>> >	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.

continued here ...

You are right, it's not updated until scheduling is disabled or a query 
falls in the else block. We can run into a case where the 
stats.runtime.total has not yet been updated because the context is 
switching in/out too frequently and the query always lands in the active 
block. In this case busyness values will accumulate the time between 
switch-out to switch-in and eventually drop when ce->stats.runtime.total 
is updated. This is an issue we need to resolve.

I think we should track the previous value of total_runtime. On a query, 
if total_runtime has changed, we should just return that (irrespective 
of whether engine is active or not. Active time should be part of the 
busyness calculations only if total_runtime has not changed (compared to 
previous sample).

>
>> > +
>> >	/*
>> >	 * 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.

Let's wait for Tvrtko's inputs. If this can sleep, I'd just use the 
original changes you had.

>
>> > }
>> >
>> > -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?

The locking you suggested earlier is fine. I think I assumed the 
ce->stats.runtime.total is being updated atomically, but I see your 
point.

Thanks,
Umesh

>
>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
>> >