Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 11 Apr 2019 07:24:32 -0700
From:   Christoph Hellwig <hch@infradead.org>
To:     Mao Han <han_mao@c-sky.com>
Cc:     linux-kernel@vger.kernel.org, Palmer Dabbelt <palmer@sifive.com>
Subject: Re: [PATCH 1/3] riscv: Add perf callchain support
Message-ID: <20190411142432.GB8343@infradead.org>
References: <cover.1554961908.git.han_mao@c-sky.com>
 <195185ea63240ed396026505d96d1f6449963482.1554961908.git.han_mao@c-sky.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <195185ea63240ed396026505d96d1f6449963482.1554961908.git.han_mao@c-sky.com>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> --- /dev/null
> +++ b/arch/riscv/kernel/perf_callchain.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2019 Hangzhou C-SKY Microsystems co.,ltd.

Please use normal /* */ Style comment for everything but the SPDX
tags.

> +static int unwind_frame_kernel(struct stackframe *frame)
> +{
> +	int graph = 0;
> +
> +	/* 0x3 means misalignment */
> +	if (!kstack_end((void *)frame->fp) &&
> +	    !((unsigned long)frame->fp & 0x3) &&
> +	    ((unsigned long)frame->fp >= TASK_SIZE)) {
> +		frame->ra = ((struct stackframe *)frame->fp - 1)->ra;
> +		frame->fp = ((struct stackframe *)frame->fp - 1)->fp;
> +		/* make sure CONFIG_FUNCTION_GRAPH_TRACER is turned on */
> +		if (__kernel_text_address(frame->ra))
> +			frame->ra = ftrace_graph_ret_addr(NULL, &graph,
> +							frame->ra, NULL);
> +		return 0;
> +	} else {
> +		return -EPERM;
> +	}

Please do early exists for all the error conditions.  Also no casts
are needed for using ->fp as a scalar value, and we should probably
just do a struct copy instead of copying both values individually.

The function should look something like:

static int unwind_frame_kernel(struct stackframe *frame)
{
	if (!kstack_end((void *)frame->fp))
		return -EPERM;
	if ((frame->fp & 0x3 || frame->fp >= TASK_SIZE)
		return -EPERM;

	*frame = *((struct stackframe *)frame->fp - 1);
	if (__kernel_text_address(frame->ra)) {
		int graph = 0;

		frame->ra = ftrace_graph_ret_addr(NULL, &graph, frame->ra,
				NULL);
	}
	return 0;
}

> +static void notrace walk_stackframe(struct stackframe *fr,
> +			struct perf_callchain_entry_ctx *entry)
> +{
> +	while (1) {
> +		int ret;
> +
> +		perf_callchain_store(entry, fr->ra);
> +
> +		ret = unwind_frame_kernel(fr);
> +		if (ret < 0)
> +			break;
> +	}
> +}

Why not:

	do {
		perf_callchain_store(entry, fr->ra);
	} while (unwind_frame_kernel(fr) >= 0);

> +/*
> + * Get the return address for a single stackframe and return a pointer to the
> + * next frame tail.
> + */
> +static unsigned long user_backtrace(struct perf_callchain_entry_ctx *entry,
> +			unsigned long fp, unsigned long reg_ra)
> +{
> +	struct stackframe buftail;
> +	unsigned long ra = 0;
> +	unsigned long *user_frame_tail = (unsigned long *)(fp - sizeof(struct stackframe));

Overly long line.

> +	fp = user_backtrace(entry, fp, regs->ra);
> +	while ((entry->nr < entry->max_stack) &&
> +		fp && !((unsigned long)fp & 0x3))
> +		fp = user_backtrace(entry, fp, 0);

Please don't indent the condition continuation and the loop body
by the same amount.