Date: Mon, 13 Apr 2015 16:07:20 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Rabin Vincent <rabin@rab.in>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tracing: use GFP_ATOMIC in graph_trace_open()
Message-ID: <20150413160720.20df1029@gandalf.local.home>
In-Reply-To: <1428953721-31349-1-git-send-email-rabin@rab.in>
References: <1428953721-31349-1-git-send-email-rabin@rab.in>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4499
Lines: 111

On Mon, 13 Apr 2015 21:35:21 +0200
Rabin Vincent <rabin@rab.in> wrote:

> graph_trace_open() can be called in atomic context from ftrace_dump().
> Use GFP_ATOMIC to avoid the following splat when that happens.
> 
>  BUG: sleeping function called from invalid context at mm/slab.c:2849
>  in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/0
>  Backtrace:

Interesting.

>  ..
>  [<8004dc94>] (__might_sleep) from [<801371f4>] (kmem_cache_alloc_trace+0x160/0x238)
>   r7:87800040 r6:000080d0 r5:810d16e8 r4:000080d0
>  [<80137094>] (kmem_cache_alloc_trace) from [<800cbd60>] (graph_trace_open+0x30/0xd0)
>   r10:00000100 r9:809171a8 r8:00008e28 r7:810d16f0 r6:00000001 r5:810d16e8
>   r4:810d16f0
>  [<800cbd30>] (graph_trace_open) from [<800c79c4>] (trace_init_global_iter+0x50/0x9c)
>   r8:00008e28 r7:808c853c r6:00000001 r5:810d16e8 r4:810d16f0 r3:800cbd30
>  [<800c7974>] (trace_init_global_iter) from [<800c7aa0>] (ftrace_dump+0x90/0x2ec)
>   r4:810d2580 r3:00000000
>  [<800c7a10>] (ftrace_dump) from [<80414b2c>] (sysrq_ftrace_dump+0x1c/0x20)
>   r10:00000100 r9:809171a8 r8:808f6e7c r7:00000001 r6:00000007 r5:0000007a
>   r4:808d5394
>  [<80414b10>] (sysrq_ftrace_dump) from [<800169b8>] (return_to_handler+0x0/0x18)
>  [<80415498>] (__handle_sysrq) from [<800169b8>] (return_to_handler+0x0/0x18)
>   r8:808c8100 r7:808c8444 r6:00000101 r5:00000010 r4:84eb3210
>  [<80415668>] (handle_sysrq) from [<800169b8>] (return_to_handler+0x0/0x18)
>  [<8042a760>] (pl011_int) from [<800169b8>] (return_to_handler+0x0/0x18)
>   r10:809171bc r9:809171a8 r8:00000001 r7:00000026 r6:808c6000 r5:84f01e60
>   r4:8454fe00
>  [<8007782c>] (handle_irq_event_percpu) from [<80077b44>] (handle_irq_event+0x4c/0x6c)
>   r10:808c7ef0 r9:87283e00 r8:00000001 r7:00000000 r6:8454fe00 r5:84f01e60
>   r4:84f01e00
>  [<80077af8>] (handle_irq_event) from [<8007aa28>] (handle_fasteoi_irq+0xf0/0x1ac)
>   r6:808f52a4 r5:84f01e60 r4:84f01e00 r3:00000000
>  [<8007a938>] (handle_fasteoi_irq) from [<80076dc0>] (generic_handle_irq+0x3c/0x4c)
>   r6:00000026 r5:00000000 r4:00000026 r3:8007a938
>  [<80076d84>] (generic_handle_irq) from [<80077128>] (__handle_domain_irq+0x8c/0xfc)
>   r4:808c1e38 r3:0000002e
>  [<8007709c>] (__handle_domain_irq) from [<800087b8>] (gic_handle_irq+0x34/0x6c)
>   r10:80917748 r9:00000001 r8:88802100 r7:808c7ef0 r6:808c8fb0 r5:00000015
>   r4:8880210c r3:808c7ef0
>  [<80008784>] (gic_handle_irq) from [<80014044>] (__irq_svc+0x44/0x7c)
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  kernel/trace/trace_functions_graph.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 2d25ad1..a610872 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -1313,11 +1313,11 @@ void graph_trace_open(struct trace_iterator *iter)
>  
>  	iter->private = NULL;
>  
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	data = kzalloc(sizeof(*data), GFP_ATOMIC);
>  	if (!data)
>  		goto out_err;
>  
> -	data->cpu_data = alloc_percpu(struct fgraph_cpu_data);
> +	data->cpu_data = alloc_percpu_gfp(struct fgraph_cpu_data, GFP_ATOMIC);
>  	if (!data->cpu_data)
>  		goto out_err_free;
>  

Couple of things. First, this deserves a comment, because I can see
someone looking at this in 5 years and saying, WTF is this ATOMIC, and
converting it back to GFP_KERNEL.

Also, since this is really the rare case, and memory may still be freed
in the normal case for this, we should have both. That is, something
like:

	/* This can be called by ftrace_dump() in an atomic location. */
	data = kzalloc(sizeof(*data), GFP_ATOMIC);
	if (unlikely(!data)) {
		/* Try the normal case */
		if (in_atomic() || irqs_disabled())
			goto out_err;
		/* Try the normal case */
		data = kzalloc(sizeof(*data), GPF_KERNEL);
		if (!data)
			goto out_err;
	}

The same for the per_cpu alloc.

I did it that way to avoid doing the in_atomic() calls, but really,
this is not a fast path. We could also just do:

	data = (in_atomic() || irqs_disabled()) ?
		kzalloc(sizeof(*data), GFP_ATOMIC) :
		kzalloc(sizeof(*data), GPF_KERNEL);

Yeah, that way looks better.

But it definitely needs a comment.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/