Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

On Sun, Oct 8, 2017 at 11:42 AM, Steven Rostedt  wrote:
>> > "Joel Fernandes (Google)"  wrote:
>> > Also could you let me know what is the correct behavior of the filters
>> > after a module being traced is unloaded, are the filters supposed to
>> > be setup again after the module is unloaded, before the module can be
>> > loaded again?
>>
>> Actually I learnt that it doesn't get preserved and wrote a patch to fix 
>> that, let me know
>> what you think, thanks. (its only lightly tested - checked that the filters 
>> are preserved,
>> will do more testing tomorrow):
>
> They should not be preserved, it's too non-deterministic.
>
> I'm wondering why the setting of the ip to zero didn't keep it from
> showing up again. I'll have to investigate that.
>

I found that the entries weren't being cleared for the init functions
as the records weren't available at module unload time.
Posted a patch to fix this: https://lkml.org/lkml/2017/10/8/196

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

On Sun, Oct 8, 2017 at 11:42 AM, Steven Rostedt  wrote:
>> > "Joel Fernandes (Google)"  wrote:
>> > Also could you let me know what is the correct behavior of the filters
>> > after a module being traced is unloaded, are the filters supposed to
>> > be setup again after the module is unloaded, before the module can be
>> > loaded again?
>>
>> Actually I learnt that it doesn't get preserved and wrote a patch to fix 
>> that, let me know
>> what you think, thanks. (its only lightly tested - checked that the filters 
>> are preserved,
>> will do more testing tomorrow):
>
> They should not be preserved, it's too non-deterministic.
>
> I'm wondering why the setting of the ip to zero didn't keep it from
> showing up again. I'll have to investigate that.
>

I found that the entries weren't being cleared for the init functions
as the records weren't available at module unload time.
Posted a patch to fix this: https://lkml.org/lkml/2017/10/8/196

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

On Sat, Oct 7, 2017 at 9:52 PM, Joel Fernandes (Google)
 wrote:
[..]
>>>
>>> On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
>>> > From: "Steven Rostedt (VMware)" 
>>> >
>>> > The ftrace_mod_map is a descriptor to save module init function names in
>>> > case they were traced, and the trace output needs to reference the 
>>> > function
>>> > name from the function address. But after the function is unloaded, it
>>> > the maps should be freed, as the rest of the function names are as well.
>>>
>>> Just checking for my understanding of this patch - wouldn't this also
>>> mean that if there were any look ups of the init functions that may be
>>> needed at trace output time, then those look ups wont be possible any
>>> more after module is unloaded?
>>
>> Yes. That's true for all functions in the module. When a module is
>> unloaded, all references to it in kallsyms is also freed. Try it on a
>> current kernel. Trace a function in a module, then unload that module.
>> The trace data will just show the ip hex address of the module function
>> after that.
>>
>
> I tried your core branch and I see some weirdness with the filters:
>
> Here's the code for the module:
> 
> #include 
> #include 
> #include 
>
> void bar(void)
> {
> printk(KERN_INFO "bar!\n");
> }
>
> void foo(void)
> {
> printk(KERN_INFO "foo!\n");
> bar();
> }
>
> static int __init hello_init(void)
> {
> printk(KERN_INFO "Hello world!\n");
> foo();
> return 0;
> }
>
> static void __exit hello_cleanup(void)
> {
> printk(KERN_INFO "Cleaning up module.\n");
> }
>
> module_init(hello_init);
> module_exit(hello_cleanup);
> 
[..]
> Also I wasn't able to see the call from hello_init -> bar and bar ->
> foo so I'm not fully sure if that's a bug or I did something wrong.
> I'm happy to try out anything you suggest.

This was I think because of function inlining, using 'noinline' on the
foo and bar functions in the above module makes it all work properly
(tested on ftrace/core branch).

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

On Sat, Oct 7, 2017 at 9:52 PM, Joel Fernandes (Google)
 wrote:
