Force preemption

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index beb81f13a3cc..cec4e1653daf 100644

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 		process_csb(engine);
 
 	if (!execlists_is_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT))

Signature · 1 line

2.16.2

Quoted 7 lines

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 1012

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -1

index beb81f13a3cc..cec4e1653daf 100644
@@ -1009,7 +1009,7 @@ static void execlists_submission_tasklet(unsigned long data)
       * imposing the cost of a locked atomic transaction when submitting a
       * new request (outside of the context-switch interrupt).
       */
-     if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
+     while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))

Assuming that this accidentally went missing in the refactor. Chris?

Quoting Jeff McGee (2018-03-21 17:33:04)

Quoted 8 lines · On Wed, Mar 21, 2018 at 10:26:23AM -0700, [email protected] wrote:

Quoted 7 lines

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 11 lines

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 1012

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -1

index beb81f13a3cc..cec4e1653daf 100644
@@ -1009,7 +1009,7 @@ static void execlists_submission_tasklet(unsigned long data)
       * imposing the cost of a locked atomic transaction when submitting a
       * new request (outside of the context-switch interrupt).
       */
-     if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
+     while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))

Assuming that this accidentally went missing in the refactor. Chris?

No. process_csb became a do{} while. The caller did a test_bit to avoid the function call for normal rescheduling paths.
-Chris

Quoted 21 lines

Quoting Jeff McGee (2018-03-21 17:33:04)

Quoted 19 lines · On Wed, Mar 21, 2018 at 10:26:23AM -0700, [email protected] wrote:

Quoted 12 lines

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index beb81f13a3cc..cec4e1653daf 100644

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submission_tasklet(unsigned long data) · line 1012

Quoted 5 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -1

@@ -1009,7 +1009,7 @@ static void execlists_submission_tasklet(unsigned long data)
       * imposing the cost of a locked atomic transaction when submitting a
       * new request (outside of the context-switch interrupt).
       */
-     if (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))
+     while (test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted))

Assuming that this accidentally went missing in the refactor. Chris?

Quoted 3 lines

No. process_csb became a do{} while. The caller did a test_bit to avoid the function call for normal rescheduling paths.
-Chris

But there is no loop in process_csb().

From: Jeff McGee <[email protected]>

Engine reset is fast. A context switch interrupt may be generated just prior to the reset such that the top half handler is racing with reset post-processing. The handler may set the irq_posted bit again after the reset code has cleared it to start fresh. Then the re-enabled tasklet will read the CSB head and tail from MMIO, which will be at the hardware reset values of 0 and 7 respectively, given that no actual CSB event has occurred since the reset. Mayhem then ensues as the tasklet starts processing invalid CSB entries.

We can handle this corner case without adding any new synchronization between the irq top half and the reset work item. The tasklet can just skip CSB processing if the tail is not sane.

Signed-off-by Jeff McGee <[email protected]>

---

Quoted 15 lines

From: Jeff McGee <[email protected]>

Engine reset is fast. A context switch interrupt may be generated just prior to the reset such that the top half handler is racing with reset post-processing. The handler may set the irq_posted bit again after the reset code has cleared it to start fresh. Then the re-enabled tasklet will read the CSB head and tail from MMIO, which will be at the hardware reset values of 0 and 7 respectively, given that no actual CSB event has occurred since the reset. Mayhem then ensues as the tasklet starts processing invalid CSB entries.

We can handle this corner case without adding any new synchronization between the irq top half and the reset work item. The tasklet can just skip CSB processing if the tail is not sane.

Quoted 2 lines

Signed-off-by Jeff McGee <[email protected]>

---

If I drop this patch and substitute https://patchwork.freedesktop.org/patch/211831/
I will see irq_posted get set after reset which causes the first tasklet run to re-process a previous CSB event and hit GEM_BUG_ON that nothing was active.

Quoting Jeff McGee (2018-03-21 17:31:45)

Quoted 18 lines · On Wed, Mar 21, 2018 at 10:26:24AM -0700, [email protected] wrote:

Quoted 17 lines

From: Jeff McGee <[email protected]>

Engine reset is fast. A context switch interrupt may be generated just prior to the reset such that the top half handler is racing with reset post-processing. The handler may set the irq_posted bit again after the reset code has cleared it to start fresh. Then the re-enabled tasklet will read the CSB head and tail from MMIO, which will be at the hardware reset values of 0 and 7 respectively, given that no actual CSB event has occurred since the reset. Mayhem then ensues as the tasklet starts processing invalid CSB entries.

We can handle this corner case without adding any new synchronization between the irq top half and the reset work item. The tasklet can just skip CSB processing if the tail is not sane.

Signed-off-by Jeff McGee <[email protected]>

---

Quoted 4 lines

If I drop this patch and substitute https://patchwork.freedesktop.org/patch/211831/
I will see irq_posted get set after reset which causes the first tasklet run to re-process a previous CSB event and hit GEM_BUG_ON that nothing was active.

However, for reset+sync to be followed by an interrupt is surprising. What more do we need to do after the reset to flush the last interrupt?

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Chris Wilson (5):
drm/i915/execlists: Refactor out complete_preempt_context() drm/i915: Add control flags to i915_handle_error() drm/i915: Move engine reset prepare/finish to backends drm/i915: Split execlists/guc reset prepartions drm/i915/execlists: Flush pending preemption events during reset

Jeff McGee (3):
drm/i915: Fix loop on CSB processing
drm/i915: Skip CSB processing on invalid CSB tail drm/i915: Force preemption to complete via engine reset

  drivers/gpu/drm/i915/i915_debugfs.c              |   4 +-
  drivers/gpu/drm/i915/i915_drv.c                  |  17 +-
  drivers/gpu/drm/i915/i915_drv.h                  |  10 +-
  drivers/gpu/drm/i915/i915_gem.c                  |  69 ++--
  drivers/gpu/drm/i915/i915_gpu_error.h            |   3 +
  drivers/gpu/drm/i915/i915_irq.c                  |  55 +--
  drivers/gpu/drm/i915/i915_params.c               |   3 +
  drivers/gpu/drm/i915/i915_params.h               |   1 +
  drivers/gpu/drm/i915/i915_request.c              |   2 +-

  drivers/gpu/drm/i915/intel_engine_cs.c           |  40 +++
  drivers/gpu/drm/i915/intel_guc_submission.c      |  39 ++
  drivers/gpu/drm/i915/intel_hangcheck.c           |   8 +-
  drivers/gpu/drm/i915/intel_lrc.c                 | 436 ++++++++++++++---------
  drivers/gpu/drm/i915/intel_ringbuffer.c          |  20 +-
  drivers/gpu/drm/i915/intel_ringbuffer.h          |  13 +-
  drivers/gpu/drm/i915/selftests/intel_hangcheck.c |  13 +-
  16 files changed, 469 insertions(+), 264 deletions(-)

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Quoted 23 lines

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoted 7 lines

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

Quoting Jeff McGee (2018-03-22 14:34:58)

Quoted 32 lines · Chris Wilson

Quoted 23 lines

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoted 7 lines

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Quoted 5 lines

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

Third party code is still mediated by our userspace drivers, or are you contemplating scenarios where they talk directly to ioctls? How hostile do we have to contend with, i.e. survive a gpu fork bomb? -Chris

Quoted 23 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoted 7 lines

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

Quoted 32 lines · Jeff McGee

Quoted 23 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoted 7 lines

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Quoted 5 lines

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

My concern is whether it safe to call this force _preemption_, while it is not really expected to work as preemption from the point of view of preempted context. I may be missing some angle here, but I think a better name would include words like maximum request duration or something.

I can see a difference between allowed maximum duration when there is something else pending, and when it isn't, but I don't immediately see that we should consider this distinction for any real benefit?

So should the feature just be "maximum request duration"? This would perhaps make it just a special case of hangcheck, which ignores head progress, or whatever we do in there.

Regards,

Tvrtko

Quoted 51 lines · Tvrtko Ursulin

Quoted 32 lines · Jeff McGee

Quoted 23 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoted 7 lines

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Quoted 5 lines

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

My concern is whether it safe to call this force _preemption_, while it is not really expected to work as preemption from the point of view of preempted context. I may be missing some angle here, but I think a better name would include words like maximum request duration or something.

I can see a difference between allowed maximum duration when there is something else pending, and when it isn't, but I don't immediately see that we should consider this distinction for any real benefit?

Quoted 7 lines

So should the feature just be "maximum request duration"? This would perhaps make it just a special case of hangcheck, which ignores head progress, or whatever we do in there.

Regards,

Tvrtko

I think you might be unclear about how this works. We're not starting a preemption to see if we can cleanly remove a request who has begun to exceed its normal time slice, i.e. hangcheck. This is about bounding the time that a normal preemption can take. So first start preemption in response to higher-priority request arrival, then wait for preemption to complete within a certain amount of time. If it does not, resort to reset.

So it's really "force the resolution of a preemption", shortened to "force preemption".

Quoted 68 lines · Jeff McGee

Quoted 51 lines · Tvrtko Ursulin

Quoted 32 lines · Jeff McGee

Quoted 23 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 17 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 14 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be shared by clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may be more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would be possible to refactor hangcheck to fully incorporate force preemption as another tier of patience (or impatience) with the running request.

Quoted 4 lines

Sorry if it was mentioned elsewhere and I missed it - but does this work only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is we don't care since they are misbehaving?

Quoted 7 lines

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would be better served making sure that the timer never expired in the first place thereby eliminating the need for a forced gpu reset.
-Chris

Quoted 5 lines

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

My concern is whether it safe to call this force _preemption_, while it is not really expected to work as preemption from the point of view of preempted context. I may be missing some angle here, but I think a better name would include words like maximum request duration or something.

I can see a difference between allowed maximum duration when there is something else pending, and when it isn't, but I don't immediately see that we should consider this distinction for any real benefit?

Quoted 7 lines

So should the feature just be "maximum request duration"? This would perhaps make it just a special case of hangcheck, which ignores head progress, or whatever we do in there.

Regards,

Tvrtko

I think you might be unclear about how this works. We're not starting a preemption to see if we can cleanly remove a request who has begun to exceed its normal time slice, i.e. hangcheck. This is about bounding the time that a normal preemption can take. So first start preemption in response to higher-priority request arrival, then wait for preemption to complete within a certain amount of time. If it does not, resort to reset.

Quoted 2 lines

So it's really "force the resolution of a preemption", shortened to "force preemption".

You are right, I veered off in my thinking and ended up with something different. :)

I however still think the name is potentially misleading, since the request/context is not getting preempted. It is getting effectively killed (sooner or later, directly or indirectly).

Maybe that is OK for the specific use case when everything is only broken and not malicious.

In a more general purpose system it would be a bit random when something would work, and when it wouldn't, depending on system setup and even timings.

Hm, maybe you don't even really benefit from the standard three strikes and you are out policy, and for this specific use case, you should just kill it straight away. If it couldn't be preempted once, why pay the penalty any more?

If you don't have it already, devising a solution which blacklists the process (if it creates more contexts), or even a parent (if forking is applicable and implementation feasible), for offenders could also be beneficial.

Regards,

Tvrtko

From: Intel-gfx [mailto:[email protected]] On Behalf Of Jeff McGee
Sent: Thursday, March 22, 2018 12:09 PM
To: Tvrtko Ursulin <[email protected]>
Cc: Kondapally, Kalyan <[email protected]>; intel- [email protected]; [email protected]
Subject: Re: [Intel-gfx] [RFC 0/8] Force preemption

Quoted 6 lines · Tvrtko Ursulin

Quoted 4 lines · Jeff McGee

Quoted 3 lines · Tvrtko Ursulin

Quoted 1 line · Jeff McGee

On Thu, Mar 22, 2018 at 09:28:00AM +0000, Chris Wilson wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 7 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 5 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be

shared by

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may

be

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would

be

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

possible to refactor hangcheck to fully incorporate force

preemption

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

as another tier of patience (or impatience) with the running

request.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Sorry if it was mentioned elsewhere and I missed it - but does this

work

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is

we

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

don't care since they are misbehaving?

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would

be

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

better served making sure that the timer never expired in the first

place

Quoted 58 lines

Quoted 36 lines

Quoted 20 lines

Quoted 8 lines

Quoted 2 lines

thereby eliminating the need for a forced gpu reset. -Chris

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

My concern is whether it safe to call this force _preemption_, while it is not really expected to work as preemption from the point of view of preempted context. I may be missing some angle here, but I think a better name would include words like maximum request duration or something.

I can see a difference between allowed maximum duration when there is something else pending, and when it isn't, but I don't immediately see that we should consider this distinction for any real benefit?

Quoted 7 lines

So should the feature just be "maximum request duration"? This would perhaps make it just a special case of hangcheck, which ignores head progress, or whatever we do in there.

Regards,

Tvrtko

I think you might be unclear about how this works. We're not starting a preemption to see if we can cleanly remove a request who has begun to exceed its normal time slice, i.e. hangcheck. This is about bounding the time that a normal preemption can take. So first start preemption in response to higher-priority request arrival, then wait for preemption to complete within a certain amount of time. If it does not, resort to reset.

Quoted 2 lines

So it's really "force the resolution of a preemption", shortened to "force preemption".

You are right, I veered off in my thinking and ended up with something different. :)

I however still think the name is potentially misleading, since the request/context is not getting preempted. It is getting effectively killed (sooner or later, directly or indirectly).

Maybe that is OK for the specific use case when everything is only broken and not malicious.

In a more general purpose system it would be a bit random when something would work, and when it wouldn't, depending on system setup and even timings.

Hm, maybe you don't even really benefit from the standard three strikes and you are out policy, and for this specific use case, you should just kill it straight away. If it couldn't be preempted once, why pay the penalty any more?

Quoted 8 lines

If you don't have it already, devising a solution which blacklists the process (if it creates more contexts), or even a parent (if forking is applicable and implementation feasible), for offenders could also be beneficial.

Regards,

Tvrtko

Fair enough. There wasn't a lot of deliberation on this name. We referred to it in various ways during development. I think I started using "force preemption" because it was short. "reset to preempt" was another phrase that was used.

The handling of the guilty client/context could be tailored more. Like I said it was easiest to start with the same sort of handling that we have already for hang scenarios. Simple is good when you are rebasing without much hope to upstream. :(

If there was interest in upstreaming this capability, we could certainly incorporate nicely within a refactoring of hangcheck. And then we wouldn't even need a special name for it. The whole thing could be recast as time slice management, where your slice is condition-based. You get unlimited time if no one wants the engine, X time if someone of equal priority wants the engine, and Y time if someone of higher priority wants the engine, etc. Where 'Y' is analogous to the fpreempt_timeout value in my RFC.

Quoted 129 lines

From: Intel-gfx [mailto:[email protected]] On Behalf Of Jeff McGee
Sent: Thursday, March 22, 2018 12:09 PM
To: Tvrtko Ursulin <[email protected]>
Cc: Kondapally, Kalyan <[email protected]>; intel- [email protected]; [email protected]
Subject: Re: [Intel-gfx] [RFC 0/8] Force preemption

Quoted 6 lines · Tvrtko Ursulin

Quoted 4 lines · Jeff McGee

Quoted 3 lines · Tvrtko Ursulin

Quoted 1 line · Jeff McGee

On Thu, Mar 22, 2018 at 09:28:00AM +0000, Chris Wilson wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoting Tvrtko Ursulin (2018-03-22 09:22:55)

Quoted 7 lines · On 21/03/2018 17:26, [email protected] wrote:

Quoted 5 lines

From: Jeff McGee <[email protected]>

Force preemption uses engine reset to enforce a limit on the time that a request targeted for preemption can block. This feature is a requirement in automotive systems where the GPU may be

shared by

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

clients of critically high priority and clients of low priority that may not have been curated to be preemption friendly. There may

be

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

more general applications of this feature. I'm sharing as an RFC to stimulate that discussion and also to get any technical feedback that I can before submitting to the product kernel that needs this. I have developed the patches for ease of rebase, given that this is for the moment considered a non-upstreamable feature. It would

be

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

possible to refactor hangcheck to fully incorporate force

preemption

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

as another tier of patience (or impatience) with the running

request.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Sorry if it was mentioned elsewhere and I missed it - but does this

work

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

only with stateless clients - or in other words, what would happen to stateful clients which would be force preempted? Or the answer is

we

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

don't care since they are misbehaving?

They get notified of being guilty for causing a gpu reset; three strikes and they are out (banned from using the gpu) using the current rules. This is a very blunt hammer that requires the rest of the system to be robust; one might argue time spent making the system robust would

be

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

better served making sure that the timer never expired in the first

place

Quoted 58 lines

Quoted 36 lines

Quoted 20 lines

Quoted 8 lines

Quoted 2 lines

thereby eliminating the need for a forced gpu reset. -Chris

Yes, for simplication the policy applied to force preempted contexts is the same as for hanging contexts. It is known that this feature should not be required in a fully curated system. It's a requirement if end user will be alllowed to install 3rd party apps to run in the non-critical domain.

My concern is whether it safe to call this force _preemption_, while it is not really expected to work as preemption from the point of view of preempted context. I may be missing some angle here, but I think a better name would include words like maximum request duration or something.

I can see a difference between allowed maximum duration when there is something else pending, and when it isn't, but I don't immediately see that we should consider this distinction for any real benefit?

Quoted 7 lines

So should the feature just be "maximum request duration"? This would perhaps make it just a special case of hangcheck, which ignores head progress, or whatever we do in there.

Regards,

Tvrtko

I think you might be unclear about how this works. We're not starting a preemption to see if we can cleanly remove a request who has begun to exceed its normal time slice, i.e. hangcheck. This is about bounding the time that a normal preemption can take. So first start preemption in response to higher-priority request arrival, then wait for preemption to complete within a certain amount of time. If it does not, resort to reset.

Quoted 2 lines

So it's really "force the resolution of a preemption", shortened to "force preemption".

You are right, I veered off in my thinking and ended up with something different. :)

