[PATCH 0/4] tracing: Various cleanups

[PATCH 0/4] tracing: Various cleanups

[Jiri Olsa]

hi,
sending some small cleanups I gathered along the way ;)

attached patches:
  1/4 ftrace: Remove unused code ftrace related code
  2/4 ftrace: Remove unused ftrace_update_time variable/code
  3/4 ftrace: No return value for ftrace_process_locs
  4/4 tracing: Use seq_*_private interface for some seq files

wbr,
jirka
---
 include/linux/ftrace.h |    7 --
 kernel/trace/ftrace.c  |  199 ++++++++----------------------------------------
 kernel/trace/trace.c   |   27 +------
 3 files changed, 35 insertions(+), 198 deletions(-)

[PATCH 1/4] ftrace: Remove unused code ftrace related code

[Jiri Olsa]

Removing unused functions:
  ftrace_rec_iter_record
  ftrace_rec_iter_next
  ftrace_rec_iter_start
  ftrace_location

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 include/linux/ftrace.h |    7 ---
 kernel/trace/ftrace.c  |  100 ------------------------------------------------
 2 files changed, 0 insertions(+), 107 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 72a6cab..49596e3 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -280,16 +280,9 @@ enum {
 
 void arch_ftrace_update_code(int command);
 
-struct ftrace_rec_iter;
-
-struct ftrace_rec_iter *ftrace_rec_iter_start(void);
-struct ftrace_rec_iter *ftrace_rec_iter_next(struct ftrace_rec_iter *iter);
-struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter);
-
 int ftrace_update_record(struct dyn_ftrace *rec, int enable);
 int ftrace_test_record(struct dyn_ftrace *rec, int enable);
 void ftrace_run_stop_machine(int command);
-int ftrace_location(unsigned long ip);
 
 extern ftrace_func_t ftrace_trace_function;
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 0fa92f6..6e6c51a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1393,34 +1393,6 @@ static int ftrace_cmp_recs(const void *a, const void *b)
 	return 0;
 }
 
-/**
- * ftrace_location - return true if the ip giving is a traced location
- * @ip: the instruction pointer to check
- *
- * Returns 1 if @ip given is a pointer to a ftrace location.
- * That is, the instruction that is either a NOP or call to
- * the function tracer. It checks the ftrace internal tables to
- * determine if the address belongs or not.
- */
-int ftrace_location(unsigned long ip)
-{
-	struct ftrace_page *pg;
-	struct dyn_ftrace *rec;
-	struct dyn_ftrace key;
-
-	key.ip = ip;
-
-	for (pg = ftrace_pages_start; pg; pg = pg->next) {
-		rec = bsearch(&key, pg->records, pg->index,
-			      sizeof(struct dyn_ftrace),
-			      ftrace_cmp_recs);
-		if (rec)
-			return 1;
-	}
-
-	return 0;
-}
-
 static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
 				     int filter_hash,
 				     bool inc)
@@ -1717,78 +1689,6 @@ static void ftrace_replace_code(int update)
 	} while_for_each_ftrace_rec();
 }
 