[..]
>>>
>>> On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
>>> > From: "Steven Rostedt (VMware)" 
>>> >
>>> > The ftrace_mod_map is a descriptor to save module init function names in
>>> > case they were traced, and the trace output needs to reference the 
>>> > function
>>> > name from the function address. But after the function is unloaded, it
>>> > the maps should be freed, as the rest of the function names are as well.
>>>
>>> Just checking for my understanding of this patch - wouldn't this also
>>> mean that if there were any look ups of the init functions that may be
>>> needed at trace output time, then those look ups wont be possible any
>>> more after module is unloaded?
>>
>> Yes. That's true for all functions in the module. When a module is
>> unloaded, all references to it in kallsyms is also freed. Try it on a
>> current kernel. Trace a function in a module, then unload that module.
>> The trace data will just show the ip hex address of the module function
>> after that.
>>
>
> I tried your core branch and I see some weirdness with the filters:
>
> Here's the code for the module:
> 
> #include 
> #include 
> #include 
>
> void bar(void)
> {
> printk(KERN_INFO "bar!\n");
> }
>
> void foo(void)
> {
> printk(KERN_INFO "foo!\n");
> bar();
> }
>
> static int __init hello_init(void)
> {
> printk(KERN_INFO "Hello world!\n");
> foo();
> return 0;
> }
>
> static void __exit hello_cleanup(void)
> {
> printk(KERN_INFO "Cleaning up module.\n");
> }
>
> module_init(hello_init);
> module_exit(hello_cleanup);
> 
[..]
> Also I wasn't able to see the call from hello_init -> bar and bar ->
> foo so I'm not fully sure if that's a bug or I did something wrong.
> I'm happy to try out anything you suggest.

This was I think because of function inlining, using 'noinline' on the
foo and bar functions in the above module makes it all work properly
(tested on ftrace/core branch).

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

On Sun, Oct 8, 2017 at 11:42 AM, Steven Rostedt  wrote:
> On Sun, 8 Oct 2017 01:42:15 -0700
[..]
>> > "Joel Fernandes (Google)"  wrote:
>> [..]
>> > Also could you let me know what is the correct behavior of the filters
>> > after a module being traced is unloaded, are the filters supposed to
>> > be setup again after the module is unloaded, before the module can be
>> > loaded again?
>>
>> Actually I learnt that it doesn't get preserved and wrote a patch to fix 
>> that, let me know
>> what you think, thanks. (its only lightly tested - checked that the filters 
>> are preserved,
>> will do more testing tomorrow):
>
> They should not be preserved, it's too non-deterministic.

Could you elaborate more on non-deterministic?

Say a user setup filters with '*:mod:' and unloads and
loads, then the filter carrying forward for the second load helps
avoid having them to set it up again.

OTOH I agree its not a big deal to set up the filters again, and
probably not worth save it if it complicates the code too much to
handle all cases.

> I'm wondering why the setting of the ip to zero didn't keep it from
> showing up again. I'll have to investigate that.

Ok. Happy to help test anything you want to try :) I *think* the 'bar'
function that showed up the second time around is actually the address
of the init function in the previous load, and probably the new code
you added switched it on but I have yet to debug it (you'll probably
beat me to it since you wrote this code ;)).

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

On Sun, Oct 8, 2017 at 11:42 AM, Steven Rostedt  wrote:
> On Sun, 8 Oct 2017 01:42:15 -0700
[..]
>> > "Joel Fernandes (Google)"  wrote:
>> [..]
>> > Also could you let me know what is the correct behavior of the filters
>> > after a module being traced is unloaded, are the filters supposed to
>> > be setup again after the module is unloaded, before the module can be
>> > loaded again?
>>
>> Actually I learnt that it doesn't get preserved and wrote a patch to fix 
>> that, let me know
>> what you think, thanks. (its only lightly tested - checked that the filters 
>> are preserved,
>> will do more testing tomorrow):
>
> They should not be preserved, it's too non-deterministic.

Could you elaborate more on non-deterministic?

Say a user setup filters with '*:mod:' and unloads and
loads, then the filter carrying forward for the second load helps
avoid having them to set it up again.

OTOH I agree its not a big deal to set up the filters again, and
probably not worth save it if it complicates the code too much to
handle all cases.

> I'm wondering why the setting of the ip to zero didn't keep it from
> showing up again. I'll have to investigate that.

Ok. Happy to help test anything you want to try :) I *think* the 'bar'
function that showed up the second time around is actually the address
of the init function in the previous load, and probably the new code
you added switched it on but I have yet to debug it (you'll probably
beat me to it since you wrote this code ;)).

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Steven Rostedt]

On Sun, 8 Oct 2017 01:42:15 -0700
Joel Fernandes  wrote:

> Hi Steve,
> 
> > "Joel Fernandes (Google)"  wrote:  
> [..]
> > Also could you let me know what is the correct behavior of the filters
> > after a module being traced is unloaded, are the filters supposed to
> > be setup again after the module is unloaded, before the module can be
> > loaded again?  
> 
> Actually I learnt that it doesn't get preserved and wrote a patch to fix 
> that, let me know
> what you think, thanks. (its only lightly tested - checked that the filters 
> are preserved,
> will do more testing tomorrow):

They should not be preserved, it's too non-deterministic.

I'm wondering why the setting of the ip to zero didn't keep it from
showing up again. I'll have to investigate that.

