Re: [PATCH] markers: remove 2 exported symbols

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> 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?
> 

marker_probe_cb_noarg() does not need to setup the variable arguments,
because the format string explicitly contains the MARK_NOARGS string. So
this is a performance optimization.

> 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.
> 

It becomes hellish when we want to pass it as parameter to another C
function, because va_list is typedef'd as an array on some
architectures, and the array gets propoted to a pointer type, which is
in turn incompatible with the array. C language mess :-( Not much we can
do about it.

Mathieu

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

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68