[PATCH] tracing: Remove redundant assignment to variable ret

[PATCH] tracing: Remove redundant assignment to variable ret

[Yuntao Wang]

The value assigned to variable ret is never read, the assignment is
redundant and can be removed.

Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 kernel/trace/ftrace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 403e485bf091..48499f58ce47 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -7122,9 +7122,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);
 
 	pr_info("ftrace: allocated %ld pages with %ld groups\n",
 		ftrace_number_of_pages, ftrace_number_of_groups);
-- 
2.34.1

Re: [PATCH] tracing: Remove redundant assignment to variable ret'

[Lukas Bulwahn]

Dear Yuntao,


when you consider removing dead-store assignments guided by some static
analyzer, you need to check if the code you are looking at is actually
missing an error-handling branch.

In this case, ftrace_process_locs() may return -ENOMEM, and the caller
needs to appropriately deal with this error return code. Your patch
does not change the code at all, i.e., the compiled object code is the
same as after the patch as before.

Think about how to deal appropriately with the -ENOMEM return in this
caller and submit a patch that implements the right error-handling
branch or argue in your commit message why that is not needed at all.

If you do not understand or cannot check such basic code properties for
dead-store assignments, it might be better to work on some other aspect
and area of the kernel repository. E.g., the kernel documentation build
also has a few warnings that deserve patches to be fixed.


Best regards,

Lukas

Re: [PATCH] tracing: Remove redundant assignment to variable ret

[Yuntao Wang]

On Mon, Jan 17, 2022 at 3:47 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> Dear Yuntao,
>
>
> when you consider removing dead-store assignments guided by some static
> analyzer, you need to check if the code you are looking at is actually
> missing an error-handling branch.
>
> In this case, ftrace_process_locs() may return -ENOMEM, and the caller
> needs to appropriately deal with this error return code. Your patch
> does not change the code at all, i.e., the compiled object code is the
> same as after the patch as before.
>
> Think about how to deal appropriately with the -ENOMEM return in this
> caller and submit a patch that implements the right error-handling
> branch or argue in your commit message why that is not needed at all.
>
> If you do not understand or cannot check such basic code properties for
> dead-store assignments, it might be better to work on some other aspect
> and area of the kernel repository. E.g., the kernel documentation build
> also has a few warnings that deserve patches to be fixed.
>
>
> Best regards,
>
> Lukas

Dear Lukas,

Thanks for your reply.

Actually, I had read the source code carefully and noticed the possible
error return code -ENOMEM of the ftrace_process_locs() function.

At first I was going to implement an error-handling branch as you said,
but after digging into more details, I discovered:

- The ftrace_init() function did not handle the error return code of the ftrace_process_locs() function since the first version.
- The ftrace_module_init() function did not handle it either.

To keep consistent with the existing code, I just removed the assignment
in that patch.

Maybe we should deal with the error return code more appropriately,
at least print some warnings?

Best regards,

Yuntao

Re: [PATCH] tracing: Remove redundant assignment to variable ret

[Lukas Bulwahn]

On Wed, Jan 19, 2022 at 5:18 AM Yuntao Wang <ytcoode@gmail.com> wrote:
>
> On Mon, Jan 17, 2022 at 3:47 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > Dear Yuntao,
> >
> >
> > when you consider removing dead-store assignments guided by some static
> > analyzer, you need to check if the code you are looking at is actually
> > missing an error-handling branch.
> >
> > In this case, ftrace_process_locs() may return -ENOMEM, and the caller
> > needs to appropriately deal with this error return code. Your patch
> > does not change the code at all, i.e., the compiled object code is the
> > same as after the patch as before.
> >
> > Think about how to deal appropriately with the -ENOMEM return in this
> > caller and submit a patch that implements the right error-handling
> > branch or argue in your commit message why that is not needed at all.
> >
> > If you do not understand or cannot check such basic code properties for
> > dead-store assignments, it might be better to work on some other aspect
> > and area of the kernel repository. E.g., the kernel documentation build
> > also has a few warnings that deserve patches to be fixed.
> >
> >
> > Best regards,
> >
> > Lukas
>
> Dear Lukas,
>
> Thanks for your reply.
>
> Actually, I had read the source code carefully and noticed the possible
> error return code -ENOMEM of the ftrace_process_locs() function.
>
> At first I was going to implement an error-handling branch as you said,
> but after digging into more details, I discovered:
>
> - The ftrace_init() function did not handle the error return code of the ftrace_process_locs() function since the first version.
> - The ftrace_module_init() function did not handle it either.
>

