markers: remove 2 exported symbols

__mark_empty_function() and marker_probe_cb_noarg() should not be seen by outer code. this patch remove them.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/linux/marker.h +0 -0

index 1290653..f4d4d28 100644

include/linux/marker.h +0 -0

 /*
  * Connect a probe to a marker.

kernel/marker.c +0 -0

index 7d1faec..4440a09 100644

kernel/marker.c +1 -1

 /*
  * 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;

kernel/marker.c +0 -0

 static void free_old_closure(struct rcu_head *head)
 {

Quoted 13 lines · * Lai Jiangshan ([email protected]) 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 <[email protected]>

---

include/linux/marker.h +0 -0

index 1290653..f4d4d28 100644
@@ -132,12 +132,8 @@ static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
 			___mark_check_format(format, ## args);		\
 	} while (0)
 

include/linux/marker.h static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...) · line 136

Quoted 2 lines

include/linux/marker.h +0 -2

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

include/linux/marker.h static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...) · line 140

Quoted 5 lines

include/linux/marker.h +0 -2

 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.

Quoted 8 lines

include/linux/marker.h +0 -0

 /*
  * Connect a probe to a marker.

kernel/marker.c +0 -0

index 7d1faec..4440a09 100644

kernel/marker.c static struct hlist_head marker_table[MARKER_TABLE_SIZE]; · line 88

Quoted 10 lines

kernel/marker.c +1 -2

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

Quoted 12 lines

kernel/marker.c +1 -1

 /*
  * 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;

kernel/marker.c void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...) · line 199

Quoted 5 lines

kernel/marker.c +0 -1

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

Quoted 5 lines

kernel/marker.c +0 -0

 static void free_old_closure(struct rcu_head *head)
 {

Quoted 14 lines · Mathieu Desnoyers

Quoted 13 lines · * Lai Jiangshan ([email protected]) 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 <[email protected]>

---

include/linux/marker.h +0 -0

index 1290653..f4d4d28 100644
@@ -132,12 +132,8 @@ static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
 			___mark_check_format(format, ## args);		\
 	} while (0)
 

Quoted 8 lines

include/linux/marker.h static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...) · line 136

Quoted 2 lines

include/linux/marker.h +0 -2

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

Quoted 64 lines

include/linux/marker.h static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...) · line 140

Quoted 5 lines

include/linux/marker.h +0 -2

 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.

Quoted 8 lines

include/linux/marker.h +0 -0

 /*
  * Connect a probe to a marker.

kernel/marker.c +0 -0

index 7d1faec..4440a09 100644

kernel/marker.c static struct hlist_head marker_table[MARKER_TABLE_SIZE]; · line 88

Quoted 10 lines

kernel/marker.c +1 -2

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

Quoted 12 lines

kernel/marker.c +1 -1

 /*
  * 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;

kernel/marker.c void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...) · line 199

Quoted 5 lines

kernel/marker.c +0 -1

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

Quoted 5 lines

kernel/marker.c +0 -0

 static void free_old_closure(struct rcu_head *head)
 {

Quoted 28 lines · * Lai Jiangshan ([email protected]) wrote:

Quoted 14 lines · Mathieu Desnoyers

Quoted 13 lines · * Lai Jiangshan ([email protected]) 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 <[email protected]>

---

include/linux/marker.h +0 -0

index 1290653..f4d4d28 100644
@@ -132,12 +132,8 @@ static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
 			___mark_check_format(format, ## args);		\
 	} while (0)
 

Quoted 7 lines

include/linux/marker.h static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...) · line 136

Quoted 2 lines

include/linux/marker.h +0 -2

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

Quoted 6 lines

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.

Quoted 5 lines

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

Quoted 36 lines · Mathieu Desnoyers

Quoted 28 lines · * Lai Jiangshan ([email protected]) wrote:

Quoted 14 lines · Mathieu Desnoyers

Quoted 13 lines · * Lai Jiangshan ([email protected]) 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 <[email protected]>

---

include/linux/marker.h +0 -0

index 1290653..f4d4d28 100644
@@ -132,12 +132,8 @@ static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
 			___mark_check_format(format, ## args);		\
 	} while (0)
 

Quoted 7 lines

include/linux/marker.h static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...) · line 136

Quoted 2 lines

include/linux/marker.h +0 -2

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

Quoted 6 lines

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?

Quoted 3 lines

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.

Quoted 1 line

[...]

Quoted 6 lines

Quoted 5 lines

2)
why we use va_list *?
As I know, sizeof(va_list) = 4 or 8.

Quoted 5 lines

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,

(quoted diff) +1 -1

-		const char *fmt, va_list *args);
+		const char *fmt, va_list args);

marker_probe_cb():

		multi = mdata->multi;

(quoted diff) +1 -0

+		va_start(args, call_private);

		for (i = 0; multi[i].func; i++) {

(quoted diff) +0 -1

-			va_start(args, call_private);

multi[i].func(multi[i].probe_private, call_private,

(quoted diff) +1 -2

-				mdata->format, &args);
-			va_end(args);
+				mdata->format, args);

		}

(quoted diff) +1 -0

+		va_end(args);

The only problem is that API is changed, and we need changed LTTng and SYSTEMTAP also.

Quoted 3 lines

Mathieu

[...]

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