-struct ftrace_rec_iter {
-	struct ftrace_page	*pg;
-	int			index;
-};
-
-/**
- * ftrace_rec_iter_start, start up iterating over traced functions
- *
- * Returns an iterator handle that is used to iterate over all
- * the records that represent address locations where functions
- * are traced.
- *
- * May return NULL if no records are available.
- */
-struct ftrace_rec_iter *ftrace_rec_iter_start(void)
-{
-	/*
-	 * We only use a single iterator.
-	 * Protected by the ftrace_lock mutex.
-	 */
-	static struct ftrace_rec_iter ftrace_rec_iter;
-	struct ftrace_rec_iter *iter = &ftrace_rec_iter;
-
-	iter->pg = ftrace_pages_start;
-	iter->index = 0;
-
-	/* Could have empty pages */
-	while (iter->pg && !iter->pg->index)
-		iter->pg = iter->pg->next;
-
-	if (!iter->pg)
-		return NULL;
-
-	return iter;
-}
-
-/**
- * ftrace_rec_iter_next, get the next record to process.
- * @iter: The handle to the iterator.
- *
- * Returns the next iterator after the given iterator @iter.
- */
-struct ftrace_rec_iter *ftrace_rec_iter_next(struct ftrace_rec_iter *iter)
-{
-	iter->index++;
-
-	if (iter->index >= iter->pg->index) {
-		iter->pg = iter->pg->next;
-		iter->index = 0;
-
-		/* Could have empty pages */
-		while (iter->pg && !iter->pg->index)
-			iter->pg = iter->pg->next;
-	}
-
-	if (!iter->pg)
-		return NULL;
-
-	return iter;
-}
-
-/**
- * ftrace_rec_iter_record, get the record at the iterator location
- * @iter: The current iterator location
- *
- * Returns the record that the current @iter is at.
- */
-struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
-{
-	return &iter->pg->records[iter->index];
-}
-
 static int
 ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
 {
-- 
1.7.1

[PATCH 2/4] ftrace: Remove unused ftrace_update_time variable/code

[Jiri Olsa]

The update time of the records update is meassured but never
used. It was probably dropped along the way, removing it.

Also changing ftrace_update_cnt static variable into automatic,
since it's used only in ftrace_update_code function.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 kernel/trace/ftrace.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6e6c51a..b3ceecd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1920,9 +1920,7 @@ static void ftrace_shutdown_sysctl(void)
 		ftrace_run_update_code(FTRACE_DISABLE_CALLS);
 }
 
