Received-SPF: pass (google.com: domain of linux-kernel+bounces-65768-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 14 Feb 2024 13:04:09 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Florent Revest
 <revest@chromium.org>, linux-trace-kernel@vger.kernel.org, LKML
 <linux-kernel@vger.kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>,
 bpf <bpf@vger.kernel.org>, Sven Schnelle <svens@linux.ibm.com>, Alexei
 Starovoitov <ast@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Arnaldo
 Carvalho de Melo <acme@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
 Alan Maguire <alan.maguire@oracle.com>, Mark Rutland
 <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>, Thomas
 Gleixner <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Subject: Re: [PATCH v7 14/36] function_graph: Use a simple LRU for
 fgraph_array index number
Message-ID: <20240214130409.463ae408@gandalf.local.home>
In-Reply-To: <170723220474.502590.7646977373091779892.stgit@devnote2>
References: <170723204881.502590.11906735097521170661.stgit@devnote2>
	<170723220474.502590.7646977373091779892.stgit@devnote2>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed,  7 Feb 2024 00:10:04 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index ae42de909845..323a74623543 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -99,10 +99,44 @@ enum {
>  DEFINE_STATIC_KEY_FALSE(kill_ftrace_graph);
>  int ftrace_graph_active;
>  
> -static int fgraph_array_cnt;
> -
>  static struct fgraph_ops *fgraph_array[FGRAPH_ARRAY_SIZE];
>  
> +/* LRU index table for fgraph_array */
> +static int fgraph_lru_table[FGRAPH_ARRAY_SIZE];
> +static int fgraph_lru_next;
> +static int fgraph_lru_last;
> +
> +static void fgraph_lru_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < FGRAPH_ARRAY_SIZE; i++)
> +		fgraph_lru_table[i] = i;
> +}
> +
> +static int fgraph_lru_release_index(int idx)
> +{
> +	if (idx < 0 || idx >= FGRAPH_ARRAY_SIZE ||
> +	    fgraph_lru_table[fgraph_lru_last] != -1)

Can fgraph_lru_table[fgraph_lru_last] != -1 ever happen? If not, we should
probably add a:

	    WARN_ON_ONCE(fgraph_lru_table[fgraph_lru_last] != -1))

As the size of fgraph_lru_table is the same size as the available indexes,
if we hit this I would think we had a fgraph_lru_relaese_index() without a
fgraph_lru_alloc_index() associated with it.

> +		return -1;
> +
> +	fgraph_lru_table[fgraph_lru_last] = idx;
> +	fgraph_lru_last = (fgraph_lru_last + 1) % FGRAPH_ARRAY_SIZE;
> +	return 0;
> +}
> +
> +static int fgraph_lru_alloc_index(void)
> +{
> +	int idx = fgraph_lru_table[fgraph_lru_next];
> +
> +	if (idx == -1)
> +		return -1;
> +
> +	fgraph_lru_table[fgraph_lru_next] = -1;
> +	fgraph_lru_next = (fgraph_lru_next + 1) % FGRAPH_ARRAY_SIZE;
> +	return idx;
> +}
> +
>  static inline int get_ret_stack_index(struct task_struct *t, int offset)
>  {
>  	return t->ret_stack[offset] & FGRAPH_RET_INDEX_MASK;
> @@ -367,7 +401,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,
>  	if (index < 0)
>  		goto out;
>  
> -	for (i = 0; i < fgraph_array_cnt; i++) {
> +	for (i = 0; i < FGRAPH_ARRAY_SIZE; i++) {
>  		struct fgraph_ops *gops = fgraph_array[i];
>  
>  		if (gops == &fgraph_stub)
> @@ -935,21 +969,17 @@ int register_ftrace_graph(struct fgraph_ops *gops)
>  		/* The array must always have real data on it */
>  		for (i = 0; i < FGRAPH_ARRAY_SIZE; i++)
>  			fgraph_array[i] = &fgraph_stub;
> +		fgraph_lru_init();
>  	}
>  
> -	/* Look for an available spot */
> -	for (i = 0; i < FGRAPH_ARRAY_SIZE; i++) {
> -		if (fgraph_array[i] == &fgraph_stub)
> -			break;
> -	}
> -	if (i >= FGRAPH_ARRAY_SIZE) {
> +	i = fgraph_lru_alloc_index();
> +	if (i < 0 ||
> +	    WARN_ON_ONCE(fgraph_array[i] != &fgraph_stub)) {

The above can nicely fit on one column. No need to break it up:

	if (i < 0 || WARN_ON_ONCE(fgraph_array[i] != &fgraph_stub)) {


>  		ret = -EBUSY;
>  		goto out;
>  	}
>  
>  	fgraph_array[i] = gops;
> -	if (i + 1 > fgraph_array_cnt)
> -		fgraph_array_cnt = i + 1;
>  	gops->idx = i;
>  
>  	ftrace_graph_active++;
> @@ -979,25 +1009,22 @@ int register_ftrace_graph(struct fgraph_ops *gops)
>  void unregister_ftrace_graph(struct fgraph_ops *gops)
>  {
>  	int command = 0;
> -	int i;
>  
>  	mutex_lock(&ftrace_lock);
>  
>  	if (unlikely(!ftrace_graph_active))
>  		goto out;
>  
> -	if (unlikely(gops->idx < 0 || gops->idx >= fgraph_array_cnt))
> +	if (unlikely(gops->idx < 0 || gops->idx >= FGRAPH_ARRAY_SIZE))
> +		goto out;
> +
> +	if (WARN_ON_ONCE(fgraph_array[gops->idx] != gops))
>  		goto out;
>  
> -	WARN_ON_ONCE(fgraph_array[gops->idx] != gops);
> +	if (fgraph_lru_release_index(gops->idx) < 0)
> +		goto out;

Removing the above WARN_ON_ONCE() is more reason to add it to the release
function.

-- Steve


>  
>  	fgraph_array[gops->idx] = &fgraph_stub;
> -	if (gops->idx + 1 == fgraph_array_cnt) {
> -		i = gops->idx;
> -		while (i >= 0 && fgraph_array[i] == &fgraph_stub)
> -			i--;
> -		fgraph_array_cnt = i + 1;
> -	}
>  
>  	ftrace_graph_active--;
>