From mboxrd@z Thu Jan 1 00:00:00 1970 From: laijs@cn.fujitsu.com (Lai Jiangshan) Date: Thu, 22 Jan 2009 16:40:53 +0800 Subject: [ltt-dev] [PATCH 01/13] make marker_entry permanent In-Reply-To: <20090115141210.GC24482@Krystal> References: <496C5A15.4050204@cn.fujitsu.com> <20090115172430.EBD8.KOSAKI.MOTOHIRO@jp.fujitsu.com> <496EF661.1080601@cn.fujitsu.com> <20090115141210.GC24482@Krystal> Message-ID: <49783115.2040909@cn.fujitsu.com> 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 >>> >>> >>> >>> >>> >> >