Re: [Intel-gfx] [PATCH 0/3] Improve anti-pre-emption w/a for compute workloads

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 24/02/2022 20:02, John Harrison wrote:
> On 2/23/2022 04:00, Tvrtko Ursulin wrote:
>> On 23/02/2022 02:22, John Harrison wrote:
>>> On 2/22/2022 01:53, Tvrtko Ursulin wrote:
>>>> On 18/02/2022 21:33, John.C.Harrison@Intel.com wrote:
>>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>>
>>>>> Compute workloads are inherently not pre-emptible on current hardware.
>>>>> Thus the pre-emption timeout was disabled as a workaround to prevent
>>>>> unwanted resets. Instead, the hang detection was left to the heartbeat
>>>>> and its (longer) timeout. This is undesirable with GuC submission as
>>>>> the heartbeat is a full GT reset rather than a per engine reset and so
>>>>> is much more destructive. Instead, just bump the pre-emption timeout
>>>>
>>>> Can we have a feature request to allow asking GuC for an engine reset?
>>> For what purpose?
>>
>> To allow "stopped heartbeat" to reset the engine, however..
>>
>>> GuC manages the scheduling of contexts across engines. With virtual 
>>> engines, the KMD has no knowledge of which engine a context might be 
>>> executing on. Even without virtual engines, the KMD still has no 
>>> knowledge of which context is currently executing on any given engine 
>>> at any given time.
>>>
>>> There is a reason why hang detection should be left to the entity 
>>> that is doing the scheduling. Any other entity is second guessing at 
>>> best.
>>>
>>> The reason for keeping the heartbeat around even when GuC submission 
>>> is enabled is for the case where the KMD/GuC have got out of sync 
>>> with either other somehow or GuC itself has just crashed. I.e. when 
>>> no submission at all is working and we need to reset the GuC itself 
>>> and start over.
>>
>> .. I wasn't really up to speed to know/remember heartbeats are nerfed 
>> already in GuC mode.
> Not sure what you mean by that claim. Engine resets are handled by GuC 
> because GuC handles the scheduling. You can't do the former if you 
> aren't doing the latter. However, the heartbeat is still present and is 
> still the watchdog by which engine resets are triggered. As per the rest 
> of the submission process, the hang detection and recovery is split 
> between i915 and GuC.

I meant that "stopped heartbeat on engine XXX" can only do a full GPU reset on GuC.

	intel_gt_handle_error(engine->gt, engine->mask,
			      I915_ERROR_CAPTURE,
			      "stopped heartbeat on %s",
			      engine->name);

intel_gt_handle_error:

	/*
	 * Try engine reset when available. We fall back to full reset if
	 * single reset fails.
	 */
	if (!intel_uc_uses_guc_submission(&gt->uc) &&
	    intel_has_reset_engine(gt) && !intel_gt_is_wedged(gt)) {
		local_bh_disable();
		for_each_engine_masked(engine, gt, engine_mask, tmp) {

You said "However, the heartbeat is still present and is still the watchdog by which engine resets are triggered", now I don't know what you meant by this. It actually triggers a single engine reset in GuC mode? Where in code does that happen if this block above shows it not taking the engine reset path?

>> I am not sure it was the best way since full reset penalizes everyone 
>> for one hanging engine. Better question would be why leave heartbeats 
>> around to start with with GuC? If you want to use it to health check 
>> GuC, as you say, maybe just send a CT message and expect replies? Then 
>> full reset would make sense. It also achieves the goal of not 
>> seconding guessing the submission backend you raise.
> Adding yet another hang detection mechanism is yet more complication and 
> is unnecessary when we already have one that works well enough. As 
> above, the heartbeat is still required for sending the pulses that cause 
> pre-emptions and so let GuC detect hangs. It also provides a fallback 
> against a dead GuC by default. So why invent yet another wheel?

Lets first clarify the previous block to make sure there aren't any misunderstandings there.

Regards,

Tvrtko

>> Like it is now, and the need for this series demonstrates it, the 
>> whole thing has a pretty poor "impedance" match. Not even sure what 
>> intel_guc_find_hung_context is doing in intel_engine_hearbeat.c - why 
>> is that not in intel_gt_handle_error at least? Why is hearbeat code 
>> special and other callers of intel_gt_handle_error don't need it?
> There is no guilty context if the reset was triggered via debugfs or 
> similar. And as stated ad nauseam, i915 is no longer handling the 
> scheduling and so cannot make assumptions about what is or is not 
> running on the hardware at any given time. And obviously, if the reset 
> initiated by GuC itself then i915 should not be second guessing the 
> guilty context as the GuC notification has already told us who was 
> responsible.
> 
> And to be clear, the 'poor impedance match' is purely because we don't 
> have mid-thread pre-emption and so need a stupidly huge timeout on 
> compute capable engines. Whereas, we don't want a total heatbeat timeout 
> of a minute or more. That is the impedance mis-match. If the 640ms was 
> acceptable for RCS then none of this hacky timeout algorithm mush would 
> be needed.
> 
> John.
> 
> 
>>
>> Regards,
>>
>> Tvrtko
>