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

Commit Message

John Harrison Feb. 18, 2022, 9:33 p.m. UTC

From: John Harrison <[e-mail]>

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.

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
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 <[e-mail]>
---
 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Tvrtko Ursulin Feb. 22, 2022, 11:19 a.m. UTC | #1

On 18/02/2022 21:33, [e-mail] wrote:
> From: John Harrison <[e-mail]>
> 
> 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.)

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

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.

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 <[e-mail]>
> ---
>   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;
>

John Harrison Feb. 23, 2022, 2:45 a.m. UTC | #2

On 2/22/2022 03:19, Tvrtko Ursulin wrote:
> On 18/02/2022 21:33, [e-mail] wrote:
>> From: John Harrison <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>   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;

Tvrtko Ursulin Feb. 23, 2022, 1:58 p.m. UTC | #3

On 23/02/2022 02:45, John Harrison wrote:
> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>> On 18/02/2022 21:33, [e-mail] wrote:
>>> From: John Harrison <[e-mail]>
>>>
>>> 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.

Magnititude! O;)

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

Not sure how did you get one minute?

Regardless, crux of argument was to avoid GuC engine reset and heartbeat reset racing with each other, and to do that by considering the preempt timeout with the heartbeat interval. I was thinking about this scenario in this series:

[Please use fixed width font and no line wrap to view.]

A)

tP = preempt timeout
tH = hearbeat interval

tP = 3 * tH

1) Background load = I915_PRIORITY_DISPLAY

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> FULL RESET
                                                        |
                                                        \- preemption triggered, tP = 3 * tH ------\
                                                                                                    \-> preempt timeout would hit here

Here we have collateral damage due full reset, since we can't tell GuC to reset just one engine and we fudged tP just to "account" for heartbeats.

2) Background load = I915_CONTEXT_MAX_USER_PRIORITY

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> FULL RESET
                                     |
                                     \- preemption triggered, tP = 3 * tH ------> Preempt timeout races with "heartbeat stopped"

Here possible collateral damage due non-deterministic race (GuC and i915 run different clocks, or even if they did not). Can we do better?
                                                                                                   
3) Background load = I915_CONTEXT_MIN_USER_PRIORITY

<-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * tH] ----> full reset would be here
                |
                \- preemption triggered, tP = 3 * tH ----------------\
                                                                      \-> Preempt timeout reset

Here is is kind of least worse, but question is why we fudged tP when it gives us nothing good in this case.

B)

Instead, my idea to account for preempt timeout when calculating when to schedule next hearbeat would look like this:

First of all tP can be left at a large value unrelated to tH. Lets say tP = 640ms. tH stays 2.5s.

1) Background load = I915_PRIORITY_DISPLAY

<-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> Pulse3 <-- [tH + tP] --> full reset would be here
                                                                       |
                                                                       \-[tP]-> Engine reset here
                                                                                                    
Collateral damage avoided here.

2) Background load = I915_CONTEXT_MAX_USER_PRIORITY

<-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> Pulse3 <-- [tH + tP] --> full reset would be here
                                              |
                                              \-[tP]-> Engine reset here

No race, collateral damage avoided. Not sure race can be hit due built-in interdependency between tH and tP, unless maybe for really short timeouts which are not realistic to start with.
                                                                                                   
3) Background load = I915_CONTEXT_MIN_USER_PRIORITY

<-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> Pulse3 <-- [tH + tP] --> full reset would be here
                     |
                     \-[tP]-> Engine reset here
                                                                      
So as long as "640ms" preempt timeout is good enough for compute this option looks better to me. Or even if it is not, the timelines for option A above show engine reset effectively doesn't work for compute anyway. So if they set it to zero manually it's pretty much the same full reset for them.

Am I missing some requirement or you see another problem with this idea?

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

By stops working I meant that it stops working. :)

With execlist one can disable preempt timeout and "stopped heartbeat" can still reset the stuck engine and so avoid collateral damage. With GuC it appears this is not possible. So I was thinking this is something worthy a log notice.

> 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 

I don't think so. Preempt timeout is disabled already on TGL/RCS upstream but hearbeat is not and so hangcheck still works.

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

What is broken with context persistence? I noticed one patch claiming to be fixing something in that area which looked suspect. Has it been established no userspace relies on it?

Regards,

Tvrtko

John Harrison Feb. 23, 2022, 8 p.m. UTC | #4

On 2/23/2022 05:58, Tvrtko Ursulin wrote:
> On 23/02/2022 02:45, John Harrison wrote:
>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>> From: John Harrison <[e-mail]>
>>>>
>>>> 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.
>
> Magnititude! O;)
Doh!

[snip]

>> 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.
>
> Not sure how did you get one minute?
7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 => 
1 minute 15 seconds

Even ignoring any safety factor and just going with 7.5 * 5 still gets 
you to 37.5 seconds which is over a half a minute and likely to race.

>
> Regardless, crux of argument was to avoid GuC engine reset and 
> heartbeat reset racing with each other, and to do that by considering 
> the preempt timeout with the heartbeat interval. I was thinking about 
> this scenario in this series:
>
> [Please use fixed width font and no line wrap to view.]
>
> A)
>
> tP = preempt timeout
> tH = hearbeat interval
>
> tP = 3 * tH
>
> 1) Background load = I915_PRIORITY_DISPLAY
>
> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * 
> tH] ----> FULL RESET
>                                                        |
>                                                        \- preemption 
> triggered, tP = 3 * tH ------\
> \-> preempt timeout would hit here
>
> Here we have collateral damage due full reset, since we can't tell GuC 
> to reset just one engine and we fudged tP just to "account" for 
> heartbeats.
You are missing the whole point of the patch series which is that the 
last heartbeat period is '2 * tP' not '2 * tH'.
+        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;

By making the last period double the pre-emption timeout, it is 
guaranteed that the FULL RESET stage cannot be hit before the hardware 
has attempted and timed-out on at least one pre-emption.

[snip]


> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * 
> tH] ----> full reset would be here
>                |
>                \- preemption triggered, tP = 3 * tH ----------------\
> \-> Preempt timeout reset
>
> Here is is kind of least worse, but question is why we fudged tP when 
> it gives us nothing good in this case.
>
The point of fudging tP(RCS) is to give compute workloads longer to 
reach a pre-emptible point (given that EU walkers are basically not 
pre-emptible). The reason for doing the fudge is not connected to the 
heartbeat at all. The fact that it causes problems for the heartbeat is 
an undesired side effect.

Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
calculation that gave us something that all interested parties were 
vaguely happy with. It could just as easily be a fixed, hard coded value 
of 7.5s but having it based on something configurable seemed more 
sensible. The other option was 'tP(RCS) = tP * 12' but that felt more 
arbitrary than basing it on the average heartbeat timeout. As in, three 
heartbeat periods is about what a normal prio task gets before it gets 
pre-empted by the heartbeat. So using that for general purpose 
pre-emptions (e.g. time slicing between multiple user apps) seems 
reasonable.


> B)
>
> Instead, my idea to account for preempt timeout when calculating when 
> to schedule next hearbeat would look like this:
>
> First of all tP can be left at a large value unrelated to tH. Lets say 
> tP = 640ms. tH stays 2.5s.
640ms is not 'large'. The requirement is either zero (disabled) or 
region of 7.5s. The 640ms figure is the default for non-compute engines. 
Anything that can run EUs needs to be 'huge'.


>
> 1) Background load = I915_PRIORITY_DISPLAY
>
> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> 
> Pulse3 <-- [tH + tP] --> full reset would be here
Sure, this works but each period is now 2.5 + 7.5 = 10s. The full five 
periods is therefore 50s, which is practically a minute.

[snip]

> Am I missing some requirement or you see another problem with this idea?
>
>>> 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 
>
> By stops working I meant that it stops working. :)
>
> With execlist one can disable preempt timeout and "stopped heartbeat" 
> can still reset the stuck engine and so avoid collateral damage. With 
> GuC it appears this is not possible. So I was thinking this is 
> something worthy a log notice.
>
>> 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 
>
> I don't think so. Preempt timeout is disabled already on TGL/RCS 
> upstream but hearbeat is not and so hangcheck still works.
The pre-emption disable in upstream is not a valid solution for compute 
customers. It is a worst-of-all-worlds hack for general usage. As noted 
already, any actual compute specific customer is advised to disable all 
forms of reset and do their hang detection manually. A slightly less 
worse hack for customers that are not actually running long compute 
workloads (i.e. the vast majority of end users) is to just use a long 
pre-emption timeout.

>
>> 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).
>
> What is broken with context persistence? I noticed one patch claiming 
> to be fixing something in that area which looked suspect. Has it been 
> established no userspace relies on it?
One major issue is that it has hooks into the execlist scheduler 
backend. I forget the exact details right now. The implementation as a 
whole is incredibly complex and convoluted :(. But there's stuff about 
what happens when you disable the heartbeat after having closed a 
persistence context's handle (and thus made it persisting). There's also 
things like it sends a super high priority heartbeat pulse at the point 
of becoming persisting. That plays havoc for platforms with dependent 
engines and/or compute workloads. A context becomes persisting on RCS 
and results in your unrealted CCS work being reset. It's a mess.

The comment from Daniel Vetter is that persistence should have no 
connection to the heartbeat at all. All of that dynamic behaviour and 
complexity should just be removed.

Persistence itself can stay. There are valid UMD use cases. It is just 
massively over complicated and doesn't work in all corner cases when not 
using execlist submission or on newer platforms. The simplification that 
is planned is to allow contexts to persist until the associated DRM 
master handle is closed. At that point, all contexts associated with 
that DRM handle are killed. That is what AMD and others apparently 
implement.

John.

>
> Regards,
>
> Tvrtko

Tvrtko Ursulin Feb. 24, 2022, 11:41 a.m. UTC | #5

On 23/02/2022 20:00, John Harrison wrote:
> On 2/23/2022 05:58, Tvrtko Ursulin wrote:
>> On 23/02/2022 02:45, John Harrison wrote:
>>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>> From: John Harrison <[e-mail]>
>>>>>
>>>>> 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.
>>
>> Magnititude! O;)
> Doh!
> 
> [snip]
> 
>>> 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.
>>
>> Not sure how did you get one minute?
> 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 => 
> 1 minute 15 seconds
> 
> Even ignoring any safety factor and just going with 7.5 * 5 still gets 
> you to 37.5 seconds which is over a half a minute and likely to race.

Ah because my starting point is there should be no preempt timeout = heartbeat * 3, I just think that's too ugly.

>> Regardless, crux of argument was to avoid GuC engine reset and 
>> heartbeat reset racing with each other, and to do that by considering 
>> the preempt timeout with the heartbeat interval. I was thinking about 
>> this scenario in this series:
>>
>> [Please use fixed width font and no line wrap to view.]
>>
>> A)
>>
>> tP = preempt timeout
>> tH = hearbeat interval
>>
>> tP = 3 * tH
>>
>> 1) Background load = I915_PRIORITY_DISPLAY
>>
>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * 
>> tH] ----> FULL RESET
>>                                                        |
>>                                                        \- preemption 
>> triggered, tP = 3 * tH ------\
>> \-> preempt timeout would hit here
>>
>> Here we have collateral damage due full reset, since we can't tell GuC 
>> to reset just one engine and we fudged tP just to "account" for 
>> heartbeats.
> You are missing the whole point of the patch series which is that the 
> last heartbeat period is '2 * tP' not '2 * tH'.
> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
> 
> By making the last period double the pre-emption timeout, it is 
> guaranteed that the FULL RESET stage cannot be hit before the hardware 
> has attempted and timed-out on at least one pre-emption.

Oh well :) that probably means the overall scheme is too odd for me. tp = 3tH and last pulse after 2tP I mean.

> [snip]
> 
> 
>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 * 
>> tH] ----> full reset would be here
>>                |
>>                \- preemption triggered, tP = 3 * tH ----------------\
>> \-> Preempt timeout reset
>>
>> Here is is kind of least worse, but question is why we fudged tP when 
>> it gives us nothing good in this case.
>>
> The point of fudging tP(RCS) is to give compute workloads longer to 
> reach a pre-emptible point (given that EU walkers are basically not 
> pre-emptible). The reason for doing the fudge is not connected to the 
> heartbeat at all. The fact that it causes problems for the heartbeat is 
> an undesired side effect.
> 
> Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
> calculation that gave us something that all interested parties were 
> vaguely happy with. It could just as easily be a fixed, hard coded value 
> of 7.5s but having it based on something configurable seemed more 
> sensible. The other option was 'tP(RCS) = tP * 12' but that felt more 
> arbitrary than basing it on the average heartbeat timeout. As in, three 
> heartbeat periods is about what a normal prio task gets before it gets 
> pre-empted by the heartbeat. So using that for general purpose 
> pre-emptions (e.g. time slicing between multiple user apps) seems 
> reasonable.

I think the fact you say tP fudge is not related to heartbeats and then go to mention heartbeat even in the "formula" which uses no tH is saying something (at least that's how I read the 7.5s option). :)

>> B)
>>
>> Instead, my idea to account for preempt timeout when calculating when 
>> to schedule next hearbeat would look like this:
>>
>> First of all tP can be left at a large value unrelated to tH. Lets say 
>> tP = 640ms. tH stays 2.5s.
> 640ms is not 'large'. The requirement is either zero (disabled) or 
> region of 7.5s. The 640ms figure is the default for non-compute engines. 
> Anything that can run EUs needs to be 'huge'.
> 
> 
>>
>> 1) Background load = I915_PRIORITY_DISPLAY
>>
>> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> 
>> Pulse3 <-- [tH + tP] --> full reset would be here
> Sure, this works but each period is now 2.5 + 7.5 = 10s. The full five 
> periods is therefore 50s, which is practically a minute.

No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.

> [snip]
> 
>> Am I missing some requirement or you see another problem with this idea?
>>
>>>> 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 
>>
>> By stops working I meant that it stops working. :)
>>
>> With execlist one can disable preempt timeout and "stopped heartbeat" 
>> can still reset the stuck engine and so avoid collateral damage. With 
>> GuC it appears this is not possible. So I was thinking this is 
>> something worthy a log notice.
>>
>>> 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 
>>
>> I don't think so. Preempt timeout is disabled already on TGL/RCS 
>> upstream but hearbeat is not and so hangcheck still works.
> The pre-emption disable in upstream is not a valid solution for compute 
> customers. It is a worst-of-all-worlds hack for general usage. As noted 
> already, any actual compute specific customer is advised to disable all 
> forms of reset and do their hang detection manually. A slightly less 
> worse hack for customers that are not actually running long compute 
> workloads (i.e. the vast majority of end users) is to just use a long 
> pre-emption timeout.

If disabled preemption timeout is worst of all words and compute needs to disable heartbeat as well then why did we put it in? Perhaps it was not know at the time it would not be good enough. But anyway, do I read you correct that current thinking is it would be better to leave it at default 640ms?

If so, if we went with my proposal, would everyone be happy? If yes, isn't it a simpler scheme? No special casing when setting the preempt timeout, no special casing of the last heartbeat pulse. Works predictably regardless of the priority of the hypothetical non-preemptible workload.

>>> 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).
>>
>> What is broken with context persistence? I noticed one patch claiming 
>> to be fixing something in that area which looked suspect. Has it been 
>> established no userspace relies on it?
> One major issue is that it has hooks into the execlist scheduler 
> backend. I forget the exact details right now. The implementation as a 
> whole is incredibly complex and convoluted :(. But there's stuff about 
> what happens when you disable the heartbeat after having closed a 
> persistence context's handle (and thus made it persisting). There's also 
> things like it sends a super high priority heartbeat pulse at the point 
> of becoming persisting. That plays havoc for platforms with dependent 
> engines and/or compute workloads. A context becomes persisting on RCS 
> and results in your unrealted CCS work being reset. It's a mess.
> 
> The comment from Daniel Vetter is that persistence should have no 
> connection to the heartbeat at all. All of that dynamic behaviour and 
> complexity should just be removed.

Dependent engines is definitely a topic on it's own, outside hearbeats, persistence and all.

Otherwise there is definitely complexity in the execlists backend but I am not sure if logic persistence and heartbeats are so very connected. It does send a pulse when heartbeat interval is changed, which if going to zero, it will kick of closed contexts if it can:

static struct intel_engine_cs *
__execlists_schedule_in(struct i915_request *rq)
{
	struct intel_engine_cs * const engine = rq->engine;
	struct intel_context * const ce = rq->context;

	intel_context_get(ce);

	if (unlikely(intel_context_is_closed(ce) &&
		     !intel_engine_has_heartbeat(engine)))
		intel_context_set_banned(ce);

	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
		reset_active(rq, engine);

Is this what you mean? The point of this is to make sure persistent context does not hog the engine forever if hangcheck has been disabled.

Reminds me of my improvement to customer experience which never got in (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). Point of that one was to avoid engine reset (or worse) after user presses "Ctrl-C" if something takes just over 1ms to cleanly complete.

> Persistence itself can stay. There are valid UMD use cases. It is just 
> massively over complicated and doesn't work in all corner cases when not 
> using execlist submission or on newer platforms. The simplification that 
> is planned is to allow contexts to persist until the associated DRM 
> master handle is closed. At that point, all contexts associated with 
> that DRM handle are killed. That is what AMD and others apparently 
> implement.

Okay, that goes against one recent IGT patch which appeared to work around something by moving the position of _context_ close.

Regards,

Tvrtko

John Harrison Feb. 24, 2022, 7:45 p.m. UTC | #6

On 2/24/2022 03:41, Tvrtko Ursulin wrote:
> On 23/02/2022 20:00, John Harrison wrote:
>> On 2/23/2022 05:58, Tvrtko Ursulin wrote:
>>> On 23/02/2022 02:45, John Harrison wrote:
>>>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>>> From: John Harrison <[e-mail]>
>>>>>>
>>>>>> 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.
>>>
>>> Magnititude! O;)
>> Doh!
>>
>> [snip]
>>
>>>> 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.
>>>
>>> Not sure how did you get one minute?
>> 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 
>> => 1 minute 15 seconds
>>
>> Even ignoring any safety factor and just going with 7.5 * 5 still 
>> gets you to 37.5 seconds which is over a half a minute and likely to 
>> race.
>
> Ah because my starting point is there should be no preempt timeout = 
> heartbeat * 3, I just think that's too ugly.
Then complain at the hardware designers to give us mid-thread 
pre-emption back. The heartbeat is only one source of pre-emption 
events. For example, a user can be running multiple contexts in parallel 
and expecting them to time slice on a single engine. Or maybe a user is 
just running one compute task in the background but is doing render work 
in the foreground. Etc.

There was a reason the original hack was to disable pre-emption rather 
than increase the heartbeat. This is simply a slightly less ugly version 
of the same hack. And unfortunately, the basic idea of the hack is 
non-negotiable.

As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) * 12' 
or 'tP(RCS) = 7500' are the available options. Given that the heartbeat 
is the ever present hard limit, it seems most plausible to base the hack 
on that. Any of the others works, though. Although I think a explicit 
hardcoded value is the most ugly. I guess the other option is to add 
CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default that to 7500.

Take your pick. But 640ms is not allowed.

>
>>> Regardless, crux of argument was to avoid GuC engine reset and 
>>> heartbeat reset racing with each other, and to do that by 
>>> considering the preempt timeout with the heartbeat interval. I was 
>>> thinking about this scenario in this series:
>>>
>>> [Please use fixed width font and no line wrap to view.]
>>>
>>> A)
>>>
>>> tP = preempt timeout
>>> tH = hearbeat interval
>>>
>>> tP = 3 * tH
>>>
>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>
>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 
>>> * tH] ----> FULL RESET
>>>                                                        |
>>>                                                        \- preemption 
>>> triggered, tP = 3 * tH ------\
>>> \-> preempt timeout would hit here
>>>
>>> Here we have collateral damage due full reset, since we can't tell 
>>> GuC to reset just one engine and we fudged tP just to "account" for 
>>> heartbeats.
>> You are missing the whole point of the patch series which is that the 
>> last heartbeat period is '2 * tP' not '2 * tH'.
>> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
>>
>> By making the last period double the pre-emption timeout, it is 
>> guaranteed that the FULL RESET stage cannot be hit before the 
>> hardware has attempted and timed-out on at least one pre-emption.
>
> Oh well :) that probably means the overall scheme is too odd for me. 
> tp = 3tH and last pulse after 2tP I mean.
To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = 
tP(current) * 2;'. Seems fairly straight forward to me. It's not a 
recursive definition or anything like that. It gives us a total 
heartbeat timeout that is close to the original version but still allows 
at least one pre-emption event.

>
>> [snip]
>>
>>
>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 
>>> * tH] ----> full reset would be here
>>>                |
>>>                \- preemption triggered, tP = 3 * tH ----------------\
>>> \-> Preempt timeout reset
>>>
>>> Here is is kind of least worse, but question is why we fudged tP 
>>> when it gives us nothing good in this case.
>>>
>> The point of fudging tP(RCS) is to give compute workloads longer to 
>> reach a pre-emptible point (given that EU walkers are basically not 
>> pre-emptible). The reason for doing the fudge is not connected to the 
>> heartbeat at all. The fact that it causes problems for the heartbeat 
>> is an undesired side effect.
>>
>> Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
>> calculation that gave us something that all interested parties were 
>> vaguely happy with. It could just as easily be a fixed, hard coded 
>> value of 7.5s but having it based on something configurable seemed 
>> more sensible. The other option was 'tP(RCS) = tP * 12' but that felt 
>> more arbitrary than basing it on the average heartbeat timeout. As 
>> in, three heartbeat periods is about what a normal prio task gets 
>> before it gets pre-empted by the heartbeat. So using that for general 
>> purpose pre-emptions (e.g. time slicing between multiple user apps) 
>> seems reasonable.
>
> I think the fact you say tP fudge is not related to heartbeats and 
> then go to mention heartbeat even in the "formula" which uses no tH is 
> saying something (at least that's how I read the 7.5s option). :)
I said the tP fudge is not because of the heartbeat. It is obviously 
related.

As per comment above, the fudge factor is based on the heartbeat because 
the heartbeat is the ultimate limit. But the *reason* for the fudge fact 
has nothing to do with the heartbeat. It is required even if heartbeats 
are disabled.

>
>>> B)
>>>
>>> Instead, my idea to account for preempt timeout when calculating 
>>> when to schedule next hearbeat would look like this:
>>>
>>> First of all tP can be left at a large value unrelated to tH. Lets 
>>> say tP = 640ms. tH stays 2.5s.
>> 640ms is not 'large'. The requirement is either zero (disabled) or 
>> region of 7.5s. The 640ms figure is the default for non-compute 
>> engines. Anything that can run EUs needs to be 'huge'.
>>
>>
>>>
>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>
>>> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> 
>>> Pulse3 <-- [tH + tP] --> full reset would be here
>> Sure, this works but each period is now 2.5 + 7.5 = 10s. The full 
>> five periods is therefore 50s, which is practically a minute.
>
> No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.
Not good enough. After 2.5s, we send a pulse. After a further 640ms we 
perform an engine reset. That means your compute workload had only 640ms 
after being told to pre-empt to reach a pre-emption point. That won't 
work. It needs to be multiple seconds.

>
>> [snip]
>>
>>> Am I missing some requirement or you see another problem with this 
>>> idea?
>>>
>>>>> 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 
>>>
>>> By stops working I meant that it stops working. :)
>>>
>>> With execlist one can disable preempt timeout and "stopped 
>>> heartbeat" can still reset the stuck engine and so avoid collateral 
>>> damage. With GuC it appears this is not possible. So I was thinking 
>>> this is something worthy a log notice.
>>>
>>>> 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 
>>>
>>> I don't think so. Preempt timeout is disabled already on TGL/RCS 
>>> upstream but hearbeat is not and so hangcheck still works.
>> The pre-emption disable in upstream is not a valid solution for 
>> compute customers. It is a worst-of-all-worlds hack for general 
>> usage. As noted already, any actual compute specific customer is 
>> advised to disable all forms of reset and do their hang detection 
>> manually. A slightly less worse hack for customers that are not 
>> actually running long compute workloads (i.e. the vast majority of 
>> end users) is to just use a long pre-emption timeout.
>
> If disabled preemption timeout is worst of all words and compute needs 
> to disable heartbeat as well then why did we put it in? Perhaps it was 
> not know at the time it would not be good enough. But anyway, do I 
> read you correct that current thinking is it would be better to leave 
> it at default 640ms?
No. We cannot have the RCS default to 640ms.

Note that there is a difference between 'general end user who might run 
some compute' and 'compute focused customer'. The driver defaults 
(disabled or 7500ms) are for the general user who gets the 
out-of-the-box experience and expects to be able to run 'normal' 
workloads without hitting problems. I.e. they expect hung tasks to get 
reset in a timely manner and while they might run some AI or other 
compute workloads, they are not a HPC datacenter. Whereas the compute 
datacenter customer expects their workloads to run for arbitrarily long 
times (minutes, hours, maybe even days) without being arbitrarily 
killed. Those customers will be explicitly configuring their datacenter 
server for that scenario and thus don't care what the defaults are.


>
> If so, if we went with my proposal, would everyone be happy? If yes, 
> isn't it a simpler scheme? No special casing when setting the preempt 
> timeout, no special casing of the last heartbeat pulse. Works 
> predictably regardless of the priority of the hypothetical 
> non-preemptible workload.
No, we have to have the increased pre-emption timeout. And that has 
ripple effects of making very long heartbeats or risking races with the 
heartbeat beating the per engine reset.


>
>>>> 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).
>>>
>>> What is broken with context persistence? I noticed one patch 
>>> claiming to be fixing something in that area which looked suspect. 
>>> Has it been established no userspace relies on it?
>> One major issue is that it has hooks into the execlist scheduler 
>> backend. I forget the exact details right now. The implementation as 
>> a whole is incredibly complex and convoluted :(. But there's stuff 
>> about what happens when you disable the heartbeat after having closed 
>> a persistence context's handle (and thus made it persisting). There's 
>> also things like it sends a super high priority heartbeat pulse at 
>> the point of becoming persisting. That plays havoc for platforms with 
>> dependent engines and/or compute workloads. A context becomes 
>> persisting on RCS and results in your unrealted CCS work being reset. 
>> It's a mess.
>>
>> The comment from Daniel Vetter is that persistence should have no 
>> connection to the heartbeat at all. All of that dynamic behaviour and 
>> complexity should just be removed.
>
> Dependent engines is definitely a topic on it's own, outside 
> hearbeats, persistence and all.
Except that it has implications for persistence which the current driver 
does not take into account.

>
> Otherwise there is definitely complexity in the execlists backend but 
> I am not sure if logic persistence and heartbeats are so very 
> connected. It does send a pulse when heartbeat interval is changed, 
> which if going to zero, it will kick of closed contexts if it can:
>
> static struct intel_engine_cs *
> __execlists_schedule_in(struct i915_request *rq)
> {
>     struct intel_engine_cs * const engine = rq->engine;
>     struct intel_context * const ce = rq->context;
>
>     intel_context_get(ce);
>
>     if (unlikely(intel_context_is_closed(ce) &&
>              !intel_engine_has_heartbeat(engine)))
>         intel_context_set_banned(ce);
>
>     if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>         reset_active(rq, engine);
>
> Is this what you mean? The point of this is to make sure persistent 
> context does not hog the engine forever if hangcheck has been disabled.
>
> Reminds me of my improvement to customer experience which never got in 
> (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). 
> Point of that one was to avoid engine reset (or worse) after user 
> presses "Ctrl-C" if something takes just over 1ms to cleanly complete.
The plan is that the persistent contexts would still get the default 
grace period (pre-emption timeout at least) to finish but Ctrl+C will 
kill it within a timely manner if it does not finish.

>
>> Persistence itself can stay. There are valid UMD use cases. It is 
>> just massively over complicated and doesn't work in all corner cases 
>> when not using execlist submission or on newer platforms. The 
>> simplification that is planned is to allow contexts to persist until 
>> the associated DRM master handle is closed. At that point, all 
>> contexts associated with that DRM handle are killed. That is what AMD 
>> and others apparently implement.
>
> Okay, that goes against one recent IGT patch which appeared to work 
> around something by moving the position of _context_ close.
No it does not. The context close is not the trigger. The trigger is 
closing the top level DRM handle. If your context has persistence 
enabled (the default) then closing the context handle will have no 
effect. No pulse, no pre-emption, no kill, nothing. But when the top 
level handle is closed (application exit through whatever mechanism) 
then all GPU resources will be cleaned up. As above, with at least a 
pre-emption timeout grace period, but after that it is termination time.

The media use cases for persistence are all happy with this scheme. I 
don't actually recall if we got a reply back from the OGL people. They 
were definitely on the email thread/Jira task and did not complain. OCL 
obviously don't care as their first action is to explicitly disable 
persistence.

John.

>
> Regards,
>
> Tvrtko

Tvrtko Ursulin Feb. 25, 2022, 6:14 p.m. UTC | #7

I'll try to simplify the discussion here:

On 24/02/2022 19:45, John Harrison wrote:
> On 2/24/2022 03:41, Tvrtko Ursulin wrote:
>> On 23/02/2022 20:00, John Harrison wrote:
>>> On 2/23/2022 05:58, Tvrtko Ursulin wrote:
>>>> On 23/02/2022 02:45, John Harrison wrote:
>>>>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>>>> From: John Harrison <[e-mail]>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> Magnititude! O;)
>>> Doh!
>>>
>>> [snip]
>>>
>>>>> 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.
>>>>
>>>> Not sure how did you get one minute?
>>> 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 75 
>>> => 1 minute 15 seconds
>>>
>>> Even ignoring any safety factor and just going with 7.5 * 5 still 
>>> gets you to 37.5 seconds which is over a half a minute and likely to 
>>> race.
>>
>> Ah because my starting point is there should be no preempt timeout = 
>> heartbeat * 3, I just think that's too ugly.
> Then complain at the hardware designers to give us mid-thread 
> pre-emption back. The heartbeat is only one source of pre-emption 
> events. For example, a user can be running multiple contexts in parallel 
> and expecting them to time slice on a single engine. Or maybe a user is 
> just running one compute task in the background but is doing render work 
> in the foreground. Etc.
> 
> There was a reason the original hack was to disable pre-emption rather 
> than increase the heartbeat. This is simply a slightly less ugly version 
> of the same hack. And unfortunately, the basic idea of the hack is 
> non-negotiable.
> 
> As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) * 12' 
> or 'tP(RCS) = 7500' are the available options. Given that the heartbeat 
> is the ever present hard limit, it seems most plausible to base the hack 
> on that. Any of the others works, though. Although I think a explicit 
> hardcoded value is the most ugly. I guess the other option is to add 
> CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default that to 7500.
> 
> Take your pick. But 640ms is not allowed.
> 
>>
>>>> Regardless, crux of argument was to avoid GuC engine reset and 
>>>> heartbeat reset racing with each other, and to do that by 
>>>> considering the preempt timeout with the heartbeat interval. I was 
>>>> thinking about this scenario in this series:
>>>>
>>>> [Please use fixed width font and no line wrap to view.]
>>>>
>>>> A)
>>>>
>>>> tP = preempt timeout
>>>> tH = hearbeat interval
>>>>
>>>> tP = 3 * tH
>>>>
>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>
>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 
>>>> * tH] ----> FULL RESET
>>>>                                                        |
>>>>                                                        \- preemption 
>>>> triggered, tP = 3 * tH ------\
>>>> \-> preempt timeout would hit here
>>>>
>>>> Here we have collateral damage due full reset, since we can't tell 
>>>> GuC to reset just one engine and we fudged tP just to "account" for 
>>>> heartbeats.
>>> You are missing the whole point of the patch series which is that the 
>>> last heartbeat period is '2 * tP' not '2 * tH'.
>>> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
>>>
>>> By making the last period double the pre-emption timeout, it is 
>>> guaranteed that the FULL RESET stage cannot be hit before the 
>>> hardware has attempted and timed-out on at least one pre-emption.
>>
>> Oh well :) that probably means the overall scheme is too odd for me. 
>> tp = 3tH and last pulse after 2tP I mean.
> To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = 
> tP(current) * 2;'. Seems fairly straight forward to me. It's not a 
> recursive definition or anything like that. It gives us a total 
> heartbeat timeout that is close to the original version but still allows 
> at least one pre-emption event.
> 
>>
>>> [snip]
>>>
>>>
>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- [2 
>>>> * tH] ----> full reset would be here
>>>>                |
>>>>                \- preemption triggered, tP = 3 * tH ----------------\
>>>> \-> Preempt timeout reset
>>>>
>>>> Here is is kind of least worse, but question is why we fudged tP 
>>>> when it gives us nothing good in this case.
>>>>
>>> The point of fudging tP(RCS) is to give compute workloads longer to 
>>> reach a pre-emptible point (given that EU walkers are basically not 
>>> pre-emptible). The reason for doing the fudge is not connected to the 
>>> heartbeat at all. The fact that it causes problems for the heartbeat 
>>> is an undesired side effect.
>>>
>>> Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
>>> calculation that gave us something that all interested parties were 
>>> vaguely happy with. It could just as easily be a fixed, hard coded 
>>> value of 7.5s but having it based on something configurable seemed 
>>> more sensible. The other option was 'tP(RCS) = tP * 12' but that felt 
>>> more arbitrary than basing it on the average heartbeat timeout. As 
>>> in, three heartbeat periods is about what a normal prio task gets 
>>> before it gets pre-empted by the heartbeat. So using that for general 
>>> purpose pre-emptions (e.g. time slicing between multiple user apps) 
>>> seems reasonable.
>>
>> I think the fact you say tP fudge is not related to heartbeats and 
>> then go to mention heartbeat even in the "formula" which uses no tH is 
>> saying something (at least that's how I read the 7.5s option). :)
> I said the tP fudge is not because of the heartbeat. It is obviously 
> related.
> 
> As per comment above, the fudge factor is based on the heartbeat because 
> the heartbeat is the ultimate limit. But the *reason* for the fudge fact 
> has nothing to do with the heartbeat. It is required even if heartbeats 
> are disabled.
> 
>>
>>>> B)
>>>>
>>>> Instead, my idea to account for preempt timeout when calculating 
>>>> when to schedule next hearbeat would look like this:
>>>>
>>>> First of all tP can be left at a large value unrelated to tH. Lets 
>>>> say tP = 640ms. tH stays 2.5s.
>>> 640ms is not 'large'. The requirement is either zero (disabled) or 
>>> region of 7.5s. The 640ms figure is the default for non-compute 
>>> engines. Anything that can run EUs needs to be 'huge'.
>>>
>>>
>>>>
>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>
>>>> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] --> 
>>>> Pulse3 <-- [tH + tP] --> full reset would be here
>>> Sure, this works but each period is now 2.5 + 7.5 = 10s. The full 
>>> five periods is therefore 50s, which is practically a minute.
>>
>> No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.
> Not good enough. After 2.5s, we send a pulse. After a further 640ms we 
> perform an engine reset. That means your compute workload had only 640ms 
> after being told to pre-empt to reach a pre-emption point. That won't 
> work. It needs to be multiple seconds.
> 
>>
>>> [snip]
>>>
>>>> Am I missing some requirement or you see another problem with this 
>>>> idea?
>>>>
>>>>>> 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 
>>>>
>>>> By stops working I meant that it stops working. :)
>>>>
>>>> With execlist one can disable preempt timeout and "stopped 
>>>> heartbeat" can still reset the stuck engine and so avoid collateral 
>>>> damage. With GuC it appears this is not possible. So I was thinking 
>>>> this is something worthy a log notice.
>>>>
>>>>> 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 
>>>>
>>>> I don't think so. Preempt timeout is disabled already on TGL/RCS 
>>>> upstream but hearbeat is not and so hangcheck still works.
>>> The pre-emption disable in upstream is not a valid solution for 
>>> compute customers. It is a worst-of-all-worlds hack for general 
>>> usage. As noted already, any actual compute specific customer is 
>>> advised to disable all forms of reset and do their hang detection 
>>> manually. A slightly less worse hack for customers that are not 
>>> actually running long compute workloads (i.e. the vast majority of 
>>> end users) is to just use a long pre-emption timeout.
>>
>> If disabled preemption timeout is worst of all words and compute needs 
>> to disable heartbeat as well then why did we put it in? Perhaps it was 
>> not know at the time it would not be good enough. But anyway, do I 
>> read you correct that current thinking is it would be better to leave 
>> it at default 640ms?
> No. We cannot have the RCS default to 640ms.
> 
> Note that there is a difference between 'general end user who might run 
> some compute' and 'compute focused customer'. The driver defaults 
> (disabled or 7500ms) are for the general user who gets the 
> out-of-the-box experience and expects to be able to run 'normal' 
> workloads without hitting problems. I.e. they expect hung tasks to get 
> reset in a timely manner and while they might run some AI or other 
> compute workloads, they are not a HPC datacenter. Whereas the compute 
> datacenter customer expects their workloads to run for arbitrarily long 
> times (minutes, hours, maybe even days) without being arbitrarily 
> killed. Those customers will be explicitly configuring their datacenter 
> server for that scenario and thus don't care what the defaults are.

Okay maybe I misunderstood what you were saying earlier about worst of 
all worlds and all. But tell me this, if preemption timeout on RCS is 
not directly related to hearbeats, but to some pessimistic expected user 
workloads, what is wrong with my scheme of calculating the next 
heartbeat pulse as tH + tP?

We can leave tH as default 2.5s and tP you set for RCS to 12s if that is 
what you say is required. Or whatever long value really.

Your only objection is that ends up with too long total time before 
reset? Or something else as well?

It's long but it is correct in a way. Because we can't expect hearbeat 
to react quicker than the interval + preempt timeout (or timeslice for 
equal priority) + some scheduling latency.

