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

Lai Jiangshan laijs at cn.fujitsu.com
Thu Jan 22 03:40:53 EST 2009


Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>> 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.
> 
> For racing with ltt_channels_trace_free(), I think it's ok, because
> ltt_channels_trace_free() itself takes the lock_markers() mutex which
> is also taken when we enter in remove_marker().
> 
> ltt_channel_alive() definitely needs a comment saying that it should
> be called with lock_markers() held then for that reason and that the
> result it returns is only valid within the lock_markers() critical
> section.
> 
> However, the race between ltt_channel_alive and ltt_channels_trace_alloc
> could be a problem because we don't lock_marker() in
> ltt_channels_trace_alloc().
> 
> The following race is therefore possible :
> 
> at start time, index_kref.refcount == 0.
> 
>   CPU0                                       CPU1
> =======================================================================
> 
> remove_marker(channel, name, 1)
>   ltt_channel_alive(channel)
>    mutex_lock(&ltt_channel_mutex);
>    mutex_unlock(&ltt_channel_mutex);
>                                              ltt_channels_trace_alloc()
>                                                mutex_lock(&ltt_channel_mutex);
>                                                kref_get(&index_kref);
>                                                mutex_unlock(&ltt_channel_mutex);
> 
> Perform removal while still in use.


It's OK to perform removal. Nobody use it.
We have updated all struct marker and them perform remove_marker().
If anybody refer to this marker_entry, it means he had made
some mistake.

I'll be in a 11 days vacation.

Lai.

> 
> Therefore I think adding a lock_markers() to
> ltt_channels_trace_alloc() is also needed.
> 
> Mathieu
> 
> 
> 
>>> ==========================================================================
>>>
>>> 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
>>>
>>>
>>>
>>>
>>>
>>
> 






More information about the lttng-dev mailing list