-- Steve

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Steven Rostedt]

On Sun, 8 Oct 2017 01:42:15 -0700
Joel Fernandes  wrote:

> Hi Steve,
> 
> > "Joel Fernandes (Google)"  wrote:  
> [..]
> > Also could you let me know what is the correct behavior of the filters
> > after a module being traced is unloaded, are the filters supposed to
> > be setup again after the module is unloaded, before the module can be
> > loaded again?  
> 
> Actually I learnt that it doesn't get preserved and wrote a patch to fix 
> that, let me know
> what you think, thanks. (its only lightly tested - checked that the filters 
> are preserved,
> will do more testing tomorrow):

They should not be preserved, it's too non-deterministic.

I'm wondering why the setting of the ip to zero didn't keep it from
showing up again. I'll have to investigate that.

-- Steve

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

> "Joel Fernandes (Google)"  wrote:
[..]
> Also could you let me know what is the correct behavior of the filters
> after a module being traced is unloaded, are the filters supposed to
> be setup again after the module is unloaded, before the module can be
> loaded again?

Actually I learnt that it doesn't get preserved and wrote a patch to fix that, 
let me know
what you think, thanks. (its only lightly tested - checked that the filters are 
preserved,
will do more testing tomorrow):

---8<---
From: Joel Fernandes 
Subject: [PATCH RFC] ftrace: Preserve filters across loading/unloading of module

When a module is removed, the filters associated with it is cleared, however it
doesn't make sense to clear it in the following work flow:

1. setup filters (:mod:)
2. load module
3. unload module
4. load module

In step 4, the filters would have gone. Since the recently added mechanism to
do step 1 before 2 (deferred execution of the module command string) is already
present, we can reuse the same mechanism to preserve the filters. This patch
does that and avoids losing the filters and having to set them up again.

Cc: Steven Rostedt 
Signed-off-by: Joel Fernandes 
---
 kernel/trace/ftrace.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6abfafd7f173..4979fd2ef466 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5693,7 +5693,9 @@ static int referenced_filters(struct dyn_ftrace *rec)
 }
 
 static void
-clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
+deactivate_mod_from_hash(struct module *mod, struct ftrace_page *pg,
+struct trace_array *tr, int enable,
+struct ftrace_hash *hash)
 {
struct ftrace_func_entry *entry;
struct dyn_ftrace *rec;
@@ -5712,11 +5714,22 @@ clear_mod_from_hash(struct ftrace_page *pg, struct 
ftrace_hash *hash)
 */
if (entry)
entry->ip = 0;
+
+   /*
+* The hash has been made cleared, however lets save it as a
+* string for the next time the module is loaded.
+*/
+   if (entry) {
+   char func[KSYM_SYMBOL_LEN];
+   kallsyms_lookup(rec->ip, NULL, NULL, NULL, func);
+   ftrace_add_mod(tr, func, mod->name, enable);
+   }
}
 }
 
 /* Clear any records from hashs */
-static void clear_mod_from_hashes(struct ftrace_page *pg)
+static void deactivate_mod_from_hashes(struct module *mod,
+  struct ftrace_page *pg)
 {
struct trace_array *tr;
 
@@ -5725,8 +5738,10 @@ static void clear_mod_from_hashes(struct ftrace_page *pg)
if (!tr->ops || !tr->ops->func_hash)
continue;
mutex_lock(>ops->func_hash->regex_lock);
-   clear_mod_from_hash(pg, tr->ops->func_hash->filter_hash);
-   clear_mod_from_hash(pg, tr->ops->func_hash->notrace_hash);
+   deactivate_mod_from_hash(mod, pg, tr, 1,
+tr->ops->func_hash->filter_hash);
+   deactivate_mod_from_hash(mod, pg, tr, 0,
+tr->ops->func_hash->notrace_hash);
mutex_unlock(>ops->func_hash->regex_lock);
}
mutex_unlock(_types_lock);
@@ -5778,7 +5793,7 @@ void ftrace_release_mod(struct module *mod)
for (pg = tmp_page; pg; pg = tmp_page) {
 
/* Needs to be called outside of ftrace_lock */
-   clear_mod_from_hashes(pg);
+   deactivate_mod_from_hashes(mod, pg);
 
order = get_count_order(pg->size / ENTRIES_PER_PAGE);
free_pages((unsigned long)pg->records, order);
-- 
2.14.2.920.gcf0c67979c-goog

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes]

Hi Steve,

> "Joel Fernandes (Google)"  wrote:
[..]
> Also could you let me know what is the correct behavior of the filters
> after a module being traced is unloaded, are the filters supposed to
> be setup again after the module is unloaded, before the module can be
> loaded again?

