Messages in this thread | | | Date | Thu, 09 Oct 2008 10:19:21 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH] markers: remove 2 exported symbols |
| |
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.
> [...] >> >> 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.
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 > [...]
| |