[PATCH tip/tracing/markers] markers: simplify marker_set_format()

[PATCH tip/tracing/markers] markers: simplify marker_set_format()

[Lai Jiangshan]

current marker_set_format() is complex this patch simplify it, 
and decrease the overhead of marker_update_probes().

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/marker.c b/kernel/marker.c
index e49a9c5..ef43c86 100644
--- a/kernel/marker.c
+++ b/kernel/marker.c
@@ -65,6 +65,7 @@ struct marker_entry {
 	void *oldptr;
 	int rcu_pending;
 	unsigned char ptype:1;
+	unsigned char format_allocated:1;
 	char name[0];	/* Contains name'\0'format'\0' */
 };
 
@@ -415,6 +416,7 @@ static struct marker_entry *add_marker(const char *name, const char *format)
 	e->single.probe_private = NULL;
 	e->multi = NULL;
 	e->ptype = 0;
+	e->format_allocated = 0;
 	e->refcount = 0;
 	e->rcu_pending = 0;
 	hlist_add_head(&e->hlist, head);
@@ -446,6 +448,8 @@ static int remove_marker(const char *name)
 	if (e->single.func != __mark_empty_function)
 		return -EBUSY;
 	hlist_del(&e->hlist);
+	if (e->format_allocated)
+		kfree(e->format);
 	/* Make sure the call_rcu has been executed */
 	if (e->rcu_pending)
 		rcu_barrier_sched();
@@ -456,57 +460,34 @@ static int remove_marker(const char *name)
 /*
  * Set the mark_entry format to the format found in the element.
  */
-static int marker_set_format(struct marker_entry **entry, const char *format)
+static int marker_set_format(struct marker_entry *entry, const char *format)
 {
-	struct marker_entry *e;
-	size_t name_len = strlen((*entry)->name) + 1;
-	size_t format_len = strlen(format) + 1;
-
-
-	e = kmalloc(sizeof(struct marker_entry) + name_len + format_len,
-			GFP_KERNEL);
-	if (!e)
+	entry->format = kstrdup(format, GFP_KERNEL);
+	if (!entry->format)
 		return -ENOMEM;
-	memcpy(&e->name[0], (*entry)->name, name_len);
-	e->format = &e->name[name_len];
-	memcpy(e->format, format, format_len);
-	if (strcmp(e->format, MARK_NOARGS) == 0)
-		e->call = marker_probe_cb_noarg;
-	else
-		e->call = marker_probe_cb;
-	e->single = (*entry)->single;
-	e->multi = (*entry)->multi;
-	e->ptype = (*entry)->ptype;
-	e->refcount = (*entry)->refcount;
-	e->rcu_pending = 0;
-	hlist_add_before(&e->hlist, &(*entry)->hlist);
-	hlist_del(&(*entry)->hlist);
-	/* Make sure the call_rcu has been executed */
-	if ((*entry)->rcu_pending)
-		rcu_barrier_sched();
-	kfree(*entry);
-	*entry = e;
+	entry->format_allocated = 1;
+
 	trace_mark(core_marker_format, "name %s format %s",
-			e->name, e->format);
+			entry->name, entry->format);
 	return 0;
 }
 
 /*
  * Sets the probe callback corresponding to one marker.
  */
-static int set_marker(struct marker_entry **entry, struct marker *elem,
+static int set_marker(struct marker_entry *entry, struct marker *elem,
 		int active)
 {
 	int ret;
-	WARN_ON(strcmp((*entry)->name, elem->name) != 0);
+	WARN_ON(strcmp(entry->name, elem->name) != 0);
 
-	if ((*entry)->format) {
-		if (strcmp((*entry)->format, elem->format) != 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,
+				entry->name,
+				entry->format,
 				elem->format);
 			return -EPERM;
 		}
@@ -522,34 +503,33 @@ static int set_marker(struct marker_entry **entry, struct marker *elem,
 	 * pass from a "safe" callback (with argument) to an "unsafe"
 	 * callback (does not set arguments).
 	 */
-	elem->call = (*entry)->call;
+	elem->call = entry->call;
 	/*
 	 * Sanity check :
 	 * We only update the single probe private data when the ptr is
 	 * set to a _non_ single probe! (0 -> 1 and N -> 1, N != 1)
 	 */
 	WARN_ON(elem->single.func != __mark_empty_function
-		&& elem->single.probe_private
-		!= (*entry)->single.probe_private &&
-		!elem->ptype);
-	elem->single.probe_private = (*entry)->single.probe_private;
+		&& elem->single.probe_private != entry->single.probe_private
+		&& !elem->ptype);
+	elem->single.probe_private = entry->single.probe_private;
 	/*
 	 * Make sure the private data is valid when we update the
 	 * single probe ptr.
 	 */
 	smp_wmb();
-	elem->single.func = (*entry)->single.func;
+	elem->single.func = entry->single.func;
 	/*
 	 * We also make sure that the new probe callbacks array is consistent
 	 * before setting a pointer to it.
 	 */
-	rcu_assign_pointer(elem->multi, (*entry)->multi);
+	rcu_assign_pointer(elem->multi, entry->multi);
 	/*
 	 * Update the function or multi probe array pointer before setting the
 	 * ptype.
 	 */
 	smp_wmb();
-	elem->ptype = (*entry)->ptype;
+	elem->ptype = entry->ptype;
 	elem->state = active;
 
 	return 0;
@@ -593,8 +573,7 @@ void marker_update_probe_range(struct marker *begin,
 	for (iter = begin; iter < end; iter++) {
 		mark_entry = get_marker(iter->name);
 		if (mark_entry) {
-			set_marker(&mark_entry, iter,
-					!!mark_entry->refcount);
+			set_marker(mark_entry, iter, !!mark_entry->refcount);
 			/*
 			 * ignore error, continue
 			 */
@@ -656,7 +635,7 @@ int marker_probe_register(const char *name, const char *format,
 			ret = PTR_ERR(entry);
 	} else if (format) {
 		if (!entry->format)
-			ret = marker_set_format(&entry, format);
+			ret = marker_set_format(entry, format);
 		else if (strcmp(entry->format, format))
 			ret = -EPERM;
 	}

Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format()

[Mathieu Desnoyers]

* Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> 
> current marker_set_format() is complex this patch simplify it, 
> and decrease the overhead of marker_update_probes().
> 

Yep, I think I did put the format string after the name \0, which was
rather tricky. I agree that such change makes the code more readable at
the expense of doing 2 allocations per marker rather than one, but
readability might be a good thing here.

Thanks !

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/kernel/marker.c b/kernel/marker.c
> index e49a9c5..ef43c86 100644
> --- a/kernel/marker.c
> +++ b/kernel/marker.c
> @@ -65,6 +65,7 @@ struct marker_entry {
>  	void *oldptr;
>  	int rcu_pending;
>  	unsigned char ptype:1;
> +	unsigned char format_allocated:1;
>  	char name[0];	/* Contains name'\0'format'\0' */
>  };
>  
> @@ -415,6 +416,7 @@ static struct marker_entry *add_marker(const char *name, const char *format)
>  	e->single.probe_private = NULL;
>  	e->multi = NULL;
>  	e->ptype = 0;
> +	e->format_allocated = 0;
>  	e->refcount = 0;
>  	e->rcu_pending = 0;
>  	hlist_add_head(&e->hlist, head);
> @@ -446,6 +448,8 @@ static int remove_marker(const char *name)
>  	if (e->single.func != __mark_empty_function)
>  		return -EBUSY;
>  	hlist_del(&e->hlist);
> +	if (e->format_allocated)
> +		kfree(e->format);
>  	/* Make sure the call_rcu has been executed */
>  	if (e->rcu_pending)
>  		rcu_barrier_sched();
> @@ -456,57 +460,34 @@ static int remove_marker(const char *name)
>  /*
>   * Set the mark_entry format to the format found in the element.
>   */
> -static int marker_set_format(struct marker_entry **entry, const char *format)
> +static int marker_set_format(struct marker_entry *entry, const char *format)
>  {
> -	struct marker_entry *e;
> -	size_t name_len = strlen((*entry)->name) + 1;
> -	size_t format_len = strlen(format) + 1;
> -
> -
> -	e = kmalloc(sizeof(struct marker_entry) + name_len + format_len,
> -			GFP_KERNEL);
> -	if (!e)
> +	entry->format = kstrdup(format, GFP_KERNEL);
> +	if (!entry->format)
>  		return -ENOMEM;
> -	memcpy(&e->name[0], (*entry)->name, name_len);
> -	e->format = &e->name[name_len];
> -	memcpy(e->format, format, format_len);
> -	if (strcmp(e->format, MARK_NOARGS) == 0)
> -		e->call = marker_probe_cb_noarg;
> -	else
> -		e->call = marker_probe_cb;
> -	e->single = (*entry)->single;
> -	e->multi = (*entry)->multi;
> -	e->ptype = (*entry)->ptype;
> -	e->refcount = (*entry)->refcount;
> -	e->rcu_pending = 0;
> -	hlist_add_before(&e->hlist, &(*entry)->hlist);
> -	hlist_del(&(*entry)->hlist);
> -	/* Make sure the call_rcu has been executed */
> -	if ((*entry)->rcu_pending)
> -		rcu_barrier_sched();
> -	kfree(*entry);
> -	*entry = e;
> +	entry->format_allocated = 1;
> +
>  	trace_mark(core_marker_format, "name %s format %s",
> -			e->name, e->format);
> +			entry->name, entry->format);
>  	return 0;
>  }
>  
>  /*
>   * Sets the probe callback corresponding to one marker.
>   */
> -static int set_marker(struct marker_entry **entry, struct marker *elem,
> +static int set_marker(struct marker_entry *entry, struct marker *elem,
>  		int active)
>  {
>  	int ret;
> -	WARN_ON(strcmp((*entry)->name, elem->name) != 0);
> +	WARN_ON(strcmp(entry->name, elem->name) != 0);
>  
> -	if ((*entry)->format) {
> -		if (strcmp((*entry)->format, elem->format) != 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,
> +				entry->name,
> +				entry->format,
>  				elem->format);
>  			return -EPERM;
>  		}
> @@ -522,34 +503,33 @@ static int set_marker(struct marker_entry **entry, struct marker *elem,
>  	 * pass from a "safe" callback (with argument) to an "unsafe"
>  	 * callback (does not set arguments).
>  	 */
> -	elem->call = (*entry)->call;
> +	elem->call = entry->call;
>  	/*
>  	 * Sanity check :
>  	 * We only update the single probe private data when the ptr is
>  	 * set to a _non_ single probe! (0 -> 1 and N -> 1, N != 1)
>  	 */
>  	WARN_ON(elem->single.func != __mark_empty_function
> -		&& elem->single.probe_private
> -		!= (*entry)->single.probe_private &&
> -		!elem->ptype);
> -	elem->single.probe_private = (*entry)->single.probe_private;
> +		&& elem->single.probe_private != entry->single.probe_private
> +		&& !elem->ptype);
> +	elem->single.probe_private = entry->single.probe_private;
>  	/*
>  	 * Make sure the private data is valid when we update the
>  	 * single probe ptr.
>  	 */
>  	smp_wmb();
> -	elem->single.func = (*entry)->single.func;
> +	elem->single.func = entry->single.func;
>  	/*
>  	 * We also make sure that the new probe callbacks array is consistent
>  	 * before setting a pointer to it.
>  	 */
> -	rcu_assign_pointer(elem->multi, (*entry)->multi);
> +	rcu_assign_pointer(elem->multi, entry->multi);
>  	/*
>  	 * Update the function or multi probe array pointer before setting the
>  	 * ptype.
>  	 */
>  	smp_wmb();
> -	elem->ptype = (*entry)->ptype;
> +	elem->ptype = entry->ptype;
>  	elem->state = active;
>  
>  	return 0;
> @@ -593,8 +573,7 @@ void marker_update_probe_range(struct marker *begin,
>  	for (iter = begin; iter < end; iter++) {
>  		mark_entry = get_marker(iter->name);
>  		if (mark_entry) {
> -			set_marker(&mark_entry, iter,
> -					!!mark_entry->refcount);
> +			set_marker(mark_entry, iter, !!mark_entry->refcount);
>  			/*
>  			 * ignore error, continue
>  			 */
> @@ -656,7 +635,7 @@ int marker_probe_register(const char *name, const char *format,
>  			ret = PTR_ERR(entry);
>  	} else if (format) {
>  		if (!entry->format)
> -			ret = marker_set_format(&entry, format);
> +			ret = marker_set_format(entry, format);
>  		else if (strcmp(entry->format, format))
>  			ret = -EPERM;
>  	}
> 
> 
> 
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format()

[Lai Jiangshan]

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
>> current marker_set_format() is complex this patch simplify it, 
>> and decrease the overhead of marker_update_probes().
>>
> 
> Yep, I think I did put the format string after the name \0, which was
> rather tricky. I agree that such change makes the code more readable at
> the expense of doing 2 allocations per marker rather than one, but
> readability might be a good thing here.

this fix did help for readability and made code cleanly.
but it's still 1 allocation per marker_entry when format != NULL.
the previous benefit is not lost.

Thanks, Lai

> 
> Thanks !
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> 
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>> diff --git a/kernel/marker.c b/kernel/marker.c
>> index e49a9c5..ef43c86 100644
>> --- a/kernel/marker.c
>> +++ b/kernel/marker.c
>> @@ -65,6 +65,7 @@ struct marker_entry {
>>  	void *oldptr;
>>  	int rcu_pending;
>>  	unsigned char ptype:1;
>> +	unsigned char format_allocated:1;
>>  	char name[0];	/* Contains name'\0'format'\0' */
>>  };
>>  
>> @@ -415,6 +416,7 @@ static struct marker_entry *add_marker(const char *name, const char *format)
>>  	e->single.probe_private = NULL;
>>  	e->multi = NULL;
>>  	e->ptype = 0;
>> +	e->format_allocated = 0;
>>  	e->refcount = 0;
>>  	e->rcu_pending = 0;
>>  	hlist_add_head(&e->hlist, head);
>> @@ -446,6 +448,8 @@ static int remove_marker(const char *name)
>>  	if (e->single.func != __mark_empty_function)
>>  		return -EBUSY;
>>  	hlist_del(&e->hlist);
>> +	if (e->format_allocated)
>> +		kfree(e->format);
>>  	/* Make sure the call_rcu has been executed */
>>  	if (e->rcu_pending)
>>  		rcu_barrier_sched();
>> @@ -456,57 +460,34 @@ static int remove_marker(const char *name)
>>  /*
>>   * Set the mark_entry format to the format found in the element.
>>   */
>> -static int marker_set_format(struct marker_entry **entry, const char *format)
>> +static int marker_set_format(struct marker_entry *entry, const char *format)
>>  {
>> -	struct marker_entry *e;
>> -	size_t name_len = strlen((*entry)->name) + 1;
>> -	size_t format_len = strlen(format) + 1;
>> -
>> -
>> -	e = kmalloc(sizeof(struct marker_entry) + name_len + format_len,
>> -			GFP_KERNEL);
>> -	if (!e)
>> +	entry->format = kstrdup(format, GFP_KERNEL);
>> +	if (!entry->format)
>>  		return -ENOMEM;
>> -	memcpy(&e->name[0], (*entry)->name, name_len);
>> -	e->format = &e->name[name_len];
>> -	memcpy(e->format, format, format_len);
>> -	if (strcmp(e->format, MARK_NOARGS) == 0)
>> -		e->call = marker_probe_cb_noarg;
>> -	else
>> -		e->call = marker_probe_cb;
>> -	e->single = (*entry)->single;
>> -	e->multi = (*entry)->multi;
>> -	e->ptype = (*entry)->ptype;
>> -	e->refcount = (*entry)->refcount;
>> -	e->rcu_pending = 0;
>> -	hlist_add_before(&e->hlist, &(*entry)->hlist);
>> -	hlist_del(&(*entry)->hlist);
>> -	/* Make sure the call_rcu has been executed */
>> -	if ((*entry)->rcu_pending)
>> -		rcu_barrier_sched();
>> -	kfree(*entry);
>> -	*entry = e;
>> +	entry->format_allocated = 1;
>> +
>>  	trace_mark(core_marker_format, "name %s format %s",
>> -			e->name, e->format);
>> +			entry->name, entry->format);
>>  	return 0;
>>  }
>>  
>>  /*
>>   * Sets the probe callback corresponding to one marker.
>>   */
>> -static int set_marker(struct marker_entry **entry, struct marker *elem,
>> +static int set_marker(struct marker_entry *entry, struct marker *elem,
>>  		int active)
>>  {
>>  	int ret;
>> -	WARN_ON(strcmp((*entry)->name, elem->name) != 0);
>> +	WARN_ON(strcmp(entry->name, elem->name) != 0);
>>  
>> -	if ((*entry)->format) {
>> -		if (strcmp((*entry)->format, elem->format) != 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,
>> +				entry->name,
>> +				entry->format,
>>  				elem->format);
>>  			return -EPERM;
>>  		}
>> @@ -522,34 +503,33 @@ static int set_marker(struct marker_entry **entry, struct marker *elem,
>>  	 * pass from a "safe" callback (with argument) to an "unsafe"
>>  	 * callback (does not set arguments).
>>  	 */
>> -	elem->call = (*entry)->call;
>> +	elem->call = entry->call;
>>  	/*
>>  	 * Sanity check :
>>  	 * We only update the single probe private data when the ptr is
>>  	 * set to a _non_ single probe! (0 -> 1 and N -> 1, N != 1)
>>  	 */
>>  	WARN_ON(elem->single.func != __mark_empty_function
>> -		&& elem->single.probe_private
>> -		!= (*entry)->single.probe_private &&
>> -		!elem->ptype);
>> -	elem->single.probe_private = (*entry)->single.probe_private;
>> +		&& elem->single.probe_private != entry->single.probe_private
>> +		&& !elem->ptype);
>> +	elem->single.probe_private = entry->single.probe_private;
>>  	/*
>>  	 * Make sure the private data is valid when we update the
>>  	 * single probe ptr.
>>  	 */
>>  	smp_wmb();
>> -	elem->single.func = (*entry)->single.func;
>> +	elem->single.func = entry->single.func;
>>  	/*
>>  	 * We also make sure that the new probe callbacks array is consistent
>>  	 * before setting a pointer to it.
>>  	 */
>> -	rcu_assign_pointer(elem->multi, (*entry)->multi);
>> +	rcu_assign_pointer(elem->multi, entry->multi);
>>  	/*
>>  	 * Update the function or multi probe array pointer before setting the
>>  	 * ptype.
>>  	 */
>>  	smp_wmb();
>> -	elem->ptype = (*entry)->ptype;
>> +	elem->ptype = entry->ptype;
>>  	elem->state = active;
>>  
>>  	return 0;
>> @@ -593,8 +573,7 @@ void marker_update_probe_range(struct marker *begin,
>>  	for (iter = begin; iter < end; iter++) {
>>  		mark_entry = get_marker(iter->name);
>>  		if (mark_entry) {
>> -			set_marker(&mark_entry, iter,
>> -					!!mark_entry->refcount);
>> +			set_marker(mark_entry, iter, !!mark_entry->refcount);
>>  			/*
>>  			 * ignore error, continue
>>  			 */
>> @@ -656,7 +635,7 @@ int marker_probe_register(const char *name, const char *format,
>>  			ret = PTR_ERR(entry);
>>  	} else if (format) {
>>  		if (!entry->format)
>> -			ret = marker_set_format(&entry, format);
>> +			ret = marker_set_format(entry, format);
>>  		else if (strcmp(entry->format, format))
>>  			ret = -EPERM;
>>  	}
>>
>>
>>
>>
>>
> 

Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format()

[Lai Jiangshan]

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
>> current marker_set_format() is complex this patch simplify it, 
>> and decrease the overhead of marker_update_probes().
>>
> 
> Yep, I think I did put the format string after the name \0, which was
> rather tricky. I agree that such change makes the code more readable at
> the expense of doing 2 allocations per marker rather than one, but
> readability might be a good thing here.
> 
> Thanks !
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> 

Hi, Ingo

Do you have any objections against these patches?
[PATCH tip/tracing/markers] markers: simplify marker_set_format()
[PATCH tip/tracing/markers] markers: remove exported symbol marker_probe_cb_noarg()
[PATCH tip/tracing/markers] markers: let marker_table be close to its comments

My new patch are being made on the top of these patches.
new patch:
[PATCH tip/tracing/markers] markers: new callbacks manager
new patch remove a lot code and speed up critical region
and not change any APIs.

new patch change too much code, so I have to wait until all known
patches(which can be applied) have been applied.

Thanks, Lai

Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format()

[Ingo Molnar]

* Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> > 
> > current marker_set_format() is complex this patch simplify it, 
> > and decrease the overhead of marker_update_probes().
> > 
> 
> Yep, I think I did put the format string after the name \0, which was
> rather tricky. I agree that such change makes the code more readable at
> the expense of doing 2 allocations per marker rather than one, but
> readability might be a good thing here.
> 
> Thanks !
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

applied to tip/tracing/markers, thanks!

	Ingo

Re: [PATCH tip/tracing/markers] markers: simplify marker_set_format()

[Mathieu Desnoyers]

* Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> >> current marker_set_format() is complex this patch simplify it, 
> >> and decrease the overhead of marker_update_probes().
> >>
> > 
> > Yep, I think I did put the format string after the name \0, which was
> > rather tricky. I agree that such change makes the code more readable at
> > the expense of doing 2 allocations per marker rather than one, but
> > readability might be a good thing here.
> 
> this fix did help for readability and made code cleanly.
> but it's still 1 allocation per marker_entry when format != NULL.
> the previous benefit is not lost.
> 

Yes, but the common case is to have format != NULL, so it's 2 alloc per
marker. :)

Mathieu


> Thanks, Lai
> 
> > 
> > Thanks !
> > 
> > Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> > 
> >> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> >> ---
> >> diff --git a/kernel/marker.c b/kernel/marker.c
> >> index e49a9c5..ef43c86 100644
> >> --- a/kernel/marker.c
> >> +++ b/kernel/marker.c
> >> @@ -65,6 +65,7 @@ struct marker_entry {
> >>  	void *oldptr;
> >>  	int rcu_pending;
> >>  	unsigned char ptype:1;
> >> +	unsigned char format_allocated:1;
> >>  	char name[0];	/* Contains name'\0'format'\0' */
> >>  };
> >>  
> >> @@ -415,6 +416,7 @@ static struct marker_entry *add_marker(const char *name, const char *format)
> >>  	e->single.probe_private = NULL;
> >>  	e->multi = NULL;
> >>  	e->ptype = 0;
> >> +	e->format_allocated = 0;
> >>  	e->refcount = 0;
> >>  	e->rcu_pending = 0;
> >>  	hlist_add_head(&e->hlist, head);
> >> @@ -446,6 +448,8 @@ static int remove_marker(const char *name)
> >>  	if (e->single.func != __mark_empty_function)
> >>  		return -EBUSY;
> >>  	hlist_del(&e->hlist);
> >> +	if (e->format_allocated)
> >> +		kfree(e->format);
> >>  	/* Make sure the call_rcu has been executed */
> >>  	if (e->rcu_pending)
> >>  		rcu_barrier_sched();
> >> @@ -456,57 +460,34 @@ static int remove_marker(const char *name)
> >>  /*
> >>   * Set the mark_entry format to the format found in the element.
> >>   */
> >> -static int marker_set_format(struct marker_entry **entry, const char *format)
> >> +static int marker_set_format(struct marker_entry *entry, const char *format)
> >>  {
> >> -	struct marker_entry *e;
> >> -	size_t name_len = strlen((*entry)->name) + 1;
> >> -	size_t format_len = strlen(format) + 1;
> >> -
> >> -
> >> -	e = kmalloc(sizeof(struct marker_entry) + name_len + format_len,
> >> -			GFP_KERNEL);
> >> -	if (!e)
> >> +	entry->format = kstrdup(format, GFP_KERNEL);
> >> +	if (!entry->format)
> >>  		return -ENOMEM;
> >> -	memcpy(&e->name[0], (*entry)->name, name_len);
> >> -	e->format = &e->name[name_len];
> >> -	memcpy(e->format, format, format_len);
> >> -	if (strcmp(e->format, MARK_NOARGS) == 0)
> >> -		e->call = marker_probe_cb_noarg;
> >> -	else
> >> -		e->call = marker_probe_cb;
> >> -	e->single = (*entry)->single;
> >> -	e->multi = (*entry)->multi;
> >> -	e->ptype = (*entry)->ptype;
> >> -	e->refcount = (*entry)->refcount;
> >> -	e->rcu_pending = 0;
> >> -	hlist_add_before(&e->hlist, &(*entry)->hlist);
> >> -	hlist_del(&(*entry)->hlist);
> >> -	/* Make sure the call_rcu has been executed */
> >> -	if ((*entry)->rcu_pending)
> >> -		rcu_barrier_sched();
> >> -	kfree(*entry);
> >> -	*entry = e;
> >> +	entry->format_allocated = 1;
> >> +
> >>  	trace_mark(core_marker_format, "name %s format %s",
> >> -			e->name, e->format);
> >> +			entry->name, entry->format);
> >>  	return 0;
> >>  }
> >>  
> >>  /*
> >>   * Sets the probe callback corresponding to one marker.
> >>   */
> >> -static int set_marker(struct marker_entry **entry, struct marker *elem,
> >> +static int set_marker(struct marker_entry *entry, struct marker *elem,
> >>  		int active)
> >>  {
> >>  	int ret;
> >> -	WARN_ON(strcmp((*entry)->name, elem->name) != 0);
> >> +	WARN_ON(strcmp(entry->name, elem->name) != 0);
> >>  
> >> -	if ((*entry)->format) {
> >> -		if (strcmp((*entry)->format, elem->format) != 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,
> >> +				entry->name,
> >> +				entry->format,
> >>  				elem->format);
> >>  			return -EPERM;
> >>  		}
> >> @@ -522,34 +503,33 @@ static int set_marker(struct marker_entry **entry, struct marker *elem,
> >>  	 * pass from a "safe" callback (with argument) to an "unsafe"
> >>  	 * callback (does not set arguments).
> >>  	 */
> >> -	elem->call = (*entry)->call;
> >> +	elem->call = entry->call;
> >>  	/*
> >>  	 * Sanity check :
> >>  	 * We only update the single probe private data when the ptr is
> >>  	 * set to a _non_ single probe! (0 -> 1 and N -> 1, N != 1)
> >>  	 */
> >>  	WARN_ON(elem->single.func != __mark_empty_function
> >> -		&& elem->single.probe_private
> >> -		!= (*entry)->single.probe_private &&
> >> -		!elem->ptype);
> >> -	elem->single.probe_private = (*entry)->single.probe_private;
> >> +		&& elem->single.probe_private != entry->single.probe_private
> >> +		&& !elem->ptype);
> >> +	elem->single.probe_private = entry->single.probe_private;
> >>  	/*
> >>  	 * Make sure the private data is valid when we update the
> >>  	 * single probe ptr.
> >>  	 */
> >>  	smp_wmb();
> >> -	elem->single.func = (*entry)->single.func;
> >> +	elem->single.func = entry->single.func;
> >>  	/*
> >>  	 * We also make sure that the new probe callbacks array is consistent
> >>  	 * before setting a pointer to it.
> >>  	 */
> >> -	rcu_assign_pointer(elem->multi, (*entry)->multi);
> >> +	rcu_assign_pointer(elem->multi, entry->multi);
> >>  	/*
> >>  	 * Update the function or multi probe array pointer before setting the
> >>  	 * ptype.
> >>  	 */
> >>  	smp_wmb();
> >> -	elem->ptype = (*entry)->ptype;
> >> +	elem->ptype = entry->ptype;
> >>  	elem->state = active;
> >>  
> >>  	return 0;
> >> @@ -593,8 +573,7 @@ void marker_update_probe_range(struct marker *begin,
> >>  	for (iter = begin; iter < end; iter++) {
> >>  		mark_entry = get_marker(iter->name);
> >>  		if (mark_entry) {
> >> -			set_marker(&mark_entry, iter,
> >> -					!!mark_entry->refcount);
> >> +			set_marker(mark_entry, iter, !!mark_entry->refcount);
> >>  			/*
> >>  			 * ignore error, continue
> >>  			 */
> >> @@ -656,7 +635,7 @@ int marker_probe_register(const char *name, const char *format,
> >>  			ret = PTR_ERR(entry);
> >>  	} else if (format) {
> >>  		if (!entry->format)
> >> -			ret = marker_set_format(&entry, format);
> >> +			ret = marker_set_format(entry, format);
> >>  		else if (strcmp(entry->format, format))
> >>  			ret = -EPERM;
> >>  	}
> >>
> >>
> >>
> >>
> >>
> > 
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68