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

[Lai Jiangshan]

KOSAKI Motohiro wrote:
> 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
I can't see the racy as your said,
since now index_kref.refcount=0, we can remove
the marker_entry.

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