[ltt-dev] [PATCH take2 01/13] make marker_entry permanent

[ltt-dev] [PATCH take2 01/13] make marker_entry permanent

[Lai Jiangshan]

text output need a permanent struct for permanent eID
we make marker_entry permanent

Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
index 4a3924b..db513cb 100644
--- a/include/linux/ltt-channels.h
+++ b/include/linux/ltt-channels.h
@@ -67,6 +77,7 @@ int ltt_channels_set_default(const char *name,
 			     unsigned int subbuf_cnt);
 const char *ltt_channels_get_name_from_index(unsigned int index);
 int ltt_channels_get_index_from_name(const char *name);
+int ltt_channel_alive(const char *channel);
 struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
 						    int overwrite,
 						    int active);
diff --git a/kernel/marker.c b/kernel/marker.c
index b11a82a..6a918e2 100644
--- a/kernel/marker.c
+++ b/kernel/marker.c
@@ -19,6 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/types.h>
 #include <linux/jhash.h>
+#include <linux/hash.h>
 #include <linux/list.h>
 #include <linux/rcupdate.h>
 #include <linux/marker.h>
@@ -59,6 +60,7 @@ void unlock_markers(void)
 #define MARKER_HASH_BITS 6
 #define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
 static struct hlist_head marker_table[MARKER_TABLE_SIZE];
