Re: [RFC PATCH 1/2] Marker probes in futex.c

["K. Prasad" <prasad@linux.vnet.ibm.com>]

On Thu, Apr 17, 2008 at 04:16:44PM -0400, Mathieu Desnoyers wrote:
> * Frank Ch. Eigler (fche@redhat.com) wrote:
> [..]
> > > >+       trace_mark(futex_wake_called, "uaddr:%p fshared:%p nr_wake:%d "
> > > > +                       "bitset:%d",
> > > > +                       uaddr, fshared, nr_wake, bitset);
> > > 
> > > > +       INIT_FUTEX_DEBUG_PROBE(futex_wake_called,
> > > > +                       "uaddr:%p fshared:%p nr_wake:%d bitset:%d"),
> > > 
> > > Why the need to duplicate it; that's utter madness.
> > 
> > This second instance is optional and is used as a consistency check
> > for the event consumer to hook up exactly to the intended producer.
> > The string could be empty.
> > 
> 
> empty -> NULL , yes :)
Atleast until 2.6.25-rc8-mm2, I would expect the marker registration to
fail if the format strings don't match.
I find an early return in set_marker() in marker.c if the format
strings don't match.

Mathieu,
	Can you let me know if you would find the patch below - which
eliminates the need to pass format string to marker_probe_register()
function, acceptable?
I have done some elementary tests with the patch, but I know it would
fail if there are duplicate markers with different format strings.
However I presume that duplicate markers are meant to appear primarily
when markers are present in inlined functions (in which case their
format strings would be the same).

Thanks,
K.Prasad

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 include/linux/marker.h          |    4 ++--
 kernel/marker.c                 |   40 +++++++++++++++++-----------------------
 samples/markers/probe-example.c |    3 ---
 3 files changed, 19 insertions(+), 28 deletions(-)

Index: linux-register_marker_patch/kernel/marker.c
===================================================================
--- linux-register_marker_patch.orig/kernel/marker.c
+++ linux-register_marker_patch/kernel/marker.c
@@ -364,7 +364,7 @@ static struct marker_entry *get_marker(c
  * Add the marker to the marker hash table. Must be called with markers_mutex
  * held.
  */
-static struct marker_entry *add_marker(const char *name, const char *format)
+static struct marker_entry *add_marker(const char *name)
 {
 	struct hlist_head *head;
 	struct hlist_node *node;
@@ -372,9 +372,15 @@ static struct marker_entry *add_marker(c
 	size_t name_len = strlen(name) + 1;
 	size_t format_len = 0;
 	u32 hash = jhash(name, name_len-1, 0);
+	struct marker *iter;
+
+	/* Search for the marker with 'name' in __markers section */
+	for (iter = __start___markers;
+		((iter < __stop___markers) && (strcmp(name, iter->name)));
+			iter++);
 
-	if (format)
-		format_len = strlen(format) + 1;
+	if (iter->format)
+		format_len = strlen(iter->format) + 1;
 	head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
 	hlist_for_each_entry(e, node, head, hlist) {
 		if (!strcmp(name, e->name)) {
@@ -392,9 +398,9 @@ static struct marker_entry *add_marker(c
 	if (!e)
 		return ERR_PTR(-ENOMEM);
 	memcpy(&e->name[0], name, name_len);
-	if (format) {
+	if (iter->format) {
 		e->format = &e->name[name_len];
-		memcpy(e->format, format, format_len);
+		memcpy(e->format, iter->format, format_len);
 		if (strcmp(e->format, MARK_NOARGS) == 0)
 			e->call = marker_probe_cb_noarg;
 		else
@@ -494,21 +500,9 @@ static int set_marker(struct marker_entr
 	int ret;
 	WARN_ON(strcmp((*entry)->name, elem->name) != 0);
 
-	if ((*entry)->format) {
-		if (strcmp((*entry)->format, elem->format) != 0) {
-			printk(KERN_NOTICE
-				"Format mismatch for probe %s "
-				"(%s), marker (%s)\n",
-				(*entry)->name,
-				(*entry)->format,
-				elem->format);
-			return -EPERM;
-		}
-	} else {
-		ret = marker_set_format(entry, elem->format);
-		if (ret)
-			return ret;
-	}
+	ret = marker_set_format(entry, elem->format);
+	if (ret)
+		return ret;
 
 	/*
 	 * probe_cb setup (statically known) is done here. It is
@@ -638,8 +632,8 @@ static void marker_update_probes(void)
  * Returns 0 if ok, error value on error.
  * The probe address must at least be aligned on the architecture pointer size.
  */
-int marker_probe_register(const char *name, const char *format,
-			marker_probe_func *probe, void *probe_private)
+int marker_probe_register(const char *name, marker_probe_func *probe,
+							void *probe_private)
 {
 	struct marker_entry *entry;
 	int ret = 0;
@@ -648,7 +642,7 @@ int marker_probe_register(const char *na
 	mutex_lock(&markers_mutex);
 	entry = get_marker(name);
 	if (!entry) {
-		entry = add_marker(name, format);
+		entry = add_marker(name);
 		if (IS_ERR(entry)) {
 			ret = PTR_ERR(entry);
 			goto end;
Index: linux-register_marker_patch/include/linux/marker.h
===================================================================
--- linux-register_marker_patch.orig/include/linux/marker.h
+++ linux-register_marker_patch/include/linux/marker.h
@@ -119,8 +119,8 @@ extern void marker_probe_cb_noarg(const 
  * Connect a probe to a marker.
  * private data pointer must be a valid allocated memory address, or NULL.
  */
-extern int marker_probe_register(const char *name, const char *format,
-				marker_probe_func *probe, void *probe_private);
+extern int marker_probe_register(const char *name, marker_probe_func *probe,
+				void *probe_private);
 
 /*
  * Returns the private data given to marker_probe_register.
Index: linux-register_marker_patch/samples/markers/probe-example.c
===================================================================
--- linux-register_marker_patch.orig/samples/markers/probe-example.c
+++ linux-register_marker_patch/samples/markers/probe-example.c
@@ -49,10 +49,8 @@ void probe_subsystem_eventb(void *probe_
 static struct probe_data probe_array[] =
 {
 	{	.name = "subsystem_event",
-		.format = "integer %d string %s",
 		.probe_func = probe_subsystem_event },
 	{	.name = "subsystem_eventb",
-		.format = MARK_NOARGS,
 		.probe_func = probe_subsystem_eventb },
 };
 
@@ -63,7 +61,6 @@ static int __init probe_init(void)
 
 	for (i = 0; i < ARRAY_SIZE(probe_array); i++) {
 		result = marker_probe_register(probe_array[i].name,
-				probe_array[i].format,
 				probe_array[i].probe_func, &probe_array[i]);
 		if (result)
 			printk(KERN_INFO "Unable to register probe %s\n",