Re: [Intel-gfx] [PATCH 2/3] drm/i915/gt: Make the heartbeat play nice with long pre-emption timeouts

[John Harrison <john.c.harrison@intel.com>]

On 2/22/2022 03:19, Tvrtko Ursulin wrote:
> On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Compute workloads are inherantly not pre-emptible for long periods on
>> current hardware. As a workaround for this, the pre-emption timeout
>> for compute capable engines was disabled. This is undesirable with GuC
>> submission as it prevents per engine reset of hung contexts. Hence the
>> next patch will re-enable the timeout but bumped up by an order of
>> magnititude.
>
> (Some typos above.)
I'm spotting 'inherently' but not anything else.

>
>> However, the heartbeat might not respect that. Depending upon current
>> activity, a pre-emption to the heartbeat pulse might not even be
>> attempted until the last heartbeat period. Which means that only one
>
> Might not be attempted, but could be if something is running with 
> lower priority. In which case I think special casing the last 
> heartbeat does not feel right because it can end up resetting the 
> engine before it was intended.
>
> Like if first heartbeat decides to preempt (the decision is backend 
> specific, could be same prio + timeslicing), and preempt timeout has 
> been set to heartbeat interval * 3, then 2nd heartbeat gets queued up, 
> then 3rd, and so reset is triggered even before the first preempt 
> timeout legitimately expires (or just as it is about to react).
>
> Instead, how about preempt timeout is always considered when 
> calculating when to emit the next heartbeat? End result would be 
> similar to your patch, in terms of avoiding the direct problem, 
> although hang detection would be overall longer (but more correct I 
> think).
>
> And it also means in the next patch you don't have to add coupling 
> between preempt timeout and heartbeat to intel_engine_setup. Instead 
> just some long preempt timeout would be needed. Granted, the 
> decoupling argument is not super strong since then the heartbeat code 
> has the coupling instead, but that still feels better to me. (Since we 
> can say heartbeats only make sense on loaded engines, and so things 
> like preempt timeout can legitimately be considered from there.)
>
> Incidentally, that would be similar to a patch which Chris had a year 
> ago 
> (https://patchwork.freedesktop.org/patch/419783/?series=86841&rev=1) 
> to fix some CI issue.
>
I'm not following your arguments.

Chris' patch is about not having two i915 based resets triggered 
concurrently - i915 based engine reset and i915 based GT reset. The 
purpose of this patch is to allow the GuC based engine reset to have a 
chance to occur before the i915 based GT reset kicks in.

It sounds like your argument above is about making the engine reset 
slower so that it doesn't happen before the appropriate heartbeat period 
for that potential reset scenario has expired. I don't see why that is 
at all necessary or useful.

If an early heartbeat period triggers an engine reset then the heartbeat 
pulse will go through. The heartbeat will thus see a happy system and 
not do anything further. If the given period does not trigger an engine 
reset but still does not get the pulse through (because the pulse is of 
too low a priority) then we move on to the next period and bump the 
priority. If the pre-emption has actually already been triggered anyway 
(and we are just waiting a while for it to timeout) then that's fine. 
The priority bump will have no effect because the context is already 
attempting to run. The heartbeat code doesn't care which priority level 
actually triggers the reset. It just cares whether or not the pulse 
finally makes it through. And the GuC doesn't care which heartbeat 
period the i915 is in. All it knows is that it has a request to schedule 
and whether the current context is pre-empting or not. So if period #1 
triggers the pre-emption but the timeout doesn't happen until period #3, 
who cares? The result is the same as if period #3 triggered the 
pre-emption and the timeout was shorter. The result being that the hung 
context is reset, the pulse makes it through and the heartbeat goes to 
sleep again.

The only period that really matters is the final one. At that point the 
pulse request is at highest priority and so must trigger a pre-emption 
request. We then need at least one full pre-emption period (plus some 
wiggle room for random delays in reset time, context switching, 
processing messages, etc.) to allow the GuC based timeout and reset to 
occur. Hence ensuring that the final heartbeat period is at least twice 
the pre-emption timeout (because 1.25 times is just messy when working 
with ints!).

That guarantees that GuC will get at least one complete opportunity to 
detect and recover the hang before i915 nukes the universe.

Whereas, bumping all heartbeat periods to be greater than the 
pre-emption timeout is wasteful and unnecessary. That leads to a total 
heartbeat time of about a minute. Which is a very long time to wait for 
a hang to be detected and recovered. Especially when the official limit 
on a context responding to an 'are you dead' query is only 7.5 seconds.



> On a related topic, if GuC engine resets stop working when preempt 
> timeout is set to zero - I think we need to somehow let the user know 
> if they try to tweak it via sysfs. Perhaps go as far as -EINVAL in GuC 
> mode, if i915.reset has not explicitly disabled engine resets.
Define 'stops working'. The definition of the sysfs interface is that a 
value of zero disables pre-emption. If you don't have pre-emption and 
your hang detection mechanism relies on pre-emption then you don't have 
a hang detection mechanism either. If the user really wants to allow 
their context to run forever and never be pre-empted then that means 
they also don't want it to be reset arbitrarily. Which means they would 
also be disabling the heartbeat timer as well. Indeed, this is what we 
advise compute customers to do. It is then up to the user themselves to 
spot a hang and to manually kill (Ctrl+C, kill ###, etc.) their task. 
Killing the CPU task will automatically clear up any GPU resources 
allocated to that task (excepting context persistence, which is a) 
broken and b) something we also tell compute customers to disable).

John.

>
> Regards,
>
> Tvrtko
>
>> period is granted for the pre-emption to occur. With the aforesaid
>> bump, the pre-emption timeout could be significantly larger than this
>> heartbeat period.
>>
>> So adjust the heartbeat code to take the pre-emption timeout into
>> account. When it reaches the final (high priority) period, it now
>> ensures the delay before hitting reset is bigger than the pre-emption
>> timeout.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c 
>> b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> index a3698f611f45..72a82a6085e0 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> @@ -22,9 +22,25 @@
>>     static bool next_heartbeat(struct intel_engine_cs *engine)
>>   {
>> +    struct i915_request *rq;
>>       long delay;
>>         delay = READ_ONCE(engine->props.heartbeat_interval_ms);
>> +
>> +    rq = engine->heartbeat.systole;
>> +    if (rq && rq->sched.attr.priority >= I915_PRIORITY_BARRIER) {
>> +        long longer;
>> +
>> +        /*
>> +         * The final try is at the highest priority possible. Up 
>> until now
>> +         * a pre-emption might not even have been attempted. So make 
>> sure
>> +         * this last attempt allows enough time for a pre-emption to 
>> occur.
>> +         */
>> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
>> +        if (longer > delay)
>> +            delay = longer;
>> +    }
>> +
>>       if (!delay)
>>           return false;