lkml.org 
[lkml]   [2008]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>
[...]



\
 
 \ /
  Last update: 2008-10-09 04:39    [W:0.053 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site