[PATCH] drm/i915: Allocate atomically in execbuf path

[PATCH] drm/i915: Allocate atomically in execbuf path

[Ben Widawsky]

If we end up calling the shrinker, which in turn requires the OOM
killer, we may end up infinitely waiting for a process to die if the OOM
chooses. The case that this prevents occurs in execbuf. The forked
variants of gem_evict_everything is a good way to hit it. This is
exacerbated by Daniel's recent patch to give OOM precedence to the GEM
tests.

It's a twisted form of a deadlock.

What occurs is the following (assume just 2 procs)
1. proc A gets to execbuf while out of memory, gets struct_mutex.
2. OOM killer comes in and chooses proc B
3. proc B closes it's fds, which requires struct mutex, blocks
4, OOM killer waits for B to die before killing another process (this
part is speculative)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fb2d548..a60894d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1842,12 +1842,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
 	BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
 
-	st = kmalloc(sizeof(*st), GFP_KERNEL);
+	st = kmalloc(sizeof(*st), GFP_NOWAIT);
 	if (st == NULL)
 		return -ENOMEM;
 
 	page_count = obj->base.size / PAGE_SIZE;
-	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
+	if (sg_alloc_table(st, page_count, GFP_NOWAIT)) {
 		kfree(st);
 		return -ENOMEM;
 	}
-- 
1.8.4.2

Re: [PATCH] drm/i915: Allocate atomically in execbuf path

[Ben Widawsky]

On Tue, Nov 26, 2013 at 04:55:50PM -0800, Ben Widawsky wrote:
> If we end up calling the shrinker, which in turn requires the OOM
> killer, we may end up infinitely waiting for a process to die if the OOM
> chooses. The case that this prevents occurs in execbuf. The forked
> variants of gem_evict_everything is a good way to hit it. This is
> exacerbated by Daniel's recent patch to give OOM precedence to the GEM
> tests.
> 
> It's a twisted form of a deadlock.
> 
> What occurs is the following (assume just 2 procs)
> 1. proc A gets to execbuf while out of memory, gets struct_mutex.
> 2. OOM killer comes in and chooses proc B
> 3. proc B closes it's fds, which requires struct mutex, blocks
> 4, OOM killer waits for B to die before killing another process (this
> part is speculative)
> 

It appears that by itself, this patch is insufficient to prevent the
failure when run in piglit. Before I go on a wild goose chase of finding
all allocations, maybe I'll give people a chance to chime in. The
symptom is the same always, OOM, procs can't die because struct_mutex is
held.

> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fb2d548..a60894d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1842,12 +1842,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
>  	BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>  
> -	st = kmalloc(sizeof(*st), GFP_KERNEL);
> +	st = kmalloc(sizeof(*st), GFP_NOWAIT);
>  	if (st == NULL)
>  		return -ENOMEM;
>  
>  	page_count = obj->base.size / PAGE_SIZE;
> -	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> +	if (sg_alloc_table(st, page_count, GFP_NOWAIT)) {
>  		kfree(st);
>  		return -ENOMEM;
>  	}
> -- 
> 1.8.4.2
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Allocate atomically in execbuf path

[Ben Widawsky]

On Tue, Nov 26, 2013 at 05:10:38PM -0800, Ben Widawsky wrote:
> On Tue, Nov 26, 2013 at 04:55:50PM -0800, Ben Widawsky wrote:
> > If we end up calling the shrinker, which in turn requires the OOM
> > killer, we may end up infinitely waiting for a process to die if the OOM
> > chooses. The case that this prevents occurs in execbuf. The forked
> > variants of gem_evict_everything is a good way to hit it. This is
> > exacerbated by Daniel's recent patch to give OOM precedence to the GEM
> > tests.
> > 
> > It's a twisted form of a deadlock.
> > 
> > What occurs is the following (assume just 2 procs)
> > 1. proc A gets to execbuf while out of memory, gets struct_mutex.
> > 2. OOM killer comes in and chooses proc B
> > 3. proc B closes it's fds, which requires struct mutex, blocks
> > 4, OOM killer waits for B to die before killing another process (this
> > part is speculative)
> > 
> 
> It appears that by itself, this patch is insufficient to prevent the
> failure when run in piglit. Before I go on a wild goose chase of finding
> all allocations, maybe I'll give people a chance to chime in. The
> symptom is the same always, OOM, procs can't die because struct_mutex is
> held.
> 