I conceptually disagree with the last hearbeat pulse being special. If 
the user concept is "after N heartbeats you are out" and you want to 
make it "after N-1 heartbeats plus 2 preemption periods you are out", 
where preemption period actually depends on heartbeat period, then that 
sounds really convoluted to me.

And we don't know which of the pulses will trigger preemption since user 
priority we don't control. So low priority compute task gets reset after 
5s, normal priority gets to run for 12s. Effectively making context 
priority a variable in hangcheck.

>>
>> If so, if we went with my proposal, would everyone be happy? If yes, 
>> isn't it a simpler scheme? No special casing when setting the preempt 
>> timeout, no special casing of the last heartbeat pulse. Works 
>> predictably regardless of the priority of the hypothetical 
>> non-preemptible workload.
> No, we have to have the increased pre-emption timeout. And that has 
> ripple effects of making very long heartbeats or risking races with the 
> heartbeat beating the per engine reset.
> 
> 
>>
>>>>> 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).
>>>>
>>>> What is broken with context persistence? I noticed one patch 
>>>> claiming to be fixing something in that area which looked suspect. 
>>>> Has it been established no userspace relies on it?
>>> One major issue is that it has hooks into the execlist scheduler 
>>> backend. I forget the exact details right now. The implementation as 
>>> a whole is incredibly complex and convoluted :(. But there's stuff 
>>> about what happens when you disable the heartbeat after having closed 
>>> a persistence context's handle (and thus made it persisting). There's 
>>> also things like it sends a super high priority heartbeat pulse at 
>>> the point of becoming persisting. That plays havoc for platforms with 
>>> dependent engines and/or compute workloads. A context becomes 
>>> persisting on RCS and results in your unrealted CCS work being reset. 
>>> It's a mess.
>>>
>>> The comment from Daniel Vetter is that persistence should have no 
>>> connection to the heartbeat at all. All of that dynamic behaviour and 
>>> complexity should just be removed.
>>
>> Dependent engines is definitely a topic on it's own, outside 
>> hearbeats, persistence and all.
> Except that it has implications for persistence which the current driver 
> does not take into account.

Well current driver does not take RCS+CCS dependency into account so 
that should come first, or all in one package at least.

>> Otherwise there is definitely complexity in the execlists backend but 
>> I am not sure if logic persistence and heartbeats are so very 
>> connected. It does send a pulse when heartbeat interval is changed, 
>> which if going to zero, it will kick of closed contexts if it can:
>>
>> static struct intel_engine_cs *
>> __execlists_schedule_in(struct i915_request *rq)
>> {
>>     struct intel_engine_cs * const engine = rq->engine;
>>     struct intel_context * const ce = rq->context;
>>
>>     intel_context_get(ce);
>>
>>     if (unlikely(intel_context_is_closed(ce) &&
>>              !intel_engine_has_heartbeat(engine)))
>>         intel_context_set_banned(ce);
>>
>>     if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>>         reset_active(rq, engine);
>>
>> Is this what you mean? The point of this is to make sure persistent 
>> context does not hog the engine forever if hangcheck has been disabled.
>>
>> Reminds me of my improvement to customer experience which never got in 
>> (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). 
>> Point of that one was to avoid engine reset (or worse) after user 
>> presses "Ctrl-C" if something takes just over 1ms to cleanly complete.
> The plan is that the persistent contexts would still get the default 
> grace period (pre-emption timeout at least) to finish but Ctrl+C will 
> kill it within a timely manner if it does not finish.

Yes my patch does that. ;) Currently non-persistent is killed to quickly 
triggering pointless and alarming engine resets. Users reported this 
last year and I tried to fix it.

>>> Persistence itself can stay. There are valid UMD use cases. It is 
>>> just massively over complicated and doesn't work in all corner cases 
>>> when not using execlist submission or on newer platforms. The 
>>> simplification that is planned is to allow contexts to persist until 
>>> the associated DRM master handle is closed. At that point, all 
>>> contexts associated with that DRM handle are killed. That is what AMD 
>>> and others apparently implement.
>>
>> Okay, that goes against one recent IGT patch which appeared to work 
>> around something by moving the position of _context_ close.
> No it does not. The context close is not the trigger. The trigger is 

Well patch says:
"""
The spin all test relied on context persistence unecessarily by trying
to destroy contexts while keeping spinners active.
The current implementation of context persistence in i915 can cause
failures with GuC enabled, and persistence is not needed for this test.

Moving intel_ctx_destroy after igt_spin_end.
"""

Implying moving context close to after spin end fixes things for GuC, 
not fd close.

Regards,

Tvrtko

> closing the top level DRM handle. If your context has persistence 
> enabled (the default) then closing the context handle will have no 
> effect. No pulse, no pre-emption, no kill, nothing. But when the top 
> level handle is closed (application exit through whatever mechanism) 
> then all GPU resources will be cleaned up. As above, with at least a 
> pre-emption timeout grace period, but after that it is termination time.
> 
> The media use cases for persistence are all happy with this scheme. I 
> don't actually recall if we got a reply back from the OGL people. They 
> were definitely on the email thread/Jira task and did not complain. OCL 
> obviously don't care as their first action is to explicitly disable 
> persistence.
> 
> John.
> 
>>
>> Regards,
>>
>> Tvrtko
>

John Harrison Feb. 25, 2022, 6:48 p.m. UTC | #8

On 2/25/2022 10:14, Tvrtko Ursulin wrote:
>
> I'll try to simplify the discussion here:
>
> On 24/02/2022 19:45, John Harrison wrote:
>> On 2/24/2022 03:41, Tvrtko Ursulin wrote:
>>> On 23/02/2022 20:00, John Harrison wrote:
>>>> On 2/23/2022 05:58, Tvrtko Ursulin wrote:
>>>>> On 23/02/2022 02:45, John Harrison wrote:
>>>>>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>>>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>>>>> From: John Harrison <[e-mail]>
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> Magnititude! O;)
>>>> Doh!
>>>>
>>>> [snip]
>>>>
>>>>>> 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.
>>>>>
>>>>> Not sure how did you get one minute?
>>>> 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 
>>>> 75 => 1 minute 15 seconds
>>>>
>>>> Even ignoring any safety factor and just going with 7.5 * 5 still 
>>>> gets you to 37.5 seconds which is over a half a minute and likely 
>>>> to race.
>>>
>>> Ah because my starting point is there should be no preempt timeout = 
>>> heartbeat * 3, I just think that's too ugly.
>> Then complain at the hardware designers to give us mid-thread 
>> pre-emption back. The heartbeat is only one source of pre-emption 
>> events. For example, a user can be running multiple contexts in 
>> parallel and expecting them to time slice on a single engine. Or 
>> maybe a user is just running one compute task in the background but 
>> is doing render work in the foreground. Etc.
>>
>> There was a reason the original hack was to disable pre-emption 
>> rather than increase the heartbeat. This is simply a slightly less 
>> ugly version of the same hack. And unfortunately, the basic idea of 
>> the hack is non-negotiable.
>>
>> As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) * 
>> 12' or 'tP(RCS) = 7500' are the available options. Given that the 
>> heartbeat is the ever present hard limit, it seems most plausible to 
>> base the hack on that. Any of the others works, though. Although I 
>> think a explicit hardcoded value is the most ugly. I guess the other 
>> option is to add CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default 
>> that to 7500.
>>
>> Take your pick. But 640ms is not allowed.
>>
>>>
>>>>> Regardless, crux of argument was to avoid GuC engine reset and 
>>>>> heartbeat reset racing with each other, and to do that by 
>>>>> considering the preempt timeout with the heartbeat interval. I was 
>>>>> thinking about this scenario in this series:
>>>>>
>>>>> [Please use fixed width font and no line wrap to view.]
>>>>>
>>>>> A)
>>>>>
>>>>> tP = preempt timeout
>>>>> tH = hearbeat interval
>>>>>
>>>>> tP = 3 * tH
>>>>>
>>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>>
>>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- 
>>>>> [2 * tH] ----> FULL RESET
>>>>>                                                        |
>>>>>                                                        \- 
>>>>> preemption triggered, tP = 3 * tH ------\
>>>>> \-> preempt timeout would hit here
>>>>>
>>>>> Here we have collateral damage due full reset, since we can't tell 
>>>>> GuC to reset just one engine and we fudged tP just to "account" 
>>>>> for heartbeats.
>>>> You are missing the whole point of the patch series which is that 
>>>> the last heartbeat period is '2 * tP' not '2 * tH'.
>>>> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
>>>>
>>>> By making the last period double the pre-emption timeout, it is 
>>>> guaranteed that the FULL RESET stage cannot be hit before the 
>>>> hardware has attempted and timed-out on at least one pre-emption.
>>>
>>> Oh well :) that probably means the overall scheme is too odd for me. 
>>> tp = 3tH and last pulse after 2tP I mean.
>> To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = 
>> tP(current) * 2;'. Seems fairly straight forward to me. It's not a 
>> recursive definition or anything like that. It gives us a total 
>> heartbeat timeout that is close to the original version but still 
>> allows at least one pre-emption event.
>>
>>>
>>>> [snip]
>>>>
>>>>
>>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- 
>>>>> [2 * tH] ----> full reset would be here
>>>>>                |
>>>>>                \- preemption triggered, tP = 3 * tH ----------------\
>>>>> \-> Preempt timeout reset
>>>>>
>>>>> Here is is kind of least worse, but question is why we fudged tP 
>>>>> when it gives us nothing good in this case.
>>>>>
>>>> The point of fudging tP(RCS) is to give compute workloads longer to 
>>>> reach a pre-emptible point (given that EU walkers are basically not 
>>>> pre-emptible). The reason for doing the fudge is not connected to 
>>>> the heartbeat at all. The fact that it causes problems for the 
>>>> heartbeat is an undesired side effect.
>>>>
>>>> Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
>>>> calculation that gave us something that all interested parties were 
>>>> vaguely happy with. It could just as easily be a fixed, hard coded 
>>>> value of 7.5s but having it based on something configurable seemed 
>>>> more sensible. The other option was 'tP(RCS) = tP * 12' but that 
>>>> felt more arbitrary than basing it on the average heartbeat 
>>>> timeout. As in, three heartbeat periods is about what a normal prio 
>>>> task gets before it gets pre-empted by the heartbeat. So using that 
>>>> for general purpose pre-emptions (e.g. time slicing between 
>>>> multiple user apps) seems reasonable.
>>>
>>> I think the fact you say tP fudge is not related to heartbeats and 
>>> then go to mention heartbeat even in the "formula" which uses no tH 
>>> is saying something (at least that's how I read the 7.5s option). :)
>> I said the tP fudge is not because of the heartbeat. It is obviously 
>> related.
>>
>> As per comment above, the fudge factor is based on the heartbeat 
>> because the heartbeat is the ultimate limit. But the *reason* for the 
>> fudge fact has nothing to do with the heartbeat. It is required even 
>> if heartbeats are disabled.
>>
>>>
>>>>> B)
>>>>>
>>>>> Instead, my idea to account for preempt timeout when calculating 
>>>>> when to schedule next hearbeat would look like this:
>>>>>
>>>>> First of all tP can be left at a large value unrelated to tH. Lets 
>>>>> say tP = 640ms. tH stays 2.5s.
>>>> 640ms is not 'large'. The requirement is either zero (disabled) or 
>>>> region of 7.5s. The 640ms figure is the default for non-compute 
>>>> engines. Anything that can run EUs needs to be 'huge'.
>>>>
>>>>
>>>>>
>>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>>
>>>>> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] 
>>>>> --> Pulse3 <-- [tH + tP] --> full reset would be here
>>>> Sure, this works but each period is now 2.5 + 7.5 = 10s. The full 
>>>> five periods is therefore 50s, which is practically a minute.
>>>
>>> No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.
>> Not good enough. After 2.5s, we send a pulse. After a further 640ms 
>> we perform an engine reset. That means your compute workload had only 
>> 640ms after being told to pre-empt to reach a pre-emption point. That 
>> won't work. It needs to be multiple seconds.
>>
>>>
>>>> [snip]
>>>>
>>>>> Am I missing some requirement or you see another problem with this 
>>>>> idea?
>>>>>
>>>>>>> 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 
>>>>>
>>>>> By stops working I meant that it stops working. :)
>>>>>
>>>>> With execlist one can disable preempt timeout and "stopped 
>>>>> heartbeat" can still reset the stuck engine and so avoid 
>>>>> collateral damage. With GuC it appears this is not possible. So I 
>>>>> was thinking this is something worthy a log notice.
>>>>>
>>>>>> 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 
>>>>>
>>>>> I don't think so. Preempt timeout is disabled already on TGL/RCS 
>>>>> upstream but hearbeat is not and so hangcheck still works.
>>>> The pre-emption disable in upstream is not a valid solution for 
>>>> compute customers. It is a worst-of-all-worlds hack for general 
>>>> usage. As noted already, any actual compute specific customer is 
>>>> advised to disable all forms of reset and do their hang detection 
>>>> manually. A slightly less worse hack for customers that are not 
>>>> actually running long compute workloads (i.e. the vast majority of 
>>>> end users) is to just use a long pre-emption timeout.
>>>
>>> If disabled preemption timeout is worst of all words and compute 
>>> needs to disable heartbeat as well then why did we put it in? 
>>> Perhaps it was not know at the time it would not be good enough. But 
>>> anyway, do I read you correct that current thinking is it would be 
>>> better to leave it at default 640ms?
>> No. We cannot have the RCS default to 640ms.
>>
>> Note that there is a difference between 'general end user who might 
>> run some compute' and 'compute focused customer'. The driver defaults 
>> (disabled or 7500ms) are for the general user who gets the 
>> out-of-the-box experience and expects to be able to run 'normal' 
>> workloads without hitting problems. I.e. they expect hung tasks to 
>> get reset in a timely manner and while they might run some AI or 
>> other compute workloads, they are not a HPC datacenter. Whereas the 
>> compute datacenter customer expects their workloads to run for 
>> arbitrarily long times (minutes, hours, maybe even days) without 
>> being arbitrarily killed. Those customers will be explicitly 
>> configuring their datacenter server for that scenario and thus don't 
>> care what the defaults are.
>
> Okay maybe I misunderstood what you were saying earlier about worst of 
> all worlds and all. But tell me this, if preemption timeout on RCS is 
> not directly related to hearbeats, but to some pessimistic expected 
> user workloads, what is wrong with my scheme of calculating the next 
> heartbeat pulse as tH + tP?
>
> We can leave tH as default 2.5s and tP you set for RCS to 12s if that 
> is what you say is required. Or whatever long value really.
>
> Your only objection is that ends up with too long total time before 
> reset? Or something else as well?
An unnecessarily long total heartbeat timeout is the main objection. 
(2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the current 
12.5s.

If we are happy with that huge increase then fine. But I'm pretty sure 
you are going to get a lot more bug reports about hung systems not 
recovering. 10-20s is just about long enough for someone to wait before 
leaning on the power button of their machine. Over a minute is not. That 
kind of delay is going to cause support issues.

>
> It's long but it is correct in a way. Because we can't expect hearbeat 
> to react quicker than the interval + preempt timeout (or timeslice for 
> equal priority) + some scheduling latency.
>
> I conceptually disagree with the last hearbeat pulse being special. If 
> the user concept is "after N heartbeats you are out" and you want to 
> make it "after N-1 heartbeats plus 2 preemption periods you are out", 
> where preemption period actually depends on heartbeat period, then 
> that sounds really convoluted to me.
>
> And we don't know which of the pulses will trigger preemption since 
> user priority we don't control. So low priority compute task gets 
> reset after 5s, normal priority gets to run for 12s. Effectively 
> making context priority a variable in hangcheck.
It already is. That is no different. The pre-emption is not triggered 
until the pulse is of equal or higher priority than the busy task. That 
is the case no matter whether you are running GuC or execlist, whether 
you have the original driver or an updated one.

And it doesn't matter which pulse triggers the pre-emption. All that 
matters is that once a pre-emption is attempted, if the busy context 
fails to relinquish the hardware within the pre-emption timeout limit 
then it will be forcibly evicted.


>
>>>
>>> If so, if we went with my proposal, would everyone be happy? If yes, 
>>> isn't it a simpler scheme? No special casing when setting the 
>>> preempt timeout, no special casing of the last heartbeat pulse. 
>>> Works predictably regardless of the priority of the hypothetical 
>>> non-preemptible workload.
>> No, we have to have the increased pre-emption timeout. And that has 
>> ripple effects of making very long heartbeats or risking races with 
>> the heartbeat beating the per engine reset.
>>
>>
>>>
>>>>>> 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).
>>>>>
>>>>> What is broken with context persistence? I noticed one patch 
>>>>> claiming to be fixing something in that area which looked suspect. 
>>>>> Has it been established no userspace relies on it?
>>>> One major issue is that it has hooks into the execlist scheduler 
>>>> backend. I forget the exact details right now. The implementation 
>>>> as a whole is incredibly complex and convoluted :(. But there's 
>>>> stuff about what happens when you disable the heartbeat after 
>>>> having closed a persistence context's handle (and thus made it 
>>>> persisting). There's also things like it sends a super high 
>>>> priority heartbeat pulse at the point of becoming persisting. That 
>>>> plays havoc for platforms with dependent engines and/or compute 
>>>> workloads. A context becomes persisting on RCS and results in your 
>>>> unrealted CCS work being reset. It's a mess.
>>>>
>>>> The comment from Daniel Vetter is that persistence should have no 
>>>> connection to the heartbeat at all. All of that dynamic behaviour 
>>>> and complexity should just be removed.
>>>
>>> Dependent engines is definitely a topic on it's own, outside 
>>> hearbeats, persistence and all.
>> Except that it has implications for persistence which the current 
>> driver does not take into account.
>
> Well current driver does not take RCS+CCS dependency into account so 
> that should come first, or all in one package at least.
Not sure what you are arguing for here? Simplifying persistence as 
described will fix all the problems in one easy go. There is no point in 
adding yet more hideously complex code to make one corner case work when 
the real fix is to rip it all out.

>
>>> Otherwise there is definitely complexity in the execlists backend 
>>> but I am not sure if logic persistence and heartbeats are so very 
>>> connected. It does send a pulse when heartbeat interval is changed, 
>>> which if going to zero, it will kick of closed contexts if it can:
>>>
>>> static struct intel_engine_cs *
>>> __execlists_schedule_in(struct i915_request *rq)
>>> {
>>>     struct intel_engine_cs * const engine = rq->engine;
>>>     struct intel_context * const ce = rq->context;
>>>
>>>     intel_context_get(ce);
>>>
>>>     if (unlikely(intel_context_is_closed(ce) &&
>>>              !intel_engine_has_heartbeat(engine)))
>>>         intel_context_set_banned(ce);
>>>
>>>     if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>>>         reset_active(rq, engine);
>>>
>>> Is this what you mean? The point of this is to make sure persistent 
>>> context does not hog the engine forever if hangcheck has been disabled.
>>>
>>> Reminds me of my improvement to customer experience which never got 
>>> in 
>>> (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). 
>>> Point of that one was to avoid engine reset (or worse) after user 
>>> presses "Ctrl-C" if something takes just over 1ms to cleanly complete.
>> The plan is that the persistent contexts would still get the default 
>> grace period (pre-emption timeout at least) to finish but Ctrl+C will 
>> kill it within a timely manner if it does not finish.
>
> Yes my patch does that. ;) Currently non-persistent is killed to 
> quickly triggering pointless and alarming engine resets. Users 
> reported this last year and I tried to fix it.
Except that your patch is adding yet more complexity to an already 
complex feature. The desire is to simplify the driver and make it more 
maintainable not complicated it further.

