Re: [PATCH] markers: remove 2 exported symbols

[Lai Jiangshan <laijs@cn.fujitsu.com>]

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
>> __mark_empty_function() and marker_probe_cb_noarg()
>> should not be seen by outer code. this patch remove them.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>> diff --git a/include/linux/marker.h b/include/linux/marker.h
>> index 1290653..f4d4d28 100644
>> --- a/include/linux/marker.h
>> +++ b/include/linux/marker.h
>> @@ -132,12 +132,8 @@ static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
>>  			___mark_check_format(format, ## args);		\
>>  	} while (0)
>>  
>> -extern marker_probe_func __mark_empty_function;
>> -
> 
> Hi Lai,
> 
> Hrm ? Have a good look at the macro __trace_mark() in
> include/linux/marker.h, you'll see that __mark_empty_function is
> referenced. Have you tested this against code with declared markers ?

Sorry for this,
I have markers in my kernel test code.
I hasn't tested this patch, for I thought it's to simple.
I used "grep" to find "__mark_empty_function",
but I missed one line of the results.

Other problems:
1)
why we need marker_probe_cb_noarg()?
marker_probe_cb_noarg() has no performance optimization,
and no additional format check, or other thing?

if we remove marker_probe_cb_noarg, we can remove struct marker.call also.

2)
why we use va_list *?
As I know, sizeof(va_list) = 4 or 8.


please ignore this patch.

Thanks, Lai.

> 
>>  extern void marker_probe_cb(const struct marker *mdata,
>>  	void *call_private, ...);
>> -extern void marker_probe_cb_noarg(const struct marker *mdata,
>> -	void *call_private, ...);
> 
> This second change is correct. marker_probe_cb is referenced by
> __trace_mark(), but not marker_probe_cb_noarg, which is only connected
> when non-empty format string is found by the registration function in
> marker.c.
> 
>>  
>>  /*
>>   * Connect a probe to a marker.
>> diff --git a/kernel/marker.c b/kernel/marker.c
>> index 7d1faec..4440a09 100644
>> --- a/kernel/marker.c
>> +++ b/kernel/marker.c
>> @@ -81,11 +81,10 @@ static struct hlist_head marker_table[MARKER_TABLE_SIZE];
>>   * though the function pointer change and the marker enabling are two distinct
>>   * operations that modifies the execution flow of preemptible code.
>>   */
>> -void __mark_empty_function(void *probe_private, void *call_private,
>> +static void __mark_empty_function(void *probe_private, void *call_private,
>>  	const char *fmt, va_list *args)
>>  {
>>  }
>> -EXPORT_SYMBOL_GPL(__mark_empty_function);
>>  
> 
> Same as comment above.
> 
>>  /*
>>   * marker_probe_cb Callback that prepares the variable argument list for probes.
>> @@ -157,7 +156,7 @@ EXPORT_SYMBOL_GPL(marker_probe_cb);
>>   *
>>   * Should be connected to markers "MARK_NOARGS".
>>   */
>> -void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...)
>> +static void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...)
>>  {
>>  	va_list args;	/* not initialized */
>>  	char ptype;
>> @@ -197,7 +196,6 @@ void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...)
>>  	}
>>  	preempt_enable();
>>  }
>> -EXPORT_SYMBOL_GPL(marker_probe_cb_noarg);
>>  
> 
> This one is ok.
> 
> So overall, if you could check why you have not hit any problem when
> removing __mark_empty_function, that would be great. The only reason I
> see is that you had no markers in your kernel test code.
> 
> Mathieu
> 
>>  static void free_old_closure(struct rcu_head *head)
>>  {
>>
>>
>>
>