too late on the goose chase. vma allocation was the missing bit.

> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index fb2d548..a60894d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1842,12 +1842,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
> >  	BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
> >  
> > -	st = kmalloc(sizeof(*st), GFP_KERNEL);
> > +	st = kmalloc(sizeof(*st), GFP_NOWAIT);
> >  	if (st == NULL)
> >  		return -ENOMEM;
> >  
> >  	page_count = obj->base.size / PAGE_SIZE;
> > -	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> > +	if (sg_alloc_table(st, page_count, GFP_NOWAIT)) {
> >  		kfree(st);
> >  		return -ENOMEM;
> >  	}
> > -- 
> > 1.8.4.2
> > 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Allocate atomically in execbuf path

[Ben Widawsky]

On Tue, Nov 26, 2013 at 04:55:50PM -0800, Ben Widawsky wrote:
> If we end up calling the shrinker, which in turn requires the OOM
> killer, we may end up infinitely waiting for a process to die if the OOM
> chooses. The case that this prevents occurs in execbuf. The forked
> variants of gem_evict_everything is a good way to hit it. This is
> exacerbated by Daniel's recent patch to give OOM precedence to the GEM
> tests.
> 
> It's a twisted form of a deadlock.
> 
> What occurs is the following (assume just 2 procs)
> 1. proc A gets to execbuf while out of memory, gets struct_mutex.
> 2. OOM killer comes in and chooses proc B
> 3. proc B closes it's fds, which requires struct mutex, blocks
> 4, OOM killer waits for B to die before killing another process (this
> part is speculative)
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I'd still like to know if I am crazy, but I'm now trying to defer the
stuff we do on file close without using any allocs. Just an update...

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Allocate atomically in execbuf path

[Daniel Vetter]

On Wed, Nov 27, 2013 at 5:23 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Tue, Nov 26, 2013 at 04:55:50PM -0800, Ben Widawsky wrote:
>> If we end up calling the shrinker, which in turn requires the OOM
>> killer, we may end up infinitely waiting for a process to die if the OOM
>> chooses. The case that this prevents occurs in execbuf. The forked
>> variants of gem_evict_everything is a good way to hit it. This is
>> exacerbated by Daniel's recent patch to give OOM precedence to the GEM
>> tests.
>>
>> It's a twisted form of a deadlock.
>>
>> What occurs is the following (assume just 2 procs)
>> 1. proc A gets to execbuf while out of memory, gets struct_mutex.
>> 2. OOM killer comes in and chooses proc B
>> 3. proc B closes it's fds, which requires struct mutex, blocks
>> 4, OOM killer waits for B to die before killing another process (this
>> part is speculative)
>>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
> I'd still like to know if I am crazy, but I'm now trying to defer the
> stuff we do on file close without using any allocs. Just an update...

Sound's intrigueing, but tbh I don't really have clue about things.
What about adding the relevant stuck task backtraces to the patch and
submitting this to a wider audience (lkml, mm-devel) as an akpm-probe?
The more botched the patch, the better the probe usually.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Allocate atomically in execbuf path

[Ben Widawsky]

On Tue, Nov 26, 2013 at 08:23:46PM -0800, Ben Widawsky wrote:
> On Tue, Nov 26, 2013 at 04:55:50PM -0800, Ben Widawsky wrote:
> > If we end up calling the shrinker, which in turn requires the OOM
> > killer, we may end up infinitely waiting for a process to die if the OOM
> > chooses. The case that this prevents occurs in execbuf. The forked
> > variants of gem_evict_everything is a good way to hit it. This is
> > exacerbated by Daniel's recent patch to give OOM precedence to the GEM
> > tests.
> > 
> > It's a twisted form of a deadlock.
> > 
> > What occurs is the following (assume just 2 procs)
> > 1. proc A gets to execbuf while out of memory, gets struct_mutex.
> > 2. OOM killer comes in and chooses proc B
> > 3. proc B closes it's fds, which requires struct mutex, blocks
> > 4, OOM killer waits for B to die before killing another process (this
> > part is speculative)
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I'd still like to know if I am crazy, but I'm now trying to defer the
> stuff we do on file close without using any allocs. Just an update...
> 

