Re: ftrace/module: Move ftrace_release_mod() to ddebug_cleanup label

[Steven Rostedt]

As I mentioned, please have "[PATCH]" in the subject.

On Wed, 15 Nov 2017 15:18:54 +0530
Namit Gupta <gupta.namit@xxxxxxxxxxx> wrote:

> ftrace_module_init happen after dynamic_debug_setup, it is desired that
> cleanup should be called after this label however in current implementation
> it is called in free module label,ie:even though ftrace in not initialized,
> from so many fail case ftrace_release_mod() will be called and unnecessary
> traverse the whole list.
> In below patch we moved ftrace_release_mod() from free_module label to
> ddebug_cleanup label. that is the best possible location, other solution
> is to make new label to ftrace_release_mod() but since ftrace_module_init()
> is not return with minimum changes it should be in ddebug_cleanup label.
> 
> 
> Signed-off-by: Namit Gupta <gupta.namit@xxxxxxxxxxx>
> Reviewed-by: Amit Sahrawat <a.sahrawat@xxxxxxxxxxx>

Linus really looks down on this type of "Reviewed-by" tags. They are
meaningless. When a patch first shows up on the mailing list, it should
never have a "Reviewed-by" tag to it. Because to the maintainers, it
has not had a chance to be reviewed. We don't care about internal
company reviews. What should have happened, was this email goes to the
mailing list, and then Amit can reply to that email with a
"Reviewed-by", and any comments that Amit may have had. One reason that
we dislike internal reviews, is that we don't know what was discussed.
Since the review discussion is hidden, so should the tag.

Since the email was missing a "[PATCH]" in the subject, that means Amit
missed that too, and also takes the blame.

> ---
>  kernel/module.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 0d1cb8d..3498d62 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3523,6 +3523,12 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	unset_module_core_ro_nx(mod);
>  
>   ddebug_cleanup:
> +	/*
> +	 * Ftrace needs to clean up what it initialized.
> +	 * This does nothing if ftrace_module_init() wasn't called,
> +	 * but it must be called outside of module_mutex.
> +	 */

When I made this change, I thought the module_mutex was held for more
than it actually was, which is why I mistakenly put in further down
than it needed to be. Since it can go in the normal location (which
your patch is doing), we can nuke the comment.

Please send a v2, with "[PATCH v2]" in the subject.

Thanks!

-- Steve


> +	ftrace_release_mod(mod);
>  	dynamic_debug_remove(info->debug);
>  	synchronize_sched();
>  	kfree(mod->args);
> @@ -3541,12 +3547,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	synchronize_rcu();
>  	mutex_unlock(&module_mutex);
>   free_module:
> -	/*
> -	 * Ftrace needs to clean up what it initialized.
> -	 * This does nothing if ftrace_module_init() wasn't called,
> -	 * but it must be called outside of module_mutex.
> -	 */
> -	ftrace_release_mod(mod);
>  	/* Free lock-classes; relies on the preceding sync_rcu() */
>  	lockdep_free_key_range(mod->module_core, mod->core_size);
>  
> -