>
>>>> Persistence itself can stay. There are valid UMD use cases. It is 
>>>> just massively over complicated and doesn't work in all corner 
>>>> cases when not using execlist submission or on newer platforms. The 
>>>> simplification that is planned is to allow contexts to persist 
>>>> until the associated DRM master handle is closed. At that point, 
>>>> all contexts associated with that DRM handle are killed. That is 
>>>> what AMD and others apparently implement.
>>>
>>> Okay, that goes against one recent IGT patch which appeared to work 
>>> around something by moving the position of _context_ close.
>> No it does not. The context close is not the trigger. The trigger is 
>
> Well patch says:
> """
> The spin all test relied on context persistence unecessarily by trying
> to destroy contexts while keeping spinners active.
> The current implementation of context persistence in i915 can cause
> failures with GuC enabled, and persistence is not needed for this test.
>
> Moving intel_ctx_destroy after igt_spin_end.
> """
>
> Implying moving context close to after spin end fixes things for GuC, 
> not fd close.
That's because persistence is currently a big pile of poo and does not 
work in all the corner cases. The correct solution is to leave the IGT 
alone and just fix the implementation of persistence. However, the IGT 
update to not use the broken feature is a trivial test change (two 
lines?) whereas fixing the broken feature is a significant KMD re-work. 
It needs to be done but no-one currently has the time to do it. But 
trivially changing the test allows the test to work and test the 
features it is meant to be testing (which is not persistence).

John.

>
> Regards,
>
> Tvrtko
>
>> closing the top level DRM handle. If your context has persistence 
>> enabled (the default) then closing the context handle will have no 
>> effect. No pulse, no pre-emption, no kill, nothing. But when the top 
>> level handle is closed (application exit through whatever mechanism) 
>> then all GPU resources will be cleaned up. As above, with at least a 
>> pre-emption timeout grace period, but after that it is termination time.
>>
>> The media use cases for persistence are all happy with this scheme. I 
>> don't actually recall if we got a reply back from the OGL people. 
>> They were definitely on the email thread/Jira task and did not 
>> complain. OCL obviously don't care as their first action is to 
>> explicitly disable persistence.
>>
>> John.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>

Tvrtko Ursulin Feb. 28, 2022, 5:12 p.m. UTC | #9

On 25/02/2022 18:48, John Harrison wrote:
> On 2/25/2022 10:14, Tvrtko Ursulin wrote:
>>
>> I'll try to simplify the discussion here:
>>
>> On 24/02/2022 19:45, John Harrison wrote:
>>> On 2/24/2022 03:41, Tvrtko Ursulin wrote:
>>>> On 23/02/2022 20:00, John Harrison wrote:
>>>>> On 2/23/2022 05:58, Tvrtko Ursulin wrote:
>>>>>> On 23/02/2022 02:45, John Harrison wrote:
>>>>>>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>>>>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>>>>>> From: John Harrison <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> Magnititude! O;)
>>>>> Doh!
>>>>>
>>>>> [snip]
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Not sure how did you get one minute?
>>>>> 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 
>>>>> 75 => 1 minute 15 seconds
>>>>>
>>>>> Even ignoring any safety factor and just going with 7.5 * 5 still 
>>>>> gets you to 37.5 seconds which is over a half a minute and likely 
>>>>> to race.
>>>>
>>>> Ah because my starting point is there should be no preempt timeout = 
>>>> heartbeat * 3, I just think that's too ugly.
>>> Then complain at the hardware designers to give us mid-thread 
>>> pre-emption back. The heartbeat is only one source of pre-emption 
>>> events. For example, a user can be running multiple contexts in 
>>> parallel and expecting them to time slice on a single engine. Or 
>>> maybe a user is just running one compute task in the background but 
>>> is doing render work in the foreground. Etc.
>>>
>>> There was a reason the original hack was to disable pre-emption 
>>> rather than increase the heartbeat. This is simply a slightly less 
>>> ugly version of the same hack. And unfortunately, the basic idea of 
>>> the hack is non-negotiable.
>>>
>>> As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) * 
>>> 12' or 'tP(RCS) = 7500' are the available options. Given that the 
>>> heartbeat is the ever present hard limit, it seems most plausible to 
>>> base the hack on that. Any of the others works, though. Although I 
>>> think a explicit hardcoded value is the most ugly. I guess the other 
>>> option is to add CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and default 
>>> that to 7500.
>>>
>>> Take your pick. But 640ms is not allowed.
>>>
>>>>
>>>>>> Regardless, crux of argument was to avoid GuC engine reset and 
>>>>>> heartbeat reset racing with each other, and to do that by 
>>>>>> considering the preempt timeout with the heartbeat interval. I was 
>>>>>> thinking about this scenario in this series:
>>>>>>
>>>>>> [Please use fixed width font and no line wrap to view.]
>>>>>>
>>>>>> A)
>>>>>>
>>>>>> tP = preempt timeout
>>>>>> tH = hearbeat interval
>>>>>>
>>>>>> tP = 3 * tH
>>>>>>
>>>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>>>
>>>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- 
>>>>>> [2 * tH] ----> FULL RESET
>>>>>>                                                        |
>>>>>>                                                        \- 
>>>>>> preemption triggered, tP = 3 * tH ------\
>>>>>> \-> preempt timeout would hit here
>>>>>>
>>>>>> Here we have collateral damage due full reset, since we can't tell 
>>>>>> GuC to reset just one engine and we fudged tP just to "account" 
>>>>>> for heartbeats.
>>>>> You are missing the whole point of the patch series which is that 
>>>>> the last heartbeat period is '2 * tP' not '2 * tH'.
>>>>> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
>>>>>
>>>>> By making the last period double the pre-emption timeout, it is 
>>>>> guaranteed that the FULL RESET stage cannot be hit before the 
>>>>> hardware has attempted and timed-out on at least one pre-emption.
>>>>
>>>> Oh well :) that probably means the overall scheme is too odd for me. 
>>>> tp = 3tH and last pulse after 2tP I mean.
>>> To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = 
>>> tP(current) * 2;'. Seems fairly straight forward to me. It's not a 
>>> recursive definition or anything like that. It gives us a total 
>>> heartbeat timeout that is close to the original version but still 
>>> allows at least one pre-emption event.
>>>
>>>>
>>>>> [snip]
>>>>>
>>>>>
>>>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 <---- 
>>>>>> [2 * tH] ----> full reset would be here
>>>>>>                |
>>>>>>                \- preemption triggered, tP = 3 * tH ----------------\
>>>>>> \-> Preempt timeout reset
>>>>>>
>>>>>> Here is is kind of least worse, but question is why we fudged tP 
>>>>>> when it gives us nothing good in this case.
>>>>>>
>>>>> The point of fudging tP(RCS) is to give compute workloads longer to 
>>>>> reach a pre-emptible point (given that EU walkers are basically not 
>>>>> pre-emptible). The reason for doing the fudge is not connected to 
>>>>> the heartbeat at all. The fact that it causes problems for the 
>>>>> heartbeat is an undesired side effect.
>>>>>
>>>>> Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
>>>>> calculation that gave us something that all interested parties were 
>>>>> vaguely happy with. It could just as easily be a fixed, hard coded 
>>>>> value of 7.5s but having it based on something configurable seemed 
>>>>> more sensible. The other option was 'tP(RCS) = tP * 12' but that 
>>>>> felt more arbitrary than basing it on the average heartbeat 
>>>>> timeout. As in, three heartbeat periods is about what a normal prio 
>>>>> task gets before it gets pre-empted by the heartbeat. So using that 
>>>>> for general purpose pre-emptions (e.g. time slicing between 
>>>>> multiple user apps) seems reasonable.
>>>>
>>>> I think the fact you say tP fudge is not related to heartbeats and 
>>>> then go to mention heartbeat even in the "formula" which uses no tH 
>>>> is saying something (at least that's how I read the 7.5s option). :)
>>> I said the tP fudge is not because of the heartbeat. It is obviously 
>>> related.
>>>
>>> As per comment above, the fudge factor is based on the heartbeat 
>>> because the heartbeat is the ultimate limit. But the *reason* for the 
>>> fudge fact has nothing to do with the heartbeat. It is required even 
>>> if heartbeats are disabled.
>>>
>>>>
>>>>>> B)
>>>>>>
>>>>>> Instead, my idea to account for preempt timeout when calculating 
>>>>>> when to schedule next hearbeat would look like this:
>>>>>>
>>>>>> First of all tP can be left at a large value unrelated to tH. Lets 
>>>>>> say tP = 640ms. tH stays 2.5s.
>>>>> 640ms is not 'large'. The requirement is either zero (disabled) or 
>>>>> region of 7.5s. The 640ms figure is the default for non-compute 
>>>>> engines. Anything that can run EUs needs to be 'huge'.
>>>>>
>>>>>
>>>>>>
>>>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>>>
>>>>>> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] 
>>>>>> --> Pulse3 <-- [tH + tP] --> full reset would be here
>>>>> Sure, this works but each period is now 2.5 + 7.5 = 10s. The full 
>>>>> five periods is therefore 50s, which is practically a minute.
>>>>
>>>> No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.
>>> Not good enough. After 2.5s, we send a pulse. After a further 640ms 
>>> we perform an engine reset. That means your compute workload had only 
>>> 640ms after being told to pre-empt to reach a pre-emption point. That 
>>> won't work. It needs to be multiple seconds.
>>>
>>>>
>>>>> [snip]
>>>>>
>>>>>> Am I missing some requirement or you see another problem with this 
>>>>>> idea?
>>>>>>
>>>>>>>> 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 
>>>>>>
>>>>>> By stops working I meant that it stops working. :)
>>>>>>
>>>>>> With execlist one can disable preempt timeout and "stopped 
>>>>>> heartbeat" can still reset the stuck engine and so avoid 
>>>>>> collateral damage. With GuC it appears this is not possible. So I 
>>>>>> was thinking this is something worthy a log notice.
>>>>>>
>>>>>>> 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 
>>>>>>
>>>>>> I don't think so. Preempt timeout is disabled already on TGL/RCS 
>>>>>> upstream but hearbeat is not and so hangcheck still works.
>>>>> The pre-emption disable in upstream is not a valid solution for 
>>>>> compute customers. It is a worst-of-all-worlds hack for general 
>>>>> usage. As noted already, any actual compute specific customer is 
>>>>> advised to disable all forms of reset and do their hang detection 
>>>>> manually. A slightly less worse hack for customers that are not 
>>>>> actually running long compute workloads (i.e. the vast majority of 
>>>>> end users) is to just use a long pre-emption timeout.
>>>>
>>>> If disabled preemption timeout is worst of all words and compute 
>>>> needs to disable heartbeat as well then why did we put it in? 
>>>> Perhaps it was not know at the time it would not be good enough. But 
>>>> anyway, do I read you correct that current thinking is it would be 
>>>> better to leave it at default 640ms?
>>> No. We cannot have the RCS default to 640ms.
>>>
>>> Note that there is a difference between 'general end user who might 
>>> run some compute' and 'compute focused customer'. The driver defaults 
>>> (disabled or 7500ms) are for the general user who gets the 
>>> out-of-the-box experience and expects to be able to run 'normal' 
>>> workloads without hitting problems. I.e. they expect hung tasks to 
>>> get reset in a timely manner and while they might run some AI or 
>>> other compute workloads, they are not a HPC datacenter. Whereas the 
>>> compute datacenter customer expects their workloads to run for 
>>> arbitrarily long times (minutes, hours, maybe even days) without 
>>> being arbitrarily killed. Those customers will be explicitly 
>>> configuring their datacenter server for that scenario and thus don't 
>>> care what the defaults are.
>>
>> Okay maybe I misunderstood what you were saying earlier about worst of 
>> all worlds and all. But tell me this, if preemption timeout on RCS is 
>> not directly related to hearbeats, but to some pessimistic expected 
>> user workloads, what is wrong with my scheme of calculating the next 
>> heartbeat pulse as tH + tP?
>>
>> We can leave tH as default 2.5s and tP you set for RCS to 12s if that 
>> is what you say is required. Or whatever long value really.
>>
>> Your only objection is that ends up with too long total time before 
>> reset? Or something else as well?
> An unnecessarily long total heartbeat timeout is the main objection. 
> (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the current 
> 12.5s.
> 
> If we are happy with that huge increase then fine. But I'm pretty sure 
> you are going to get a lot more bug reports about hung systems not 
> recovering. 10-20s is just about long enough for someone to wait before 
> leaning on the power button of their machine. Over a minute is not. That 
> kind of delay is going to cause support issues.

Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen 
just so it is longer than tH * 3?

And how do you keep coming up with factor of five? Isn't it four periods 
before "heartbeat stopped"? (Prio normal, hearbeat, barrier and then reset.)

 From the point of view of user experience I agree reasonable 
responsiveness is needed before user "reaches for the power button".

In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.

Question of workloads.. what is the actual preempt timeout compute is 
happy with? And I don't mean compute setups with disabled hangcheck, 
which you say they want anyway, but if we target defaults for end users. 
Do we have some numbers on what they are likely to run?

What if we gave them a default of 2.5s? That would be 4 * (2.5s + 2.5s) 
= 20s worst case until reset, comparable to your proposal. Are there 
realistic workloads which are non-preemptable for 2.5s? I am having hard 
time imagining someone would run them on a general purpose desktop since 
it would mean frozen and unusable UI anyway.

Advantage still being in my mind that there would be no fudging of 
timeouts during driver load and heartbeat periods depending on priority. 
To me it feels more plausible to account for preempt timeout in 
heartbeat pulses that to calculate preempt timeout to be longer than 
hearbeat pulses, just to avoid races between the two.

>> It's long but it is correct in a way. Because we can't expect hearbeat 
>> to react quicker than the interval + preempt timeout (or timeslice for 
>> equal priority) + some scheduling latency.
>>
>> I conceptually disagree with the last hearbeat pulse being special. If 
>> the user concept is "after N heartbeats you are out" and you want to 
>> make it "after N-1 heartbeats plus 2 preemption periods you are out", 
>> where preemption period actually depends on heartbeat period, then 
>> that sounds really convoluted to me.
>>
>> And we don't know which of the pulses will trigger preemption since 
>> user priority we don't control. So low priority compute task gets 
>> reset after 5s, normal priority gets to run for 12s. Effectively 
>> making context priority a variable in hangcheck.
> It already is. That is no different. The pre-emption is not triggered 
> until the pulse is of equal or higher priority than the busy task. That 
> is the case no matter whether you are running GuC or execlist, whether 
> you have the original driver or an updated one.
> 
> And it doesn't matter which pulse triggers the pre-emption. All that 
> matters is that once a pre-emption is attempted, if the busy context 
> fails to relinquish the hardware within the pre-emption timeout limit 
> then it will be forcibly evicted.
> 
> 
>>
>>>>
>>>> If so, if we went with my proposal, would everyone be happy? If yes, 
>>>> isn't it a simpler scheme? No special casing when setting the 
>>>> preempt timeout, no special casing of the last heartbeat pulse. 
>>>> Works predictably regardless of the priority of the hypothetical 
>>>> non-preemptible workload.
>>> No, we have to have the increased pre-emption timeout. And that has 
>>> ripple effects of making very long heartbeats or risking races with 
>>> the heartbeat beating the per engine reset.
>>>
>>>
>>>>
>>>>>>> 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).
>>>>>>
>>>>>> What is broken with context persistence? I noticed one patch 
>>>>>> claiming to be fixing something in that area which looked suspect. 
>>>>>> Has it been established no userspace relies on it?
>>>>> One major issue is that it has hooks into the execlist scheduler 
>>>>> backend. I forget the exact details right now. The implementation 
>>>>> as a whole is incredibly complex and convoluted :(. But there's 
>>>>> stuff about what happens when you disable the heartbeat after 
>>>>> having closed a persistence context's handle (and thus made it 
>>>>> persisting). There's also things like it sends a super high 
>>>>> priority heartbeat pulse at the point of becoming persisting. That 
>>>>> plays havoc for platforms with dependent engines and/or compute 
>>>>> workloads. A context becomes persisting on RCS and results in your 
>>>>> unrealted CCS work being reset. It's a mess.
>>>>>
>>>>> The comment from Daniel Vetter is that persistence should have no 
>>>>> connection to the heartbeat at all. All of that dynamic behaviour 
>>>>> and complexity should just be removed.
>>>>
>>>> Dependent engines is definitely a topic on it's own, outside 
>>>> hearbeats, persistence and all.
>>> Except that it has implications for persistence which the current 
>>> driver does not take into account.
>>
>> Well current driver does not take RCS+CCS dependency into account so 
>> that should come first, or all in one package at least.
> Not sure what you are arguing for here? Simplifying persistence as 
> described will fix all the problems in one easy go. There is no point in 
> adding yet more hideously complex code to make one corner case work when 
> the real fix is to rip it all out.

It sounded like you were arguing there was an issue with context 
persistence and dependent engines so my reply was that if there are no 
dependent engines in upstream there is no issue with persistence. Once 
dependent engines are brought upstream then any reset caused by 
persistence can be looked at and determined if there is any conceptual 
difference between that and any random reset.

>>>> Otherwise there is definitely complexity in the execlists backend 
>>>> but I am not sure if logic persistence and heartbeats are so very 
>>>> connected. It does send a pulse when heartbeat interval is changed, 
>>>> which if going to zero, it will kick of closed contexts if it can:
>>>>
>>>> static struct intel_engine_cs *
>>>> __execlists_schedule_in(struct i915_request *rq)
>>>> {
>>>>     struct intel_engine_cs * const engine = rq->engine;
>>>>     struct intel_context * const ce = rq->context;
>>>>
>>>>     intel_context_get(ce);
>>>>
>>>>     if (unlikely(intel_context_is_closed(ce) &&
>>>>              !intel_engine_has_heartbeat(engine)))
>>>>         intel_context_set_banned(ce);
>>>>
>>>>     if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>>>>         reset_active(rq, engine);
>>>>
>>>> Is this what you mean? The point of this is to make sure persistent 
>>>> context does not hog the engine forever if hangcheck has been disabled.
>>>>
>>>> Reminds me of my improvement to customer experience which never got 
>>>> in 
>>>> (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). Point 
>>>> of that one was to avoid engine reset (or worse) after user presses 
>>>> "Ctrl-C" if something takes just over 1ms to cleanly complete.
>>> The plan is that the persistent contexts would still get the default 
>>> grace period (pre-emption timeout at least) to finish but Ctrl+C will 
>>> kill it within a timely manner if it does not finish.
>>
>> Yes my patch does that. ;) Currently non-persistent is killed to 
>> quickly triggering pointless and alarming engine resets. Users 
>> reported this last year and I tried to fix it.
> Except that your patch is adding yet more complexity to an already 
> complex feature. The desire is to simplify the driver and make it more 
> maintainable not complicated it further.