workqueue still has similar problems. It could be because deferring the
context cleanup means we don't actually free much space, and so the OOM
isn't enough, or [more likely] something else is going on.

Maybe it's my bug. I am really out of ideas at the moment. The system
just becomes unresponsive after all threads end up blocked waiting for
struct mutex. I know I'd seen such problems in the past with
gem_evict_everything, but this time around I seem to be the sole cause
(and not all my machines hit it).

Sorry for the noise - just totally burning out on this one.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: Optionally defer context closing

[Ben Widawsky]

This patch doesn't seem to be the panacea that I had hoped, although
I've yet to thoroughly test if it actually improves some of the other
tests which caused trouble.

What occurs is the following (assume just 2 procs)
1. proc A gets to execbuf while out of memory, gets struct_mutex.
2. OOM killer comes in and chooses proc B
3. proc B closes it's fds, which requires struct mutex, blocks
4, OOM killer waits for B to die before killing another process (this
part is speculative)

In order to let the OOM complete, we defer processing the context
destruction parts which are those that require struct_mutex.

This patch has not really been cleaned up, I am only posting it for
posterity. (Several of the hunks are only relevant to full PPGTT patches
which are not merged).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  6 ++-
 drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 68 ++++++++++++++++++++++++---------
 4 files changed, 61 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 835a43e..e6b5f3e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1717,11 +1717,13 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 		struct drm_i915_file_private *file_priv = file->driver_priv;
 		struct i915_hw_ppgtt *pvt_ppgtt;
 
-		pvt_ppgtt = ctx_to_ppgtt(file_priv->private_default_ctx);
+		pvt_ppgtt =
+			ctx_to_ppgtt(file_priv->ctx_info->private_default_ctx);
 		seq_printf(m, "proc: %s\n",
 			   get_pid_task(file->pid, PIDTYPE_PID)->comm);
 		seq_puts(m, "  default context:\n");
