ftrace: Factor out __ftrace_hash_move()
PATCH 1/3 ftrace: Factor out __ftrace_hash_move()
The __ftrace_hash_move() is to allocates properly-sized hash and move entries in the src ftrace_hash. It will be used to set function graph filters which has nothing to do with the dyn_ftrace records.
--- kernel/trace/ftrace.c | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)
kernel/trace/ftrace.c +24 -16
@@ -1383,9 +1383,8 @@ ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);static int ftrace_hash_ipmodify_update (struct ftrace_ops *ops,struct ftrace_hash *new_hash); -static int -ftrace_hash_move (struct ftrace_ops *ops,int enable, -struct ftrace_hash **dst,struct ftrace_hash *src) +static struct ftrace_hash * +__ftrace_hash_move (struct ftrace_hash *src) {struct ftrace_func_entry *entry;struct hlist_node *tn;
@@ -1393,21 +1392,13 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,struct ftrace_hash *new_hash;int size = src->count;int bits =0 ; -int ret;int i; -/* Reject setting notrace hash on IPMODIFY ftrace_ops */ -if (ops->flags &FTRACE_OPS_FL_IPMODIFY && !enable) -return -EINVAL ; -/* -* If the new source is empty, just free dst and assign it -* the empty_hash. +* If the new source is empty, just return the empty_hash. */ -if (!src->count) { - new_hash =EMPTY_HASH ; - goto update; - } +if (!src->count) +return EMPTY_HASH ;/* * Make the hash size about 1/2 the # found
@@ -1421,7 +1412,7 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable, new_hash =alloc_ftrace_hash (bits);if (!new_hash) -return -ENOMEM ; +return NULL ; size =1 << src->size_bits;for (i =0 ; i < size; i++) {
@@ -1432,7 +1423,24 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable, } } -update: +return new_hash; +} + +static int +ftrace_hash_move (struct ftrace_ops *ops,int enable, +struct ftrace_hash **dst,struct ftrace_hash *src) +{ +struct ftrace_hash *new_hash; +int ret; + +/* Reject setting notrace hash on IPMODIFY ftrace_ops */ +if (ops->flags &FTRACE_OPS_FL_IPMODIFY && !enable) +return -EINVAL ; + + new_hash =__ftrace_hash_move (src); +if (!new_hash) +return -ENOMEM ; +/* Make sure this can be applied if it is IPMODIFY ftrace_ops */ if (enable) {/* IPMODIFY should be updated only when filter_hash updating */
Signature · 1 line
2.11.0
PATCH 2/3 ftrace: Expose ftrace_hash_empty and ftrace_lookup_ip
It will be used when checking graph filter hashes later.
--- kernel/trace/ftrace.c | 14 +------------- kernel/trace/trace.h | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 13 deletions(-)
kernel/trace/ftrace.c +1 -13
@@ -1110,13 +1110,6 @@ struct ftrace_func_entry {unsigned long ip; }; -struct ftrace_hash { -unsigned long size_bits; -struct hlist_head *buckets; -unsigned long count; -struct rcu_head rcu; -}; -/* * We make these constant because no one should touch them, * but they are used as the default "empty hash", to avoid allocating
@@ -1192,12 +1185,7 @@ struct ftrace_page {static struct ftrace_page *ftrace_pages_start;static struct ftrace_page *ftrace_pages; -static bool __always_inline ftrace_hash_empty(struct ftrace_hash *hash) -{ -return !hash || !hash->count; -} - -static struct ftrace_func_entry * +struct ftrace_func_entry *ftrace_lookup_ip (struct ftrace_hash *hash,unsigned long ip) {unsigned long key;
kernel/trace/trace.h +14 -0
@@ -787,6 +787,20 @@ extern void __trace_graph_return(struct trace_array *tr,struct ftrace_graph_ret *trace, unsigned long flags, int pc); +struct ftrace_hash { +unsigned long size_bits; +struct hlist_head *buckets; +unsigned long count; +struct rcu_head rcu; +}; + +struct ftrace_func_entry * +ftrace_lookup_ip (struct ftrace_hash *hash,unsigned long ip); + +static bool __always_inline ftrace_hash_empty(struct ftrace_hash *hash) +{ +return !hash || !hash->count; +}#ifdef CONFIG_DYNAMIC_FTRACE /* TODO: make this variable */
Signature · 1 line
2.11.0
Steven Rostedt
maintainerRe: [PATCH 2/3] ftrace: Expose ftrace_hash_empty and ftrace_lookup_ip
Quoted 10 lines · On Fri, 20 Jan 2017 11:44:46 +0900 Namhyung Kim <[email protected]> wrote:
kernel/trace/trace.h +4
-0@@ -787,6 +787,20 @@ extern void __trace_graph_return(struct trace_array *tr,struct ftrace_graph_ret *trace, unsigned long flags, int pc); +struct ftrace_hash { +unsigned long size_bits; +struct hlist_head *buckets; +unsigned long count;
kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 803
Quoted 10 lines
kernel/trace/trace.h +10
-0+struct rcu_head rcu; +}; + +struct ftrace_func_entry * +ftrace_lookup_ip (struct ftrace_hash *hash,unsigned long ip); + +static bool __always_inline ftrace_hash_empty(struct ftrace_hash *hash) +{ +return !hash || !hash->count; +}
Note, I had to modify this patch and move this declaration outside of the #ifdef CONFIG_FUNCTION_GRAPH_TRACER, as it failed to build when function graph wasn't enabled. Function tracer uses this too.
-- Steve
Quoted 3 lines
kernel/trace/trace.h +0
-0#ifdef CONFIG_DYNAMIC_FTRACE /* TODO: make this variable */
Re: [PATCH 2/3] ftrace: Expose ftrace_hash_empty and ftrace_lookup_ip
Quoted 20 lines · Steven Rostedt
Quoted 16 lines · On Fri, 20 Jan 2017 11:44:46 +0900 Namhyung Kim <[email protected]> wrote:
kernel/trace/trace.h +10
-0@@ -787,6 +787,20 @@ extern void __trace_graph_return(struct trace_array *tr,struct ftrace_graph_ret *trace, unsigned long flags, int pc); +struct ftrace_hash { +unsigned long size_bits; +struct hlist_head *buckets; +unsigned long count; +struct rcu_head rcu; +}; + +struct ftrace_func_entry * +ftrace_lookup_ip (struct ftrace_hash *hash,unsigned long ip); +
Quoted 8 lines
kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 803
Quoted 4 lines
kernel/trace/trace.h +4
-0+static bool __always_inline ftrace_hash_empty(struct ftrace_hash *hash) +{ +return !hash || !hash->count; +}Note, I had to modify this patch and move this declaration outside of the #ifdef
CONFIG_FUNCTION_GRAPH_TRACER, as it failed to build when function graph wasn't enabled. Function tracer uses this too.
Oops, my bad.
Did you modify it in your tree? Or do you want me to resend v2?
Thanks,
Namhyung
Quoted 4 lines
Quoted 3 lines
kernel/trace/trace.h +0
-0#ifdef CONFIG_DYNAMIC_FTRACE /* TODO: make this variable */
Steven Rostedt
maintainerRe: [PATCH 2/3] ftrace: Expose ftrace_hash_empty and ftrace_lookup_ip
Quoted 7 lines · On Sat, 21 Jan 2017 09:27:06 +0900 Namhyung Kim <[email protected]> wrote:
Quoted 3 lines
Note, I had to modify this patch and move this declaration outside of the #ifdef
CONFIG_FUNCTION_GRAPH_TRACER, as it failed to build when function graph wasn't enabled. Function tracer uses this too.Oops, my bad.
Did you modify it in your tree? Or do you want me to resend v2?
No, it's a trivial change and I made the modification and noted it in the change log. You can see the change in my tree under the ftrace/core branch. I'm taking off next week and I wanted to get this series tested before I go. The tests are still running.
-- Steve
Re: [PATCH 2/3] ftrace: Expose ftrace_hash_empty and ftrace_lookup_ip
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Sat, 21 Jan 2017 09:27:06 +0900 Namhyung Kim <[email protected]> wrote:
Quoted 3 lines
Note, I had to modify this patch and move this declaration outside of the #ifdef
CONFIG_FUNCTION_GRAPH_TRACER, as it failed to build when function graph wasn't enabled. Function tracer uses this too.
Quoted 8 lines
Quoted 3 lines
Oops, my bad.
Did you modify it in your tree? Or do you want me to resend v2?
No, it's a trivial change and I made the modification and noted it in the change log. You can see the change in my tree under the ftrace/core branch. I'm taking off next week and I wanted to get this series tested before I go. The tests are still running.
Thanks for doing that!
Namhyung
PATCH 3/3 ftrace: Convert graph filter to use hash tables
In reply to Namhyung KimUse ftrace_hash instead of a static array of a fixed size. This is useful when a graph filter pattern matches to a large number of functions. Now hash lookup is done with preemption disabled to protect from the hash being changed/freed.
--- kernel/trace/ftrace.c | 190 +++++++++++++++++++++++++++++++++----------------- kernel/trace/trace.h | 64 ++++++++--------- 2 files changed, 158 insertions(+), 96 deletions(-)
kernel/trace/ftrace.c +127 -63
@@ -4378,7 +4378,7 @@ __setup("ftrace_filter=", set_ftrace_filter);#ifdef CONFIG_FUNCTION_GRAPH_TRACER static char ftrace_graph_buf[FTRACE_FILTER_SIZE ]__initdata ;static char ftrace_graph_notrace_buf[FTRACE_FILTER_SIZE ]__initdata ; -static int ftrace_set_func (unsigned long *array,int *idx,int size,char *buffer); +static int ftrace_graph_set_hash (struct ftrace_hash *hash,char *buffer);static unsigned long save_global_trampoline;static unsigned long save_global_flags;
@@ -4401,18 +4401,17 @@ static void __init set_ftrace_early_graph(char *buf, int enable) {int ret;char *func; -unsigned long *table = ftrace_graph_funcs; -int *count = &ftrace_graph_count; +struct ftrace_hash *hash; -if (!enable) { - table = ftrace_graph_notrace_funcs; - count = &ftrace_graph_notrace_count; - } +if (enable) + hash = ftrace_graph_hash; +else + hash = ftrace_graph_notrace_hash;while (buf) { func =strsep (&buf,"," );/* we allow only one expression at a time */ - ret =ftrace_set_func (table, count,FTRACE_GRAPH_MAX_FUNCS , func); + ret =ftrace_graph_set_hash (hash, func);if (ret)printk (KERN_DEBUG "ftrace: function %s not " "traceable\n" , func);
@@ -4536,15 +4535,20 @@ static const struct file_operations ftrace_notrace_fops = {static DEFINE_MUTEX (graph_lock); -int ftrace_graph_count; -int ftrace_graph_notrace_count; -unsigned long ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS ]__read_mostly ; -unsigned long ftrace_graph_notrace_funcs[FTRACE_GRAPH_MAX_FUNCS ]__read_mostly ; +struct ftrace_hash *ftrace_graph_hash =EMPTY_HASH ; +struct ftrace_hash *ftrace_graph_notrace_hash =EMPTY_HASH ; + +enum graph_filter_type { +GRAPH_FILTER_NOTRACE =0 , +GRAPH_FILTER_FUNCTION , +};struct ftrace_graph_data { -unsigned long *table; -size_t size; -int *count; +struct ftrace_hash *hash; +struct ftrace_func_entry *entry; +int idx;/* for hash table iteration */ +enum graph_filter_type type; +struct ftrace_hash *new_hash;const struct seq_operations *seq_ops; };
@@ -4552,10 +4556,31 @@ static void * __g_next(struct seq_file *m, loff_t *pos) { struct ftrace_graph_data *fgd = m->private; +struct ftrace_func_entry *entry = fgd->entry; +struct hlist_head *head; +int i, idx = fgd->idx; -if (*pos >= *fgd->count) +if (*pos >= fgd->hash->count)return NULL ; -return &fgd->table[*pos]; + +if (entry) { +hlist_for_each_entry_continue (entry, hlist) { + fgd->entry = entry; +return entry; + } + + idx++; + } + +for (i = idx; i <1 << fgd->hash->size_bits; i++) { + head = &fgd->hash->buckets[i]; +hlist_for_each_entry (entry, head, hlist) { + fgd->entry = entry; + fgd->idx = i; +return entry; + } + } +return NULL ; } static void *
@@ -4572,9 +4597,11 @@ static void *g_start(struct seq_file *m, loff_t *pos)mutex_lock (&graph_lock);/* Nothing, tell g_show to print all functions are enabled */ -if (!*fgd->count && !*pos) +if (ftrace_hash_empty (fgd->hash) && !*pos)return (void *)1 ; + fgd->idx =0 ; + fgd->entry =NULL ;return __g_next (m, pos); }
@@ -4585,22 +4612,22 @@ static void g_stop(struct seq_file *m, void *p)static int g_show (struct seq_file *m,void *v) { -unsigned long *ptr = v; +struct ftrace_func_entry *entry = v; -if (!ptr) +if (!entry)return 0 ; -if (ptr == (unsigned long *)1 ) { +if (entry == (void *)1 ) {struct ftrace_graph_data *fgd = m->private; -if (fgd->table == ftrace_graph_funcs) +if (fgd->type ==GRAPH_FILTER_FUNCTION )seq_puts (m,"#### all functions enabled ####\n" );else seq_puts (m,"#### no functions disabled ####\n" );return 0 ; } -seq_printf (m,"%ps\n" , (void *)*ptr); +seq_printf (m,"%ps\n" , (void *)entry->ip);return 0 ; }
@@ -4617,24 +4644,37 @@ __ftrace_graph_open(struct inode *inode, struct file *file, struct ftrace_graph_data *fgd) {int ret =0 ; +struct ftrace_hash *new_hash =NULL ; -mutex_lock (&graph_lock); -if ((file->f_mode &FMODE_WRITE ) && - (file->f_flags &O_TRUNC )) { - *fgd->count =0 ; -memset (fgd->table,0 , fgd->size *sizeof (*fgd->table)); +if (file->f_mode &FMODE_WRITE ) { +const int size_bits =FTRACE_HASH_DEFAULT_BITS ; + +if (file->f_flags &O_TRUNC ) + new_hash =alloc_ftrace_hash (size_bits); +else + new_hash =alloc_and_copy_ftrace_hash (size_bits, + fgd->hash); +if (!new_hash) { + ret = -ENOMEM ; + goto out; + } } -mutex_unlock (&graph_lock);if (file->f_mode &FMODE_READ ) { - ret =seq_open (file, fgd->seq_ops); + ret =seq_open (file, &ftrace_graph_seq_ops);if (!ret) {struct seq_file *m = file->private_data; m->private = fgd; + }else { +/* Failed */ +free_ftrace_hash (new_hash); + new_hash =NULL ; } }else file->private_data = fgd; +out: + fgd->new_hash = new_hash;return ret; }
@@ -4642,6 +4682,7 @@ static int ftrace_graph_open(struct inode *inode, struct file *file) { struct ftrace_graph_data *fgd; +int ret;if (unlikely (ftrace_disabled))return -ENODEV ;
@@ -4650,18 +4691,25 @@ ftrace_graph_open(struct inode *inode, struct file *file)if (fgd ==NULL )return -ENOMEM ; - fgd->table = ftrace_graph_funcs; - fgd->size =FTRACE_GRAPH_MAX_FUNCS ; - fgd->count = &ftrace_graph_count; +mutex_lock (&graph_lock); + + fgd->hash = ftrace_graph_hash; + fgd->type =GRAPH_FILTER_FUNCTION ; fgd->seq_ops = &ftrace_graph_seq_ops; -return __ftrace_graph_open (inode, file, fgd); + ret =__ftrace_graph_open (inode, file, fgd); +if (ret <0 ) +kfree (fgd); + +mutex_unlock (&graph_lock); +return ret; }static int ftrace_graph_notrace_open (struct inode *inode,struct file *file) {struct ftrace_graph_data *fgd; +int ret;if (unlikely (ftrace_disabled))return -ENODEV ;
@@ -4670,45 +4718,53 @@ ftrace_graph_notrace_open(struct inode *inode, struct file *file)if (fgd ==NULL )return -ENOMEM ; - fgd->table = ftrace_graph_notrace_funcs; - fgd->size =FTRACE_GRAPH_MAX_FUNCS ; - fgd->count = &ftrace_graph_notrace_count; +mutex_lock (&graph_lock); + + fgd->hash = ftrace_graph_notrace_hash; + fgd->type =GRAPH_FILTER_NOTRACE ; fgd->seq_ops = &ftrace_graph_seq_ops; -return __ftrace_graph_open (inode, file, fgd); + ret =__ftrace_graph_open (inode, file, fgd); +if (ret <0 ) +kfree (fgd); + +mutex_unlock (&graph_lock); +return ret; }static int ftrace_graph_release (struct inode *inode,struct file *file) { +struct ftrace_graph_data *fgd; +if (file->f_mode &FMODE_READ ) {struct seq_file *m = file->private_data; -kfree (m->private); + fgd = m->private;seq_release (inode, file); }else { -kfree (file->private_data); + fgd = file->private_data; } +kfree (fgd->new_hash); +kfree (fgd); +return 0 ; }static int -ftrace_set_func (unsigned long *array,int *idx,int size,char *buffer) +ftrace_graph_set_hash (struct ftrace_hash *hash,char *buffer) {struct ftrace_glob func_g;struct dyn_ftrace *rec;struct ftrace_page *pg; +struct ftrace_func_entry *entry;int fail =1 ;int not; -bool exists; -int i;/* decode regex */ func_g.type =filter_parse_regex (buffer,strlen (buffer), &func_g.search, ¬); -if (!not && *idx >= size) -return -EBUSY ; func_g.len =strlen (func_g.search);
@@ -4725,26 +4781,18 @@ ftrace_set_func(unsigned long *array, int *idx, int size, char *buffer)continue ;if (ftrace_match_record (rec, &func_g,NULL ,0 )) { -/* if it is in the array */ - exists = false; -for (i =0 ; i < *idx; i++) { -if (array[i] == rec->ip) { - exists = true; -break ; - } - } + entry =ftrace_lookup_ip (hash, rec->ip);if (!not) { fail =0 ; -if (!exists) { - array[(*idx)++] = rec->ip; -if (*idx >= size) - goto out; - } + +if (entry) +continue ; +if (add_hash_entry (hash, rec->ip) <0 ) + goto out; }else { -if (exists) { - array[i] = array[--(*idx)]; - array[*idx] =0 ; +if (entry) { +free_hash_entry (hash, entry); fail =0 ; } }
@@ -4766,6 +4814,7 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,struct trace_parser parser;ssize_t read, ret =0 ;struct ftrace_graph_data *fgd = file->private_data; +struct ftrace_hash *old_hash, *new_hash;if (!cnt)return 0 ;
@@ -4781,10 +4830,25 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,mutex_lock (&graph_lock);/* we allow only one expression at a time */ - ret =ftrace_set_func (fgd->table, fgd->count, fgd->size, - parser.buffer); + ret =ftrace_graph_set_hash (fgd->new_hash, + parser.buffer); + + old_hash = fgd->hash; + new_hash =__ftrace_hash_move (fgd->new_hash); +if (!new_hash) + ret = -ENOMEM ; + +if (fgd->type ==GRAPH_FILTER_FUNCTION ) +rcu_assign_pointer (ftrace_graph_hash, new_hash); +else +rcu_assign_pointer (ftrace_graph_notrace_hash, new_hash);mutex_unlock (&graph_lock); + +/* Wait till all users are no longer using the old hash */ +synchronize_sched (); + +free_ftrace_hash (old_hash); }if (!ret)
kernel/trace/trace.h +31 -33
82 lines not on this page · raw message
Signature · 1 line
2.11.0
Steven Rostedt
maintainerQuoted 5 lines · On Fri, 20 Jan 2017 11:44:45 +0900 Namhyung Kim <[email protected]> wrote:
The
__ftrace_hash_move()is to allocates properly-sized hash and move entries in the srcftrace_hash. It will be used to set function graph filters which has nothing to do with thedyn_ftracerecords.Signed-off-by Namhyung Kim <[email protected]>
Hmm, no cover letter?
Anyway, I applied the patches and I'm testing them now. I took a look over them and they seem good, although, there's a few optimizations I want to add. But I'll do that later.
Thanks!
-- Steve
Hi Steve,
Quoted 10 lines · Steven Rostedt
Quoted 5 lines · On Fri, 20 Jan 2017 11:44:45 +0900 Namhyung Kim <[email protected]> wrote:
The
__ftrace_hash_move()is to allocates properly-sized hash and move entries in the srcftrace_hash. It will be used to set function graph filters which has nothing to do with thedyn_ftracerecords.Signed-off-by Namhyung Kim <[email protected]>Hmm, no cover letter?
Oh, I thought it's a conceptually simple and small patch series. So I didn't add a cover letter. Do you prefer seeing a cover letter anyway?
Quoted 4 lines
Anyway, I applied the patches and I'm testing them now. I took a look over them and they seem good, although, there's a few optimizations I want to add. But I'll do that later.
Thanks, if you have anything for me to do, please let me know. Namhyung