[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(<t_channel_mutex);
>>> mutex_unlock(<t_channel_mutex);
>>> ltt_channels_trace_free()
>>> mutex_lock(<t_channel_mutex);
>>> kref_put()
>>> mutex_unlock(<t_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(<t_channel_mutex);
> mutex_unlock(<t_channel_mutex);
> ltt_channels_trace_alloc()
> mutex_lock(<t_channel_mutex);
> kref_get(&index_kref);
> mutex_unlock(<t_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(<t_channel_mutex);
>>>> + ret = !!atomic_read(&index_kref.refcount);
>>>> + mutex_unlock(<t_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