linux-kernel - Re: [PATCH v7 14/36] function_graph: Use a simple LRU for fgraph

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 15 Feb 2024 08:48:03 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Florent Revest
 <revest@...omium.org>, linux-trace-kernel@...r.kernel.org, LKML
 <linux-kernel@...r.kernel.org>, Martin KaFai Lau <martin.lau@...ux.dev>,
 bpf <bpf@...r.kernel.org>, Sven Schnelle <svens@...ux.ibm.com>, Alexei
 Starovoitov <ast@...nel.org>, Jiri Olsa <jolsa@...nel.org>, Arnaldo
 Carvalho de Melo <acme@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Alan Maguire <alan.maguire@...cle.com>, Mark Rutland
 <mark.rutland@....com>, Peter Zijlstra <peterz@...radead.org>, Thomas
 Gleixner <tglx@...utronix.de>, Guo Ren <guoren@...nel.org>
Subject: Re: [PATCH v7 14/36] function_graph: Use a simple LRU for
 fgraph_array index number

On Wed, 14 Feb 2024 13:04:09 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Wed,  7 Feb 2024 00:10:04 +0900
> "Masami Hiramatsu (Google)" <mhiramat@...nel.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.

OK, let me make it warning.

> 
> > +		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)) {

OK. 

> 
> 
> >  		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.

OK.

Thank you for review!

> 
> -- 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--;
> >  
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>