Actually I learnt that it doesn't get preserved and wrote a patch to fix that, 
let me know
what you think, thanks. (its only lightly tested - checked that the filters are 
preserved,
will do more testing tomorrow):

---8<---
From: Joel Fernandes 
Subject: [PATCH RFC] ftrace: Preserve filters across loading/unloading of module

When a module is removed, the filters associated with it is cleared, however it
doesn't make sense to clear it in the following work flow:

1. setup filters (:mod:)
2. load module
3. unload module
4. load module

In step 4, the filters would have gone. Since the recently added mechanism to
do step 1 before 2 (deferred execution of the module command string) is already
present, we can reuse the same mechanism to preserve the filters. This patch
does that and avoids losing the filters and having to set them up again.

Cc: Steven Rostedt 
Signed-off-by: Joel Fernandes 
---
 kernel/trace/ftrace.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6abfafd7f173..4979fd2ef466 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5693,7 +5693,9 @@ static int referenced_filters(struct dyn_ftrace *rec)
 }
 
 static void
-clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
+deactivate_mod_from_hash(struct module *mod, struct ftrace_page *pg,
+struct trace_array *tr, int enable,
+struct ftrace_hash *hash)
 {
struct ftrace_func_entry *entry;
struct dyn_ftrace *rec;
@@ -5712,11 +5714,22 @@ clear_mod_from_hash(struct ftrace_page *pg, struct 
ftrace_hash *hash)
 */
if (entry)
entry->ip = 0;
+
+   /*
+* The hash has been made cleared, however lets save it as a
+* string for the next time the module is loaded.
+*/
+   if (entry) {
+   char func[KSYM_SYMBOL_LEN];
+   kallsyms_lookup(rec->ip, NULL, NULL, NULL, func);
+   ftrace_add_mod(tr, func, mod->name, enable);
+   }
}
 }
 
 /* Clear any records from hashs */
-static void clear_mod_from_hashes(struct ftrace_page *pg)
+static void deactivate_mod_from_hashes(struct module *mod,
+  struct ftrace_page *pg)
 {
struct trace_array *tr;
 
@@ -5725,8 +5738,10 @@ static void clear_mod_from_hashes(struct ftrace_page *pg)
if (!tr->ops || !tr->ops->func_hash)
continue;
mutex_lock(>ops->func_hash->regex_lock);
-   clear_mod_from_hash(pg, tr->ops->func_hash->filter_hash);
-   clear_mod_from_hash(pg, tr->ops->func_hash->notrace_hash);
+   deactivate_mod_from_hash(mod, pg, tr, 1,
+tr->ops->func_hash->filter_hash);
+   deactivate_mod_from_hash(mod, pg, tr, 0,
+tr->ops->func_hash->notrace_hash);
mutex_unlock(>ops->func_hash->regex_lock);
}
mutex_unlock(_types_lock);
@@ -5778,7 +5793,7 @@ void ftrace_release_mod(struct module *mod)
for (pg = tmp_page; pg; pg = tmp_page) {
 
/* Needs to be called outside of ftrace_lock */
-   clear_mod_from_hashes(pg);
+   deactivate_mod_from_hashes(mod, pg);
 
order = get_count_order(pg->size / ENTRIES_PER_PAGE);
free_pages((unsigned long)pg->records, order);
-- 
2.14.2.920.gcf0c67979c-goog

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes (Google)]

Hi Steve,

On Sat, Oct 7, 2017 at 6:32 AM, Steven Rostedt  wrote:
> On Fri, 6 Oct 2017 23:41:25 -0700
> "Joel Fernandes (Google)"  wrote:
>
>> Hi Steve,
>>
>> On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
>> > From: "Steven Rostedt (VMware)" 
>> >
>> > The ftrace_mod_map is a descriptor to save module init function names in
>> > case they were traced, and the trace output needs to reference the function
>> > name from the function address. But after the function is unloaded, it
>> > the maps should be freed, as the rest of the function names are as well.
>>
>> Just checking for my understanding of this patch - wouldn't this also
>> mean that if there were any look ups of the init functions that may be
>> needed at trace output time, then those look ups wont be possible any
>> more after module is unloaded?
>
> Yes. That's true for all functions in the module. When a module is
> unloaded, all references to it in kallsyms is also freed. Try it on a
> current kernel. Trace a function in a module, then unload that module.
> The trace data will just show the ip hex address of the module function
> after that.
>

I tried your core branch and I see some weirdness with the filters:

Here's the code for the module:

#include 
#include 
#include 

void bar(void)
{
printk(KERN_INFO "bar!\n");
}

void foo(void)
{
printk(KERN_INFO "foo!\n");
bar();
}

static int __init hello_init(void)
{
printk(KERN_INFO "Hello world!\n");
foo();
return 0;
}

static void __exit hello_cleanup(void)
{
printk(KERN_INFO "Cleaning up module.\n");
}

module_init(hello_init);
module_exit(hello_cleanup);


Following is a run with function tracing, during the first run I see
foo and bar are setup in the filters. After that when I unload and
load the module, I see that only "bar" is in the filters:

bash-4.3# echo '*:mod:test' > /d/tracing/set_ftrace_filter
bash-4.3# echo function > /d/tracing/current_tracer
bash-4.3# cat /d/tracing/set_ftrace_filter
*:mod:test
bash-4.3# modprobe test
bash-4.3# cat /d/tracing/trace
# tracer: function
#
#  _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
#   TASK-PID   CPU#  TIMESTAMP  FUNCTION
#  | |   |      | |
modprobe-1050  [003]  1.653000: hello_init <-do_one_initcall
bash-4.3# cat /d/tracing/set_ftrace_filter
bar [test]
foo [test]
bash-4.3# rmmod test
bash-4.3# cat /d/tracing/set_ftrace_filter
bash-4.3# sleep 2
bash-4.3# modprobe test
bash-4.3# cat /d/tracing/trace
# tracer: function
#
#  _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
#   TASK-PID   CPU#  TIMESTAMP  FUNCTION
#  | |   |      | |
modprobe-1050  [003]  1.653000: bar <-do_one_initcall
bash-4.3# cat /d/tracing/set_ftrace_filter
bar [test]   <--- bar is still in
the filter list
bash-4.3#

Interestingly this also shows that the symbol conversion can be
unreliable if another module was loaded into the same address, in this
case 'bar' was loaded at the same address as 'hello_init' the second
time so during the second cat of the trace file, it shows a different
symbol name.

Also could you let me know what is the correct behavior of the filters
after a module being traced is unloaded, are the filters supposed to
be setup again after the module is unloaded, before the module can be
loaded again?

Also I wasn't able to see the call from hello_init -> bar and bar ->
foo so I'm not fully sure if that's a bug or I did something wrong.
I'm happy to try out anything you suggest.

>> I guess having a reference somehow on the ftrace_mod_map descriptor if
>> there are any entries in the trace buffer that need it can help
>> prevent that but it could be too expensive for not much return since
>> most likely the user wouldn't unload modules before trace collection
>> in normal usage.
>
> Right, I have thought about this, and I haven't come up with an
> inexpensive way to do this. As this has been the default operation of
> all module functions, and I haven't heard much complaining about it (I
> think I may have had a single complaint), I didn't put too much effort
> into it.

> I need to look at the approach that Jessica sent me. Perhaps there's

Yes, I think that approach is better. But I think it would have the
same issue (that if the init sections are freed and say used by 

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes (Google)]

Hi Steve,

On Sat, Oct 7, 2017 at 6:32 AM, Steven Rostedt  wrote:
> On Fri, 6 Oct 2017 23:41:25 -0700
> "Joel Fernandes (Google)"  wrote:
>
>> Hi Steve,
>>
>> On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
>> > From: "Steven Rostedt (VMware)" 
>> >
>> > The ftrace_mod_map is a descriptor to save module init function names in
>> > case they were traced, and the trace output needs to reference the function
>> > name from the function address. But after the function is unloaded, it
>> > the maps should be freed, as the rest of the function names are as well.
>>
>> Just checking for my understanding of this patch - wouldn't this also
>> mean that if there were any look ups of the init functions that may be
>> needed at trace output time, then those look ups wont be possible any
>> more after module is unloaded?
>
> Yes. That's true for all functions in the module. When a module is
> unloaded, all references to it in kallsyms is also freed. Try it on a
> current kernel. Trace a function in a module, then unload that module.
> The trace data will just show the ip hex address of the module function
> after that.
>

I tried your core branch and I see some weirdness with the filters:

Here's the code for the module:

#include 
#include 
#include 

void bar(void)
{
printk(KERN_INFO "bar!\n");
}

void foo(void)
{
printk(KERN_INFO "foo!\n");
bar();
}

static int __init hello_init(void)
{
printk(KERN_INFO "Hello world!\n");
foo();
return 0;
}

static void __exit hello_cleanup(void)
{
printk(KERN_INFO "Cleaning up module.\n");
}

module_init(hello_init);
module_exit(hello_cleanup);


Following is a run with function tracing, during the first run I see
foo and bar are setup in the filters. After that when I unload and
load the module, I see that only "bar" is in the filters:

bash-4.3# echo '*:mod:test' > /d/tracing/set_ftrace_filter
bash-4.3# echo function > /d/tracing/current_tracer
bash-4.3# cat /d/tracing/set_ftrace_filter
*:mod:test
bash-4.3# modprobe test
bash-4.3# cat /d/tracing/trace
# tracer: function
#
#  _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
#   TASK-PID   CPU#  TIMESTAMP  FUNCTION
#  | |   |      | |
modprobe-1050  [003]  1.653000: hello_init <-do_one_initcall
bash-4.3# cat /d/tracing/set_ftrace_filter
bar [test]
foo [test]
bash-4.3# rmmod test
bash-4.3# cat /d/tracing/set_ftrace_filter
bash-4.3# sleep 2
bash-4.3# modprobe test
bash-4.3# cat /d/tracing/trace
# tracer: function
#
#  _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
#   TASK-PID   CPU#  TIMESTAMP  FUNCTION
#  | |   |      | |
modprobe-1050  [003]  1.653000: bar <-do_one_initcall
bash-4.3# cat /d/tracing/set_ftrace_filter
bar [test]   <--- bar is still in
the filter list
bash-4.3#

Interestingly this also shows that the symbol conversion can be
unreliable if another module was loaded into the same address, in this
case 'bar' was loaded at the same address as 'hello_init' the second
time so during the second cat of the trace file, it shows a different
symbol name.

Also could you let me know what is the correct behavior of the filters
after a module being traced is unloaded, are the filters supposed to
be setup again after the module is unloaded, before the module can be
loaded again?

Also I wasn't able to see the call from hello_init -> bar and bar ->
foo so I'm not fully sure if that's a bug or I did something wrong.
I'm happy to try out anything you suggest.

>> I guess having a reference somehow on the ftrace_mod_map descriptor if
>> there are any entries in the trace buffer that need it can help
>> prevent that but it could be too expensive for not much return since
>> most likely the user wouldn't unload modules before trace collection
>> in normal usage.
>
> Right, I have thought about this, and I haven't come up with an
> inexpensive way to do this. As this has been the default operation of
> all module functions, and I haven't heard much complaining about it (I
> think I may have had a single complaint), I didn't put too much effort
> into it.

> I need to look at the approach that Jessica sent me. Perhaps there's

Yes, I think that approach is better. But I think it would have the
same issue (that if the init sections are freed and say used by other
loaded modules), then the trace output would incorrectly show a
different symbol 

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Steven Rostedt]

On Fri, 6 Oct 2017 23:41:25 -0700
"Joel Fernandes (Google)"  wrote:

> Hi Steve,
> 
> On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
> > From: "Steven Rostedt (VMware)" 
> >
> > The ftrace_mod_map is a descriptor to save module init function names in
> > case they were traced, and the trace output needs to reference the function
> > name from the function address. But after the function is unloaded, it
> > the maps should be freed, as the rest of the function names are as well.  
> 
> Just checking for my understanding of this patch - wouldn't this also
> mean that if there were any look ups of the init functions that may be
> needed at trace output time, then those look ups wont be possible any
> more after module is unloaded?

Yes. That's true for all functions in the module. When a module is
unloaded, all references to it in kallsyms is also freed. Try it on a
current kernel. Trace a function in a module, then unload that module.
The trace data will just show the ip hex address of the module function
after that.

> 
> I guess having a reference somehow on the ftrace_mod_map descriptor if
> there are any entries in the trace buffer that need it can help
> prevent that but it could be too expensive for not much return since
> most likely the user wouldn't unload modules before trace collection
> in normal usage.

Right, I have thought about this, and I haven't come up with an
inexpensive way to do this. As this has been the default operation of
all module functions, and I haven't heard much complaining about it (I
think I may have had a single complaint), I didn't put too much effort
into it.

I need to look at the approach that Jessica sent me. Perhaps there's
ways to have all module function names be saved by a trace. But mapping
which trace buffer has these names may be difficult.

-- Steve

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Steven Rostedt]

On Fri, 6 Oct 2017 23:41:25 -0700
"Joel Fernandes (Google)"  wrote:

> Hi Steve,
> 
> On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
> > From: "Steven Rostedt (VMware)" 
> >
> > The ftrace_mod_map is a descriptor to save module init function names in
> > case they were traced, and the trace output needs to reference the function
> > name from the function address. But after the function is unloaded, it
> > the maps should be freed, as the rest of the function names are as well.  
> 
> Just checking for my understanding of this patch - wouldn't this also
> mean that if there were any look ups of the init functions that may be
> needed at trace output time, then those look ups wont be possible any
> more after module is unloaded?