-		idr_for_each(&file_priv->context_idr, per_file_ctx, m);
+		idr_for_each(&file_priv->ctx_info->context_idr, per_file_ctx,
+			     m);
 	}
 	seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6fdab40..931fc42 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1735,15 +1735,18 @@ struct drm_i915_gem_request {
 
 struct drm_i915_file_private {
 	struct drm_i915_private *dev_priv;
-
 	struct {
 		spinlock_t lock;
 		struct list_head request_list;
 		struct delayed_work idle_work;
 	} mm;
-	struct idr context_idr;
+	struct i915_gem_ctx_info {
+		struct drm_i915_private *dev_priv;
+		struct idr context_idr;
+		struct work_struct close_work;
+		struct i915_hw_context *private_default_ctx;
+	} *ctx_info;
 
-	struct i915_hw_context *private_default_ctx;
 	atomic_t rps_wait_boost;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6312d61..a32f6df 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2326,7 +2326,7 @@ static void i915_set_reset_status(struct intel_ring_buffer *ring,
 	if (request->ctx && request->ctx->id != DEFAULT_CONTEXT_ID)
 		hs = &request->ctx->hang_stats;
 	else if (request->file_priv)
-		hs = &request->file_priv->private_default_ctx->hang_stats;
+		hs = &request->file_priv->ctx_info->private_default_ctx->hang_stats;
 
 	if (hs) {
 		if (guilty) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f55f0a9..770b394 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -209,8 +209,8 @@ __create_hw_context(struct drm_device *dev,
 	if (file_priv == NULL)
 		return ctx;
 
-	ret = idr_alloc(&file_priv->context_idr, ctx, DEFAULT_CONTEXT_ID, 0,
-			GFP_KERNEL);
+	ret = idr_alloc(&file_priv->ctx_info->context_idr,
+			ctx, DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
 	if (ret < 0)
 		goto err_out;
 
@@ -481,29 +481,54 @@ static int context_idr_cleanup(int id, void *p, void *data)
 	return 0;
 }
 
+static void i915_gem_context_close_work_handler(struct work_struct *work)
+{
+	struct i915_gem_ctx_info *ctx_info = container_of(work,
+							  struct i915_gem_ctx_info,
+							  close_work);
+	struct drm_i915_private *dev_priv = ctx_info->dev_priv;
+
+	mutex_lock(&dev_priv->dev->struct_mutex);
+	idr_for_each(&ctx_info->context_idr, context_idr_cleanup, NULL);
+	i915_gem_context_unreference(ctx_info->private_default_ctx);
+	idr_destroy(&ctx_info->context_idr);
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+
+	kfree(ctx_info);
+}
+
 int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	file_priv->ctx_info = kzalloc(sizeof(*file_priv->ctx_info), GFP_KERNEL);
+	if (!file_priv->ctx_info)
+		return -ENOMEM;
+
+	file_priv->ctx_info->dev_priv = file_priv->dev_priv;
+	INIT_WORK(&file_priv->ctx_info->close_work,
+		  i915_gem_context_close_work_handler);
+
 	if (!HAS_HW_CONTEXTS(dev)) {
 		/* Cheat for hang stats */
-		file_priv->private_default_ctx =
+		file_priv->ctx_info->private_default_ctx =
 			kzalloc(sizeof(struct i915_hw_context), GFP_KERNEL);
-		file_priv->private_default_ctx->vm = &dev_priv->gtt.base;
+		file_priv->ctx_info->private_default_ctx->vm =
+			&dev_priv->gtt.base;
 		return 0;
 	}
 
-	idr_init(&file_priv->context_idr);
+	idr_init(&file_priv->ctx_info->context_idr);
 
 	mutex_lock(&dev->struct_mutex);
-	file_priv->private_default_ctx =
+	file_priv->ctx_info->private_default_ctx =
 		i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev));
 	mutex_unlock(&dev->struct_mutex);
 
-	if (IS_ERR(file_priv->private_default_ctx)) {
-		idr_destroy(&file_priv->context_idr);
-		return PTR_ERR(file_priv->private_default_ctx);
+	if (IS_ERR(file_priv->ctx_info->private_default_ctx)) {
+		idr_destroy(&file_priv->ctx_info->context_idr);
+		return PTR_ERR(file_priv->ctx_info->private_default_ctx);
 	}
 
 	return 0;
@@ -511,27 +536,34 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 
 	if (!HAS_HW_CONTEXTS(dev)) {
-		kfree(file_priv->private_default_ctx);
+		kfree(file_priv->ctx_info->private_default_ctx);
+		kfree(file_priv->ctx_info);
 		return;
 	}
 
-	mutex_lock(&dev->struct_mutex);
-	idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
-	i915_gem_context_unreference(file_priv->private_default_ctx);
-	idr_destroy(&file_priv->context_idr);
-	mutex_unlock(&dev->struct_mutex);
+	if (mutex_trylock(&dev->struct_mutex)) {
+		idr_for_each(&file_priv->ctx_info->context_idr,
+			     context_idr_cleanup, NULL);
+		i915_gem_context_unreference(file_priv->ctx_info->private_default_ctx);
+		idr_destroy(&file_priv->ctx_info->context_idr);
+		mutex_unlock(&dev->struct_mutex);
+		kfree(file_priv->ctx_info);
+	} else {
+		queue_work(dev_priv->wq, &file_priv->ctx_info->close_work);
+	}
 }
 
 struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
 	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
-		return file_priv->private_default_ctx;
+		return file_priv->ctx_info->private_default_ctx;
 
-	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	return (struct i915_hw_context *)idr_find(&file_priv->ctx_info->context_idr, id);
 }
 
 static inline int
@@ -773,7 +805,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 	}
 
-	idr_remove(&ctx->file_priv->context_idr, ctx->id);
+	idr_remove(&ctx->file_priv->ctx_info->context_idr, ctx->id);
 	i915_gem_context_unreference(ctx);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.8.4.2