-static cycle_t		ftrace_update_time;
-static unsigned long	ftrace_update_cnt;
-unsigned long		ftrace_update_tot_cnt;
+unsigned long ftrace_update_tot_cnt;
 
 static int ops_traces_mod(struct ftrace_ops *ops)
 {
@@ -1936,8 +1934,7 @@ static int ftrace_update_code(struct module *mod)
 {
 	struct ftrace_page *pg;
 	struct dyn_ftrace *p;
-	cycle_t start, stop;
-	unsigned long ref = 0;
+	unsigned long ref = 0, cnt = 0;
 	int i;
 
 	/*
@@ -1957,9 +1954,6 @@ static int ftrace_update_code(struct module *mod)
 		}
 	}
 
-	start = ftrace_now(raw_smp_processor_id());
-	ftrace_update_cnt = 0;
-
 	for (pg = ftrace_new_pgs; pg; pg = pg->next) {
 
 		for (i = 0; i < pg->index; i++) {
@@ -1977,7 +1971,7 @@ static int ftrace_update_code(struct module *mod)
 			if (!ftrace_code_disable(mod, p))
 				break;
 
-			ftrace_update_cnt++;
+			cnt++;
 
 			/*
 			 * If the tracing is enabled, go ahead and enable the record.
@@ -1997,11 +1991,7 @@ static int ftrace_update_code(struct module *mod)
 	}
 
 	ftrace_new_pgs = NULL;
-
-	stop = ftrace_now(raw_smp_processor_id());
-	ftrace_update_time = stop - start;
-	ftrace_update_tot_cnt += ftrace_update_cnt;
-
+	ftrace_update_tot_cnt += cnt;
 	return 0;
 }
 
-- 
1.7.1

[PATCH 3/4] ftrace: No return value for ftrace_process_locs

[Jiri Olsa]

The return value of ftrace_process_locs is never checked. The function
tries to update as many calls as possible and in case of error there's
either warning output or ftrace_bug call.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 kernel/trace/ftrace.c |   37 ++++++++++++++++---------------------
 1 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b3ceecd..e67f5b3 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1930,7 +1930,7 @@ static int ops_traces_mod(struct ftrace_ops *ops)
 	return ftrace_hash_empty(hash);
 }
 
-static int ftrace_update_code(struct module *mod)
+static void ftrace_update_code(struct module *mod)
 {
 	struct ftrace_page *pg;
 	struct dyn_ftrace *p;
@@ -1959,7 +1959,7 @@ static int ftrace_update_code(struct module *mod)
 		for (i = 0; i < pg->index; i++) {
 			/* If something went wrong, bail without enabling anything */
 			if (unlikely(ftrace_disabled))
-				return -1;
+				return;
 
 			p = &pg->records[i];
 			p->flags = ref;
@@ -1968,8 +1968,8 @@ static int ftrace_update_code(struct module *mod)
 			 * Do the initial record conversion from mcount jump
 			 * to the NOP instructions.
 			 */
-			if (!ftrace_code_disable(mod, p))
-				break;
+			if (WARN_ON_ONCE(!ftrace_code_disable(mod, p)))
+				return;
 
 			cnt++;
 
@@ -1992,7 +1992,6 @@ static int ftrace_update_code(struct module *mod)
 
 	ftrace_new_pgs = NULL;
 	ftrace_update_tot_cnt += cnt;
-	return 0;
 }
 
 static int ftrace_allocate_records(struct ftrace_page *pg, int count)
@@ -2041,11 +2040,11 @@ ftrace_allocate_pages(unsigned long num_to_init)
 	int cnt;
 
 	if (!num_to_init)
-		return 0;
+		return NULL;
 
 	start_pg = pg = kzalloc(sizeof(*pg), GFP_KERNEL);
 	if (!pg)
-		return NULL;
+		goto out;
 
 	/*
 	 * Try to allocate as much as possible in one continues
@@ -2078,6 +2077,7 @@ ftrace_allocate_pages(unsigned long num_to_init)
 		kfree(pg);
 		pg = start_pg;
 	}
+ out:
 	pr_info("ftrace: FAILED to allocate memory for functions\n");
 	return NULL;
 }
@@ -3589,25 +3589,23 @@ static void ftrace_swap_recs(void *a, void *b, int size)
 	*recb = t;
 }
 
-static int ftrace_process_locs(struct module *mod,
-			       unsigned long *start,
-			       unsigned long *end)
+static void ftrace_process_locs(struct module *mod,
+				unsigned long *start,
+				unsigned long *end)
 {
 	struct ftrace_page *pg;
 	unsigned long count;
 	unsigned long *p;
 	unsigned long addr;
 	unsigned long flags = 0; /* Shut up gcc */
-	int ret = -ENOMEM;
 
 	count = end - start;
-
 	if (!count)
-		return 0;
+		return;
 
 	pg = ftrace_allocate_pages(count);
 	if (!pg)
-		return -ENOMEM;
+		return;
 
 	mutex_lock(&ftrace_lock);
 
@@ -3621,7 +3619,7 @@ static int ftrace_process_locs(struct module *mod,
 		/* First initialization */
 		ftrace_pages = ftrace_pages_start = pg;
 	} else {
-		if (!ftrace_pages)
+		if (!WARN_ON_ONCE(ftrace_pages))
 			goto out;
 
 		if (WARN_ON(ftrace_pages->next)) {
@@ -3670,11 +3668,8 @@ static int ftrace_process_locs(struct module *mod,
 	ftrace_update_code(mod);
 	if (!mod)
 		local_irq_restore(flags);
-	ret = 0;
  out:
 	mutex_unlock(&ftrace_lock);
-
-	return ret;
 }
 
 #ifdef CONFIG_MODULES
@@ -3789,9 +3784,9 @@ void __init ftrace_init(void)
 
 	last_ftrace_enabled = ftrace_enabled = 1;
 
-	ret = ftrace_process_locs(NULL,
-				  __start_mcount_loc,
-				  __stop_mcount_loc);
+	ftrace_process_locs(NULL,
+			    __start_mcount_loc,
+			    __stop_mcount_loc);
 
 	ret = register_module_notifier(&ftrace_module_nb);
 	if (ret)
-- 
1.7.1

[PATCH 4/4] tracing: Use seq_*_private interface for some seq files

[Jiri Olsa]

It's appropriate to use __seq_open_private interface to open
some of trace seq files, because it covers all steps we are
duplicating in tracing code - zallocating the iterator and
setting it as seq_file's private.

Using this for following files:
  trace
  available_filter_functions
  enabled_functions

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 kernel/trace/ftrace.c |   44 +++++++++++---------------------------------
 kernel/trace/trace.c  |   27 ++++-----------------------
 2 files changed, 15 insertions(+), 56 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index e67f5b3..14989a7 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2359,57 +2359,35 @@ static int
 ftrace_avail_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_iterator *iter;
-	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
-	if (!iter)
-		return -ENOMEM;
-
-	iter->pg = ftrace_pages_start;
-	iter->ops = &global_ops;
-
-	ret = seq_open(file, &show_ftrace_seq_ops);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-
-		m->private = iter;
-	} else {
-		kfree(iter);
+	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
+	if (iter) {
+		iter->pg = ftrace_pages_start;
+		iter->ops = &global_ops;
 	}
 
-	return ret;
+	return iter ? 0 : -ENOMEM;
 }
 
 static int
 ftrace_enabled_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_iterator *iter;
-	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
-	if (!iter)
-		return -ENOMEM;
-
-	iter->pg = ftrace_pages_start;
-	iter->flags = FTRACE_ITER_ENABLED;
-	iter->ops = &global_ops;
-
-	ret = seq_open(file, &show_ftrace_seq_ops);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-
-		m->private = iter;
-	} else {
-		kfree(iter);
+	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
+	if (iter) {
+		iter->pg = ftrace_pages_start;
+		iter->flags = FTRACE_ITER_ENABLED;
+		iter->ops = &global_ops;
 	}
 
-	return ret;
+	return iter ? 0 : -ENOMEM;
 }
 
 static void ftrace_filter_reset(struct ftrace_hash *hash)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ed7b5d1..cf382a4 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2332,7 +2332,6 @@ static struct trace_iterator *
 __tracing_open(struct inode *inode, struct file *file)
 {
 	long cpu_file = (long) inode->i_private;
-	void *fail_ret = ERR_PTR(-ENOMEM);
 	struct trace_iterator *iter;
 	struct seq_file *m;
 	int cpu, ret;
@@ -2340,7 +2339,7 @@ __tracing_open(struct inode *inode, struct file *file)
 	if (tracing_disabled)
 		return ERR_PTR(-ENODEV);
 
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
+	iter = __seq_open_private(file, &tracer_seq_ops, sizeof(*iter));
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
@@ -2397,32 +2396,15 @@ __tracing_open(struct inode *inode, struct file *file)
 		tracing_iter_reset(iter, cpu);
 	}
 
-	ret = seq_open(file, &tracer_seq_ops);
-	if (ret < 0) {
-		fail_ret = ERR_PTR(ret);
-		goto fail_buffer;
-	}
-
-	m = file->private_data;
-	m->private = iter;
-
 	mutex_unlock(&trace_types_lock);
 
 	return iter;
 
- fail_buffer:
-	for_each_tracing_cpu(cpu) {
-		if (iter->buffer_iter[cpu])
-			ring_buffer_read_finish(iter->buffer_iter[cpu]);
-	}
-	free_cpumask_var(iter->started);
-	tracing_start();
  fail:
 	mutex_unlock(&trace_types_lock);
 	kfree(iter->trace);
-	kfree(iter);
-
-	return fail_ret;
+	seq_release_private(inode, file);
+	return ERR_PTR(-ENOMEM);
 }
 
 int tracing_open_generic(struct inode *inode, struct file *filp)
@@ -2458,11 +2440,10 @@ static int tracing_release(struct inode *inode, struct file *file)
 	tracing_start();
 	mutex_unlock(&trace_types_lock);
 
-	seq_release(inode, file);
 	mutex_destroy(&iter->mutex);
 	free_cpumask_var(iter->started);
 	kfree(iter->trace);
-	kfree(iter);
+	seq_release_private(inode, file);
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCH 1/4] ftrace: Remove unused code ftrace related code

[Steven Rostedt]

On Wed, 2012-04-25 at 10:23 +0200, Jiri Olsa wrote:
> Removing unused functions:
>   ftrace_rec_iter_record
>   ftrace_rec_iter_next
>   ftrace_rec_iter_start
>   ftrace_location

No, no, no!!

These are used in the core infrastructure of removing stop machine from
ftrace.

Correct, no one currently uses them, but that's because they were needed
for the arch dependent code. I have patches for both PPC and x86 to
remove stop machine, and I want the PPC changes to go through the PPC
tree.

I pushed the changes for the core infrastructure so that we don't have
dependencies between different archs.

Please read: commit c88fd8634ea68e74c7d19fd2621b4078fd22864c

If you remove these you would break:

 https://lkml.org/lkml/2012/4/23/620

Big NACK!

-- Steve

