ftrace/module: Move ftrace_release_mod() to ddebug_cleanup label

ftrace/module: Move ftrace_release_mod() to ddebug_cleanup label

[Namit Gupta]

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@samsung.com>
Reviewed-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 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.
+	 */
+	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);
 
-- 
1.9.1

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

[Steven Rostedt]

On Wed, 29 Nov 2017 05:36:35 +0000
NAMIT GUPTA <gupta.namit@samsung.com> wrote:

> ping
> 

Thanks for the ping. I should have sent a note that I see your email.
I'm just a bit backed up in other patches, I haven't gotten to it yet.
I should get to it this week, if not, ping me again next week.

-- Steve

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

[Steven Rostedt]

On Wed, 06 Dec 2017 03:17:07 +0000
NAMIT GUPTA <gupta.namit@samsung.com> wrote:

> Hi Steve,
> 
>  
> 
> Please check the patch.

Sure. BTW, when you send a patch, please add '[PATCH]' in the subject.
I currently have over 17 thousand emails in my INBOX, and I filter
it on "[PATCH" to find patches that I missed. Which means your patch
will not show up.

-- Steve

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@samsung.com> 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@samsung.com>
> Reviewed-by: Amit Sahrawat <a.sahrawat@samsung.com>

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

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

[AMIT SAHRAWAT]

Hi Steven,

>> 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@samsung.com>
>> Reviewed-by: Amit Sahrawat <a.sahrawat@samsung.com>
> 
>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.
> 

I fully agree to what you say and the overall perspective for the open source.
When i consider the situation, it was reviewed for our internal issue fixing and adopting 
to our kernel. And with respect to "internal reviews" it ended there itself.

After adopting it was realized to push this to open source as well.
Which as a policy is correct, but the submission method went wrong.
May be it is first patch from Namit and i did not notice due to our mail client.

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

Yes, i did not notice that in my mailbox as well. 
It can just be put to my skipping of this mail as the code was finalized for internal purpose,
i did not notice that the external patch is having "Reviewed-by" and obviously the subject as you mentioned
[PATCH] was missing from it. I am aware of the guidelines and method for contribution, this particular patch submission can be put to my ignorance.
So, I take the blame for this as well.

Thanks & Regards,
Amit Sahrawat

Re: ftrace/module: Move ftrace_release_mod to ddebug_cleanup label

[Jessica Yu]

+++ Steven Rostedt [28/12/17 19:57 -0500]:
>On Fri, 29 Dec 2017 01:36:48 +0100
>Jessica Yu <jeyu@kernel.org> wrote:
>
>> +++ Steven Rostedt [28/12/17 11:32 -0500]:
>> >
>> >Jessica,
>> >
>> >Can you take this patch. You can add:
>> >
>> >Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>
>> Sure, thanks Steven.
>>
>> Namit, your patch does not apply cleanly to modules-next nor
>> linux-next. It looks like you based your patch on an old tree?
>> (e.g., we got rid of the module_core and core_size fields a few years
>> ago...) Could you please rebase your patch on linux-next and resend?
>
>I should probably re-review the rebased patch.

Yeah, that'd be much appreciated :-)

Thanks!

Jessica

Re: ftrace/module: Move ftrace_release_mod to ddebug_cleanup label

[Steven Rostedt]

On Fri, 29 Dec 2017 01:36:48 +0100
Jessica Yu <jeyu@kernel.org> wrote:

> +++ Steven Rostedt [28/12/17 11:32 -0500]:
> >
> >Jessica,
> >
> >Can you take this patch. You can add:
> >
> >Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> Sure, thanks Steven.
> 
> Namit, your patch does not apply cleanly to modules-next nor
> linux-next. It looks like you based your patch on an old tree?
> (e.g., we got rid of the module_core and core_size fields a few years
> ago...) Could you please rebase your patch on linux-next and resend? 

I should probably re-review the rebased patch.

-- Steve

Re: ftrace/module: Move ftrace_release_mod to ddebug_cleanup label

[Jessica Yu]

+++ Steven Rostedt [28/12/17 11:32 -0500]:
>
>Jessica,
>
>Can you take this patch. You can add:
>
>Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Sure, thanks Steven.

Namit, your patch does not apply cleanly to modules-next nor
linux-next. It looks like you based your patch on an old tree?
(e.g., we got rid of the module_core and core_size fields a few years
ago...) Could you please rebase your patch on linux-next and resend? 

Thanks,

Jessica

>On Tue, 12 Dec 2017 17:24:32 +0530
>Namit Gupta <gupta.namit@samsung.com> 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@samsung.com>
>>
>> ---
>>  kernel/module.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 0d1cb8d..4be966a 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3523,6 +3523,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>  	unset_module_core_ro_nx(mod);
>>
>>   ddebug_cleanup:
>> +	ftrace_release_mod(mod);
>>  	dynamic_debug_remove(info->debug);
>>  	synchronize_sched();
>>  	kfree(mod->args);
>> @@ -3541,12 +3542,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);
>>
>