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

John Harrison
2022-03-02T17:41:01Z

On 3/2/2022 03:21, Tvrtko Ursulin wrote:
> On 28/02/2022 19:17, John Harrison wrote:
>> On 2/28/2022 07:32, Tvrtko Ursulin wrote:
>>> On 25/02/2022 19:03, John Harrison wrote:
>>>> On 2/25/2022 10:29, Tvrtko Ursulin wrote:
>>>>> On 25/02/2022 18:01, John Harrison wrote:
>>>>>> On 2/25/2022 09:39, Tvrtko Ursulin wrote:
>>>>>>> On 25/02/2022 17:11, John Harrison wrote:
>>>>>>>> On 2/25/2022 08:36, Tvrtko Ursulin wrote:
>>>>>>>>> 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.
>>>>>>>> I mean that there is no 'stopped heartbeat on engine XXX' when
>>>>>>>> i915 is not handling the recovery part of the process.
>>>>>>>
>>>>>>> Hmmmm?
>>>>>>>
>>>>>>> static void
>>>>>>> reset_engine(struct intel_engine_cs *engine, struct i915_request
>>>>>>> *rq)
>>>>>>> {
>>>>>>>     if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
>>>>>>>         show_heartbeat(rq, engine);
>>>>>>>
>>>>>>>     if (intel_engine_uses_guc(engine))
>>>>>>>         /*
>>>>>>>          * GuC itself is toast or GuC's hang detection
>>>>>>>          * is disabled. Either way, need to find the
>>>>>>>          * hang culprit manually.
>>>>>>>          */
>>>>>>>         intel_guc_find_hung_context(engine);
>>>>>>>
>>>>>>>     intel_gt_handle_error(engine->gt, engine->mask,
>>>>>>>                   I915_ERROR_CAPTURE,
>>>>>>>                   "stopped heartbeat on %s",
>>>>>>>                   engine->name);
>>>>>>> }
>>>>>>>
>>>>>>> How there is no "stopped hearbeat" in guc mode? From this code
>>>>>>> it certainly looks there is.
>>>>>> Only when the GuC is toast and it is no longer an engine reset
>>>>>> but a full GT reset that is required. So technically, it is not a
>>>>>> 'stopped heartbeat on engine XXX' it is 'stopped heartbeat on GT#'.
>>>>>>
>>>>>>>
>>>>>>> You say below heartbeats are going in GuC mode. Now I totally
>>>>>>> don't understand how they are going but there is allegedly no
>>>>>>> "stopped hearbeat".
>>>>>> Because if GuC is handling the detection and recovery then i915
>>>>>> will not reach that point. GuC will do the engine reset and start
>>>>>> scheduling the next context before the heartbeat period expires.
>>>>>> So the notification will be a G2H about a specific context being
>>>>>> reset rather than the i915 notification about a stopped heartbeat.
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>     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?
>>>>>>>> i915 sends down the per engine pulse.
>>>>>>>> GuC schedules the pulse
>>>>>>>> GuC attempts to pre-empt the currently active context
>>>>>>>> GuC detects the pre-emption timeout
>>>>>>>> GuC resets the engine
>>>>>>>>
>>>>>>>> The fundamental process is exactly the same as in execlist
>>>>>>>> mode. It's just that the above blocks of code (calls to
>>>>>>>> intel_gt_handle_error and such) are now inside the GuC not i915.
>>>>>>>>
>>>>>>>> Without the heartbeat going ping, there would be no context
>>>>>>>> switching and thus no pre-emption, no pre-emption timeout and
>>>>>>>> so no hang and reset recovery. And GuC cannot sent pulses by
>>>>>>>> itself - it has no ability to generate context workloads. So we
>>>>>>>> need i915 to send the pings and to gradually raise their
>>>>>>>> priority. But the back half of the heartbeat code is now inside
>>>>>>>> the GuC. It will simply never reach the i915 side timeout if
>>>>>>>> GuC is doing the recovery (unless the heartbeat's final period
>>>>>>>> is too short). We should only reach the i915 side timeout if
>>>>>>>> GuC itself is toast. At which point we need the full GT reset
>>>>>>>> to recover the GuC.
>>>>>>>
>>>>>>> If workload is not preempting and reset does not work, like
>>>>>>> engine is truly stuck, does the current code hit "stopped
>>>>>>> heartbeat" or not in GuC mode?
>>>>>> Hang on, where did 'reset does not work' come into this?
>>>>>>
>>>>>> If GuC is alive and the hardware is not broken then no, it won't.
>>>>>> That's the whole point. GuC does the detection and recovery. The
>>>>>> KMD will never reach 'stopped heartbeat'.
>>>>>>
>>>>>> If the hardware is broken and the reset does not work then GuC
>>>>>> will send a 'failed reset' notification to the KMD. The KMD
>>>>>> treats that as a major error and immediately does a full GT
>>>>>> reset. So there is still no 'stopped heartbeat'.
>>>>>>
>>>>>> If GuC has died (or a KMD bug has caused sufficient confusion to
>>>>>> make it think the GuC has died) then yes, you will reach that
>>>>>> code. But in that case it is not an engine reset that is
>>>>>> required, it is a full GT reset including a reset of the GuC.
>>>>>
>>>>> Got it, so what is actually wrong is calling intel_gt_handle_error
>>>>> with an engine->mask in GuC mode.
>>>>> intel_engine_hearbeat.c/reset_engine should fork into two (in some
>>>>> way), depending on backend, so in the case of GuC it can call a
>>>>> variant of intel_gt_handle_error which would be explicitly about a
>>>>> full GPU reset only, instead of a sprinkle of
>>>>> intel_uc_uses_guc_submission in that function. Possibly even off
>>>>> load the reset to a single per gt worker, so that if multiple
>>>>> active engines trigger the reset roughly simultaneously, there is
>>>>> only one full GPU reset. And it gets correctly labeled as "dead
>>>>> GuC" or something.
>>>>>
>>>> Sure. Feel free to re-write the reset code yet again. That's
>>>> another exceedingly fragile area of the driver.
>>>
>>>> However, that is unrelated to this patch set.
>>>
>>> It's still okay to talk about improving things in my opinion.
>>> Especially since I think it is the same issue I raised late 2019
>>> during internal review.
>>>
>>> And I don't think it is fair to say that I should go and rewrite it,
>>> since I do not own the GuC backend area. Especially given the above.
>>>
>>> If there is no motivation to improve it now then please at least
>>> track this, if it isn't already, in that internal Jira which was
>>> tracking all the areas of the GuC backend which could be improved.
>>>
>>> I am also pretty sure if the design was cleaner it would have been
>>> more obvious to me, or anyone who happens to stumble there, what the
>>> purpose of intel_engine_heartbeat.c/reset_engine() is in GuC mode.
>>> So we wouldn't have to spend this long explaining things.
>> My point is that redesigning it to be cleaner is not just a GuC task.
>> It means redesigning the entire reset sequence to more compatible
>> with externally handled resets. That's a big task. Sure it can be
>> added to the todo list but it is totally not in the scope of this
>> patch set.
>
> My point was that was something which was raised years ago ("don't
> just shoe-horn, redesign, refactor"). Anyway, stopping flogging of
> this dead horse.. onto below..
>> This is purely about enabling per engine resets again so that end
>> users have a better experience when GPU hangs occur. Or at least,
>> don't have a greatly worse experience on our newest platforms than
>> they did on all the previous ones because we have totally hacked that
>> feature out. And actually getting that feature enabled before we ship
>> too many products and the maintainers/architects decide we are no
>> longer allowed to turn it on because it is now a behaviour change
>> that end users are not expecting. So forever more ADL-P/DG2 are stuck
>> on full GT only resets.
>
> Is any platform with GuC outside force probe already? Either way
> blocking re-addition of engine resets will not be a thing from
> maintainers point of view. Whether or not the fail of not having them
> is a conscious or accidental miss, we certainly want it back ASAP.
>
ADL-P is released and running GuC submission.

John.

> Regards,
>
> Tvrtko