Re: [PATCH 2/4] ftrace: Remove unused ftrace_update_time variable/code

[Steven Rostedt]

On Wed, 2012-04-25 at 10:23 +0200, Jiri Olsa wrote:
> The update time of the records update is meassured but never
> used. It was probably dropped along the way, removing it.

No it's not dropped. I have patches that read it. But you are correct,
that I should either add those patches, or moved this code out into
those patches, and keep the kernel clean.

Actually, as there's a debug file in the debugfs code now
(dyn_ftrace_total_info). That would be a good place to put this info.

-- Steve

> 
> Also changing ftrace_update_cnt static variable into automatic,
> since it's used only in ftrace_update_code function.

Re: [PATCH 3/4] ftrace: No return value for ftrace_process_locs

[Steven Rostedt]

On Wed, 2012-04-25 at 10:23 +0200, Jiri Olsa wrote:
> The return value of ftrace_process_locs is never checked. The function
> tries to update as many calls as possible and in case of error there's
> either warning output or ftrace_bug call.

No, the real bug is...

>  #ifdef CONFIG_MODULES
> @@ -3789,9 +3784,9 @@ void __init ftrace_init(void)
>  
>  	last_ftrace_enabled = ftrace_enabled = 1;
>  
> -	ret = ftrace_process_locs(NULL,
> -				  __start_mcount_loc,
> -				  __stop_mcount_loc);
> +	ftrace_process_locs(NULL,
> +			    __start_mcount_loc,
> +			    __stop_mcount_loc);

We should check the return code of ftrace_process_locs().

-- Steve

>  
>  	ret = register_module_notifier(&ftrace_module_nb);
>  	if (ret)

Re: [PATCH 1/4] ftrace: Remove unused code ftrace related code

[Jiri Olsa]

On Wed, Apr 25, 2012 at 07:21:29AM -0400, Steven Rostedt wrote:
> On Wed, 2012-04-25 at 10:23 +0200, Jiri Olsa wrote:
> > Removing unused functions:
> >   ftrace_rec_iter_record
> >   ftrace_rec_iter_next
> >   ftrace_rec_iter_start
> >   ftrace_location
> 
> No, no, no!!
> 
> These are used in the core infrastructure of removing stop machine from
> ftrace.
> 
> Correct, no one currently uses them, but that's because they were needed
> for the arch dependent code. I have patches for both PPC and x86 to
> remove stop machine, and I want the PPC changes to go through the PPC
> tree.
> 
> I pushed the changes for the core infrastructure so that we don't have
> dependencies between different archs.
> 
> Please read: commit c88fd8634ea68e74c7d19fd2621b4078fd22864c
> 
> If you remove these you would break:
> 
>  https://lkml.org/lkml/2012/4/23/620
> 
> Big NACK!

I see, thought they were just leftover not seeing any user..
big sorry then ;-)

jirka

> 
> -- Steve
> 
> 
> 

Re: [PATCH 3/4] ftrace: No return value for ftrace_process_locs

[Steven Rostedt]