It is fixing an user reported issue and is not really adding complexity 
apart from enabling us to distinguish the reason for revoking a request 
(ban vs soft close). I think we should care to fix user reported issues 
in a reasonable time frame. It's a patch from last summer or so..

>>>>> Persistence itself can stay. There are valid UMD use cases. It is 
>>>>> just massively over complicated and doesn't work in all corner 
>>>>> cases when not using execlist submission or on newer platforms. The 
>>>>> simplification that is planned is to allow contexts to persist 
>>>>> until the associated DRM master handle is closed. At that point, 
>>>>> all contexts associated with that DRM handle are killed. That is 
>>>>> what AMD and others apparently implement.
>>>>
>>>> Okay, that goes against one recent IGT patch which appeared to work 
>>>> around something by moving the position of _context_ close.
>>> No it does not. The context close is not the trigger. The trigger is 
>>
>> Well patch says:
>> """
>> The spin all test relied on context persistence unecessarily by trying
>> to destroy contexts while keeping spinners active.
>> The current implementation of context persistence in i915 can cause
>> failures with GuC enabled, and persistence is not needed for this test.
>>
>> Moving intel_ctx_destroy after igt_spin_end.
>> """
>>
>> Implying moving context close to after spin end fixes things for GuC, 
>> not fd close.
> That's because persistence is currently a big pile of poo and does not 
> work in all the corner cases. The correct solution is to leave the IGT 
> alone and just fix the implementation of persistence. However, the IGT 
> update to not use the broken feature is a trivial test change (two 
> lines?) whereas fixing the broken feature is a significant KMD re-work. 
> It needs to be done but no-one currently has the time to do it. But 
> trivially changing the test allows the test to work and test the 
> features it is meant to be testing (which is not persistence).

Clear as mud. If the statement is that persistence cannot simply be 
removed, then for sure it cannot be said that anything can be fixed or 
unblocked by allowing some test to pass with GuC, by making them avoid 
using persistence (and not even explicitly with a context param). It 
implies persistence does not work with the GuC, which is then in 
contradiction with the statement that we cannot just remove persistence. 
I truly have no idea what the argument is here.

Regards,

Tvrtko

John Harrison Feb. 28, 2022, 6:55 p.m. UTC | #10

On 2/28/2022 09:12, Tvrtko Ursulin wrote:
> On 25/02/2022 18:48, John Harrison wrote:
>> On 2/25/2022 10:14, Tvrtko Ursulin wrote:
>>>
>>> I'll try to simplify the discussion here:
>>>
>>> On 24/02/2022 19:45, John Harrison wrote:
>>>> On 2/24/2022 03:41, Tvrtko Ursulin wrote:
>>>>> On 23/02/2022 20:00, John Harrison wrote:
>>>>>> On 2/23/2022 05:58, Tvrtko Ursulin wrote:
>>>>>>> On 23/02/2022 02:45, John Harrison wrote:
>>>>>>>> On 2/22/2022 03:19, Tvrtko Ursulin wrote:
>>>>>>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>>>>>>> From: John Harrison <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> Magnititude! O;)
>>>>>> Doh!
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> Not sure how did you get one minute?
>>>>>> 7.5 * 2 (to be safe) = 15. 15 * 5 (number of heartbeat periods) = 
>>>>>> 75 => 1 minute 15 seconds
>>>>>>
>>>>>> Even ignoring any safety factor and just going with 7.5 * 5 still 
>>>>>> gets you to 37.5 seconds which is over a half a minute and likely 
>>>>>> to race.
>>>>>
>>>>> Ah because my starting point is there should be no preempt timeout 
>>>>> = heartbeat * 3, I just think that's too ugly.
>>>> Then complain at the hardware designers to give us mid-thread 
>>>> pre-emption back. The heartbeat is only one source of pre-emption 
>>>> events. For example, a user can be running multiple contexts in 
>>>> parallel and expecting them to time slice on a single engine. Or 
>>>> maybe a user is just running one compute task in the background but 
>>>> is doing render work in the foreground. Etc.
>>>>
>>>> There was a reason the original hack was to disable pre-emption 
>>>> rather than increase the heartbeat. This is simply a slightly less 
>>>> ugly version of the same hack. And unfortunately, the basic idea of 
>>>> the hack is non-negotiable.
>>>>
>>>> As per other comments, 'tP(RCS) = tH *3' or 'tP(RCS) = tP(default) 
>>>> * 12' or 'tP(RCS) = 7500' are the available options. Given that the 
>>>> heartbeat is the ever present hard limit, it seems most plausible 
>>>> to base the hack on that. Any of the others works, though. Although 
>>>> I think a explicit hardcoded value is the most ugly. I guess the 
>>>> other option is to add CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE and 
>>>> default that to 7500.
>>>>
>>>> Take your pick. But 640ms is not allowed.
>>>>
>>>>>
>>>>>>> Regardless, crux of argument was to avoid GuC engine reset and 
>>>>>>> heartbeat reset racing with each other, and to do that by 
>>>>>>> considering the preempt timeout with the heartbeat interval. I 
>>>>>>> was thinking about this scenario in this series:
>>>>>>>
>>>>>>> [Please use fixed width font and no line wrap to view.]
>>>>>>>
>>>>>>> A)
>>>>>>>
>>>>>>> tP = preempt timeout
>>>>>>> tH = hearbeat interval
>>>>>>>
>>>>>>> tP = 3 * tH
>>>>>>>
>>>>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>>>>
>>>>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 
>>>>>>> <---- [2 * tH] ----> FULL RESET
>>>>>>> |
>>>>>>> \- preemption triggered, tP = 3 * tH ------\
>>>>>>> \-> preempt timeout would hit here
>>>>>>>
>>>>>>> Here we have collateral damage due full reset, since we can't 
>>>>>>> tell GuC to reset just one engine and we fudged tP just to 
>>>>>>> "account" for heartbeats.
>>>>>> You are missing the whole point of the patch series which is that 
>>>>>> the last heartbeat period is '2 * tP' not '2 * tH'.
>>>>>> +        longer = READ_ONCE(engine->props.preempt_timeout_ms) * 2;
>>>>>>
>>>>>> By making the last period double the pre-emption timeout, it is 
>>>>>> guaranteed that the FULL RESET stage cannot be hit before the 
>>>>>> hardware has attempted and timed-out on at least one pre-emption.
>>>>>
>>>>> Oh well :) that probably means the overall scheme is too odd for 
>>>>> me. tp = 3tH and last pulse after 2tP I mean.
>>>> To be accurate, it is 'tP(RCS) = 3 * tH(default); tH(final) = 
>>>> tP(current) * 2;'. Seems fairly straight forward to me. It's not a 
>>>> recursive definition or anything like that. It gives us a total 
>>>> heartbeat timeout that is close to the original version but still 
>>>> allows at least one pre-emption event.
>>>>
>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>
>>>>>>> <-- [tH] --> Pulse1 <-- [tH] --> Pulse2 <-- [tH] --> Pulse3 
>>>>>>> <---- [2 * tH] ----> full reset would be here
>>>>>>>                |
>>>>>>>                \- preemption triggered, tP = 3 * tH 
>>>>>>> ----------------\
>>>>>>> \-> Preempt timeout reset
>>>>>>>
>>>>>>> Here is is kind of least worse, but question is why we fudged tP 
>>>>>>> when it gives us nothing good in this case.
>>>>>>>
>>>>>> The point of fudging tP(RCS) is to give compute workloads longer 
>>>>>> to reach a pre-emptible point (given that EU walkers are 
>>>>>> basically not pre-emptible). The reason for doing the fudge is 
>>>>>> not connected to the heartbeat at all. The fact that it causes 
>>>>>> problems for the heartbeat is an undesired side effect.
>>>>>>
>>>>>> Note that the use of 'tP(RCS) = tH * 3' was just an arbitrary 
>>>>>> calculation that gave us something that all interested parties 
>>>>>> were vaguely happy with. It could just as easily be a fixed, hard 
>>>>>> coded value of 7.5s but having it based on something configurable 
>>>>>> seemed more sensible. The other option was 'tP(RCS) = tP * 12' 
>>>>>> but that felt more arbitrary than basing it on the average 
>>>>>> heartbeat timeout. As in, three heartbeat periods is about what a 
>>>>>> normal prio task gets before it gets pre-empted by the heartbeat. 
>>>>>> So using that for general purpose pre-emptions (e.g. time slicing 
>>>>>> between multiple user apps) seems reasonable.
>>>>>
>>>>> I think the fact you say tP fudge is not related to heartbeats and 
>>>>> then go to mention heartbeat even in the "formula" which uses no 
>>>>> tH is saying something (at least that's how I read the 7.5s 
>>>>> option). :)
>>>> I said the tP fudge is not because of the heartbeat. It is 
>>>> obviously related.
>>>>
>>>> As per comment above, the fudge factor is based on the heartbeat 
>>>> because the heartbeat is the ultimate limit. But the *reason* for 
>>>> the fudge fact has nothing to do with the heartbeat. It is required 
>>>> even if heartbeats are disabled.
>>>>
>>>>>
>>>>>>> B)
>>>>>>>
>>>>>>> Instead, my idea to account for preempt timeout when calculating 
>>>>>>> when to schedule next hearbeat would look like this:
>>>>>>>
>>>>>>> First of all tP can be left at a large value unrelated to tH. 
>>>>>>> Lets say tP = 640ms. tH stays 2.5s.
>>>>>> 640ms is not 'large'. The requirement is either zero (disabled) 
>>>>>> or region of 7.5s. The 640ms figure is the default for 
>>>>>> non-compute engines. Anything that can run EUs needs to be 'huge'.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> 1) Background load = I915_PRIORITY_DISPLAY
>>>>>>>
>>>>>>> <-- [tH + tP] --> Pulse1 <-- [tH + tP] --> Pulse2 <-- [tH + tP] 
>>>>>>> --> Pulse3 <-- [tH + tP] --> full reset would be here
>>>>>> Sure, this works but each period is now 2.5 + 7.5 = 10s. The full 
>>>>>> five periods is therefore 50s, which is practically a minute.
>>>>>
>>>>> No, in my proposal it is 3 * (2.5s + 640ms) =~ 9.3s.
>>>> Not good enough. After 2.5s, we send a pulse. After a further 640ms 
>>>> we perform an engine reset. That means your compute workload had 
>>>> only 640ms after being told to pre-empt to reach a pre-emption 
>>>> point. That won't work. It needs to be multiple seconds.
>>>>
>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> Am I missing some requirement or you see another problem with 
>>>>>>> this idea?
>>>>>>>
>>>>>>>>> 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 
>>>>>>>
>>>>>>> By stops working I meant that it stops working. :)
>>>>>>>
>>>>>>> With execlist one can disable preempt timeout and "stopped 
>>>>>>> heartbeat" can still reset the stuck engine and so avoid 
>>>>>>> collateral damage. With GuC it appears this is not possible. So 
>>>>>>> I was thinking this is something worthy a log notice.
>>>>>>>
>>>>>>>> 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 
>>>>>>>
>>>>>>> I don't think so. Preempt timeout is disabled already on TGL/RCS 
>>>>>>> upstream but hearbeat is not and so hangcheck still works.
>>>>>> The pre-emption disable in upstream is not a valid solution for 
>>>>>> compute customers. It is a worst-of-all-worlds hack for general 
>>>>>> usage. As noted already, any actual compute specific customer is 
>>>>>> advised to disable all forms of reset and do their hang detection 
>>>>>> manually. A slightly less worse hack for customers that are not 
>>>>>> actually running long compute workloads (i.e. the vast majority 
>>>>>> of end users) is to just use a long pre-emption timeout.
>>>>>
>>>>> If disabled preemption timeout is worst of all words and compute 
>>>>> needs to disable heartbeat as well then why did we put it in? 
>>>>> Perhaps it was not know at the time it would not be good enough. 
>>>>> But anyway, do I read you correct that current thinking is it 
>>>>> would be better to leave it at default 640ms?
>>>> No. We cannot have the RCS default to 640ms.
>>>>
>>>> Note that there is a difference between 'general end user who might 
>>>> run some compute' and 'compute focused customer'. The driver 
>>>> defaults (disabled or 7500ms) are for the general user who gets the 
>>>> out-of-the-box experience and expects to be able to run 'normal' 
>>>> workloads without hitting problems. I.e. they expect hung tasks to 
>>>> get reset in a timely manner and while they might run some AI or 
>>>> other compute workloads, they are not a HPC datacenter. Whereas the 
>>>> compute datacenter customer expects their workloads to run for 
>>>> arbitrarily long times (minutes, hours, maybe even days) without 
>>>> being arbitrarily killed. Those customers will be explicitly 
>>>> configuring their datacenter server for that scenario and thus 
>>>> don't care what the defaults are.
>>>
>>> Okay maybe I misunderstood what you were saying earlier about worst 
>>> of all worlds and all. But tell me this, if preemption timeout on 
>>> RCS is not directly related to hearbeats, but to some pessimistic 
>>> expected user workloads, what is wrong with my scheme of calculating 
>>> the next heartbeat pulse as tH + tP?
>>>
>>> We can leave tH as default 2.5s and tP you set for RCS to 12s if 
>>> that is what you say is required. Or whatever long value really.
>>>
>>> Your only objection is that ends up with too long total time before 
>>> reset? Or something else as well?
>> An unnecessarily long total heartbeat timeout is the main objection. 
>> (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the 
>> current 12.5s.
>>
>> If we are happy with that huge increase then fine. But I'm pretty 
>> sure you are going to get a lot more bug reports about hung systems 
>> not recovering. 10-20s is just about long enough for someone to wait 
>> before leaning on the power button of their machine. Over a minute is 
>> not. That kind of delay is going to cause support issues.
>
> Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen 
> just so it is longer than tH * 3?
>
> And how do you keep coming up with factor of five? Isn't it four 
> periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier 
> and then reset.)
Prio starts at low not normal.

>
> From the point of view of user experience I agree reasonable 
> responsiveness is needed before user "reaches for the power button".
>
> In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.
>
> Question of workloads.. what is the actual preempt timeout compute is 
> happy with? And I don't mean compute setups with disabled hangcheck, 
> which you say they want anyway, but if we target defaults for end 
> users. Do we have some numbers on what they are likely to run?
Not that I have ever seen. This is all just finger in the air stuff. I 
don't recall if we invented the number and the compute people agreed 
with it or if they proposed the number to us.

>
> What if we gave them a default of 2.5s? That would be 4 * (2.5s + 
> 2.5s) = 20s worst case until reset, comparable to your proposal. Are 
> there realistic workloads which are non-preemptable for 2.5s? I am 
> having hard time imagining someone would run them on a general purpose 
> desktop since it would mean frozen and unusable UI anyway.
>
> Advantage still being in my mind that there would be no fudging of 
> timeouts during driver load and heartbeat periods depending on 
> priority. To me it feels more plausible to account for preempt timeout 
> in heartbeat pulses that to calculate preempt timeout to be longer 
> than hearbeat pulses, just to avoid races between the two.
Except that when the user asks for a heartbeat period of 2.5s you are 
actually setting it to 5s. How is that not a major fudge that is totally 
disregarding the user's request?


>
>>> It's long but it is correct in a way. Because we can't expect 
>>> hearbeat to react quicker than the interval + preempt timeout (or 
>>> timeslice for equal priority) + some scheduling latency.
>>>
>>> I conceptually disagree with the last hearbeat pulse being special. 
>>> If the user concept is "after N heartbeats you are out" and you want 
>>> to make it "after N-1 heartbeats plus 2 preemption periods you are 
>>> out", where preemption period actually depends on heartbeat period, 
>>> then that sounds really convoluted to me.
>>>
>>> And we don't know which of the pulses will trigger preemption since 
>>> user priority we don't control. So low priority compute task gets 
>>> reset after 5s, normal priority gets to run for 12s. Effectively 
>>> making context priority a variable in hangcheck.
>> It already is. That is no different. The pre-emption is not triggered 
>> until the pulse is of equal or higher priority than the busy task. 
>> That is the case no matter whether you are running GuC or execlist, 
>> whether you have the original driver or an updated one.
>>
>> And it doesn't matter which pulse triggers the pre-emption. All that 
>> matters is that once a pre-emption is attempted, if the busy context 
>> fails to relinquish the hardware within the pre-emption timeout limit 
>> then it will be forcibly evicted.
>>
>>
>>>
>>>>>
>>>>> If so, if we went with my proposal, would everyone be happy? If 
>>>>> yes, isn't it a simpler scheme? No special casing when setting the 
>>>>> preempt timeout, no special casing of the last heartbeat pulse. 
>>>>> Works predictably regardless of the priority of the hypothetical 
>>>>> non-preemptible workload.
>>>> No, we have to have the increased pre-emption timeout. And that has 
>>>> ripple effects of making very long heartbeats or risking races with 
>>>> the heartbeat beating the per engine reset.
>>>>
>>>>
>>>>>
>>>>>>>> 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).
>>>>>>>
>>>>>>> What is broken with context persistence? I noticed one patch 
>>>>>>> claiming to be fixing something in that area which looked 
>>>>>>> suspect. Has it been established no userspace relies on it?
>>>>>> One major issue is that it has hooks into the execlist scheduler 
>>>>>> backend. I forget the exact details right now. The implementation 
>>>>>> as a whole is incredibly complex and convoluted :(. But there's 
>>>>>> stuff about what happens when you disable the heartbeat after 
>>>>>> having closed a persistence context's handle (and thus made it 
>>>>>> persisting). There's also things like it sends a super high 
>>>>>> priority heartbeat pulse at the point of becoming persisting. 
>>>>>> That plays havoc for platforms with dependent engines and/or 
>>>>>> compute workloads. A context becomes persisting on RCS and 
>>>>>> results in your unrealted CCS work being reset. It's a mess.
>>>>>>
>>>>>> The comment from Daniel Vetter is that persistence should have no 
>>>>>> connection to the heartbeat at all. All of that dynamic behaviour 
>>>>>> and complexity should just be removed.
>>>>>
>>>>> Dependent engines is definitely a topic on it's own, outside 
>>>>> hearbeats, persistence and all.
>>>> Except that it has implications for persistence which the current 
>>>> driver does not take into account.
>>>
>>> Well current driver does not take RCS+CCS dependency into account so 
>>> that should come first, or all in one package at least.
>> Not sure what you are arguing for here? Simplifying persistence as 
>> described will fix all the problems in one easy go. There is no point 
>> in adding yet more hideously complex code to make one corner case 
>> work when the real fix is to rip it all out.
>
> It sounded like you were arguing there was an issue with context 
> persistence and dependent engines so my reply was that if there are no 
> dependent engines in upstream there is no issue with persistence. Once 
> dependent engines are brought upstream then any reset caused by 
> persistence can be looked at and determined if there is any conceptual 
> difference between that and any random reset.
There are issues with context persistence and *everything*. Dependent 
engines is just one of may problems with persistence.

What I am arguing for is ripping out all the hideously complex 
persistence code that it is extremely fragile and does not work with 
modern platforms. Instead, replacing it with some massively simpler, 
much more maintainable and compatible with all platforms - past, present 
and future.


>
>>>>> Otherwise there is definitely complexity in the execlists backend 
>>>>> but I am not sure if logic persistence and heartbeats are so very 
>>>>> connected. It does send a pulse when heartbeat interval is 
>>>>> changed, which if going to zero, it will kick of closed contexts 
>>>>> if it can:
>>>>>
>>>>> static struct intel_engine_cs *
>>>>> __execlists_schedule_in(struct i915_request *rq)
>>>>> {
>>>>>     struct intel_engine_cs * const engine = rq->engine;
>>>>>     struct intel_context * const ce = rq->context;
>>>>>
>>>>>     intel_context_get(ce);
>>>>>
>>>>>     if (unlikely(intel_context_is_closed(ce) &&
>>>>>              !intel_engine_has_heartbeat(engine)))
>>>>>         intel_context_set_banned(ce);
>>>>>
>>>>>     if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>>>>>         reset_active(rq, engine);
>>>>>
>>>>> Is this what you mean? The point of this is to make sure 
>>>>> persistent context does not hog the engine forever if hangcheck 
>>>>> has been disabled.
>>>>>
>>>>> Reminds me of my improvement to customer experience which never 
>>>>> got in 
>>>>> (https://patchwork.freedesktop.org/patch/451491/?series=93420&rev=2). 
>>>>> Point of that one was to avoid engine reset (or worse) after user 
>>>>> presses "Ctrl-C" if something takes just over 1ms to cleanly 
>>>>> complete.
>>>> The plan is that the persistent contexts would still get the 
>>>> default grace period (pre-emption timeout at least) to finish but 
>>>> Ctrl+C will kill it within a timely manner if it does not finish.
>>>
>>> Yes my patch does that. ;) Currently non-persistent is killed to 
>>> quickly triggering pointless and alarming engine resets. Users 
>>> reported this last year and I tried to fix it.
>> Except that your patch is adding yet more complexity to an already 
>> complex feature. The desire is to simplify the driver and make it 
>> more maintainable not complicated it further.
>
> It is fixing an user reported issue and is not really adding 
> complexity apart from enabling us to distinguish the reason for 
> revoking a request (ban vs soft close). I think we should care to fix 
> user reported issues in a reasonable time frame. It's a patch from 
> last summer or so..
>
>>>>>> Persistence itself can stay. There are valid UMD use cases. It is 
>>>>>> just massively over complicated and doesn't work in all corner 
>>>>>> cases when not using execlist submission or on newer platforms. 
>>>>>> The simplification that is planned is to allow contexts to 
>>>>>> persist until the associated DRM master handle is closed. At that 
>>>>>> point, all contexts associated with that DRM handle are killed. 
>>>>>> That is what AMD and others apparently implement.
>>>>>
>>>>> Okay, that goes against one recent IGT patch which appeared to 
>>>>> work around something by moving the position of _context_ close.
>>>> No it does not. The context close is not the trigger. The trigger is 
>>>
>>> Well patch says:
>>> """
>>> The spin all test relied on context persistence unecessarily by trying
>>> to destroy contexts while keeping spinners active.
>>> The current implementation of context persistence in i915 can cause
>>> failures with GuC enabled, and persistence is not needed for this test.
>>>
>>> Moving intel_ctx_destroy after igt_spin_end.
>>> """
>>>
>>> Implying moving context close to after spin end fixes things for 
>>> GuC, not fd close.
>> That's because persistence is currently a big pile of poo and does 
>> not work in all the corner cases. The correct solution is to leave 
>> the IGT alone and just fix the implementation of persistence. 
>> However, the IGT update to not use the broken feature is a trivial 
>> test change (two lines?) whereas fixing the broken feature is a 
>> significant KMD re-work. It needs to be done but no-one currently has 
>> the time to do it. But trivially changing the test allows the test to 
>> work and test the features it is meant to be testing (which is not 
>> persistence).
>
> Clear as mud. If the statement is that persistence cannot simply be 
> removed, then for sure it cannot be said that anything can be fixed or 
> unblocked by allowing some test to pass with GuC, by making them avoid 
> using persistence (and not even explicitly with a context param). It 
> implies persistence does not work with the GuC, which is then in 
> contradiction with the statement that we cannot just remove 
> persistence. I truly have no idea what the argument is here.
Persistence works in the right set of circumstances. Those circumstances 
do not involve dynamically changing heartbeat settings, platforms with 
dependent engines, etc. The correct fix is to leave the IGT test alone 
and fix the persistence implementation. However, that is not trivial and 
we have many other high priority holes still to plug. Whereas changing 
the IGT to not use a feature it is not intended to be testing anyway is 
a trivial change and gets us the test coverage of what that IGT is meant 
to be for.

John.


>
> Regards,
>
> Tvrtko

Tvrtko Ursulin March 1, 2022, 12:09 p.m. UTC | #11

I'll trim it a bit again..

On 28/02/2022 18:55, John Harrison wrote:
> On 2/28/2022 09:12, Tvrtko Ursulin wrote:
>> On 25/02/2022 18:48, John Harrison wrote:
>>> On 2/25/2022 10:14, Tvrtko Ursulin wrote:

[snip]

>>>> Your only objection is that ends up with too long total time before 
>>>> reset? Or something else as well?
>>> An unnecessarily long total heartbeat timeout is the main objection. 
>>> (2.5 + 12) * 5 = 72.5 seconds. That is a massive change from the 
>>> current 12.5s.
>>>
>>> If we are happy with that huge increase then fine. But I'm pretty 
>>> sure you are going to get a lot more bug reports about hung systems 
>>> not recovering. 10-20s is just about long enough for someone to wait 
>>> before leaning on the power button of their machine. Over a minute is 
>>> not. That kind of delay is going to cause support issues.
>>
>> Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen 
>> just so it is longer than tH * 3?
>>
>> And how do you keep coming up with factor of five? Isn't it four 
>> periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier 
>> and then reset.)
> Prio starts at low not normal.

Right, slipped my mind since I only keep seeing that one priority ladder 
block in intel_engine_heartbeat.c/heartbeat()..

>> From the point of view of user experience I agree reasonable 
>> responsiveness is needed before user "reaches for the power button".
>>
>> In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.
>>
>> Question of workloads.. what is the actual preempt timeout compute is 
>> happy with? And I don't mean compute setups with disabled hangcheck, 
>> which you say they want anyway, but if we target defaults for end 
>> users. Do we have some numbers on what they are likely to run?
> Not that I have ever seen. This is all just finger in the air stuff. I 
> don't recall if we invented the number and the compute people agreed 
> with it or if they proposed the number to us.

Yeah me neither. And found nothing in my email archives. :(

Thinking about it today I don't see that disabled timeout is a practical 
default.

With it, if users have something un-preemptable to run (assuming prio 
normal), it would get killed after ~13s (5 * 2.5).

If we go for my scheme it gets killed in ~17.5s (3 * (2.5 + 2.5) + 2.5 
(third pulse triggers preempt timeout)).

And if we go for your scheme it gets killed in ~22.5s (4 * 2.5 + 2 * 3 * 
2.5).

If I did not confuse any calculation this time round, then the 
differences for default case for normal priority sound pretty immaterial 
to me.

>> What if we gave them a default of 2.5s? That would be 4 * (2.5s + 
>> 2.5s) = 20s worst case until reset, comparable to your proposal. Are 
>> there realistic workloads which are non-preemptable for 2.5s? I am 
>> having hard time imagining someone would run them on a general purpose 
>> desktop since it would mean frozen and unusable UI anyway.
>>
>> Advantage still being in my mind that there would be no fudging of 
>> timeouts during driver load and heartbeat periods depending on 
>> priority. To me it feels more plausible to account for preempt timeout 
>> in heartbeat pulses that to calculate preempt timeout to be longer 
>> than hearbeat pulses, just to avoid races between the two.
> Except that when the user asks for a heartbeat period of 2.5s you are 
> actually setting it to 5s. How is that not a major fudge that is totally 
> disregarding the user's request?

This is indeed the core question. My thinking:

It is not defined in the heartbeat ABI that N pulses should do anything, 
just that they are periodic pulses which check the health of an engine.

If we view user priority as not under our control then we can say that 
any heartbeat pulse can trigger preempt timeout and we should let it do 
that.

 From that it follows that it is justified to account for preempt 
timeout in the decision when to schedule heartbeat pulses and that it is 
legitimate to do it for all of them.

It also avoids the double reset problem, regardless of the backend and 
regardless of how the user configured the timeouts. Without the need to 
fudge them neither during driver load or during sysfs store.

User has configured that heartbeat pulses should be sent every N 
seconds, yes, but I think we are free to account for inherent hardware 
and software latencies in our calculations. Especially since other than 
flawed Gen12 RCS, other engines will be much closer to the configured 
period.

It is just the same as user asking for preempt timeout N and we say on 
driver load "oh no you won't get it". Same for heartbeats, they said 
2.5s, we said 2.5s + broken engine factor...

I don't see a problem there. Nothing timing sensitive relies on the 
heartbeat interval nor we provided any guarantees.

That patch from Chris for instance AFAIR accounted for scheduling or 
context switch latencies. Because what is the point of sending further 
elevated priority pulses if we did not leave enough time to the engine 
to schedule them in, react with preemption, or signalling completion?

>>>>>>> Persistence itself can stay. There are valid UMD use cases. It is 
>>>>>>> just massively over complicated and doesn't work in all corner 
>>>>>>> cases when not using execlist submission or on newer platforms. 
>>>>>>> The simplification that is planned is to allow contexts to 
>>>>>>> persist until the associated DRM master handle is closed. At that 
>>>>>>> point, all contexts associated with that DRM handle are killed. 
>>>>>>> That is what AMD and others apparently implement.
>>>>>>
>>>>>> Okay, that goes against one recent IGT patch which appeared to 
>>>>>> work around something by moving the position of _context_ close.
>>>>> No it does not. The context close is not the trigger. The trigger is 
>>>>
>>>> Well patch says:
>>>> """
>>>> The spin all test relied on context persistence unecessarily by trying
>>>> to destroy contexts while keeping spinners active.
>>>> The current implementation of context persistence in i915 can cause
>>>> failures with GuC enabled, and persistence is not needed for this test.
>>>>
>>>> Moving intel_ctx_destroy after igt_spin_end.
>>>> """
>>>>
>>>> Implying moving context close to after spin end fixes things for 
>>>> GuC, not fd close.
>>> That's because persistence is currently a big pile of poo and does 
>>> not work in all the corner cases. The correct solution is to leave 
>>> the IGT alone and just fix the implementation of persistence. 
>>> However, the IGT update to not use the broken feature is a trivial 
>>> test change (two lines?) whereas fixing the broken feature is a 
>>> significant KMD re-work. It needs to be done but no-one currently has 
>>> the time to do it. But trivially changing the test allows the test to 
>>> work and test the features it is meant to be testing (which is not 
>>> persistence).
>>
>> Clear as mud. If the statement is that persistence cannot simply be 
>> removed, then for sure it cannot be said that anything can be fixed or 
>> unblocked by allowing some test to pass with GuC, by making them avoid 
>> using persistence (and not even explicitly with a context param). It 
>> implies persistence does not work with the GuC, which is then in 
>> contradiction with the statement that we cannot just remove 
>> persistence. I truly have no idea what the argument is here.
> Persistence works in the right set of circumstances. Those circumstances 
> do not involve dynamically changing heartbeat settings, platforms with 
> dependent engines, etc. The correct fix is to leave the IGT test alone 
> and fix the persistence implementation. However, that is not trivial and 
> we have many other high priority holes still to plug. Whereas changing 
> the IGT to not use a feature it is not intended to be testing anyway is 
> a trivial change and gets us the test coverage of what that IGT is meant 
> to be for.

It may be acceptable if someone is reviewing overall coverage and making 
sure all is not removed and so a missing ABI in GuC backend not swept 
under the carpet. That's my main concern. If it is acknowledged 
persistence is a needed ABI, then how can we upstream dependent engine 
support without making sure this ABI is respected? Removing it's use 
from random tests does not fill me with confidence that we are on top of 
this topic.

Regards,

Tvrtko

John Harrison March 1, 2022, 8:59 p.m. UTC | #12

On 3/1/2022 04:09, Tvrtko Ursulin wrote:
>
> I'll trim it a bit again..
>
> On 28/02/2022 18:55, John Harrison wrote:
>> On 2/28/2022 09:12, Tvrtko Ursulin wrote:
>>> On 25/02/2022 18:48, John Harrison wrote:
>>>> On 2/25/2022 10:14, Tvrtko Ursulin wrote:
>
> [snip]
>
>>>>> Your only objection is that ends up with too long total time 
>>>>> before reset? Or something else as well?
>>>> An unnecessarily long total heartbeat timeout is the main 
>>>> objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive change 
>>>> from the current 12.5s.
>>>>
>>>> If we are happy with that huge increase then fine. But I'm pretty 
>>>> sure you are going to get a lot more bug reports about hung systems 
>>>> not recovering. 10-20s is just about long enough for someone to 
>>>> wait before leaning on the power button of their machine. Over a 
>>>> minute is not. That kind of delay is going to cause support issues.
>>>
>>> Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen 
>>> just so it is longer than tH * 3?
>>>
>>> And how do you keep coming up with factor of five? Isn't it four 
>>> periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier 
>>> and then reset.)
>> Prio starts at low not normal.
>
> Right, slipped my mind since I only keep seeing that one priority 
> ladder block in intel_engine_heartbeat.c/heartbeat()..
>
>>> From the point of view of user experience I agree reasonable 
>>> responsiveness is needed before user "reaches for the power button".
>>>
>>> In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.
>>>
>>> Question of workloads.. what is the actual preempt timeout compute 
>>> is happy with? And I don't mean compute setups with disabled 
>>> hangcheck, which you say they want anyway, but if we target defaults 
>>> for end users. Do we have some numbers on what they are likely to run?
>> Not that I have ever seen. This is all just finger in the air stuff. 
>> I don't recall if we invented the number and the compute people 
>> agreed with it or if they proposed the number to us.
>
> Yeah me neither. And found nothing in my email archives. :(
>
> Thinking about it today I don't see that disabled timeout is a 
> practical default.
>
> With it, if users have something un-preemptable to run (assuming prio 
> normal), it would get killed after ~13s (5 * 2.5).
>
> If we go for my scheme it gets killed in ~17.5s (3 * (2.5 + 2.5) + 2.5 
> (third pulse triggers preempt timeout)).
>
> And if we go for your scheme it gets killed in ~22.5s (4 * 2.5 + 2 * 3 
> * 2.5).
Erm, that is not an apples to apples comparison. Your 17.5 is for an 
engine reset tripped by the pre-emption timeout, but your 22.5s is for a 
GT reset tripped by the heartbeat reaching the end and nuking the universe.

If you are saying that the first pulse at sufficient priority (third 
being normal prio) is what causes the reset because the system is 
working as expected and the pre-emption timeout trips the reset. In that 
case, you have two periods to get to normal prio plus one pre-emption 
timeout to trip the reset. I.e. (tH * 2) + tP.

Your scheme is then tH(actual) = tH(user) + tP, yes?
So pre-emption based reset is after ((tH(user) + tP) * 2) + tP => (3 * 
tP) + (2 * tH)
And GT based reset is after (tH(user) + tP) * 5 => (5 * tP) + (5 * tH)

My scheme is tH(actual) = tH(user) for first four, then max(tH(user), 
tP) for fifth.
So pre-emption based reset is after tH(user) * 2 + tP = > tP + (2 * tH);
And GT based reset is after (tH(user) * 4) + (max(tH(user), tP) * 1) => 
greater of ((4 * tH) + tP) or (5 * tH)

Either way your scheme is longer. With tH(user) = 2.5s, tP(RCS) = 7.5s, 
we get 27.5s for engine and 50s for GT versus my 12.5s for engine and 
17.5s for GT. With tP(RCS) = 2.5s, yours is 12.5s for engine and 25s for 
GT versus my 7.5s for engine and 12.5s for GT.

Plus, not sure why your calculations above are using 2.5 for tP? Are you 
still arguing that 7.5s is too long? That is a separate issue and not 
related to the heartbeat algorithms. tP must be long enough to allow 
'out of box OpenCL workloads to complete'. That doesn't just mean not 
being killed by the heartbeat, it also means not being killed by running 
two of them concurrently (or one plus desktop OpenGL rendering) and not 
having it killed by basic time slicing between the two contexts. The 
heartbeat is not involved in that process. That is purely the 
pre-emption timeout. And that is the fundamental reason why tP needs to 
be much larger on RCS/CCS.



>
> If I did not confuse any calculation this time round, then the 
> differences for default case for normal priority sound pretty 
> immaterial to me.
>
>>> What if we gave them a default of 2.5s? That would be 4 * (2.5s + 
>>> 2.5s) = 20s worst case until reset, comparable to your proposal. Are 
>>> there realistic workloads which are non-preemptable for 2.5s? I am 
>>> having hard time imagining someone would run them on a general 
>>> purpose desktop since it would mean frozen and unusable UI anyway.
>>>
>>> Advantage still being in my mind that there would be no fudging of 
>>> timeouts during driver load and heartbeat periods depending on 
>>> priority. To me it feels more plausible to account for preempt 
>>> timeout in heartbeat pulses that to calculate preempt timeout to be 
>>> longer than hearbeat pulses, just to avoid races between the two.
>> Except that when the user asks for a heartbeat period of 2.5s you are 
>> actually setting it to 5s. How is that not a major fudge that is 
>> totally disregarding the user's request?
>
> This is indeed the core question. My thinking:
>
> It is not defined in the heartbeat ABI that N pulses should do 
> anything, just that they are periodic pulses which check the health of 
> an engine.
>
> If we view user priority as not under our control then we can say that 
> any heartbeat pulse can trigger preempt timeout and we should let it 
> do that.
>
> From that it follows that it is justified to account for preempt 
> timeout in the decision when to schedule heartbeat pulses and that it 
> is legitimate to do it for all of them.
But it can be optimised to say that it doesn't matter if you bump the 
priority of a pulse before waiting for the pre-emption period to expire. 
If the pulse was already high enough prio then the pre-emption has 
already been triggered and bumping the prio has no effect. If was too 
low then waiting for longer has no benefit at all.

All that matters is that you don't hit the end stop and trigger the GT 
reset too early.

>
> It also avoids the double reset problem, regardless of the backend and 
> regardless of how the user configured the timeouts. Without the need 
> to fudge them neither during driver load or during sysfs store.
>
> User has configured that heartbeat pulses should be sent every N 
> seconds, yes, but I think we are free to account for inherent hardware 
> and software latencies in our calculations. Especially since other 
> than flawed Gen12 RCS, other engines will be much closer to the 
> configured period.
>
> It is just the same as user asking for preempt timeout N and we say on 
> driver load "oh no you won't get it". Same for heartbeats, they said 
> 2.5s, we said 2.5s + broken engine factor...
Why would you not get the pre-emption timeout? Because it is too large? 
That is a physical limitation (of the GuC firmware) not an override 
because we think we know better. If we can obey the user then we should 
do so.

>
> I don't see a problem there. Nothing timing sensitive relies on the 
> heartbeat interval nor we provided any guarantees.
>
> That patch from Chris for instance AFAIR accounted for scheduling or 
> context switch latencies. Because what is the point of sending further 
> elevated priority pulses if we did not leave enough time to the engine 
> to schedule them in, react with preemption, or signalling completion?
>
>>>>>>>> Persistence itself can stay. There are valid UMD use cases. It 
>>>>>>>> is just massively over complicated and doesn't work in all 
>>>>>>>> corner cases when not using execlist submission or on newer 
>>>>>>>> platforms. The simplification that is planned is to allow 
>>>>>>>> contexts to persist until the associated DRM master handle is 
>>>>>>>> closed. At that point, all contexts associated with that DRM 
>>>>>>>> handle are killed. That is what AMD and others apparently 
>>>>>>>> implement.
>>>>>>>
>>>>>>> Okay, that goes against one recent IGT patch which appeared to 
>>>>>>> work around something by moving the position of _context_ close.
>>>>>> No it does not. The context close is not the trigger. The trigger is 
>>>>>
>>>>> Well patch says:
>>>>> """
>>>>> The spin all test relied on context persistence unecessarily by 
>>>>> trying
>>>>> to destroy contexts while keeping spinners active.
>>>>> The current implementation of context persistence in i915 can cause
>>>>> failures with GuC enabled, and persistence is not needed for this 
>>>>> test.
>>>>>
>>>>> Moving intel_ctx_destroy after igt_spin_end.
>>>>> """
>>>>>
>>>>> Implying moving context close to after spin end fixes things for 
>>>>> GuC, not fd close.
>>>> That's because persistence is currently a big pile of poo and does 
>>>> not work in all the corner cases. The correct solution is to leave 
>>>> the IGT alone and just fix the implementation of persistence. 
>>>> However, the IGT update to not use the broken feature is a trivial 
>>>> test change (two lines?) whereas fixing the broken feature is a 
>>>> significant KMD re-work. It needs to be done but no-one currently 
>>>> has the time to do it. But trivially changing the test allows the 
>>>> test to work and test the features it is meant to be testing (which 
>>>> is not persistence).
>>>
>>> Clear as mud. If the statement is that persistence cannot simply be 
>>> removed, then for sure it cannot be said that anything can be fixed 
>>> or unblocked by allowing some test to pass with GuC, by making them 
>>> avoid using persistence (and not even explicitly with a context 
>>> param). It implies persistence does not work with the GuC, which is 
>>> then in contradiction with the statement that we cannot just remove 
>>> persistence. I truly have no idea what the argument is here.
>> Persistence works in the right set of circumstances. Those 
>> circumstances do not involve dynamically changing heartbeat settings, 
>> platforms with dependent engines, etc. The correct fix is to leave 
>> the IGT test alone and fix the persistence implementation. However, 
>> that is not trivial and we have many other high priority holes still 
>> to plug. Whereas changing the IGT to not use a feature it is not 
>> intended to be testing anyway is a trivial change and gets us the 
>> test coverage of what that IGT is meant to be for.
>
> It may be acceptable if someone is reviewing overall coverage and 
> making sure all is not removed and so a missing ABI in GuC backend not 
> swept under the carpet. That's my main concern. If it is acknowledged 
> persistence is a needed ABI, then how can we upstream dependent engine 
> support without making sure this ABI is respected? Removing it's use 
> from random tests does not fill me with confidence that we are on top 
> of this topic.
Maybe if it didn't take three+ weeks to get a trivial change merged then 
I might have time to work on the non-trivial tasks that are in the backlog.

John.


>
> Regards,
>
> Tvrtko

Tvrtko Ursulin March 2, 2022, 11:07 a.m. UTC | #13

On 01/03/2022 20:59, John Harrison wrote:
> On 3/1/2022 04:09, Tvrtko Ursulin wrote:
>>
>> I'll trim it a bit again..
>>
>> On 28/02/2022 18:55, John Harrison wrote:
>>> On 2/28/2022 09:12, Tvrtko Ursulin wrote:
>>>> On 25/02/2022 18:48, John Harrison wrote:
>>>>> On 2/25/2022 10:14, Tvrtko Ursulin wrote:
>>
>> [snip]
>>
>>>>>> Your only objection is that ends up with too long total time 
>>>>>> before reset? Or something else as well?
>>>>> An unnecessarily long total heartbeat timeout is the main 
>>>>> objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive change 
>>>>> from the current 12.5s.
>>>>>
>>>>> If we are happy with that huge increase then fine. But I'm pretty 
>>>>> sure you are going to get a lot more bug reports about hung systems 
>>>>> not recovering. 10-20s is just about long enough for someone to 
>>>>> wait before leaning on the power button of their machine. Over a 
>>>>> minute is not. That kind of delay is going to cause support issues.
>>>>
>>>> Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, chosen 
>>>> just so it is longer than tH * 3?
>>>>
>>>> And how do you keep coming up with factor of five? Isn't it four 
>>>> periods before "heartbeat stopped"? (Prio normal, hearbeat, barrier 
>>>> and then reset.)
>>> Prio starts at low not normal.
>>
>> Right, slipped my mind since I only keep seeing that one priority 
>> ladder block in intel_engine_heartbeat.c/heartbeat()..
>>
>>>> From the point of view of user experience I agree reasonable 
>>>> responsiveness is needed before user "reaches for the power button".
>>>>
>>>> In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.
>>>>
>>>> Question of workloads.. what is the actual preempt timeout compute 
>>>> is happy with? And I don't mean compute setups with disabled 
>>>> hangcheck, which you say they want anyway, but if we target defaults 
>>>> for end users. Do we have some numbers on what they are likely to run?
>>> Not that I have ever seen. This is all just finger in the air stuff. 
>>> I don't recall if we invented the number and the compute people 
>>> agreed with it or if they proposed the number to us.
>>
>> Yeah me neither. And found nothing in my email archives. :(
>>
>> Thinking about it today I don't see that disabled timeout is a 
>> practical default.
>>
>> With it, if users have something un-preemptable to run (assuming prio 
>> normal), it would get killed after ~13s (5 * 2.5).
>>
>> If we go for my scheme it gets killed in ~17.5s (3 * (2.5 + 2.5) + 2.5 
>> (third pulse triggers preempt timeout)).
>>
>> And if we go for your scheme it gets killed in ~22.5s (4 * 2.5 + 2 * 3 
>> * 2.5).
> Erm, that is not an apples to apples comparison. Your 17.5 is for an 
> engine reset tripped by the pre-emption timeout, but your 22.5s is for a 
> GT reset tripped by the heartbeat reaching the end and nuking the universe.

Right, in your scheme I did get it wrong. It would wait for GuC to reset 
the engine at the end, rather than hit the fake "hearbeat stopped" in 
that case, full reset path.

4 * 2.5 to trigger a max prio pulse, then 3 * 2.5 preempt timeout for 
GuC to reset (last hearbeat delay extended so it does not trigger). So 
17.5 as well.

> If you are saying that the first pulse at sufficient priority (third 
> being normal prio) is what causes the reset because the system is 
> working as expected and the pre-emption timeout trips the reset. In that 
> case, you have two periods to get to normal prio plus one pre-emption 
> timeout to trip the reset. I.e. (tH * 2) + tP.
> 
> Your scheme is then tH(actual) = tH(user) + tP, yes?
> So pre-emption based reset is after ((tH(user) + tP) * 2) + tP => (3 * 
> tP) + (2 * tH)
> And GT based reset is after (tH(user) + tP) * 5 => (5 * tP) + (5 * tH)
> 
> My scheme is tH(actual) = tH(user) for first four, then max(tH(user), 
> tP) for fifth.
> So pre-emption based reset is after tH(user) * 2 + tP = > tP + (2 * tH);
> And GT based reset is after (tH(user) * 4) + (max(tH(user), tP) * 1) => 
> greater of ((4 * tH) + tP) or (5 * tH)
> 
> Either way your scheme is longer. With tH(user) = 2.5s, tP(RCS) = 7.5s, 
> we get 27.5s for engine and 50s for GT versus my 12.5s for engine and 
> 17.5s for GT. With tP(RCS) = 2.5s, yours is 12.5s for engine and 25s for 
> GT versus my 7.5s for engine and 12.5s for GT.
> 
> Plus, not sure why your calculations above are using 2.5 for tP? Are you 
> still arguing that 7.5s is too long? That is a separate issue and not 
> related to the heartbeat algorithms. tP must be long enough to allow 
> 'out of box OpenCL workloads to complete'. That doesn't just mean not 
> being killed by the heartbeat, it also means not being killed by running 
> two of them concurrently (or one plus desktop OpenGL rendering) and not 
> having it killed by basic time slicing between the two contexts. The 
> heartbeat is not involved in that process. That is purely the 
> pre-emption timeout. And that is the fundamental reason why tP needs to 
> be much larger on RCS/CCS.

I was assuming 2.5s tP is enough and basing all calculation on that. 
Heartbeat or timeslicing regardless. I thought we established neither of 
us knows how long is enough.

Are you now saying 2.5s is definitely not enough? How is that usable for 
a default out of the box desktop?

>> If I did not confuse any calculation this time round, then the 
>> differences for default case for normal priority sound pretty 
>> immaterial to me.
>>
>>>> What if we gave them a default of 2.5s? That would be 4 * (2.5s + 
>>>> 2.5s) = 20s worst case until reset, comparable to your proposal. Are 
>>>> there realistic workloads which are non-preemptable for 2.5s? I am 
>>>> having hard time imagining someone would run them on a general 
>>>> purpose desktop since it would mean frozen and unusable UI anyway.
>>>>
>>>> Advantage still being in my mind that there would be no fudging of 
>>>> timeouts during driver load and heartbeat periods depending on 
>>>> priority. To me it feels more plausible to account for preempt 
>>>> timeout in heartbeat pulses that to calculate preempt timeout to be 
>>>> longer than hearbeat pulses, just to avoid races between the two.
>>> Except that when the user asks for a heartbeat period of 2.5s you are 
>>> actually setting it to 5s. How is that not a major fudge that is 
>>> totally disregarding the user's request?
>>
>> This is indeed the core question. My thinking:
>>
>> It is not defined in the heartbeat ABI that N pulses should do 
>> anything, just that they are periodic pulses which check the health of 
>> an engine.
>>
>> If we view user priority as not under our control then we can say that 
>> any heartbeat pulse can trigger preempt timeout and we should let it 
>> do that.
>>
>> From that it follows that it is justified to account for preempt 
>> timeout in the decision when to schedule heartbeat pulses and that it 
>> is legitimate to do it for all of them.
> But it can be optimised to say that it doesn't matter if you bump the 
> priority of a pulse before waiting for the pre-emption period to expire. 
> If the pulse was already high enough prio then the pre-emption has 
> already been triggered and bumping the prio has no effect. If was too 
> low then waiting for longer has no benefit at all.
> 
> All that matters is that you don't hit the end stop and trigger the GT 
> reset too early.

Yes I agree that it can also be argued what you are saying.

I was trying to weigh pros&cons of both approaches by bringing into the 
discussing the question of what are heartbeats. Given they are 
loosely/vaguely defined I think we have freedom to tweak things.

And I don't have a problem with extending the last pulse. It is 
fundamentally correct to do regardless of the backend. I just raised the 
question of whether to extend all heartbeats to account for preemption 
(and scheduling delays). (What is the point of bumping their priority 
and re-scheduling if we didn't give enough time to the engine to react? 
So opposite of the question you raise.)

What I do have a problem with is deriving the preempt timeout from the 
hearbeat period. Hence I am looking if we can instead find a fixed 
number which works, and so avoid having bi-directional coupling.

>> It also avoids the double reset problem, regardless of the backend and 
>> regardless of how the user configured the timeouts. Without the need 
>> to fudge them neither during driver load or during sysfs store.
>>
>> User has configured that heartbeat pulses should be sent every N 
>> seconds, yes, but I think we are free to account for inherent hardware 
>> and software latencies in our calculations. Especially since other 
>> than flawed Gen12 RCS, other engines will be much closer to the 
>> configured period.
>>
>> It is just the same as user asking for preempt timeout N and we say on 
>> driver load "oh no you won't get it". Same for heartbeats, they said 
>> 2.5s, we said 2.5s + broken engine factor...
> Why would you not get the pre-emption timeout? Because it is too large? 
> That is a physical limitation (of the GuC firmware) not an override 
> because we think we know better. If we can obey the user then we should 
> do so.

I was simply referring to the override in intel_engine_setup.

Regards,

Tvrtko

John Harrison March 2, 2022, 5:55 p.m. UTC | #14

On 3/2/2022 03:07, Tvrtko Ursulin wrote:
> On 01/03/2022 20:59, John Harrison wrote:
>> On 3/1/2022 04:09, Tvrtko Ursulin wrote:
>>>
>>> I'll trim it a bit again..
>>>
>>> On 28/02/2022 18:55, John Harrison wrote:
>>>> On 2/28/2022 09:12, Tvrtko Ursulin wrote:
>>>>> On 25/02/2022 18:48, John Harrison wrote:
>>>>>> On 2/25/2022 10:14, Tvrtko Ursulin wrote:
>>>
>>> [snip]
>>>
>>>>>>> Your only objection is that ends up with too long total time 
>>>>>>> before reset? Or something else as well?
>>>>>> An unnecessarily long total heartbeat timeout is the main 
>>>>>> objection. (2.5 + 12) * 5 = 72.5 seconds. That is a massive 
>>>>>> change from the current 12.5s.
>>>>>>
>>>>>> If we are happy with that huge increase then fine. But I'm pretty 
>>>>>> sure you are going to get a lot more bug reports about hung 
>>>>>> systems not recovering. 10-20s is just about long enough for 
>>>>>> someone to wait before leaning on the power button of their 
>>>>>> machine. Over a minute is not. That kind of delay is going to 
>>>>>> cause support issues.
>>>>>
>>>>> Sorry I wrote 12s, while you actually said tP * 12, so 7.68s, 
>>>>> chosen just so it is longer than tH * 3?
>>>>>
>>>>> And how do you keep coming up with factor of five? Isn't it four 
>>>>> periods before "heartbeat stopped"? (Prio normal, hearbeat, 
>>>>> barrier and then reset.)
>>>> Prio starts at low not normal.
>>>
>>> Right, slipped my mind since I only keep seeing that one priority 
>>> ladder block in intel_engine_heartbeat.c/heartbeat()..
>>>
>>>>> From the point of view of user experience I agree reasonable 
>>>>> responsiveness is needed before user "reaches for the power button".
>>>>>
>>>>> In your proposal we are talking about 3 * 2.5s + 2 * 7.5s, so 22.5s.
>>>>>
>>>>> Question of workloads.. what is the actual preempt timeout compute 
>>>>> is happy with? And I don't mean compute setups with disabled 
>>>>> hangcheck, which you say they want anyway, but if we target 
>>>>> defaults for end users. Do we have some numbers on what they are 
>>>>> likely to run?
>>>> Not that I have ever seen. This is all just finger in the air 
>>>> stuff. I don't recall if we invented the number and the compute 
>>>> people agreed with it or if they proposed the number to us.
>>>
>>> Yeah me neither. And found nothing in my email archives. :(
>>>
>>> Thinking about it today I don't see that disabled timeout is a 
>>> practical default.
>>>
>>> With it, if users have something un-preemptable to run (assuming 
>>> prio normal), it would get killed after ~13s (5 * 2.5).
>>>
>>> If we go for my scheme it gets killed in ~17.5s (3 * (2.5 + 2.5) + 
>>> 2.5 (third pulse triggers preempt timeout)).
>>>
>>> And if we go for your scheme it gets killed in ~22.5s (4 * 2.5 + 2 * 
>>> 3 * 2.5).
>> Erm, that is not an apples to apples comparison. Your 17.5 is for an 
>> engine reset tripped by the pre-emption timeout, but your 22.5s is 
>> for a GT reset tripped by the heartbeat reaching the end and nuking 
>> the universe.
>
> Right, in your scheme I did get it wrong. It would wait for GuC to 
> reset the engine at the end, rather than hit the fake "hearbeat 
> stopped" in that case, full reset path.
>
> 4 * 2.5 to trigger a max prio pulse, then 3 * 2.5 preempt timeout for 
> GuC to reset (last hearbeat delay extended so it does not trigger). So 
> 17.5 as well.

Again, apples or oranges? I was using your tP(RCS) == 2.5s assumption in 
all the above calculations given that the discussion was about the 
heartbeat algorithm, not the choice of pre-emption timeout. In which 
case the last heartbeat is max(tP * 2, tH) == 2 * 2.5s.

>
>> If you are saying that the first pulse at sufficient priority (third 
>> being normal prio) is what causes the reset because the system is 
>> working as expected and the pre-emption timeout trips the reset. In 
>> that case, you have two periods to get to normal prio plus one 
>> pre-emption timeout to trip the reset. I.e. (tH * 2) + tP.
>>
>> Your scheme is then tH(actual) = tH(user) + tP, yes?
>> So pre-emption based reset is after ((tH(user) + tP) * 2) + tP => (3 
>> * tP) + (2 * tH)
>> And GT based reset is after (tH(user) + tP) * 5 => (5 * tP) + (5 * tH)
>>
>> My scheme is tH(actual) = tH(user) for first four, then max(tH(user), 
>> tP) for fifth.
>> So pre-emption based reset is after tH(user) * 2 + tP = > tP + (2 * tH);
>> And GT based reset is after (tH(user) * 4) + (max(tH(user), tP) * 1) 
>> => greater of ((4 * tH) + tP) or (5 * tH)
>>
>> Either way your scheme is longer. With tH(user) = 2.5s, tP(RCS) = 
>> 7.5s, we get 27.5s for engine and 50s for GT versus my 12.5s for 
>> engine and 17.5s for GT. With tP(RCS) = 2.5s, yours is 12.5s for 
>> engine and 25s for GT versus my 7.5s for engine and 12.5s for GT.
>>
>> Plus, not sure why your calculations above are using 2.5 for tP? Are 
>> you still arguing that 7.5s is too long? That is a separate issue and 
>> not related to the heartbeat algorithms. tP must be long enough to 
>> allow 'out of box OpenCL workloads to complete'. That doesn't just 
>> mean not being killed by the heartbeat, it also means not being 
>> killed by running two of them concurrently (or one plus desktop 
>> OpenGL rendering) and not having it killed by basic time slicing 
>> between the two contexts. The heartbeat is not involved in that 
>> process. That is purely the pre-emption timeout. And that is the 
>> fundamental reason why tP needs to be much larger on RCS/CCS.
>
> I was assuming 2.5s tP is enough and basing all calculation on that. 
> Heartbeat or timeslicing regardless. I thought we established neither 
> of us knows how long is enough.
>
> Are you now saying 2.5s is definitely not enough? How is that usable 
> for a default out of the box desktop?
Show me your proof that 2.5s is enough.

7.5s is what we have been using internally for a very long time. It has 
approval from all relevant parties. If you wish to pick a new random 
number then please provide data to back it up along with buy in from all 
UMD teams and project management.

>
>>> If I did not confuse any calculation this time round, then the 
>>> differences for default case for normal priority sound pretty 
>>> immaterial to me.
>>>
>>>>> What if we gave them a default of 2.5s? That would be 4 * (2.5s + 
>>>>> 2.5s) = 20s worst case until reset, comparable to your proposal. 
>>>>> Are there realistic workloads which are non-preemptable for 2.5s? 
>>>>> I am having hard time imagining someone would run them on a 
>>>>> general purpose desktop since it would mean frozen and unusable UI 
>>>>> anyway.
>>>>>
>>>>> Advantage still being in my mind that there would be no fudging of 
>>>>> timeouts during driver load and heartbeat periods depending on 
>>>>> priority. To me it feels more plausible to account for preempt 
>>>>> timeout in heartbeat pulses that to calculate preempt timeout to 
>>>>> be longer than hearbeat pulses, just to avoid races between the two.
>>>> Except that when the user asks for a heartbeat period of 2.5s you 
>>>> are actually setting it to 5s. How is that not a major fudge that 
>>>> is totally disregarding the user's request?
>>>
>>> This is indeed the core question. My thinking:
>>>
>>> It is not defined in the heartbeat ABI that N pulses should do 
>>> anything, just that they are periodic pulses which check the health 
>>> of an engine.
>>>
>>> If we view user priority as not under our control then we can say 
>>> that any heartbeat pulse can trigger preempt timeout and we should 
>>> let it do that.
>>>
>>> From that it follows that it is justified to account for preempt 
>>> timeout in the decision when to schedule heartbeat pulses and that 
>>> it is legitimate to do it for all of them.
>> But it can be optimised to say that it doesn't matter if you bump the 
>> priority of a pulse before waiting for the pre-emption period to 
>> expire. If the pulse was already high enough prio then the 
>> pre-emption has already been triggered and bumping the prio has no 
>> effect. If was too low then waiting for longer has no benefit at all.
>>
>> All that matters is that you don't hit the end stop and trigger the 
>> GT reset too early.
>
> Yes I agree that it can also be argued what you are saying.
>
> I was trying to weigh pros&cons of both approaches by bringing into 
> the discussing the question of what are heartbeats. Given they are 
> loosely/vaguely defined I think we have freedom to tweak things.
>
> And I don't have a problem with extending the last pulse. It is 
> fundamentally correct to do regardless of the backend. I just raised 
> the question of whether to extend all heartbeats to account for 
> preemption (and scheduling delays). (What is the point of bumping 
> their priority and re-scheduling if we didn't give enough time to the 
> engine to react? So opposite of the question you raise.)
The point is that it we are giving enough time to react. Raising the 
priority of a pre-emption that has already been triggered will have no 
effect. So as long as the total time from when the pre-emption is 
triggered (prio becomes sufficiently high) to the point when the reset 
is decided is longer than the pre-emption timeout then it works. Given 
that, it is unnecessary to increase the intermediate periods. It has no 
advantage and has the disadvantage of making the total time unreasonably 
long.

So again, what is the point of making every period longer? What benefit 
does it *actually* give?

>
> What I do have a problem with is deriving the preempt timeout from the 
> hearbeat period. Hence I am looking if we can instead find a fixed 
> number which works, and so avoid having bi-directional coupling.
Fine. "tP(RCS) = 7500;" can I merge the patch now?

>
>>> It also avoids the double reset problem, regardless of the backend 
>>> and regardless of how the user configured the timeouts. Without the 
>>> need to fudge them neither during driver load or during sysfs store.
>>>
>>> User has configured that heartbeat pulses should be sent every N 
>>> seconds, yes, but I think we are free to account for inherent 
>>> hardware and software latencies in our calculations. Especially 
>>> since other than flawed Gen12 RCS, other engines will be much closer 
>>> to the configured period.
>>>
>>> It is just the same as user asking for preempt timeout N and we say 
>>> on driver load "oh no you won't get it". Same for heartbeats, they 
>>> said 2.5s, we said 2.5s + broken engine factor...
>> Why would you not get the pre-emption timeout? Because it is too 
>> large? That is a physical limitation (of the GuC firmware) not an 
>> override because we think we know better. If we can obey the user 
>> then we should do so.
>
> I was simply referring to the override in intel_engine_setup.
Meaning the boost rather than the clamp? The other option would be to 
add a CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE option for specifying 
tP(RCS). I can do that as a follow up patch if other maintainers agree 
to adding yet more CONFIG options. My understanding was that they are 
frowned upon and only to be added when there is no other way.

John.

>
> Regards,
>
> Tvrtko

Tvrtko Ursulin March 3, 2022, 9:55 a.m. UTC | #15

On 02/03/2022 17:55, John Harrison wrote:

>> I was assuming 2.5s tP is enough and basing all calculation on that. 
>> Heartbeat or timeslicing regardless. I thought we established neither 
>> of us knows how long is enough.
>>
>> Are you now saying 2.5s is definitely not enough? How is that usable 
>> for a default out of the box desktop?
> Show me your proof that 2.5s is enough.
> 
> 7.5s is what we have been using internally for a very long time. It has 
> approval from all relevant parties. If you wish to pick a new random 
> number then please provide data to back it up along with buy in from all 
> UMD teams and project management.

And upstream disabled preemption has acks from compute. "Internally" is 
as far away from out of the box desktop experiences we have been arguing 
about. In fact you have argued compute disables the hearbeat anyway.

Lets jump to the end of this reply please for actions.

>> And I don't have a problem with extending the last pulse. It is 
>> fundamentally correct to do regardless of the backend. I just raised 
>> the question of whether to extend all heartbeats to account for 
>> preemption (and scheduling delays). (What is the point of bumping 
>> their priority and re-scheduling if we didn't give enough time to the 
>> engine to react? So opposite of the question you raise.)
> The point is that it we are giving enough time to react. Raising the 
> priority of a pre-emption that has already been triggered will have no 
> effect. So as long as the total time from when the pre-emption is 
> triggered (prio becomes sufficiently high) to the point when the reset 
> is decided is longer than the pre-emption timeout then it works. Given 
> that, it is unnecessary to increase the intermediate periods. It has no 
> advantage and has the disadvantage of making the total time unreasonably 
> long.
> 
> So again, what is the point of making every period longer? What benefit 
> does it *actually* give?

Less special casing and pointless prio bumps ahead of giving time to 
engine to even react. You wouldn't have to have the last pulse 2 * tP 
but normal tH + tP. So again, it is nicer for me to derive all heartbeat 
pulses from the same input parameters.

The whole "it is very long" argument is IMO moot because now proposed 
7.5s preempt period is I suspect wholly impractical for desktop. 
Combined with the argument that real compute disables heartbeats anyway 
even extra so.

> Fine. "tP(RCS) = 7500;" can I merge the patch now?
I could live with setting preempt timeout to 7.5s. The downside is 
slower time to restoring frozen desktops. Worst case today 5 * 2.5s, 
with changes 4 * 2.5s + 2 * 7.5s; so from 12.5s to 25s, doubling..

Actions:

1)
Get a number from compute/OpenCL people for what they say is minimum 
preempt timeout for default out of the box Linux desktop experience.

This does not mean them running some tests and can't be bothered to 
setup up the machine for the extreme use cases, but workloads average 
users can realistically be expected to run.

Say for instance some image manipulation software which is OpenCL 
accelerated or similar. How long unpreemptable sections are expected 
there. Or similar. I am not familiar what all OpenCL accelerated use 
cases there are on Linux.

And this number should be purely about minimum preempt timeout, not 
considering heartbeats. This is because preempt timeout may kick in 
sooner than stopped heartbeat if the user workload is low priority.

2)
Commit message should explain the effect on the worst case time until 
engine reset.

3)
OpenCL/compute should ack the change publicly as well since they acked 
the disabling of preemption.

4)
I really want overflows_type in the first patch.

My position is that with the above satisfied it is okay to merge.

Regards,

Tvrtko

John Harrison March 3, 2022, 7:09 p.m. UTC | #16

On 3/3/2022 01:55, Tvrtko Ursulin wrote:
> On 02/03/2022 17:55, John Harrison wrote:
>
>>> I was assuming 2.5s tP is enough and basing all calculation on that. 
>>> Heartbeat or timeslicing regardless. I thought we established 
>>> neither of us knows how long is enough.
>>>
>>> Are you now saying 2.5s is definitely not enough? How is that usable 
>>> for a default out of the box desktop?
>> Show me your proof that 2.5s is enough.
>>
>> 7.5s is what we have been using internally for a very long time. It 
>> has approval from all relevant parties. If you wish to pick a new 
>> random number then please provide data to back it up along with buy 
>> in from all UMD teams and project management.
>
> And upstream disabled preemption has acks from compute. "Internally" 
> is as far away from out of the box desktop experiences we have been 
> arguing about. In fact you have argued compute disables the hearbeat 
> anyway.
>
> Lets jump to the end of this reply please for actions.
>
>>> And I don't have a problem with extending the last pulse. It is 
>>> fundamentally correct to do regardless of the backend. I just raised 
>>> the question of whether to extend all heartbeats to account for 
>>> preemption (and scheduling delays). (What is the point of bumping 
>>> their priority and re-scheduling if we didn't give enough time to 
>>> the engine to react? So opposite of the question you raise.)
>> The point is that it we are giving enough time to react. Raising the 
>> priority of a pre-emption that has already been triggered will have 
>> no effect. So as long as the total time from when the pre-emption is 
>> triggered (prio becomes sufficiently high) to the point when the 
>> reset is decided is longer than the pre-emption timeout then it 
>> works. Given that, it is unnecessary to increase the intermediate 
>> periods. It has no advantage and has the disadvantage of making the 
>> total time unreasonably long.
>>
>> So again, what is the point of making every period longer? What 
>> benefit does it *actually* give?
>
> Less special casing and pointless prio bumps ahead of giving time to 
> engine to even react. You wouldn't have to have the last pulse 2 * tP 
> but normal tH + tP. So again, it is nicer for me to derive all 
> heartbeat pulses from the same input parameters.
>
> The whole "it is very long" argument is IMO moot because now proposed 
> 7.5s preempt period is I suspect wholly impractical for desktop. 
> Combined with the argument that real compute disables heartbeats 
> anyway even extra so.
The whole thing is totally fubar already. Right now pre-emption is 
totally disabled. So you are currently waiting for the entire heartbeat 
sequence to complete and then nuking the entire machine. So arguing that 
7.5s is too long is pointless. 7.5s is a big improvement over what is 
currently enabled.

And 'nice' would be having hardware that worked in a sensible manner. 
There is no nice here. There is only 'what is the least worst option'. 
And the least worst option for an end user is a long pre-emption timeout 
with a not massively long heartbeat. If that means a very slight 
complication in the heartbeat code, that is a trivial concern.


>
>> Fine. "tP(RCS) = 7500;" can I merge the patch now?
> I could live with setting preempt timeout to 7.5s. The downside is 
> slower time to restoring frozen desktops. Worst case today 5 * 2.5s, 
> with changes 4 * 2.5s + 2 * 7.5s; so from 12.5s to 25s, doubling..
But that is worst case scenario (when something much more severe than an 
application hang has occurred). Regular case would be second heartbeat 
period + pre-emption timeout and an engine only reset not a full GT 
reset. So it's still better than what we have at present.

>
> Actions:
>
> 1)
> Get a number from compute/OpenCL people for what they say is minimum 
> preempt timeout for default out of the box Linux desktop experience.
That would be the one that has been agreed upon by both linux software 
arch and all UMD teams and has been in use for the past year or more in 
the internal tree.

>
> This does not mean them running some tests and can't be bothered to 
> setup up the machine for the extreme use cases, but workloads average 
> users can realistically be expected to run.
>
> Say for instance some image manipulation software which is OpenCL 
> accelerated or similar. How long unpreemptable sections are expected 
> there. Or similar. I am not familiar what all OpenCL accelerated use 
> cases there are on Linux.
>
> And this number should be purely about minimum preempt timeout, not 
> considering heartbeats. This is because preempt timeout may kick in 
> sooner than stopped heartbeat if the user workload is low priority.
>
And driver is simply hosed in the intervening six months or more that it 
takes for the right people to find the time to do this.

Right now, it is broken. This patch set improves things. Actual numbers 
can be refined later as/when some random use case that we hadn't 
previously thought of pops up. But not fixing the basic problem at all 
until we have an absolutely perfect for all parties solution is 
pointless. Not least because there is no perfect solution. No matter 
what number you pick it is going to be wrong for someone.

2.5s, 7.5s, X.Ys, I really don't care. 2.5s is a number you seem to have 
picked out of the air totally at random, or maybe based on it being the 
heartbeat period (except that you keep arguing that basing tP on tH is 
wrong). 7.5s is a number that has been in active use for a lot of 
testing for quite some time - KMD CI, UMD CI, E2E, etc. But either way, 
the initial number is almost irrelevant as long as it is not zero. So 
can we please just get something merged now as a starting point?


> 2)
> Commit message should explain the effect on the worst case time until 
> engine reset.
>
> 3)
> OpenCL/compute should ack the change publicly as well since they acked 
> the disabling of preemption.
This patch set has already been publicly acked by the compute team. See 
the 'acked-by' tag.


>
> 4)
> I really want overflows_type in the first patch.
In the final GuC assignment? Only if it is a BUG_ON. If we get a failure 
there it is an internal driver error and cannot be corrected for. It is 
too late for any plausible range check action.

And if you mean in the the actual helper function with the rest of the 
clamping then you are bleeding internal GuC API structure details into 
non-GuC code. Plus the test would be right next to the 'if (size < 
OFFICIAL_GUC_RANGE_LIMIT)' test which just looks dumb as well as being 
redundant duplication - "if ((value < GUC_LIMIT) && (value < 
NO_WE_REALLY_MEAN_IT_GUC_LIMIT))". And putting it inside the GuC limit 
definition looks even worse "#define LIMIT min(MAX_U32, 100*1000) /* 
because the developer doesn't know how big a u32 is */".

John.

>
> My position is that with the above satisfied it is okay to merge.
>
> Regards,
>
> Tvrtko

Tvrtko Ursulin March 4, 2022, 12:36 p.m. UTC | #17

On 03/03/2022 19:09, John Harrison wrote:

>> Actions:
>>
>> 1)
>> Get a number from compute/OpenCL people for what they say is minimum 
>> preempt timeout for default out of the box Linux desktop experience.
> That would be the one that has been agreed upon by both linux software 
> arch and all UMD teams and has been in use for the past year or more in 
> the internal tree.

What has been used in the internal tree is irrelevant when UMD ack is needed for changes which affect upstream shipping platforms like Tigerlake.

>> This does not mean them running some tests and can't be bothered to 
>> setup up the machine for the extreme use cases, but workloads average 
>> users can realistically be expected to run.
>>
>> Say for instance some image manipulation software which is OpenCL 
>> accelerated or similar. How long unpreemptable sections are expected 
>> there. Or similar. I am not familiar what all OpenCL accelerated use 
>> cases there are on Linux.
>>
>> And this number should be purely about minimum preempt timeout, not 
>> considering heartbeats. This is because preempt timeout may kick in 
>> sooner than stopped heartbeat if the user workload is low priority.
>>
> And driver is simply hosed in the intervening six months or more that it 
> takes for the right people to find the time to do this.

What is hosed? Driver currently contains a patch which was acked by the compute UMD to disable preemption. If it takes six months for compute UMD to give us a different number which works for the open source stack and typical use cases then what can we do.

> Right now, it is broken. This patch set improves things. Actual numbers 
> can be refined later as/when some random use case that we hadn't 
> previously thought of pops up. But not fixing the basic problem at all 
> until we have an absolutely perfect for all parties solution is 
> pointless. Not least because there is no perfect solution. No matter 
> what number you pick it is going to be wrong for someone.
> 
> 2.5s, 7.5s, X.Ys, I really don't care. 2.5s is a number you seem to have 
> picked out of the air totally at random, or maybe based on it being the 
> heartbeat period (except that you keep arguing that basing tP on tH is 
> wrong). 7.5s is a number that has been in active use for a lot of 
> testing for quite some time - KMD CI, UMD CI, E2E, etc. But either way, 
> the initial number is almost irrelevant as long as it is not zero. So 
> can we please just get something merged now as a starting point?
> 
> 
>> 2)
>> Commit message should explain the effect on the worst case time until 
>> engine reset.
>>
>> 3)
>> OpenCL/compute should ack the change publicly as well since they acked 
>> the disabling of preemption.
> This patch set has already been publicly acked by the compute team. See 
> the 'acked-by' tag.

I can't find the reply which contained the ack on the mailing list - do you have a msg-id or an archive link?

Also, ack needs to be against the the fixed timeout patch and not one dependent on the heartbeat interval.

>> 4)
>> I really want overflows_type in the first patch.
> In the final GuC assignment? Only if it is a BUG_ON. If we get a failure 
> there it is an internal driver error and cannot be corrected for. It is 
> too late for any plausible range check action.

If you can find a test which exercises setting insane values to the relevant timeouts and so would hit the problem in our CI then BUG_ON is fine. Otherwise I think BUG_ON is too anti-social and prefer drm_warn or drm_WARN_ON. I don't think adding a test is strictly necessary, if we don't already have one, given how unlikely this is too be hit, but if you insist on a BUG_ON instead of a flavour of a warn then I think we need one so we can catch in CI 100% of the time.
  
> And if you mean in the the actual helper function with the rest of the 
> clamping then you are bleeding internal GuC API structure details into 
> non-GuC code. Plus the test would be right next to the 'if (size < 

In my other reply I exactly described that would be a downside and that I prefer checks at the assignment sites.

Also regarding this comment in the relevant patch:

+	/*
+	 * NB: The GuC API only supports 32bit values. However, the limit is further
+	 * reduced due to internal calculations which would otherwise overflow.
+	 */

I would suggest clarifying this as "The GuC API only supports timeouts up to U32_MAX micro-seconds. However, ...". Given the function at hand deals in milliseconds explicitly calling out that additional scaling factor makes sense I think.

Big picture - it's really still very simple. Public ack for a fixed number and a warn on is not really a lot to ask.

Regards,

Tvrtko

Patch

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;