[ltt-dev] [PATCH 01/13] make marker_entry permanent
Mathieu Desnoyers
mathieu.desnoyers at polymtl.ca
Thu Jan 15 09:12:10 EST 2009
* 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
More information about the lttng-dev
mailing list