* [ltt-dev] [PATCH 01/13] make marker_entry permanent
@ 2009-01-13 9:08 Lai Jiangshan
2009-01-15 8:31 ` KOSAKI Motohiro
0 siblings, 1 reply; 5+ messages in thread
From: Lai Jiangshan @ 2009-01-13 9:08 UTC (permalink / raw)
text output need a permanent struct for permanent eID
we make marker_entry permanent
Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
b/kernel/marker.c | 43 +++++++++++++++++++++++++++++++++++--------
b/ltt/ltt-channels.c | 9 +++++++++
include/linux/ltt-channels.h | 1 +
3 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
index 4a3924b..db513cb 100644
--- a/include/linux/ltt-channels.h
+++ b/include/linux/ltt-channels.h
@@ -67,6 +77,7 @@ int ltt_channels_set_default(const char *name,
unsigned int subbuf_cnt);
const char *ltt_channels_get_name_from_index(unsigned int index);
int ltt_channels_get_index_from_name(const char *name);
+int ltt_channel_alive(const char *channel);
struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
int overwrite,
int active);
diff --git a/kernel/marker.c b/kernel/marker.c
index b11a82a..6a918e2 100644
--- a/kernel/marker.c
+++ b/kernel/marker.c
@@ -19,6 +19,7 @@
#include <linux/mutex.h>
#include <linux/types.h>
#include <linux/jhash.h>
+#include <linux/hash.h>
#include <linux/list.h>
#include <linux/rcupdate.h>
#include <linux/marker.h>
@@ -59,6 +60,7 @@ void unlock_markers(void)
#define MARKER_HASH_BITS 6
#define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
static struct hlist_head marker_table[MARKER_TABLE_SIZE];
+static struct hlist_head id_table[MARKER_TABLE_SIZE];
/*
* Note about RCU :
@@ -70,6 +72,7 @@ static struct hlist_head marker_table[MARKER_TABLE_SIZE];
*/
struct marker_entry {
struct hlist_node hlist;
+ struct hlist_node id_list;
char *format;
char *name;
/* Probe wrapper */
@@ -465,7 +468,7 @@ static struct marker_entry *add_marker(const char *channel, const char *name,
* Remove the marker from the marker hash table. Must be called with mutex_lock
* held.
*/
-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;
+
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ltt-dev] [PATCH 01/13] make marker_entry permanent
2009-01-13 9:08 [ltt-dev] [PATCH 01/13] make marker_entry permanent Lai Jiangshan
@ 2009-01-15 8:31 ` KOSAKI Motohiro
2009-01-15 8:40 ` Lai Jiangshan
0 siblings, 1 reply; 5+ messages in thread
From: KOSAKI Motohiro @ 2009-01-15 8:31 UTC (permalink / raw)
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
==========================================================================
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ltt-dev] [PATCH 01/13] make marker_entry permanent
2009-01-15 8:31 ` KOSAKI Motohiro
@ 2009-01-15 8:40 ` Lai Jiangshan
2009-01-15 14:12 ` Mathieu Desnoyers
0 siblings, 1 reply; 5+ messages in thread
From: Lai Jiangshan @ 2009-01-15 8:40 UTC (permalink / raw)
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.
> ==========================================================================
>
> 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
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ltt-dev] [PATCH 01/13] make marker_entry permanent
2009-01-15 8:40 ` Lai Jiangshan
@ 2009-01-15 14:12 ` Mathieu Desnoyers
2009-01-22 8:40 ` Lai Jiangshan
0 siblings, 1 reply; 5+ messages in thread
From: Mathieu Desnoyers @ 2009-01-15 14:12 UTC (permalink / raw)
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ltt-dev] [PATCH 01/13] make marker_entry permanent
2009-01-15 14:12 ` Mathieu Desnoyers
@ 2009-01-22 8:40 ` Lai Jiangshan
0 siblings, 0 replies; 5+ messages in thread
From: Lai Jiangshan @ 2009-01-22 8:40 UTC (permalink / raw)
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
>>>
>>>
>>>
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-01-22 8:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-13 9:08 [ltt-dev] [PATCH 01/13] make marker_entry permanent Lai Jiangshan
2009-01-15 8:31 ` KOSAKI Motohiro
2009-01-15 8:40 ` Lai Jiangshan
2009-01-15 14:12 ` Mathieu Desnoyers
2009-01-22 8:40 ` Lai Jiangshan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox