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:
> >> 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.
> 
> marker_probe_cb_noarg()/marker_probe_cb() are really critical path,
> but I think saving a "va_start" is not performance optimization.
> "va_start" is just several machine instructions after compiled.
> 
> if marker_probe_cb_noarg() is removed, kernel size will be reduced
> also, and cache missing will be reduced.
> 

I'd like some performance numbers on this. A good way I found to test
this is to run tbench with LTTng connected on the default markers, with
flight recorder tracing on. It's a good macro-benchmark (although I've
seen 100MB/s (over 1900MB/s) difference between -rc6 and -rc7, so it's
easily influenced by kernel changes). The other thing I do is to use the
specialized test modules I created, available at
http://ltt.polymtl.ca/svn/trunk/tests/kernel/. They basically loop doing
the same operation (e.g. calling a marker) so you can see how fast the
operation is in terms of cycles-per-loop. It's always cache-hot however.


> > 
> [...]
> >>
> >> 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.
> 
> va_list is platform-dependent, but it's transplantable. So I don't think
> it's a problem.
> 

See my comment in marker.h :

 * @args: variable argument list pointer. Use a pointer to overcome C's
 *        inability to pass this around as a pointer in a portable manner in
 *        the callee otherwise.

It's an information hard to find on the web (cannot find my original
source anymore, it's mainly through forums saying that the
http://c-faq.com/varargs/handoff.html _doesn't_ work), but you'll
understand that promotion of array to pointer when passed to a function
poses problem when you try to pass this array to another function. The
following won't work on architectures where va_list is defined as an
array :

void C(const char *fmt, va_list argp)
{
  ....
}

void B(const char *fmt, va_list argp)
{
  C(fmt, argp);  <--- this won't work, because we try to pass a pointer
                      to a function expecting an array.
}

void A(const char *fmt, ...)
{
  va_list argp;

  argp = va_start(fmt);
  B(fmt, argp);
  va_end(argp);
}

The way to permit it is to pass a pointer to argp instead :

void C(const char *fmt, va_list *argp)
{
  ....
}

void B(const char *fmt, va_list *argp)
{
  C(fmt, argp);
}

void A(const char *fmt, ...)
{
  va_list argp;

  argp = va_start(fmt);
  B(fmt, &argp);
  va_end(argp);
}

Mathieu

> And pass-by-value vs. pass-by-reference:
> marker_probe_cb() don't need see what have been changed with "args"
> by the probes/callbacks.
> 
> So I think pass-by-value is better than pass-by-reference here.
> 
> code piece:
>  typedef void marker_probe_func(void *probe_private, void *call_private,
> -		const char *fmt, va_list *args);
> +		const char *fmt, va_list args);
> 
> marker_probe_cb():
> 		multi = mdata->multi;
> +		va_start(args, call_private);
> 		for (i = 0; multi[i].func; i++) {
> -			va_start(args, call_private);
> 			multi[i].func(multi[i].probe_private, call_private,
> -				mdata->format, &args);
> -			va_end(args);
> +				mdata->format, args);
> 		}
> +		va_end(args);
> 
> 
> The only problem is that API is changed, and we need changed LTTng
> and SYSTEMTAP also.
> 
> 
> 
> > 
> > Mathieu
> > 
> [...]
> 

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