I however still think the name is potentially misleading, since the request/context is not getting preempted. It is getting effectively killed (sooner or later, directly or indirectly).

Maybe that is OK for the specific use case when everything is only broken and not malicious.

In a more general purpose system it would be a bit random when something would work, and when it wouldn't, depending on system setup and even timings.

Hm, maybe you don't even really benefit from the standard three strikes and you are out policy, and for this specific use case, you should just kill it straight away. If it couldn't be preempted once, why pay the penalty any more?

Quoted 8 lines

If you don't have it already, devising a solution which blacklists the process (if it creates more contexts), or even a parent (if forking is applicable and implementation feasible), for offenders could also be beneficial.

Regards,

Tvrtko

Fair enough. There wasn't a lot of deliberation on this name. We referred to it in various ways during development. I think I started using "force preemption" because it was short. "reset to preempt" was another phrase that was used.

The handling of the guilty client/context could be tailored more. Like I said it was easiest to start with the same sort of handling that we have already for hang scenarios. Simple is good when you are rebasing without much hope to upstream. :(

Quoted 9 lines

If there was interest in upstreaming this capability, we could certainly incorporate nicely within a refactoring of hangcheck. And then we wouldn't even need a special name for it. The whole thing could be recast as time slice management, where your slice is condition-based. You get unlimited time if no one wants the engine, X time if someone of equal priority wants the engine, and Y time if someone of higher priority wants the engine, etc. Where 'Y' is analogous to the fpreempt_timeout value in my RFC.

On the subject of it being "a bit random when something would work":
Currently it's the well behaved app (and everything else behind it) that pays the price for the badly-behaved umd being a bad citizen. 12s is a really long time for the GUI to freeze before a bad context can be nuked.

Yes, with enforced pre-emption latency, the bad umd pays a price in that, if it's lucky, it will run to completion. If unlucky it gets nuked. But it's a bad umd at the end of the day. And we should find it and fix it.

With fair-scheduling, this becomes even more important - all umd's need to ensure that they are capable of pre-empting to the finest granularity supported by the h/w for their respective workloads. If they don't they really should be obliterated (aka detected and fixed).

The big benefit I see with the new approach (vs TDR) is that there's actually no need to enforce the (slightly arbitrarily determined 3x4s timeout) for a context at all, providing it plays nicely. If you want to run a really long GPGPU workload, do so by all means, just don't hog the GPU.

BTW, the same context banning should be applied to a context nuked by forced-preemption as with TDR. That's the intention at least. This is just an RFC right now.
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

So far nobody has been succesful in selling this to the userspace compositors (the most likely user) or has somebody?