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

[kosaki.motohiro@jp.fujitsu.com (KOSAKI Motohiro)]

Hi Lai-san,

> -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;
> +

I don't know lttng so detail.
but It seems racy.


at start time, index_kref.refcount == 1.

  CPU0						CPU1
=======================================================================
 remove_marker(channel, name, 1)
   ltt_channel_alive(channel)
    mutex_lock(&ltt_channel_mutex);
    mutex_unlock(&ltt_channel_mutex);
						ltt_channels_trace_free()
						  mutex_lock(&ltt_channel_mutex);
						  kref_put()
						  mutex_unlock(&ltt_channel_mutex);

 return 0
==========================================================================

this scenario can't happen? if so, you should write helpful comment.
at least, this code confuse to reviewer.



>  	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 mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev