| Message ID | 20180629075348.27358-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/06/2018 08:53, Chris Wilson wrote: > Our long standing defense against a single client from flooding the > system with requests (causing mempressure and stalls across the whole > system) is to retire the old request on every allocation. (By retiring > the oldest, we try to keep returning requests back to the system in a > steady flow.) This adds an extra step into the submission path that we > can reduce simply by moving it to after the submission itself. > > We already do try to clean up a stale request list after submission, so > always retiring all completed requests fits in as a natural extension. Please add to the commit message what motivated you to do this. Like this benchmark, or something, is now better in this or that respect. Or something. Regards, Tvrtko > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_request.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index a2f7e9358450..4eea7a27291f 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -694,12 +694,6 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx) > if (ret) > goto err_unreserve; > > - /* Move our oldest request to the slab-cache (if not in use!) */ > - rq = list_first_entry(&ce->ring->request_list, typeof(*rq), ring_link); > - if (!list_is_last(&rq->ring_link, &ce->ring->request_list) && > - i915_request_completed(rq)) > - i915_request_retire(rq); > - > /* > * Beware: Dragons be flying overhead. > * > @@ -1110,6 +1104,8 @@ void i915_request_add(struct i915_request *request) > local_bh_enable(); /* Kick the execlists tasklet if just scheduled */ > > /* > + * Move our oldest requests to the slab-cache (if not in use!) > + * > * In typical scenarios, we do not expect the previous request on > * the timeline to be still tracked by timeline->last_request if it > * has been completed. If the completed request is still here, that > @@ -1126,8 +1122,22 @@ void i915_request_add(struct i915_request *request) > * work on behalf of others -- but instead we should benefit from > * improved resource management. (Well, that's the theory at least.) > */ > - if (prev && i915_request_completed(prev)) > - i915_request_retire_upto(prev); > + do { > + prev = list_first_entry(&ring->request_list, > + typeof(*prev), ring_link); > + > + /* > + * Keep the current request, the caller may not be > + * expecting it to be retired (and freed!) immediately, > + * and preserving one request from the client allows us to > + * carry forward frequently reused state onto the next > + * submission. > + */ > + if (prev == request || !i915_request_completed(prev)) > + break; > + > + i915_request_retire(prev); > + } while (1); > } > > static unsigned long local_clock_us(unsigned int *cpu) >
Quoting Tvrtko Ursulin (2018-06-29 11:00:30) > > On 29/06/2018 08:53, Chris Wilson wrote: > > Our long standing defense against a single client from flooding the > > system with requests (causing mempressure and stalls across the whole > > system) is to retire the old request on every allocation. (By retiring > > the oldest, we try to keep returning requests back to the system in a > > steady flow.) This adds an extra step into the submission path that we > > can reduce simply by moving it to after the submission itself. > > > > We already do try to clean up a stale request list after submission, so > > always retiring all completed requests fits in as a natural extension. > > Please add to the commit message what motivated you to do this. Like > this benchmark, or something, is now better in this or that respect. Or > something. This was mentioned in commit 9512f985c32d45ab439a210981e0d73071da395a Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Thu Jun 28 21:12:11 2018 +0100 drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd) -Chris
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index a2f7e9358450..4eea7a27291f 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -694,12 +694,6 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx) if (ret) goto err_unreserve; - /* Move our oldest request to the slab-cache (if not in use!) */ - rq = list_first_entry(&ce->ring->request_list, typeof(*rq), ring_link); - if (!list_is_last(&rq->ring_link, &ce->ring->request_list) && - i915_request_completed(rq)) - i915_request_retire(rq); - /* * Beware: Dragons be flying overhead. * @@ -1110,6 +1104,8 @@ void i915_request_add(struct i915_request *request) local_bh_enable(); /* Kick the execlists tasklet if just scheduled */ /* + * Move our oldest requests to the slab-cache (if not in use!) + * * In typical scenarios, we do not expect the previous request on * the timeline to be still tracked by timeline->last_request if it * has been completed. If the completed request is still here, that @@ -1126,8 +1122,22 @@ void i915_request_add(struct i915_request *request) * work on behalf of others -- but instead we should benefit from * improved resource management. (Well, that's the theory at least.) */ - if (prev && i915_request_completed(prev)) - i915_request_retire_upto(prev); + do { + prev = list_first_entry(&ring->request_list, + typeof(*prev), ring_link); + + /* + * Keep the current request, the caller may not be + * expecting it to be retired (and freed!) immediately, + * and preserving one request from the client allows us to + * carry forward frequently reused state onto the next + * submission. + */ + if (prev == request || !i915_request_completed(prev)) + break; + + i915_request_retire(prev); + } while (1); } static unsigned long local_clock_us(unsigned int *cpu)
Our long standing defense against a single client from flooding the system with requests (causing mempressure and stalls across the whole system) is to retire the old request on every allocation. (By retiring the oldest, we try to keep returning requests back to the system in a steady flow.) This adds an extra step into the submission path that we can reduce simply by moving it to after the submission itself. We already do try to clean up a stale request list after submission, so always retiring all completed requests fits in as a natural extension. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_request.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)