This is certainly important information on the rationale, and hence,
this needs to go into the commit message explaining why you propose
this change.

Now, you should also explain: why do you consider it not a problem
that this error situation -ENOMEM is not handled by the caller?

And if so, why should ftrace_process_locs() even return an error code
if this error return is not considered?

Your commit message should really explain this reasoning.

> To keep consistent with the existing code, I just removed the assignment
> in that patch.
>
> Maybe we should deal with the error return code more appropriately,
> at least print some warnings?
>

This might be one way of dealing with it.

Lukas

Re: [PATCH] tracing: Remove redundant assignment to variable ret

[Steven Rostedt]

On Wed, 19 Jan 2022 08:15:10 +0100
Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:

> > Maybe we should deal with the error return code more appropriately,
> > at least print some warnings?
> >  
> 
> This might be one way of dealing with it.

Actually that is the way it was suppose to be dealt with. I just never got
around to writing the message :-p

-- Steve

[PATCH v2] tracing: Deal with error return code of the ftrace_process_locs() function

[Yuntao Wang]

The ftrace_process_locs() function may return -ENOMEM error code, which
should be handled by the callers.

Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
Changes since v1:
- Deal with error return code instead of just removing it

 kernel/trace/ftrace.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6163b6f762f7..eb98797b399b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6791,11 +6791,16 @@ void ftrace_module_enable(struct module *mod)
 
 void ftrace_module_init(struct module *mod)
 {
+	int ret;
+
 	if (ftrace_disabled || !mod->num_ftrace_callsites)
 		return;
 
-	ftrace_process_locs(mod, mod->ftrace_callsites,
-			    mod->ftrace_callsites + mod->num_ftrace_callsites);
+	ret = ftrace_process_locs(mod, mod->ftrace_callsites,
+				  mod->ftrace_callsites + mod->num_ftrace_callsites);
+	if (ret)
+		pr_warn("ftrace: failed to allocate entries for module '%s' functions\n",
+			mod->name);
 }
 
 static void save_ftrace_mod_rec(struct ftrace_mod_map *mod_map,
@@ -7126,15 +7131,19 @@ void __init ftrace_init(void)
 	pr_info("ftrace: allocating %ld entries in %ld pages\n",
 		count, count / ENTRIES_PER_PAGE + 1);
 
-	last_ftrace_enabled = ftrace_enabled = 1;
-
 	ret = ftrace_process_locs(NULL,
 				  __start_mcount_loc,
 				  __stop_mcount_loc);
+	if (ret) {
+		pr_warn("ftrace: failed to allocate entries for functions\n");
+		goto failed;
+	}
 
 	pr_info("ftrace: allocated %ld pages with %ld groups\n",
 		ftrace_number_of_pages, ftrace_number_of_groups);
 
+	last_ftrace_enabled = ftrace_enabled = 1;
+
 	set_ftrace_early_filters();
 
 	return;
-- 
2.34.1.75.gabe6bb3905

Re: [PATCH v2] tracing: Deal with error return code of the ftrace_process_locs() function

[Steven Rostedt]

On Thu, 20 Jan 2022 06:59:49 +0000
Yuntao Wang <ytcoode@gmail.com> wrote:

> The ftrace_process_locs() function may return -ENOMEM error code, which
> should be handled by the callers.
> 
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
> Changes since v1:
> - Deal with error return code instead of just removing it
> 
> 

Thanks, I added this to my queue. Note, I renamed the subject to start with
"ftrace:" instead of "tracing:" as this is specific to ftrace (the function
hook infrastructure).

-- Steve