Yes. That's true for all functions in the module. When a module is
unloaded, all references to it in kallsyms is also freed. Try it on a
current kernel. Trace a function in a module, then unload that module.
The trace data will just show the ip hex address of the module function
after that.

> 
> I guess having a reference somehow on the ftrace_mod_map descriptor if
> there are any entries in the trace buffer that need it can help
> prevent that but it could be too expensive for not much return since
> most likely the user wouldn't unload modules before trace collection
> in normal usage.

Right, I have thought about this, and I haven't come up with an
inexpensive way to do this. As this has been the default operation of
all module functions, and I haven't heard much complaining about it (I
think I may have had a single complaint), I didn't put too much effort
into it.

I need to look at the approach that Jessica sent me. Perhaps there's
ways to have all module function names be saved by a trace. But mapping
which trace buffer has these names may be difficult.

-- Steve

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes (Google)]

Hi Steve,

On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
> From: "Steven Rostedt (VMware)" 
>
> The ftrace_mod_map is a descriptor to save module init function names in
> case they were traced, and the trace output needs to reference the function
> name from the function address. But after the function is unloaded, it
> the maps should be freed, as the rest of the function names are as well.

Just checking for my understanding of this patch - wouldn't this also
mean that if there were any look ups of the init functions that may be
needed at trace output time, then those look ups wont be possible any
more after module is unloaded?

I guess having a reference somehow on the ftrace_mod_map descriptor if
there are any entries in the trace buffer that need it can help
prevent that but it could be too expensive for not much return since
most likely the user wouldn't unload modules before trace collection
in normal usage.

thanks,

- Joel

Re: [for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Joel Fernandes (Google)]

Hi Steve,

On Fri, Oct 6, 2017 at 11:07 AM, Steven Rostedt  wrote:
> From: "Steven Rostedt (VMware)" 
>
> The ftrace_mod_map is a descriptor to save module init function names in
> case they were traced, and the trace output needs to reference the function
> name from the function address. But after the function is unloaded, it
> the maps should be freed, as the rest of the function names are as well.

Just checking for my understanding of this patch - wouldn't this also
mean that if there were any look ups of the init functions that may be
needed at trace output time, then those look ups wont be possible any
more after module is unloaded?

I guess having a reference somehow on the ftrace_mod_map descriptor if
there are any entries in the trace buffer that need it can help
prevent that but it could be too expensive for not much return since
most likely the user wouldn't unload modules before trace collection
in normal usage.

thanks,

- Joel

[for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Steven Rostedt]

From: "Steven Rostedt (VMware)" 

The ftrace_mod_map is a descriptor to save module init function names in
case they were traced, and the trace output needs to reference the function
name from the function address. But after the function is unloaded, it
the maps should be freed, as the rest of the function names are as well.

