public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] markers: remove 2 exported symbols
@ 2008-10-08  2:23 Lai Jiangshan
  2008-10-08  2:43 ` Mathieu Desnoyers
  0 siblings, 1 reply; 7+ messages in thread
From: Lai Jiangshan @ 2008-10-08  2:23 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Mathieu Desnoyers, Linux Kernel Mailing List


__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;
-
 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, ...);
 
 /*
  * 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);
 
 /*
  * 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);
 
 static void free_old_closure(struct rcu_head *head)
 {




^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] markers: remove 2 exported symbols
  2008-10-08  2:23 [PATCH] markers: remove 2 exported symbols Lai Jiangshan
@ 2008-10-08  2:43 ` Mathieu Desnoyers
  2008-10-08  3:05   ` Lai Jiangshan
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Desnoyers @ 2008-10-08  2:43 UTC (permalink / raw)
  To: Lai Jiangshan; +Cc: Ingo Molnar, Linux Kernel Mailing List

* 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 ?

>  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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] markers: remove 2 exported symbols
  2008-10-08  2:43 ` Mathieu Desnoyers
@ 2008-10-08  3:05   ` Lai Jiangshan
  2008-10-08  3:53     ` Mathieu Desnoyers
  0 siblings, 1 reply; 7+ messages in thread
From: Lai Jiangshan @ 2008-10-08  3:05 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: Ingo Molnar, Linux Kernel Mailing List

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] markers: remove 2 exported symbols
  2008-10-08  3:05   ` Lai Jiangshan
@ 2008-10-08  3:53     ` Mathieu Desnoyers
  2008-10-09  2:19       ` Lai Jiangshan
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Desnoyers @ 2008-10-08  3:53 UTC (permalink / raw)
  To: Lai Jiangshan; +Cc: Ingo Molnar, Linux Kernel Mailing List

* 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] markers: remove 2 exported symbols
  2008-10-08  3:53     ` Mathieu Desnoyers
@ 2008-10-09  2:19       ` Lai Jiangshan
  2008-10-09 14:27         ` Mathieu Desnoyers
  0 siblings, 1 reply; 7+ messages in thread
From: Lai Jiangshan @ 2008-10-09  2:19 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: Ingo Molnar, Linux Kernel Mailing List

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] markers: remove 2 exported symbols
  2008-10-09  2:19       ` Lai Jiangshan
@ 2008-10-09 14:27         ` Mathieu Desnoyers
  2008-10-10  8:02           ` Lai Jiangshan
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Desnoyers @ 2008-10-09 14:27 UTC (permalink / raw)
  To: Lai Jiangshan; +Cc: Ingo Molnar, Linux Kernel Mailing List

* 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] markers: remove 2 exported symbols
  2008-10-09 14:27         ` Mathieu Desnoyers
@ 2008-10-10  8:02           ` Lai Jiangshan
  0 siblings, 0 replies; 7+ messages in thread
From: Lai Jiangshan @ 2008-10-10  8:02 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: Ingo Molnar, Linux Kernel Mailing List

Mathieu Desnoyers wrote:
> 
> 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
>

Hi Mathieu,

I understood, and the comp.lang.c FAQ seems very authoritative.
but in the kernel source I found these:

asmlinkage int vprintk(const char *fmt, va_list args)
{
......
	printed_len += vscnprintf(printk_buf + printed_len,
				  sizeof(printk_buf) - printed_len, fmt, args);
.....
}


int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
{
	int i;

	i=vsnprintf(buf,size,fmt,args);
	return (i >= size) ? (size - 1) : i;
}

int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);


Thanks, Lai



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-10-10  8:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-08  2:23 [PATCH] markers: remove 2 exported symbols Lai Jiangshan
2008-10-08  2:43 ` Mathieu Desnoyers
2008-10-08  3:05   ` Lai Jiangshan
2008-10-08  3:53     ` Mathieu Desnoyers
2008-10-09  2:19       ` Lai Jiangshan
2008-10-09 14:27         ` Mathieu Desnoyers
2008-10-10  8:02           ` Lai Jiangshan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox