From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@polymtl.ca (Mathieu Desnoyers) Date: Thu, 15 Jan 2009 09:12:10 -0500 Subject: [ltt-dev] [PATCH 01/13] make marker_entry permanent In-Reply-To: <496EF661.1080601@cn.fujitsu.com> References: <496C5A15.4050204@cn.fujitsu.com> <20090115172430.EBD8.KOSAKI.MOTOHIRO@jp.fujitsu.com> <496EF661.1080601@cn.fujitsu.com> Message-ID: <20090115141210.GC24482@Krystal> * 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. 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 > > > > > > > > > > > > > > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68