Re: [Intel-gfx] [PATCH v2] drm/i915: Use per-engine request pools
On Thu, 2020-04-02 at 13:16 +0100, Chris Wilson wrote:
> Add a per-engine request mempool so that we should always have a couple
> of requests available for powermanagement allocations from tricky
> contexts. These reserves are expected to be only used for kernel
> contexts when barriers must be emitted [almost] without fail.
>
> When using the mempool, requests are first allocated from the global
> slab cache (utilising all the per-cpu lockless freelists and caches) and
> only if that is empty and cannot be filled under the gfp_t do we
> fallback to using the per-engine cache of recently freed requests. For
> our use cases, this will never be empty for long as there will always be
> at least the previous powermanagent request to reuse.
>
> The downside is that this is quite a bulky addition and abstraction to
> use, but it will ensure that we never fail to park the engine due to
> oom.
>
> v2: Only use the mempool for nonblocking allocations which are not
> expected to fail.
LGTM. Thanks for addressing the issue.
Reviewed-by: Janusz Krzysztofik
Thanks,
Janusz
>
> Signed-off-by: Chris Wilson
> Cc: Janusz Krzysztofik
> Cc: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/gt/intel_engine.h | 3 +++
> drivers/gpu/drm/i915/gt/intel_engine_cs.c| 14 ++
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 4
> drivers/gpu/drm/i915/gt/intel_lrc.c | 3 +++
> drivers/gpu/drm/i915/i915_request.c | 20 +---
> drivers/gpu/drm/i915/i915_request.h | 2 ++
> 6 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> b/drivers/gpu/drm/i915/gt/intel_engine.h
> index b469de0dd9b6..c1159bd17989 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -324,6 +324,9 @@ void intel_engine_init_active(struct intel_engine_cs
> *engine,
> #define ENGINE_MOCK 1
> #define ENGINE_VIRTUAL 2
>
> +void intel_engine_init_request_pool(struct intel_engine_cs *engine);
> +void intel_engine_fini_request_pool(struct intel_engine_cs *engine);
> +
> static inline bool
> intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
> {
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 843cb6f2f696..16bbd9174937 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -602,6 +602,18 @@ static int init_status_page(struct intel_engine_cs
> *engine)
> return ret;
> }
>
> +void intel_engine_init_request_pool(struct intel_engine_cs *engine)
> +{
> + mempool_init_slab_pool(&engine->request_pool,
> +INTEL_ENGINE_REQUEST_POOL_RESERVED,
> +i915_request_slab_cache());
> +}
> +
> +void intel_engine_fini_request_pool(struct intel_engine_cs *engine)
> +{
> + mempool_exit(&engine->request_pool);
> +}
> +
> static int engine_setup_common(struct intel_engine_cs *engine)
> {
> int err;
> @@ -617,6 +629,7 @@ static int engine_setup_common(struct intel_engine_cs
> *engine)
> intel_engine_init_execlists(engine);
> intel_engine_init_cmd_parser(engine);
> intel_engine_init__pm(engine);
> + intel_engine_init_request_pool(engine);
> intel_engine_init_retire(engine);
>
> intel_engine_pool_init(&engine->pool);
> @@ -817,6 +830,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs
> *engine)
> cleanup_status_page(engine);
>
> intel_engine_fini_retire(engine);
> + intel_engine_fini_request_pool(engine);
> intel_engine_pool_fini(&engine->pool);
> intel_engine_fini_breadcrumbs(engine);
> intel_engine_cleanup_cmd_parser(engine);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 80cdde712842..0db03215127b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -308,6 +309,9 @@ struct intel_engine_cs {
> struct list_head hold; /* ready requests, but on hold */
> } active;
>
> + mempool_t request_pool; /* keep some in reserve for powermanagement */
> +#define INTEL_ENGINE_REQUEST_POOL_RESERVED 2
> +
> struct llist_head barrier_tasks;
>
> struct intel_context *kernel_context; /* pinned */
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 3479cda37fdc..afc9107e5d04 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -4892,6 +4892,8 @@ static void virtual_context_destroy(struct kref *kref)
> __execlists_context_fini(&ve->context);
> intel_context_fini(&ve->context);
>
> + intel_engine_fini_request_pool(&ve->base);
[Intel-gfx] [PATCH v2] drm/i915: Use per-engine request pools
Add a per-engine request mempool so that we should always have a couple
of requests available for powermanagement allocations from tricky
contexts. These reserves are expected to be only used for kernel
contexts when barriers must be emitted [almost] without fail.
When using the mempool, requests are first allocated from the global
slab cache (utilising all the per-cpu lockless freelists and caches) and
only if that is empty and cannot be filled under the gfp_t do we
fallback to using the per-engine cache of recently freed requests. For
our use cases, this will never be empty for long as there will always be
at least the previous powermanagent request to reuse.
The downside is that this is quite a bulky addition and abstraction to
use, but it will ensure that we never fail to park the engine due to
oom.
v2: Only use the mempool for nonblocking allocations which are not
expected to fail.
Signed-off-by: Chris Wilson
Cc: Janusz Krzysztofik
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/gt/intel_engine.h | 3 +++
drivers/gpu/drm/i915/gt/intel_engine_cs.c| 14 ++
drivers/gpu/drm/i915/gt/intel_engine_types.h | 4
drivers/gpu/drm/i915/gt/intel_lrc.c | 3 +++
drivers/gpu/drm/i915/i915_request.c | 20 +---
drivers/gpu/drm/i915/i915_request.h | 2 ++
6 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index b469de0dd9b6..c1159bd17989 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -324,6 +324,9 @@ void intel_engine_init_active(struct intel_engine_cs
*engine,
#define ENGINE_MOCK1
#define ENGINE_VIRTUAL 2
+void intel_engine_init_request_pool(struct intel_engine_cs *engine);
+void intel_engine_fini_request_pool(struct intel_engine_cs *engine);
+
static inline bool
intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
{
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 843cb6f2f696..16bbd9174937 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -602,6 +602,18 @@ static int init_status_page(struct intel_engine_cs *engine)
return ret;
}
+void intel_engine_init_request_pool(struct intel_engine_cs *engine)
+{
+ mempool_init_slab_pool(&engine->request_pool,
+ INTEL_ENGINE_REQUEST_POOL_RESERVED,
+ i915_request_slab_cache());
+}
+
+void intel_engine_fini_request_pool(struct intel_engine_cs *engine)
+{
+ mempool_exit(&engine->request_pool);
+}
+
static int engine_setup_common(struct intel_engine_cs *engine)
{
int err;
@@ -617,6 +629,7 @@ static int engine_setup_common(struct intel_engine_cs
*engine)
intel_engine_init_execlists(engine);
intel_engine_init_cmd_parser(engine);
intel_engine_init__pm(engine);
+ intel_engine_init_request_pool(engine);
intel_engine_init_retire(engine);
intel_engine_pool_init(&engine->pool);
@@ -817,6 +830,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs
*engine)
cleanup_status_page(engine);
intel_engine_fini_retire(engine);
+ intel_engine_fini_request_pool(engine);
intel_engine_pool_fini(&engine->pool);
intel_engine_fini_breadcrumbs(engine);
intel_engine_cleanup_cmd_parser(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 80cdde712842..0db03215127b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -308,6 +309,9 @@ struct intel_engine_cs {
struct list_head hold; /* ready requests, but on hold */
} active;
+ mempool_t request_pool; /* keep some in reserve for powermanagement */
+#define INTEL_ENGINE_REQUEST_POOL_RESERVED 2
+
struct llist_head barrier_tasks;
struct intel_context *kernel_context; /* pinned */
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 3479cda37fdc..afc9107e5d04 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -4892,6 +4892,8 @@ static void virtual_context_destroy(struct kref *kref)
__execlists_context_fini(&ve->context);
intel_context_fini(&ve->context);
+ intel_engine_fini_request_pool(&ve->base);
+
kfree(ve->bonds);
kfree(ve);
}
@@ -5203,6 +5205,7 @@ intel_execlists_create_virtual(struct intel_engine_cs
**siblings,
intel_engine_init_active(&ve->base, ENGINE_VIRTUAL);
intel_engine_init_breadcrumbs(&ve->base);
intel_engine_init_execlists(&ve->base);
+ intel_engine_init_request_pool(&ve->base
