Re: Re: [PATCH v2 2/2] ftrace: clean up of using ftrace_event_enable_disable()

[Zhaolei]

Frederic Weisbecker wrote:
> On Fri, May 22, 2009 at 06:06:20PM +0800, Zhaolei wrote:
>> Always use tracing_stop_cmdline_record() to enable/disable a event.
>>
>> Impact: cleanup, no functionality changed
>>
>> Signed-off-by: Zhao Lei <zhaolei@xxxxxxxxxxxxxx>
>> ---
>>  kernel/trace/trace_events.c |   44 +++++++++++++-----------------------------
>>  1 files changed, 14 insertions(+), 30 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index df35e5e..16ef47a 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -76,26 +76,9 @@ static void trace_destroy_fields(struct ftrace_event_call *call)
>>  
>>  #endif /* CONFIG_MODULES */
>>  
>> -static void ftrace_clear_events(void)
>> -{
>> -	struct ftrace_event_call *call;
>> -
>> -	mutex_lock(&event_mutex);
>> -	list_for_each_entry(call, &ftrace_events, list) {
> 
> 
> Don't we have a "for_each_event" ?

Hello, Frederic

Thanks for your review.

IMHO, for_each_event is for iter each tracepoints in one module(or kernel),
but we need to iter whole tracepoints in manage.
So, list_for_each_entry(call, &ftrace_events, list) maybe better here.

Thanks
Zhaolei

> 
> 
>> -
>> -		if (call->enabled) {
>> -			call->enabled = 0;
>> -			tracing_stop_cmdline_record();
>> -			call->unregfunc();
>> -		}
>> -	}
>> -	mutex_unlock(&event_mutex);
>> -}
>> -
>>  static void ftrace_event_enable_disable(struct ftrace_event_call *call,
>>  					int enable)
>>  {
>> -
>>  	switch (enable) {
>>  	case 0:
>>  		if (call->enabled) {
>> @@ -114,6 +97,17 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
>>  	}
>>  }
>>  
>> +static void ftrace_clear_events(void)
>> +{
>> +	struct ftrace_event_call *call;
>> +
>> +	mutex_lock(&event_mutex);
>> +	list_for_each_entry(call, &ftrace_events, list) {
>> +		ftrace_event_enable_disable(call, 0);
>> +	}
>> +	mutex_unlock(&event_mutex);
>> +}
>> +
>>  /*
>>   * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
>>   */
>> @@ -1059,11 +1053,7 @@ static void trace_module_remove_events(struct module *mod)
>>  	list_for_each_entry_safe(call, p, &ftrace_events, list) {
>>  		if (call->mod == mod) {
>>  			found = true;
>> -			if (call->enabled) {
>> -				call->enabled = 0;
>> -				tracing_stop_cmdline_record();
>> -				call->unregfunc();
>> -			}
>> +			ftrace_event_enable_disable(call, 0);
>>  			if (call->event)
>>  				unregister_ftrace_event(call->event);
>>  			debugfs_remove_recursive(call->dir);
>> @@ -1265,15 +1255,9 @@ static __init void event_trace_self_tests(void)
>>  			continue;
>>  		}
>>  
>> -		call->enabled = 1;
>> -		tracing_start_cmdline_record();
>> -		call->regfunc();
>> -
>> +		ftrace_event_enable_disable(call, 1);
>>  		event_test_stuff();
>> -
>> -		call->unregfunc();
>> -		tracing_stop_cmdline_record();
>> -		call->enabled = 0;
>> +		ftrace_event_enable_disable(call, 0);
>>  
>>  		pr_cont("OK\n");
>>  	}
> 
> 
> Acked-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/