+static struct hlist_head id_table[MARKER_TABLE_SIZE];
 
 /*
  * Note about RCU :
@@ -70,6 +72,7 @@ static struct hlist_head marker_table[MARKER_TABLE_SIZE];
  */
 struct marker_entry {
 	struct hlist_node hlist;
+	struct hlist_node id_list;
 	char *format;
 	char *name;
 			/* Probe wrapper */
@@ -465,7 +468,7 @@ static struct marker_entry *add_marker(const char *channel, const char *name,
  * Remove the marker from the marker hash table. Must be called with mutex_lock
  * held.
  */
-static int remove_marker(const char *channel, const char *name)
+static int remove_marker(const char *channel, const char *name, int registered)
 {
 	struct hlist_head *head;
 	struct hlist_node *node;
@@ -488,11 +491,18 @@ static int remove_marker(const char *channel, const char *name)
 		return -ENOENT;
 	if (e->single.func != __mark_empty_function)
 		return -EBUSY;
+
+	if (registered && ltt_channel_alive(channel))
+		return 0;
+
 	hlist_del(&e->hlist);
+	hlist_del(&e->id_list);
+	if (registered) {
+		ret = ltt_channels_unregister(e->channel);
+		WARN_ON(ret);
+	}
 	if (e->format_allocated)
 		kfree(e->format);
-	ret = ltt_channels_unregister(e->channel);
-	WARN_ON(ret);
 	/* Make sure the call_rcu has been executed */
 	if (e->rcu_pending)
 		rcu_barrier_sched();
@@ -749,6 +759,9 @@ int marker_probe_register(const char *channel, const char *name,
 		if (ret < 0)
 			goto error_unregister_channel;
 		entry->event_id = ret;
+		hlist_add_head(&entry->id_list, id_table + hash_32(
+				(entry->channel_id << 16) | entry->event_id,
+				MARKER_HASH_BITS));
 		ret = 0;
 		trace_mark(metadata, core_marker_id,
 			   "channel %s name %s event_id %hu "
@@ -801,7 +814,7 @@ error_unregister_channel:
 	ret_err = ltt_channels_unregister(channel);
 	WARN_ON(ret_err);
 error_remove_marker:
-	ret_err = remove_marker(channel, name);
+	ret_err = remove_marker(channel, name, 0);
 	WARN_ON(ret_err);
 end:
 	mutex_unlock(&markers_mutex);
@@ -851,7 +864,7 @@ int marker_probe_unregister(const char *channel, const char *name,
 	/* write rcu_pending before calling the RCU callback */
 	smp_wmb();
 	call_rcu_sched(&entry->rcu, free_old_closure);
-	remove_marker(channel, name);	/* Ignore busy error message */
+	remove_marker(channel, name, 1);	/* Ignore busy error message */
 	ret = 0;
 end:
 	mutex_unlock(&markers_mutex);
@@ -938,7 +951,7 @@ int marker_probe_unregister_private_data(marker_probe_func *probe,
 	smp_wmb();
 	call_rcu_sched(&entry->rcu, free_old_closure);
 	/* Ignore busy error message */
-	remove_marker(channel, name);
+	remove_marker(channel, name, 1);
 end:
 	mutex_unlock(&markers_mutex);
 	kfree(channel);
@@ -1008,12 +1055,16 @@ void markers_compact_event_ids(void)
 	struct marker_entry *entry;
 	unsigned int i;
 	struct hlist_head *head;
-	struct hlist_node *node;
+	struct hlist_node *node, *next;
 	int ret;
 
 	for (i = 0; i < MARKER_TABLE_SIZE; i++) {
 		head = &marker_table[i];
-		hlist_for_each_entry(entry, node, head, hlist) {
+		hlist_for_each_entry_safe(entry, node, next, head, hlist) {
+			if (!entry->refcount) {
+				remove_marker(entry->channel, entry->name, 1);
+				continue;
+			}
 			ret = ltt_channels_get_index_from_name(entry->channel);
 			WARN_ON(ret < 0);
 			entry->channel_id = ret;
@@ -1023,6 +1074,16 @@ void markers_compact_event_ids(void)
 			entry->event_id = ret;
 		}
 	}
+
+	memset(id_table, 0, sizeof(id_table));
+	for (i = 0; i < MARKER_TABLE_SIZE; i++) {
+		head = &marker_table[i];
+		hlist_for_each_entry(entry, node, head, hlist) {
+			hlist_add_head(&entry->id_list, id_table + hash_32(
+					(entry->channel_id << 16)
+					| entry->event_id, MARKER_HASH_BITS));
+		}
+	}
 }
 
 #ifdef CONFIG_MODULES
diff --git a/ltt/ltt-channels.c b/ltt/ltt-channels.c
index e01b8dd..0badd9d 100644
--- a/ltt/ltt-channels.c
+++ b/ltt/ltt-channels.c
@@ -40,6 +40,15 @@ static struct ltt_channel_setting *lookup_channel(const char *name)
 	return NULL;
 }
 
+int ltt_channel_alive(const char *channel)
+{
+	int ret;
+	mutex_lock(&ltt_channel_mutex);
+	ret = !!atomic_read(&index_kref.refcount);
+	mutex_unlock(&ltt_channel_mutex);
+	return ret;
+}
+
 /*
  * Must be called when channel refcount falls to 0 _and_ also when the last
  * trace is freed. This function is responsible for compacting the channel and

[ltt-dev] [PATCH take2 01/13] make marker_entry permanent

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> text output need a permanent struct for permanent eID
> we make marker_entry permanent
> 
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>

Hi Lai,

This patch looks good, except for a small detail below,

> ---
> diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
> index 4a3924b..db513cb 100644
> --- a/include/linux/ltt-channels.h
> +++ b/include/linux/ltt-channels.h
> @@ -67,6 +77,7 @@ int ltt_channels_set_default(const char *name,
>  			     unsigned int subbuf_cnt);
>  const char *ltt_channels_get_name_from_index(unsigned int index);
>  int ltt_channels_get_index_from_name(const char *name);
> +int ltt_channel_alive(const char *channel);
>  struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
>  						    int overwrite,
>  						    int active);
> diff --git a/kernel/marker.c b/kernel/marker.c
> index b11a82a..6a918e2 100644
> --- a/kernel/marker.c
> +++ b/kernel/marker.c
> @@ -19,6 +19,7 @@
>  #include <linux/mutex.h>
>  #include <linux/types.h>
>  #include <linux/jhash.h>
> +#include <linux/hash.h>
>  #include <linux/list.h>
>  #include <linux/rcupdate.h>
>  #include <linux/marker.h>
> @@ -59,6 +60,7 @@ void unlock_markers(void)
>  #define MARKER_HASH_BITS 6
>  #define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
>  static struct hlist_head marker_table[MARKER_TABLE_SIZE];
> +static struct hlist_head id_table[MARKER_TABLE_SIZE];
>  
>  /*
>   * Note about RCU :
> @@ -70,6 +72,7 @@ static struct hlist_head marker_table[MARKER_TABLE_SIZE];
>   */
>  struct marker_entry {
>  	struct hlist_node hlist;
> +	struct hlist_node id_list;
>  	char *format;
>  	char *name;
>  			/* Probe wrapper */
> @@ -465,7 +468,7 @@ static struct marker_entry *add_marker(const char *channel, const char *name,
>   * Remove the marker from the marker hash table. Must be called with mutex_lock
>   * held.
>   */
> -static int remove_marker(const char *channel, const char *name)
> +static int remove_marker(const char *channel, const char *name, int registered)
>  {
>  	struct hlist_head *head;
>  	struct hlist_node *node;
> @@ -488,11 +491,18 @@ static int remove_marker(const char *channel, const char *name)
>  		return -ENOENT;
>  	if (e->single.func != __mark_empty_function)
>  		return -EBUSY;
> +
> +	if (registered && ltt_channel_alive(channel))
> +		return 0;
> +
>  	hlist_del(&e->hlist);
> +	hlist_del(&e->id_list);
> +	if (registered) {
> +		ret = ltt_channels_unregister(e->channel);
> +		WARN_ON(ret);
> +	}
>  	if (e->format_allocated)
>  		kfree(e->format);
> -	ret = ltt_channels_unregister(e->channel);
> -	WARN_ON(ret);
>  	/* Make sure the call_rcu has been executed */
>  	if (e->rcu_pending)
>  		rcu_barrier_sched();
> @@ -749,6 +759,9 @@ int marker_probe_register(const char *channel, const char *name,
>  		if (ret < 0)
>  			goto error_unregister_channel;
>  		entry->event_id = ret;
> +		hlist_add_head(&entry->id_list, id_table + hash_32(
> +				(entry->channel_id << 16) | entry->event_id,
> +				MARKER_HASH_BITS));
>  		ret = 0;
>  		trace_mark(metadata, core_marker_id,
>  			   "channel %s name %s event_id %hu "
> @@ -801,7 +814,7 @@ error_unregister_channel:
>  	ret_err = ltt_channels_unregister(channel);
>  	WARN_ON(ret_err);
>  error_remove_marker:
> -	ret_err = remove_marker(channel, name);
> +	ret_err = remove_marker(channel, name, 0);
>  	WARN_ON(ret_err);
>  end:
>  	mutex_unlock(&markers_mutex);
> @@ -851,7 +864,7 @@ int marker_probe_unregister(const char *channel, const char *name,
>  	/* write rcu_pending before calling the RCU callback */
>  	smp_wmb();
>  	call_rcu_sched(&entry->rcu, free_old_closure);
> -	remove_marker(channel, name);	/* Ignore busy error message */
> +	remove_marker(channel, name, 1);	/* Ignore busy error message */
>  	ret = 0;
>  end:
>  	mutex_unlock(&markers_mutex);
> @@ -938,7 +951,7 @@ int marker_probe_unregister_private_data(marker_probe_func *probe,
>  	smp_wmb();
>  	call_rcu_sched(&entry->rcu, free_old_closure);
>  	/* Ignore busy error message */
> -	remove_marker(channel, name);
> +	remove_marker(channel, name, 1);
>  end:
>  	mutex_unlock(&markers_mutex);
>  	kfree(channel);
> @@ -1008,12 +1055,16 @@ void markers_compact_event_ids(void)
>  	struct marker_entry *entry;
>  	unsigned int i;
>  	struct hlist_head *head;
> -	struct hlist_node *node;
> +	struct hlist_node *node, *next;
>  	int ret;
>  
>  	for (i = 0; i < MARKER_TABLE_SIZE; i++) {
>  		head = &marker_table[i];
> -		hlist_for_each_entry(entry, node, head, hlist) {
> +		hlist_for_each_entry_safe(entry, node, next, head, hlist) {
> +			if (!entry->refcount) {
> +				remove_marker(entry->channel, entry->name, 1);
> +				continue;
> +			}
>  			ret = ltt_channels_get_index_from_name(entry->channel);
>  			WARN_ON(ret < 0);
>  			entry->channel_id = ret;
> @@ -1023,6 +1074,16 @@ void markers_compact_event_ids(void)
>  			entry->event_id = ret;
>  		}
>  	}
> +
> +	memset(id_table, 0, sizeof(id_table));
> +	for (i = 0; i < MARKER_TABLE_SIZE; i++) {
> +		head = &marker_table[i];
> +		hlist_for_each_entry(entry, node, head, hlist) {
> +			hlist_add_head(&entry->id_list, id_table + hash_32(
> +					(entry->channel_id << 16)
> +					| entry->event_id, MARKER_HASH_BITS));
> +		}
> +	}
>  }
>  
>  #ifdef CONFIG_MODULES
> diff --git a/ltt/ltt-channels.c b/ltt/ltt-channels.c
> index e01b8dd..0badd9d 100644
> --- a/ltt/ltt-channels.c
> +++ b/ltt/ltt-channels.c
> @@ -40,6 +40,15 @@ static struct ltt_channel_setting *lookup_channel(const char *name)
>  	return NULL;
>  }
>  
> +int ltt_channel_alive(const char *channel)
> +{
> +	int ret;
> +	mutex_lock(&ltt_channel_mutex);
> +	ret = !!atomic_read(&index_kref.refcount);
> +	mutex_unlock(&ltt_channel_mutex);

Why are you taking the ltt_channel_mutex to read a variable atomically ?
There is probably something you intend to protect (race between stopping
to use a channel and removing a marker), but I am pretty much sure this
locking does not do what you intend.

Mathieu

> +	return ret;
> +}
> +
>  /*
>   * Must be called when channel refcount falls to 0 _and_ also when the last
>   * trace is freed. This function is responsible for compacting the channel and
> 
> 
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 

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

[ltt-dev] [PATCH take2 01/13] make marker_entry permanent

[Lai Jiangshan]

Mathieu Desnoyers ??:
>>  
>> +int ltt_channel_alive(const char *channel)
>> +{
>> +	int ret;
>> +	mutex_lock(&ltt_channel_mutex);
>> +	ret = !!atomic_read(&index_kref.refcount);
>> +	mutex_unlock(&ltt_channel_mutex);
> 
> Why are you taking the ltt_channel_mutex to read a variable atomically ?
> There is probably something you intend to protect (race between stopping
> to use a channel and removing a marker), but I am pretty much sure this
> locking does not do what you intend.
> 
> Mathieu
> 
>> +	return ret;
>> +}
>> +


Hi, Mathieu,

You can feel free to fix patches(comments, changelog, code) and
merge them.

Lai.

[ltt-dev] [PATCH take2 01/13] make marker_entry permanent

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> Mathieu Desnoyers ??:
> >>  
> >> +int ltt_channel_alive(const char *channel)
> >> +{
> >> +	int ret;
> >> +	mutex_lock(&ltt_channel_mutex);
> >> +	ret = !!atomic_read(&index_kref.refcount);
> >> +	mutex_unlock(&ltt_channel_mutex);
> > 
> > Why are you taking the ltt_channel_mutex to read a variable atomically ?
> > There is probably something you intend to protect (race between stopping
> > to use a channel and removing a marker), but I am pretty much sure this
> > locking does not do what you intend.
> > 
> > Mathieu
> > 
> >> +	return ret;
> >> +}
> >> +
> 
> 
> Hi, Mathieu,
> 
> You can feel free to fix patches(comments, changelog, code) and
> merge them.
> 

OK. By the way, thanks for doing all this work, this saves me a lot of
time. I'll take care of the last mile and get the patches in shape to
merge them into LTTng.

Mathieu

> Lai.
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

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