On Wed, 2012-04-25 at 10:23 +0200, Jiri Olsa wrote:
> The return value of ftrace_process_locs is never checked. The function
> tries to update as many calls as possible and in case of error there's
> either warning output or ftrace_bug call.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  kernel/trace/ftrace.c |   37 ++++++++++++++++---------------------
>  1 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b3ceecd..e67f5b3 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1930,7 +1930,7 @@ static int ops_traces_mod(struct ftrace_ops *ops)
>  	return ftrace_hash_empty(hash);
>  }
>  
> -static int ftrace_update_code(struct module *mod)
> +static void ftrace_update_code(struct module *mod)
>  {
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *p;
> @@ -1959,7 +1959,7 @@ static int ftrace_update_code(struct module *mod)
>  		for (i = 0; i < pg->index; i++) {
>  			/* If something went wrong, bail without enabling anything */
>  			if (unlikely(ftrace_disabled))
> -				return -1;
> +				return;
>  
>  			p = &pg->records[i];
>  			p->flags = ref;
> @@ -1968,8 +1968,8 @@ static int ftrace_update_code(struct module *mod)
>  			 * Do the initial record conversion from mcount jump
>  			 * to the NOP instructions.
>  			 */
> -			if (!ftrace_code_disable(mod, p))
> -				break;
> +			if (WARN_ON_ONCE(!ftrace_code_disable(mod, p)))
> +				return;

Why the warning? If ftrace_disabled is set, something broke a long time
ago, and the ftrace_bug gives its own warning.

I don't think this is needed.

-- Steve

[tip:perf/core] tracing: Use seq_*_private interface for some seq files

[tip-bot for Jiri Olsa]

Commit-ID:  50e18b94c695644d824381e7574b9c44acc25ffe
Gitweb:     http://git.kernel.org/tip/50e18b94c695644d824381e7574b9c44acc25ffe
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Wed, 25 Apr 2012 10:23:39 +0200
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Tue, 8 May 2012 21:04:12 -0400

tracing: Use seq_*_private interface for some seq files

It's appropriate to use __seq_open_private interface to open
some of trace seq files, because it covers all steps we are
duplicating in tracing code - zallocating the iterator and
setting it as seq_file's private.

Using this for following files:
  trace
  available_filter_functions
  enabled_functions

Link: http://lkml.kernel.org/r/1335342219-2782-5-git-send-email-jolsa@redhat.com

Signed-off-by: Jiri Olsa <jolsa@redhat.com>

[
 Fixed warnings for:
   kernel/trace/trace.c: In function '__tracing_open':
   kernel/trace/trace.c:2418:11: warning: unused variable 'ret' [-Wunused-variable]
   kernel/trace/trace.c:2417:19: warning: unused variable 'm' [-Wunused-variable]
]

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |   44 +++++++++++---------------------------------
 kernel/trace/trace.c  |   30 +++++-------------------------
 2 files changed, 16 insertions(+), 58 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 0fa92f6..cf81f27 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2469,57 +2469,35 @@ static int
 ftrace_avail_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_iterator *iter;
-	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
-	if (!iter)
-		return -ENOMEM;
-
-	iter->pg = ftrace_pages_start;
-	iter->ops = &global_ops;
-
-	ret = seq_open(file, &show_ftrace_seq_ops);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-
-		m->private = iter;
-	} else {
-		kfree(iter);
+	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
+	if (iter) {
+		iter->pg = ftrace_pages_start;
+		iter->ops = &global_ops;
 	}
 
-	return ret;
+	return iter ? 0 : -ENOMEM;
 }
 
 static int
 ftrace_enabled_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_iterator *iter;
-	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;
 
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
-	if (!iter)
-		return -ENOMEM;
-
-	iter->pg = ftrace_pages_start;
-	iter->flags = FTRACE_ITER_ENABLED;
-	iter->ops = &global_ops;
-
-	ret = seq_open(file, &show_ftrace_seq_ops);
-	if (!ret) {
-		struct seq_file *m = file->private_data;
-
-		m->private = iter;
-	} else {
-		kfree(iter);
+	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
+	if (iter) {
+		iter->pg = ftrace_pages_start;
+		iter->flags = FTRACE_ITER_ENABLED;
+		iter->ops = &global_ops;
 	}
 
-	return ret;
+	return iter ? 0 : -ENOMEM;
 }
 
 static void ftrace_filter_reset(struct ftrace_hash *hash)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f11a285..4fb10ef 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2413,15 +2413,13 @@ static struct trace_iterator *
 __tracing_open(struct inode *inode, struct file *file)
 {
 	long cpu_file = (long) inode->i_private;
-	void *fail_ret = ERR_PTR(-ENOMEM);
 	struct trace_iterator *iter;
-	struct seq_file *m;
-	int cpu, ret;
+	int cpu;
 
 	if (tracing_disabled)
 		return ERR_PTR(-ENODEV);
 
-	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
+	iter = __seq_open_private(file, &tracer_seq_ops, sizeof(*iter));
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
@@ -2478,32 +2476,15 @@ __tracing_open(struct inode *inode, struct file *file)
 		tracing_iter_reset(iter, cpu);
 	}
 
-	ret = seq_open(file, &tracer_seq_ops);
-	if (ret < 0) {
-		fail_ret = ERR_PTR(ret);
-		goto fail_buffer;
-	}
-
-	m = file->private_data;
-	m->private = iter;
-
 	mutex_unlock(&trace_types_lock);
 
 	return iter;
 
- fail_buffer:
-	for_each_tracing_cpu(cpu) {
-		if (iter->buffer_iter[cpu])
-			ring_buffer_read_finish(iter->buffer_iter[cpu]);
-	}
-	free_cpumask_var(iter->started);
-	tracing_start();
  fail:
 	mutex_unlock(&trace_types_lock);
 	kfree(iter->trace);
-	kfree(iter);
-
-	return fail_ret;
+	seq_release_private(inode, file);
+	return ERR_PTR(-ENOMEM);
 }
 
 int tracing_open_generic(struct inode *inode, struct file *filp)
@@ -2539,11 +2520,10 @@ static int tracing_release(struct inode *inode, struct file *file)
 	tracing_start();
 	mutex_unlock(&trace_types_lock);
 
-	seq_release(inode, file);
 	mutex_destroy(&iter->mutex);
 	free_cpumask_var(iter->started);
 	kfree(iter->trace);
-	kfree(iter);
+	seq_release_private(inode, file);
 	return 0;
 }
 

Re: [PATCH 3/4] ftrace: No return value for ftrace_process_locs

[Jiri Olsa]

On Tue, May 08, 2012 at 10:19:20AM -0400, Steven Rostedt wrote:
> On Wed, 2012-04-25 at 10:23 +0200, Jiri Olsa wrote:
> > The return value of ftrace_process_locs is never checked. The function
> > tries to update as many calls as possible and in case of error there's
> > either warning output or ftrace_bug call.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> > ---
> >  kernel/trace/ftrace.c |   37 ++++++++++++++++---------------------
> >  1 files changed, 16 insertions(+), 21 deletions(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index b3ceecd..e67f5b3 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1930,7 +1930,7 @@ static int ops_traces_mod(struct ftrace_ops *ops)
> >  	return ftrace_hash_empty(hash);
> >  }
> >  
> > -static int ftrace_update_code(struct module *mod)
> > +static void ftrace_update_code(struct module *mod)
> >  {
> >  	struct ftrace_page *pg;
> >  	struct dyn_ftrace *p;
> > @@ -1959,7 +1959,7 @@ static int ftrace_update_code(struct module *mod)
> >  		for (i = 0; i < pg->index; i++) {
> >  			/* If something went wrong, bail without enabling anything */
> >  			if (unlikely(ftrace_disabled))
> > -				return -1;
> > +				return;
> >  
> >  			p = &pg->records[i];
> >  			p->flags = ref;
> > @@ -1968,8 +1968,8 @@ static int ftrace_update_code(struct module *mod)
> >  			 * Do the initial record conversion from mcount jump
> >  			 * to the NOP instructions.
> >  			 */
> > -			if (!ftrace_code_disable(mod, p))
> > -				break;
> > +			if (WARN_ON_ONCE(!ftrace_code_disable(mod, p)))
> > +				return;
> 
> Why the warning? If ftrace_disabled is set, something broke a long time
> ago, and the ftrace_bug gives its own warning.
> 
> I don't think this is needed.

right, I missed the ftrace_disabled case..

About the ftrace_update_code/ftrace_process_locs return values..
I still think the void is better, because we dont change the code
path in case it fails and all errors are already reported.

jirka