Signed-off-by: Steven Rostedt (VMware) 
---
 kernel/trace/ftrace.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 86dbbfb353db..a5824408bed9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5683,6 +5683,7 @@ struct ftrace_mod_func {
 };
 
 struct ftrace_mod_map {
+   struct rcu_head rcu;
struct list_headlist;
struct module   *mod;
unsigned long   start_addr;
@@ -5694,6 +5695,8 @@ struct ftrace_mod_map {
 
 #define next_to_ftrace_page(p) container_of(p, struct ftrace_page, next)
 
+static LIST_HEAD(ftrace_mod_maps);
+
 static int referenced_filters(struct dyn_ftrace *rec)
 {
struct ftrace_ops *ops;
@@ -5747,8 +5750,26 @@ static void clear_mod_from_hashes(struct ftrace_page *pg)
mutex_unlock(_types_lock);
 }
 
+static void ftrace_free_mod_map(struct rcu_head *rcu)
+{
+   struct ftrace_mod_map *mod_map = container_of(rcu, struct 
ftrace_mod_map, rcu);
+   struct ftrace_mod_func *mod_func;
+   struct ftrace_mod_func *n;
+
+   /* All the contents of mod_map are now not visible to readers */
+   list_for_each_entry_safe(mod_func, n, _map->funcs, list) {
+   kfree(mod_func->name);
+   list_del(_func->list);
+   kfree(mod_func);
+   }
+
+   kfree(mod_map);
+}
+
 void ftrace_release_mod(struct module *mod)
 {
+   struct ftrace_mod_map *mod_map;
+   struct ftrace_mod_map *n;
struct dyn_ftrace *rec;
struct ftrace_page **last_pg;
struct ftrace_page *tmp_page = NULL;
@@ -5760,6 +5781,14 @@ void ftrace_release_mod(struct module *mod)
if (ftrace_disabled)
goto out_unlock;
 
+   list_for_each_entry_safe(mod_map, n, _mod_maps, list) {
+   if (mod_map->mod == mod) {
+   list_del_rcu(_map->list);
+   call_rcu_sched(_map->rcu, ftrace_free_mod_map);
+   break;
+   }
+   }
+
/*
 * Each module has its own ftrace_pages, remove
 * them from the list.
@@ -5914,8 +5943,6 @@ static void save_ftrace_mod_rec(struct ftrace_mod_map 
*mod_map,
list_add_rcu(_func->list, _map->funcs);
 }
 
-static LIST_HEAD(ftrace_mod_maps);
-
 static struct ftrace_mod_map *
 allocate_ftrace_mod_map(struct module *mod,
unsigned long start, unsigned long end)
@@ -5974,6 +6001,7 @@ ftrace_mod_address_lookup(unsigned long addr, unsigned 
long *size,
struct ftrace_mod_map *mod_map;
const char *ret = NULL;
 
+   /* mod_map is freed via call_rcu_sched() */
preempt_disable();
list_for_each_entry_rcu(mod_map, _mod_maps, list) {
ret = ftrace_func_address_lookup(mod_map, addr, size, off, sym);
-- 
2.13.2

[for-next][PATCH 15/16] ftrace: Add freeing algorithm to free ftrace_mod_maps

[Steven Rostedt]

From: "Steven Rostedt (VMware)" 

The ftrace_mod_map is a descriptor to save module init function names in
case they were traced, and the trace output needs to reference the function
name from the function address. But after the function is unloaded, it
the maps should be freed, as the rest of the function names are as well.

Signed-off-by: Steven Rostedt (VMware) 
---
 kernel/trace/ftrace.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 86dbbfb353db..a5824408bed9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5683,6 +5683,7 @@ struct ftrace_mod_func {
 };
 
 struct ftrace_mod_map {
+   struct rcu_head rcu;
struct list_headlist;
struct module   *mod;
unsigned long   start_addr;
@@ -5694,6 +5695,8 @@ struct ftrace_mod_map {
 
 #define next_to_ftrace_page(p) container_of(p, struct ftrace_page, next)
 
+static LIST_HEAD(ftrace_mod_maps);
+
 static int referenced_filters(struct dyn_ftrace *rec)
 {
struct ftrace_ops *ops;
@@ -5747,8 +5750,26 @@ static void clear_mod_from_hashes(struct ftrace_page *pg)
mutex_unlock(_types_lock);
 }
 
+static void ftrace_free_mod_map(struct rcu_head *rcu)
+{
+   struct ftrace_mod_map *mod_map = container_of(rcu, struct 
ftrace_mod_map, rcu);
+   struct ftrace_mod_func *mod_func;
+   struct ftrace_mod_func *n;
+
+   /* All the contents of mod_map are now not visible to readers */
+   list_for_each_entry_safe(mod_func, n, _map->funcs, list) {
+   kfree(mod_func->name);
+   list_del(_func->list);
+   kfree(mod_func);
+   }
+
+   kfree(mod_map);
+}
+
 void ftrace_release_mod(struct module *mod)
 {
+   struct ftrace_mod_map *mod_map;
+   struct ftrace_mod_map *n;
struct dyn_ftrace *rec;
struct ftrace_page **last_pg;
struct ftrace_page *tmp_page = NULL;
@@ -5760,6 +5781,14 @@ void ftrace_release_mod(struct module *mod)
if (ftrace_disabled)
goto out_unlock;
 
+   list_for_each_entry_safe(mod_map, n, _mod_maps, list) {
+   if (mod_map->mod == mod) {
+   list_del_rcu(_map->list);
+   call_rcu_sched(_map->rcu, ftrace_free_mod_map);
+   break;
+   }
+   }
+
/*
 * Each module has its own ftrace_pages, remove
 * them from the list.
@@ -5914,8 +5943,6 @@ static void save_ftrace_mod_rec(struct ftrace_mod_map 
*mod_map,
list_add_rcu(_func->list, _map->funcs);
 }
 
-static LIST_HEAD(ftrace_mod_maps);
-
 static struct ftrace_mod_map *
 allocate_ftrace_mod_map(struct module *mod,
unsigned long start, unsigned long end)
@@ -5974,6 +6001,7 @@ ftrace_mod_address_lookup(unsigned long addr, unsigned 
long *size,
struct ftrace_mod_map *mod_map;
const char *ret = NULL;
 
+   /* mod_map is freed via call_rcu_sched() */
preempt_disable();
list_for_each_entry_rcu(mod_map, _mod_maps, list) {
ret = ftrace_func_address_lookup(mod_map, addr, size, off, sym);
-- 
2.13.2