* [ltt-dev] [PATCH] ltt-channel: permanent eID
@ 2008-12-16 13:28 Lai Jiangshan
2008-12-17 19:40 ` Mathieu Desnoyers
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2008-12-16 13:28 UTC (permalink / raw)
introduce permanent eID for text output, and change
the ltt-channel's management.
Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
---
include/linux/ltt-channels.h | 45 ++++++-
include/linux/marker.h | 4
kernel/marker.c | 95 +++------------
ltt/ltt-channels.c | 271 +++++++++++++++++++++++++------------------
ltt/ltt-tracer.c | 9 +
5 files changed, 230 insertions(+), 194 deletions(-)
diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
index ce19eda..edf959b 100644
--- a/include/linux/ltt-channels.h
+++ b/include/linux/ltt-channels.h
@@ -8,8 +8,10 @@
*/
#include <linux/limits.h>
+#include <linux/marker.h>
#define EVENTS_PER_CHANNEL 65536
+#define EVENTS_MOST_WASTE_IDS 128
struct ltt_trace_struct;
struct rchan_buf;
@@ -51,15 +53,51 @@ struct ltt_channel_struct {
struct ltt_channel_setting {
unsigned int subbuf_size;
unsigned int subbuf_cnt;
- struct kref kref; /* Number of references to structure content */
+ int trace_count;
+ int live_count;
struct list_head list;
+ struct list_head marker_entry;
unsigned int index; /* index of channel in trace channel array */
u16 free_event_id; /* Next event ID to allocate */
char name[PATH_MAX];
};
-int ltt_channels_register(const char *name);
-int ltt_channels_unregister(const char *name);
+/*
+ * Note about RCU :
+ * It is used to make sure every handler has finished using its private data
+ * between two consecutive operation (add or remove) on a given marker. It is
+ * also used to delay the free of multiple probes array until a quiescent state
+ * is reached.
+ * marker entries modifications are protected by the markers_mutex.
+ */
+struct marker_entry {
+ struct hlist_node hlist;
+ char *format;
+ char *name;
+ /* Probe wrapper */
+ void (*call)(const struct marker *mdata, void *call_private, ...);
+ struct marker_probe_closure single;
+ struct marker_probe_closure *multi;
+ int refcount; /* Number of times armed. 0 if disarmed. */
+ struct rcu_head rcu;
+ void *oldptr;
+ int rcu_pending;
+ u16 channel_id;
+ u16 event_id;
+ unsigned char ptype:1;
+ unsigned char format_allocated:1;
+ unsigned char used:1;
+ struct list_head sibling; /* sibling of channel's maker_entry */
+ struct ltt_channel_setting *lcs;
+ char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
+};
+int __makrer_set_channel(struct marker_entry *entry, const char *channel);
+
+int start_trace_channel(const char *channel);
+int stop_trace_channel(const char *channel);
+const char *get_name_from_eid(const char *channel, u16 eID);
+const char *get_fmt_from_eid(const char *channel, u16 eID);
+
int ltt_channels_set_default(const char *name,
unsigned int subbuf_size,
unsigned int subbuf_cnt);
@@ -69,7 +107,6 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
int overwrite,
int active);
void ltt_channels_trace_free(struct ltt_channel_struct *channels);
-int _ltt_channels_get_event_id(const char *channel, const char *name);
int ltt_channels_get_event_id(const char *channel, const char *name);
#endif /* _LTT_CHANNELS_H */
diff --git a/include/linux/marker.h b/include/linux/marker.h
index 32387e8..82e4f7c 100644
--- a/include/linux/marker.h
+++ b/include/linux/marker.h
@@ -15,7 +15,6 @@
#include <stdarg.h>
#include <linux/types.h>
#include <linux/immediate.h>
-#include <linux/ltt-channels.h>
struct module;
struct task_struct;
@@ -126,6 +125,7 @@ struct marker {
extern void marker_update_probe_range(struct marker *begin,
struct marker *end);
+extern void marker_update_probes(void);
#define GET_MARKER(channel, name) (__mark_##channel##_##name)
@@ -199,8 +199,6 @@ static inline void marker_update_probe_range(struct marker *begin,
extern void lock_markers(void);
extern void unlock_markers(void);
-extern void markers_compact_event_ids(void);
-
/* To be used for string format validity checking with gcc */
static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
{
diff --git a/kernel/marker.c b/kernel/marker.c
index b11a82a..cd38ba0 100644
--- a/kernel/marker.c
+++ b/kernel/marker.c
@@ -22,6 +22,7 @@
#include <linux/list.h>
#include <linux/rcupdate.h>
#include <linux/marker.h>
+#include <linux/ltt-channels.h>
#include <linux/err.h>
#include <linux/slab.h>
#include <linux/immediate.h>
@@ -60,33 +61,6 @@ void unlock_markers(void)
#define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
static struct hlist_head marker_table[MARKER_TABLE_SIZE];
-/*
- * Note about RCU :
- * It is used to make sure every handler has finished using its private data
- * between two consecutive operation (add or remove) on a given marker. It is
- * also used to delay the free of multiple probes array until a quiescent state
- * is reached.
- * marker entries modifications are protected by the markers_mutex.
- */
-struct marker_entry {
- struct hlist_node hlist;
- char *format;
- char *name;
- /* Probe wrapper */
- void (*call)(const struct marker *mdata, void *call_private, ...);
- struct marker_probe_closure single;
- struct marker_probe_closure *multi;
- int refcount; /* Number of times armed. 0 if disarmed. */
- struct rcu_head rcu;
- void *oldptr;
- int rcu_pending;
- u16 channel_id;
- u16 event_id;
- unsigned char ptype:1;
- unsigned char format_allocated:1;
- char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
-};
-
#ifdef CONFIG_MARKERS_USERSPACE
static void marker_update_processes(void);
#else
@@ -474,7 +448,6 @@ static int remove_marker(const char *channel, const char *name)
size_t channel_len = strlen(channel) + 1;
size_t name_len = strlen(name) + 1;
u32 hash;
- int ret;
hash = jhash(channel, channel_len-1, 0) ^ jhash(name, name_len-1, 0);
head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
@@ -488,11 +461,26 @@ static int remove_marker(const char *channel, const char *name)
return -ENOENT;
if (e->single.func != __mark_empty_function)
return -EBUSY;
+
+ if (e->lcs) {
+ e->lcs->live_count--;
+ if (!e->lcs->live_count && !e->lcs->trace_count) {
+ BUG_ON(!list_empty(&e->lcs->marker_entry));
+ list_del(&e->lcs->list);
+ kfree(e->lcs);
+ e->lcs = NULL;
+ }
+ }
+
+ /* It will be released in remove_chanel_marker_entries() */
+ if (e->used && e->lcs && e->lcs->trace_count)
+ return 0;
+
hlist_del(&e->hlist);
if (e->format_allocated)
kfree(e->format);
- ret = ltt_channels_unregister(e->channel);
- WARN_ON(ret);
+ if (e->lcs)
+ list_del(&e->sibling);
/* Make sure the call_rcu has been executed */
if (e->rcu_pending)
rcu_barrier_sched();
@@ -540,6 +528,7 @@ static int set_marker(struct marker_entry *entry, struct marker *elem,
if (ret)
return ret;
}
+ entry->used = 1;
/*
* probe_cb setup (statically known) is done here. It is
@@ -695,7 +684,7 @@ void marker_update_probe_range(struct marker *begin,
* Site effect : marker_set_format may delete the marker entry (creating a
* replacement).
*/
-static void marker_update_probes(void)
+void marker_update_probes(void)
{
/* Core kernel markers */
marker_update_probe_range(__start___markers, __stop___markers);
@@ -738,18 +727,9 @@ int marker_probe_register(const char *channel, const char *name,
ret = PTR_ERR(entry);
if (ret)
goto end;
- ret = ltt_channels_register(channel);
+ ret = __makrer_set_channel(entry, channel);
if (ret)
goto error_remove_marker;
- ret = ltt_channels_get_index_from_name(channel);
- if (ret < 0)
- goto error_unregister_channel;
- entry->channel_id = ret;
- ret = ltt_channels_get_event_id(channel, name);
- if (ret < 0)
- goto error_unregister_channel;
- entry->event_id = ret;
- ret = 0;
trace_mark(metadata, core_marker_id,
"channel %s name %s event_id %hu "
"int #1u%zu long #1u%zu pointer #1u%zu "
@@ -776,7 +756,7 @@ int marker_probe_register(const char *channel, const char *name,
if (IS_ERR(old)) {
ret = PTR_ERR(old);
if (first_probe)
- goto error_unregister_channel;
+ goto error_remove_marker;
else
goto end;
}
@@ -797,9 +777,6 @@ int marker_probe_register(const char *channel, const char *name,
call_rcu_sched(&entry->rcu, free_old_closure);
goto end;
-error_unregister_channel:
- ret_err = ltt_channels_unregister(channel);
- WARN_ON(ret_err);
error_remove_marker:
ret_err = remove_marker(channel, name);
WARN_ON(ret_err);
@@ -997,34 +974,6 @@ void *marker_get_private_data(const char *channel, const char *name,
}
EXPORT_SYMBOL_GPL(marker_get_private_data);
-/**
- * markers_compact_event_ids: Compact markers event IDs and reassign channels
- *
- * Called when no channel users are active by the channel infrastructure.
- * Called with lock_markers() and channel mutex held.
- */
-void markers_compact_event_ids(void)
-{
- struct marker_entry *entry;
- unsigned int i;
- struct hlist_head *head;
- struct hlist_node *node;
- int ret;
-
- for (i = 0; i < MARKER_TABLE_SIZE; i++) {
- head = &marker_table[i];
- hlist_for_each_entry(entry, node, head, hlist) {
- ret = ltt_channels_get_index_from_name(entry->channel);
- WARN_ON(ret < 0);
- entry->channel_id = ret;
- ret = _ltt_channels_get_event_id(entry->channel,
- entry->name);
- WARN_ON(ret < 0);
- entry->event_id = ret;
- }
- }
-}
-
#ifdef CONFIG_MODULES
/**
diff --git a/ltt/ltt-channels.c b/ltt/ltt-channels.c
index c57e737..e876f04 100644
--- a/ltt/ltt-channels.c
+++ b/ltt/ltt-channels.c
@@ -13,13 +13,12 @@
#include <linux/ltt-channels.h>
#include <linux/mutex.h>
#include <linux/vmalloc.h>
+#include <linux/marker.h>
+#include <linux/err.h>
+#include <linux/ltt-tracer.h>
+
+LIST_HEAD(ltt_channels);
-/*
- * ltt_channel_mutex may be nested inside the LTT trace mutex.
- * ltt_channel_mutex mutex may be nested inside markers mutex.
- */
-static DEFINE_MUTEX(ltt_channel_mutex);
-static LIST_HEAD(ltt_channels);
/*
* Index of next channel in array. Makes sure that as long as a trace channel is
* allocated, no array index will be re-used when a channel is freed and then
@@ -40,104 +39,165 @@ static struct ltt_channel_setting *lookup_channel(const char *name)
return NULL;
}
-/*
- * 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
- * event IDs when no users are active.
- *
- * Called with lock_markers() and channels mutex held.
- */
-static void release_channel_setting(struct kref *kref)
-{
- struct ltt_channel_setting *setting = container_of(kref,
- struct ltt_channel_setting, kref);
- struct ltt_channel_setting *iter;
+static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
+ const char *name);
- if (atomic_read(&index_kref.refcount) == 0
- && atomic_read(&setting->kref.refcount) == 0) {
- list_del(&setting->list);
- kfree(setting);
+int __makrer_set_channel(struct marker_entry *entry, const char *channel)
+{
+ struct ltt_channel_setting *setting = lookup_channel(channel);
+ if (!setting) {
+ setting = kzalloc(sizeof(*setting), GFP_KERNEL);
+ if (!setting)
+ return -ENOMEM;
+ list_add_tail(&setting->list, <t_channels);
+ strncpy(setting->name, channel, PATH_MAX-1);
+ setting->index = free_index++;
+ INIT_LIST_HEAD(&setting->marker_entry);
+ }else if (setting->free_event_id == EVENTS_PER_CHANNEL - 1)
+ return -ENOSPC;
+ setting->live_count++;
+ list_add(&entry->sibling, &setting->marker_entry);
+ entry->channel_id = setting->index;
+ entry->event_id = _ltt_channels_get_event_id(setting, entry->name);
+ return 0;
+}
- free_index = 0;
- list_for_each_entry(iter, <t_channels, list) {
- iter->index = free_index++;
- iter->free_event_id = 0;
+static void defrag_id(struct ltt_channel_setting *setting)
+{
+ struct marker_entry *e;
+ int free_count = setting->free_event_id;
+ if (setting->live_count + EVENTS_MOST_WASTE_IDS > free_count)
+ return;
+ free_count = 0;
+ list_for_each_entry(e, &setting->marker_entry, sibling) {
+ if (e->event_id != free_count) {
+ trace_mark(metadata, core_marker_id,
+ "channel %s name %s event_id %hu "
+ "int #1u%zu long #1u%zu pointer #1u%zu "
+ "size_t #1u%zu alignment #1u%u",
+ e->channel, e->name, (u16)free_count,
+ sizeof(int), sizeof(long), sizeof(void *),
+ sizeof(size_t), ltt_get_alignment());
}
- markers_compact_event_ids();
+ e->event_id = free_count++;
}
+ setting->free_event_id = (u16)free_count;
+ marker_update_probes();
}
-/*
- * Perform channel index compaction when the last trace channel is freed.
- *
- * Called with lock_markers() and channels mutex held.
- */
-static void release_trace_channel(struct kref *kref)
+static void remove_chanel_marker_entries(struct ltt_channel_setting *setting)
{
- struct ltt_channel_setting *iter, *n;
+ struct marker_entry *e, *n;
+
+ list_for_each_entry_safe(e, n, &setting->marker_entry, sibling) {
+ if (e->single.func != __mark_empty_function)
+ continue;
- list_for_each_entry_safe(iter, n, <t_channels, list)
- release_channel_setting(&iter->kref);
+ hlist_del(&e->hlist);
+ if (e->format_allocated)
+ kfree(e->format);
+ list_del(&e->sibling);
+ /* Make sure the call_rcu has been executed */
+ if (e->rcu_pending)
+ rcu_barrier_sched();
+ kfree(e);
+ }
}
-/**
- * ltt_channels_register: Register a trace channel.
- *
- * Uses refcounting.
- */
-int ltt_channels_register(const char *name)
+int start_trace_channel(const char *channel)
{
+ int ret = -ENOENT;
struct ltt_channel_setting *setting;
- int ret = 0;
- mutex_lock(<t_channel_mutex);
- setting = lookup_channel(name);
+ lock_markers();
+ setting = lookup_channel(channel);
+ if (setting) {
+ if (!setting->trace_count)
+ defrag_id(setting);
+ setting->trace_count++;
+ ret = 0;
+ }
+ unlock_markers();
+ return ret;
+}
+EXPORT_SYMBOL_GPL(start_trace_channel);
+
+int stop_trace_channel(const char *channel)
+{
+ int ret = -ENOENT;
+ struct ltt_channel_setting *setting;
+
+ lock_markers();
+ setting = lookup_channel(channel);
if (setting) {
- if (atomic_read(&setting->kref.refcount) == 0)
- goto init_kref;
- else {
- kref_get(&setting->kref);
- goto end;
+ setting->trace_count--;
+ if (!setting->trace_count)
+ remove_chanel_marker_entries(setting);
+ ret = 0;
+ if (!setting->live_count && !setting->trace_count) {
+ BUG_ON(!list_empty(&setting->marker_entry));
+ list_del(&setting->list);
+ kfree(setting);
}
}
- setting = kzalloc(sizeof(*setting), GFP_KERNEL);
- if (!setting) {
- ret = -ENOMEM;
- goto end;
+ unlock_markers();
+ return ret;
+}
+EXPORT_SYMBOL_GPL(stop_trace_channel);
+
+/* permanent! the return value is valid before stop_strace_channel() */
+
+const char *get_name_from_eid(const char *channel, u16 eID)
+{
+ char *ret = ERR_PTR(-ENOENT);
+ struct ltt_channel_setting *setting;
+ struct marker_entry *e;
+
+ lock_markers();
+ setting = lookup_channel(channel);
+ list_for_each_entry(e, &setting->marker_entry, sibling) {
+ if (e->event_id == eID) {
+ ret = e->name;
+ break;
+ }
}
- list_add(&setting->list, <t_channels);
- strncpy(setting->name, name, PATH_MAX-1);
- setting->index = free_index++;
-init_kref:
- kref_init(&setting->kref);
-end:
- mutex_unlock(<t_channel_mutex);
+ unlock_markers();
return ret;
}
-EXPORT_SYMBOL_GPL(ltt_channels_register);
+EXPORT_SYMBOL_GPL(get_name_from_eid);
-/**
- * ltt_channels_unregister(): Unregister a trace channel.
- *
- * Must be called with markers mutex held.
- */
-int ltt_channels_unregister(const char *name)
+const char *get_fmt_from_eid(const char *channel, u16 eID)
{
+ char *ret = ERR_PTR(-ENOENT);
struct ltt_channel_setting *setting;
- int ret = 0;
+ struct marker_entry *e;
- mutex_lock(<t_channel_mutex);
- setting = lookup_channel(name);
- if (!setting || atomic_read(&setting->kref.refcount) == 0) {
- ret = -ENOENT;
- goto end;
+ lock_markers();
+ setting = lookup_channel(channel);
+ list_for_each_entry(e, &setting->marker_entry, sibling) {
+ if (e->event_id == eID) {
+ if (e->used)
+ ret = e->format;
+ break;
+ }
}
- kref_put(&setting->kref, release_channel_setting);
-end:
- mutex_unlock(<t_channel_mutex);
+ unlock_markers();
return ret;
}
-EXPORT_SYMBOL_GPL(ltt_channels_unregister);
+EXPORT_SYMBOL_GPL(get_fmt_from_eid);
+
+/*
+ * 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
+ * index when no users are active.
+ */
+static void release_channel_index(struct kref *kref)
+{
+ struct ltt_channel_setting *setting;
+ free_index = 0;
+ list_for_each_entry(setting, <t_channels, list)
+ setting->index = free_index++;
+}
/**
* ltt_channels_set_default: Set channel default behavior.
@@ -149,16 +209,16 @@ int ltt_channels_set_default(const char *name,
struct ltt_channel_setting *setting;
int ret = 0;
- mutex_lock(<t_channel_mutex);
+ lock_markers();
setting = lookup_channel(name);
- if (!setting || atomic_read(&setting->kref.refcount) == 0) {
+ if (!setting) {
ret = -ENOENT;
goto end;
}
setting->subbuf_size = subbuf_size;
setting->subbuf_cnt = subbuf_cnt;
end:
- mutex_unlock(<t_channel_mutex);
+ unlock_markers();
return ret;
}
EXPORT_SYMBOL_GPL(ltt_channels_set_default);
@@ -174,7 +234,7 @@ const char *ltt_channels_get_name_from_index(unsigned int index)
struct ltt_channel_setting *iter;
list_for_each_entry(iter, <t_channels, list)
- if (iter->index == index && atomic_read(&iter->kref.refcount))
+ if (iter->index == index)
return iter->name;
return NULL;
}
@@ -186,8 +246,7 @@ ltt_channels_get_setting_from_name(const char *name)
struct ltt_channel_setting *iter;
list_for_each_entry(iter, <t_channels, list)
- if (!strcmp(iter->name, name)
- && atomic_read(&iter->kref.refcount))
+ if (!strcmp(iter->name, name))
return iter;
return NULL;
}
@@ -228,7 +287,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
struct ltt_channel_struct *channel = NULL;
struct ltt_channel_setting *iter;
- mutex_lock(<t_channel_mutex);
+ lock_markers();
if (!free_index)
goto end;
if (!atomic_read(&index_kref.refcount))
@@ -241,7 +300,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
if (!channel)
goto end;
list_for_each_entry(iter, <t_channels, list) {
- if (!atomic_read(&iter->kref.refcount))
+ if (!iter->live_count)
continue;
channel[iter->index].subbuf_size = iter->subbuf_size;
channel[iter->index].subbuf_cnt = iter->subbuf_cnt;
@@ -250,7 +309,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
channel[iter->index].channel_name = iter->name;
}
end:
- mutex_unlock(<t_channel_mutex);
+ unlock_markers();
return channel;
}
EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
@@ -264,33 +323,18 @@ EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
void ltt_channels_trace_free(struct ltt_channel_struct *channels)
{
lock_markers();
- mutex_lock(<t_channel_mutex);
- kfree(channels);
- kref_put(&index_kref, release_trace_channel);
- mutex_unlock(<t_channel_mutex);
+ kref_put(&index_kref, release_channel_index);
unlock_markers();
+ kfree(channels);
}
EXPORT_SYMBOL_GPL(ltt_channels_trace_free);
-/**
- * _ltt_channels_get_event_id: get next event ID for a marker
- * @channel: channel name
- * @name: event name
- *
- * Returns a unique event ID (for this channel) or < 0 on error.
- * Must be called with channels mutex held.
- */
-int _ltt_channels_get_event_id(const char *channel, const char *name)
+static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
+ const char *name)
{
- struct ltt_channel_setting *setting;
int ret;
- setting = ltt_channels_get_setting_from_name(channel);
- if (!setting) {
- ret = -ENOENT;
- goto end;
- }
- if (strcmp(channel, "metadata") == 0) {
+ if (strcmp(setting->name, "metadata") == 0) {
if (strcmp(name, "core_marker_id") == 0)
ret = 0;
else if (strcmp(name, "core_marker_format") == 0)
@@ -299,7 +343,7 @@ int _ltt_channels_get_event_id(const char *channel, const char *name)
ret = -ENOENT;
goto end;
}
- if (setting->free_event_id == EVENTS_PER_CHANNEL) {
+ if (setting->free_event_id == EVENTS_PER_CHANNEL - 1) {
ret = -ENOSPC;
goto end;
}
@@ -316,11 +360,14 @@ end:
*/
int ltt_channels_get_event_id(const char *channel, const char *name)
{
- int ret;
+ int ret = -ENOENT;
+ struct ltt_channel_setting *setting;
- mutex_lock(<t_channel_mutex);
- ret = _ltt_channels_get_event_id(channel, name);
- mutex_unlock(<t_channel_mutex);
+ lock_markers();
+ setting = ltt_channels_get_setting_from_name(channel);
+ if (setting)
+ ret = _ltt_channels_get_event_id(setting, name);
+ unlock_markers();
return ret;
}
diff --git a/ltt/ltt-tracer.c b/ltt/ltt-tracer.c
index af14135..71289f9 100644
--- a/ltt/ltt-tracer.c
+++ b/ltt/ltt-tracer.c
@@ -772,6 +772,7 @@ int ltt_trace_alloc(const char *trace_name)
subbuf_size = trace->channels[chan].subbuf_size;
subbuf_cnt = trace->channels[chan].subbuf_cnt;
prepare_chan_size_num(&subbuf_size, &subbuf_cnt);
+ start_trace_channel(channel_name);
err = trace->ops->create_channel(trace_name, trace,
trace->dentry.trace_root,
channel_name,
@@ -801,8 +802,10 @@ int ltt_trace_alloc(const char *trace_name)
create_channel_error:
for (chan--; chan >= 0; chan--)
- if (trace->channels[chan].active)
+ if (trace->channels[chan].active) {
trace->ops->remove_channel(&trace->channels[chan]);
+ stop_trace_channel(trace->channels[chan].channel_name);
+ }
dirs_error:
module_put(trace->transport->owner);
@@ -901,8 +904,10 @@ static void __ltt_trace_destroy(struct ltt_trace_struct *trace)
for (i = 0; i < trace->nr_channels; i++) {
chan = &trace->channels[i];
- if (chan->active)
+ if (chan->active) {
trace->ops->remove_channel(chan);
+ stop_trace_channel(chan->channel_name);
+ }
}
kref_put(&trace->ltt_transport_kref, ltt_release_transport);
^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH] ltt-channel: permanent eID
2008-12-16 13:28 [ltt-dev] [PATCH] ltt-channel: permanent eID Lai Jiangshan
@ 2008-12-17 19:40 ` Mathieu Desnoyers
2008-12-18 2:22 ` Lai Jiangshan
0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Desnoyers @ 2008-12-17 19:40 UTC (permalink / raw)
* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>
> introduce permanent eID for text output, and change
> the ltt-channel's management.
>
Hi Lai,
I'd like to review this work, but unfortunately, some documentation is
missing. Would it be possible to describe :
- The reason why each change is done.
- How each change is done.
- Modification done to locking (mutex changes, order of mutexes...)
- Why the "change to ltt-channel's management" is required.
Both in a complete changelog message and with added comments in the
code.
Thanks,
Mathieu
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> ---
> include/linux/ltt-channels.h | 45 ++++++-
> include/linux/marker.h | 4
> kernel/marker.c | 95 +++------------
> ltt/ltt-channels.c | 271 +++++++++++++++++++++++++------------------
> ltt/ltt-tracer.c | 9 +
> 5 files changed, 230 insertions(+), 194 deletions(-)
> diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
> index ce19eda..edf959b 100644
> --- a/include/linux/ltt-channels.h
> +++ b/include/linux/ltt-channels.h
> @@ -8,8 +8,10 @@
> */
>
> #include <linux/limits.h>
> +#include <linux/marker.h>
>
> #define EVENTS_PER_CHANNEL 65536
> +#define EVENTS_MOST_WASTE_IDS 128
>
> struct ltt_trace_struct;
> struct rchan_buf;
> @@ -51,15 +53,51 @@ struct ltt_channel_struct {
> struct ltt_channel_setting {
> unsigned int subbuf_size;
> unsigned int subbuf_cnt;
> - struct kref kref; /* Number of references to structure content */
> + int trace_count;
> + int live_count;
> struct list_head list;
> + struct list_head marker_entry;
> unsigned int index; /* index of channel in trace channel array */
> u16 free_event_id; /* Next event ID to allocate */
> char name[PATH_MAX];
> };
>
> -int ltt_channels_register(const char *name);
> -int ltt_channels_unregister(const char *name);
> +/*
> + * Note about RCU :
> + * It is used to make sure every handler has finished using its private data
> + * between two consecutive operation (add or remove) on a given marker. It is
> + * also used to delay the free of multiple probes array until a quiescent state
> + * is reached.
> + * marker entries modifications are protected by the markers_mutex.
> + */
> +struct marker_entry {
> + struct hlist_node hlist;
> + char *format;
> + char *name;
> + /* Probe wrapper */
> + void (*call)(const struct marker *mdata, void *call_private, ...);
> + struct marker_probe_closure single;
> + struct marker_probe_closure *multi;
> + int refcount; /* Number of times armed. 0 if disarmed. */
> + struct rcu_head rcu;
> + void *oldptr;
> + int rcu_pending;
> + u16 channel_id;
> + u16 event_id;
> + unsigned char ptype:1;
> + unsigned char format_allocated:1;
> + unsigned char used:1;
> + struct list_head sibling; /* sibling of channel's maker_entry */
> + struct ltt_channel_setting *lcs;
> + char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
> +};
> +int __makrer_set_channel(struct marker_entry *entry, const char *channel);
> +
> +int start_trace_channel(const char *channel);
> +int stop_trace_channel(const char *channel);
> +const char *get_name_from_eid(const char *channel, u16 eID);
> +const char *get_fmt_from_eid(const char *channel, u16 eID);
> +
> int ltt_channels_set_default(const char *name,
> unsigned int subbuf_size,
> unsigned int subbuf_cnt);
> @@ -69,7 +107,6 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> int overwrite,
> int active);
> void ltt_channels_trace_free(struct ltt_channel_struct *channels);
> -int _ltt_channels_get_event_id(const char *channel, const char *name);
> int ltt_channels_get_event_id(const char *channel, const char *name);
>
> #endif /* _LTT_CHANNELS_H */
> diff --git a/include/linux/marker.h b/include/linux/marker.h
> index 32387e8..82e4f7c 100644
> --- a/include/linux/marker.h
> +++ b/include/linux/marker.h
> @@ -15,7 +15,6 @@
> #include <stdarg.h>
> #include <linux/types.h>
> #include <linux/immediate.h>
> -#include <linux/ltt-channels.h>
>
> struct module;
> struct task_struct;
> @@ -126,6 +125,7 @@ struct marker {
>
> extern void marker_update_probe_range(struct marker *begin,
> struct marker *end);
> +extern void marker_update_probes(void);
>
> #define GET_MARKER(channel, name) (__mark_##channel##_##name)
>
> @@ -199,8 +199,6 @@ static inline void marker_update_probe_range(struct marker *begin,
> extern void lock_markers(void);
> extern void unlock_markers(void);
>
> -extern void markers_compact_event_ids(void);
> -
> /* To be used for string format validity checking with gcc */
> static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
> {
> diff --git a/kernel/marker.c b/kernel/marker.c
> index b11a82a..cd38ba0 100644
> --- a/kernel/marker.c
> +++ b/kernel/marker.c
> @@ -22,6 +22,7 @@
> #include <linux/list.h>
> #include <linux/rcupdate.h>
> #include <linux/marker.h>
> +#include <linux/ltt-channels.h>
> #include <linux/err.h>
> #include <linux/slab.h>
> #include <linux/immediate.h>
> @@ -60,33 +61,6 @@ void unlock_markers(void)
> #define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
> static struct hlist_head marker_table[MARKER_TABLE_SIZE];
>
> -/*
> - * Note about RCU :
> - * It is used to make sure every handler has finished using its private data
> - * between two consecutive operation (add or remove) on a given marker. It is
> - * also used to delay the free of multiple probes array until a quiescent state
> - * is reached.
> - * marker entries modifications are protected by the markers_mutex.
> - */
> -struct marker_entry {
> - struct hlist_node hlist;
> - char *format;
> - char *name;
> - /* Probe wrapper */
> - void (*call)(const struct marker *mdata, void *call_private, ...);
> - struct marker_probe_closure single;
> - struct marker_probe_closure *multi;
> - int refcount; /* Number of times armed. 0 if disarmed. */
> - struct rcu_head rcu;
> - void *oldptr;
> - int rcu_pending;
> - u16 channel_id;
> - u16 event_id;
> - unsigned char ptype:1;
> - unsigned char format_allocated:1;
> - char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
> -};
> -
> #ifdef CONFIG_MARKERS_USERSPACE
> static void marker_update_processes(void);
> #else
> @@ -474,7 +448,6 @@ static int remove_marker(const char *channel, const char *name)
> size_t channel_len = strlen(channel) + 1;
> size_t name_len = strlen(name) + 1;
> u32 hash;
> - int ret;
>
> hash = jhash(channel, channel_len-1, 0) ^ jhash(name, name_len-1, 0);
> head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
> @@ -488,11 +461,26 @@ static int remove_marker(const char *channel, const char *name)
> return -ENOENT;
> if (e->single.func != __mark_empty_function)
> return -EBUSY;
> +
> + if (e->lcs) {
> + e->lcs->live_count--;
> + if (!e->lcs->live_count && !e->lcs->trace_count) {
> + BUG_ON(!list_empty(&e->lcs->marker_entry));
> + list_del(&e->lcs->list);
> + kfree(e->lcs);
> + e->lcs = NULL;
> + }
> + }
> +
> + /* It will be released in remove_chanel_marker_entries() */
> + if (e->used && e->lcs && e->lcs->trace_count)
> + return 0;
> +
> hlist_del(&e->hlist);
> if (e->format_allocated)
> kfree(e->format);
> - ret = ltt_channels_unregister(e->channel);
> - WARN_ON(ret);
> + if (e->lcs)
> + list_del(&e->sibling);
> /* Make sure the call_rcu has been executed */
> if (e->rcu_pending)
> rcu_barrier_sched();
> @@ -540,6 +528,7 @@ static int set_marker(struct marker_entry *entry, struct marker *elem,
> if (ret)
> return ret;
> }
> + entry->used = 1;
>
> /*
> * probe_cb setup (statically known) is done here. It is
> @@ -695,7 +684,7 @@ void marker_update_probe_range(struct marker *begin,
> * Site effect : marker_set_format may delete the marker entry (creating a
> * replacement).
> */
> -static void marker_update_probes(void)
> +void marker_update_probes(void)
> {
> /* Core kernel markers */
> marker_update_probe_range(__start___markers, __stop___markers);
> @@ -738,18 +727,9 @@ int marker_probe_register(const char *channel, const char *name,
> ret = PTR_ERR(entry);
> if (ret)
> goto end;
> - ret = ltt_channels_register(channel);
> + ret = __makrer_set_channel(entry, channel);
> if (ret)
> goto error_remove_marker;
> - ret = ltt_channels_get_index_from_name(channel);
> - if (ret < 0)
> - goto error_unregister_channel;
> - entry->channel_id = ret;
> - ret = ltt_channels_get_event_id(channel, name);
> - if (ret < 0)
> - goto error_unregister_channel;
> - entry->event_id = ret;
> - ret = 0;
> trace_mark(metadata, core_marker_id,
> "channel %s name %s event_id %hu "
> "int #1u%zu long #1u%zu pointer #1u%zu "
> @@ -776,7 +756,7 @@ int marker_probe_register(const char *channel, const char *name,
> if (IS_ERR(old)) {
> ret = PTR_ERR(old);
> if (first_probe)
> - goto error_unregister_channel;
> + goto error_remove_marker;
> else
> goto end;
> }
> @@ -797,9 +777,6 @@ int marker_probe_register(const char *channel, const char *name,
> call_rcu_sched(&entry->rcu, free_old_closure);
> goto end;
>
> -error_unregister_channel:
> - ret_err = ltt_channels_unregister(channel);
> - WARN_ON(ret_err);
> error_remove_marker:
> ret_err = remove_marker(channel, name);
> WARN_ON(ret_err);
> @@ -997,34 +974,6 @@ void *marker_get_private_data(const char *channel, const char *name,
> }
> EXPORT_SYMBOL_GPL(marker_get_private_data);
>
> -/**
> - * markers_compact_event_ids: Compact markers event IDs and reassign channels
> - *
> - * Called when no channel users are active by the channel infrastructure.
> - * Called with lock_markers() and channel mutex held.
> - */
> -void markers_compact_event_ids(void)
> -{
> - struct marker_entry *entry;
> - unsigned int i;
> - struct hlist_head *head;
> - struct hlist_node *node;
> - int ret;
> -
> - for (i = 0; i < MARKER_TABLE_SIZE; i++) {
> - head = &marker_table[i];
> - hlist_for_each_entry(entry, node, head, hlist) {
> - ret = ltt_channels_get_index_from_name(entry->channel);
> - WARN_ON(ret < 0);
> - entry->channel_id = ret;
> - ret = _ltt_channels_get_event_id(entry->channel,
> - entry->name);
> - WARN_ON(ret < 0);
> - entry->event_id = ret;
> - }
> - }
> -}
> -
> #ifdef CONFIG_MODULES
>
> /**
> diff --git a/ltt/ltt-channels.c b/ltt/ltt-channels.c
> index c57e737..e876f04 100644
> --- a/ltt/ltt-channels.c
> +++ b/ltt/ltt-channels.c
> @@ -13,13 +13,12 @@
> #include <linux/ltt-channels.h>
> #include <linux/mutex.h>
> #include <linux/vmalloc.h>
> +#include <linux/marker.h>
> +#include <linux/err.h>
> +#include <linux/ltt-tracer.h>
> +
> +LIST_HEAD(ltt_channels);
>
> -/*
> - * ltt_channel_mutex may be nested inside the LTT trace mutex.
> - * ltt_channel_mutex mutex may be nested inside markers mutex.
> - */
> -static DEFINE_MUTEX(ltt_channel_mutex);
> -static LIST_HEAD(ltt_channels);
> /*
> * Index of next channel in array. Makes sure that as long as a trace channel is
> * allocated, no array index will be re-used when a channel is freed and then
> @@ -40,104 +39,165 @@ static struct ltt_channel_setting *lookup_channel(const char *name)
> return NULL;
> }
>
> -/*
> - * 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
> - * event IDs when no users are active.
> - *
> - * Called with lock_markers() and channels mutex held.
> - */
> -static void release_channel_setting(struct kref *kref)
> -{
> - struct ltt_channel_setting *setting = container_of(kref,
> - struct ltt_channel_setting, kref);
> - struct ltt_channel_setting *iter;
> +static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
> + const char *name);
>
> - if (atomic_read(&index_kref.refcount) == 0
> - && atomic_read(&setting->kref.refcount) == 0) {
> - list_del(&setting->list);
> - kfree(setting);
> +int __makrer_set_channel(struct marker_entry *entry, const char *channel)
> +{
> + struct ltt_channel_setting *setting = lookup_channel(channel);
> + if (!setting) {
> + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> + if (!setting)
> + return -ENOMEM;
> + list_add_tail(&setting->list, <t_channels);
> + strncpy(setting->name, channel, PATH_MAX-1);
> + setting->index = free_index++;
> + INIT_LIST_HEAD(&setting->marker_entry);
> + }else if (setting->free_event_id == EVENTS_PER_CHANNEL - 1)
> + return -ENOSPC;
> + setting->live_count++;
> + list_add(&entry->sibling, &setting->marker_entry);
> + entry->channel_id = setting->index;
> + entry->event_id = _ltt_channels_get_event_id(setting, entry->name);
> + return 0;
> +}
>
> - free_index = 0;
> - list_for_each_entry(iter, <t_channels, list) {
> - iter->index = free_index++;
> - iter->free_event_id = 0;
> +static void defrag_id(struct ltt_channel_setting *setting)
> +{
> + struct marker_entry *e;
> + int free_count = setting->free_event_id;
> + if (setting->live_count + EVENTS_MOST_WASTE_IDS > free_count)
> + return;
> + free_count = 0;
> + list_for_each_entry(e, &setting->marker_entry, sibling) {
> + if (e->event_id != free_count) {
> + trace_mark(metadata, core_marker_id,
> + "channel %s name %s event_id %hu "
> + "int #1u%zu long #1u%zu pointer #1u%zu "
> + "size_t #1u%zu alignment #1u%u",
> + e->channel, e->name, (u16)free_count,
> + sizeof(int), sizeof(long), sizeof(void *),
> + sizeof(size_t), ltt_get_alignment());
> }
> - markers_compact_event_ids();
> + e->event_id = free_count++;
> }
> + setting->free_event_id = (u16)free_count;
> + marker_update_probes();
> }
>
> -/*
> - * Perform channel index compaction when the last trace channel is freed.
> - *
> - * Called with lock_markers() and channels mutex held.
> - */
> -static void release_trace_channel(struct kref *kref)
> +static void remove_chanel_marker_entries(struct ltt_channel_setting *setting)
> {
> - struct ltt_channel_setting *iter, *n;
> + struct marker_entry *e, *n;
> +
> + list_for_each_entry_safe(e, n, &setting->marker_entry, sibling) {
> + if (e->single.func != __mark_empty_function)
> + continue;
>
> - list_for_each_entry_safe(iter, n, <t_channels, list)
> - release_channel_setting(&iter->kref);
> + hlist_del(&e->hlist);
> + if (e->format_allocated)
> + kfree(e->format);
> + list_del(&e->sibling);
> + /* Make sure the call_rcu has been executed */
> + if (e->rcu_pending)
> + rcu_barrier_sched();
> + kfree(e);
> + }
> }
>
> -/**
> - * ltt_channels_register: Register a trace channel.
> - *
> - * Uses refcounting.
> - */
> -int ltt_channels_register(const char *name)
> +int start_trace_channel(const char *channel)
> {
> + int ret = -ENOENT;
> struct ltt_channel_setting *setting;
> - int ret = 0;
>
> - mutex_lock(<t_channel_mutex);
> - setting = lookup_channel(name);
> + lock_markers();
> + setting = lookup_channel(channel);
> + if (setting) {
> + if (!setting->trace_count)
> + defrag_id(setting);
> + setting->trace_count++;
> + ret = 0;
> + }
> + unlock_markers();
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(start_trace_channel);
> +
> +int stop_trace_channel(const char *channel)
> +{
> + int ret = -ENOENT;
> + struct ltt_channel_setting *setting;
> +
> + lock_markers();
> + setting = lookup_channel(channel);
> if (setting) {
> - if (atomic_read(&setting->kref.refcount) == 0)
> - goto init_kref;
> - else {
> - kref_get(&setting->kref);
> - goto end;
> + setting->trace_count--;
> + if (!setting->trace_count)
> + remove_chanel_marker_entries(setting);
> + ret = 0;
> + if (!setting->live_count && !setting->trace_count) {
> + BUG_ON(!list_empty(&setting->marker_entry));
> + list_del(&setting->list);
> + kfree(setting);
> }
> }
> - setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> - if (!setting) {
> - ret = -ENOMEM;
> - goto end;
> + unlock_markers();
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(stop_trace_channel);
> +
> +/* permanent! the return value is valid before stop_strace_channel() */
> +
> +const char *get_name_from_eid(const char *channel, u16 eID)
> +{
> + char *ret = ERR_PTR(-ENOENT);
> + struct ltt_channel_setting *setting;
> + struct marker_entry *e;
> +
> + lock_markers();
> + setting = lookup_channel(channel);
> + list_for_each_entry(e, &setting->marker_entry, sibling) {
> + if (e->event_id == eID) {
> + ret = e->name;
> + break;
> + }
> }
> - list_add(&setting->list, <t_channels);
> - strncpy(setting->name, name, PATH_MAX-1);
> - setting->index = free_index++;
> -init_kref:
> - kref_init(&setting->kref);
> -end:
> - mutex_unlock(<t_channel_mutex);
> + unlock_markers();
> return ret;
> }
> -EXPORT_SYMBOL_GPL(ltt_channels_register);
> +EXPORT_SYMBOL_GPL(get_name_from_eid);
>
> -/**
> - * ltt_channels_unregister(): Unregister a trace channel.
> - *
> - * Must be called with markers mutex held.
> - */
> -int ltt_channels_unregister(const char *name)
> +const char *get_fmt_from_eid(const char *channel, u16 eID)
> {
> + char *ret = ERR_PTR(-ENOENT);
> struct ltt_channel_setting *setting;
> - int ret = 0;
> + struct marker_entry *e;
>
> - mutex_lock(<t_channel_mutex);
> - setting = lookup_channel(name);
> - if (!setting || atomic_read(&setting->kref.refcount) == 0) {
> - ret = -ENOENT;
> - goto end;
> + lock_markers();
> + setting = lookup_channel(channel);
> + list_for_each_entry(e, &setting->marker_entry, sibling) {
> + if (e->event_id == eID) {
> + if (e->used)
> + ret = e->format;
> + break;
> + }
> }
> - kref_put(&setting->kref, release_channel_setting);
> -end:
> - mutex_unlock(<t_channel_mutex);
> + unlock_markers();
> return ret;
> }
> -EXPORT_SYMBOL_GPL(ltt_channels_unregister);
> +EXPORT_SYMBOL_GPL(get_fmt_from_eid);
> +
> +/*
> + * 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
> + * index when no users are active.
> + */
> +static void release_channel_index(struct kref *kref)
> +{
> + struct ltt_channel_setting *setting;
> + free_index = 0;
> + list_for_each_entry(setting, <t_channels, list)
> + setting->index = free_index++;
> +}
>
> /**
> * ltt_channels_set_default: Set channel default behavior.
> @@ -149,16 +209,16 @@ int ltt_channels_set_default(const char *name,
> struct ltt_channel_setting *setting;
> int ret = 0;
>
> - mutex_lock(<t_channel_mutex);
> + lock_markers();
> setting = lookup_channel(name);
> - if (!setting || atomic_read(&setting->kref.refcount) == 0) {
> + if (!setting) {
> ret = -ENOENT;
> goto end;
> }
> setting->subbuf_size = subbuf_size;
> setting->subbuf_cnt = subbuf_cnt;
> end:
> - mutex_unlock(<t_channel_mutex);
> + unlock_markers();
> return ret;
> }
> EXPORT_SYMBOL_GPL(ltt_channels_set_default);
> @@ -174,7 +234,7 @@ const char *ltt_channels_get_name_from_index(unsigned int index)
> struct ltt_channel_setting *iter;
>
> list_for_each_entry(iter, <t_channels, list)
> - if (iter->index == index && atomic_read(&iter->kref.refcount))
> + if (iter->index == index)
> return iter->name;
> return NULL;
> }
> @@ -186,8 +246,7 @@ ltt_channels_get_setting_from_name(const char *name)
> struct ltt_channel_setting *iter;
>
> list_for_each_entry(iter, <t_channels, list)
> - if (!strcmp(iter->name, name)
> - && atomic_read(&iter->kref.refcount))
> + if (!strcmp(iter->name, name))
> return iter;
> return NULL;
> }
> @@ -228,7 +287,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> struct ltt_channel_struct *channel = NULL;
> struct ltt_channel_setting *iter;
>
> - mutex_lock(<t_channel_mutex);
> + lock_markers();
> if (!free_index)
> goto end;
> if (!atomic_read(&index_kref.refcount))
> @@ -241,7 +300,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> if (!channel)
> goto end;
> list_for_each_entry(iter, <t_channels, list) {
> - if (!atomic_read(&iter->kref.refcount))
> + if (!iter->live_count)
> continue;
> channel[iter->index].subbuf_size = iter->subbuf_size;
> channel[iter->index].subbuf_cnt = iter->subbuf_cnt;
> @@ -250,7 +309,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> channel[iter->index].channel_name = iter->name;
> }
> end:
> - mutex_unlock(<t_channel_mutex);
> + unlock_markers();
> return channel;
> }
> EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
> @@ -264,33 +323,18 @@ EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
> void ltt_channels_trace_free(struct ltt_channel_struct *channels)
> {
> lock_markers();
> - mutex_lock(<t_channel_mutex);
> - kfree(channels);
> - kref_put(&index_kref, release_trace_channel);
> - mutex_unlock(<t_channel_mutex);
> + kref_put(&index_kref, release_channel_index);
> unlock_markers();
> + kfree(channels);
> }
> EXPORT_SYMBOL_GPL(ltt_channels_trace_free);
>
> -/**
> - * _ltt_channels_get_event_id: get next event ID for a marker
> - * @channel: channel name
> - * @name: event name
> - *
> - * Returns a unique event ID (for this channel) or < 0 on error.
> - * Must be called with channels mutex held.
> - */
> -int _ltt_channels_get_event_id(const char *channel, const char *name)
> +static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
> + const char *name)
> {
> - struct ltt_channel_setting *setting;
> int ret;
>
> - setting = ltt_channels_get_setting_from_name(channel);
> - if (!setting) {
> - ret = -ENOENT;
> - goto end;
> - }
> - if (strcmp(channel, "metadata") == 0) {
> + if (strcmp(setting->name, "metadata") == 0) {
> if (strcmp(name, "core_marker_id") == 0)
> ret = 0;
> else if (strcmp(name, "core_marker_format") == 0)
> @@ -299,7 +343,7 @@ int _ltt_channels_get_event_id(const char *channel, const char *name)
> ret = -ENOENT;
> goto end;
> }
> - if (setting->free_event_id == EVENTS_PER_CHANNEL) {
> + if (setting->free_event_id == EVENTS_PER_CHANNEL - 1) {
> ret = -ENOSPC;
> goto end;
> }
> @@ -316,11 +360,14 @@ end:
> */
> int ltt_channels_get_event_id(const char *channel, const char *name)
> {
> - int ret;
> + int ret = -ENOENT;
> + struct ltt_channel_setting *setting;
>
> - mutex_lock(<t_channel_mutex);
> - ret = _ltt_channels_get_event_id(channel, name);
> - mutex_unlock(<t_channel_mutex);
> + lock_markers();
> + setting = ltt_channels_get_setting_from_name(channel);
> + if (setting)
> + ret = _ltt_channels_get_event_id(setting, name);
> + unlock_markers();
> return ret;
> }
>
> diff --git a/ltt/ltt-tracer.c b/ltt/ltt-tracer.c
> index af14135..71289f9 100644
> --- a/ltt/ltt-tracer.c
> +++ b/ltt/ltt-tracer.c
> @@ -772,6 +772,7 @@ int ltt_trace_alloc(const char *trace_name)
> subbuf_size = trace->channels[chan].subbuf_size;
> subbuf_cnt = trace->channels[chan].subbuf_cnt;
> prepare_chan_size_num(&subbuf_size, &subbuf_cnt);
> + start_trace_channel(channel_name);
> err = trace->ops->create_channel(trace_name, trace,
> trace->dentry.trace_root,
> channel_name,
> @@ -801,8 +802,10 @@ int ltt_trace_alloc(const char *trace_name)
>
> create_channel_error:
> for (chan--; chan >= 0; chan--)
> - if (trace->channels[chan].active)
> + if (trace->channels[chan].active) {
> trace->ops->remove_channel(&trace->channels[chan]);
> + stop_trace_channel(trace->channels[chan].channel_name);
> + }
>
> dirs_error:
> module_put(trace->transport->owner);
> @@ -901,8 +904,10 @@ static void __ltt_trace_destroy(struct ltt_trace_struct *trace)
>
> for (i = 0; i < trace->nr_channels; i++) {
> chan = &trace->channels[i];
> - if (chan->active)
> + if (chan->active) {
> trace->ops->remove_channel(chan);
> + stop_trace_channel(chan->channel_name);
> + }
> }
>
> kref_put(&trace->ltt_transport_kref, ltt_release_transport);
>
>
>
> _______________________________________________
> 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] 6+ messages in thread
* [ltt-dev] [PATCH] ltt-channel: permanent eID
2008-12-17 19:40 ` Mathieu Desnoyers
@ 2008-12-18 2:22 ` Lai Jiangshan
2008-12-18 4:42 ` Mathieu Desnoyers
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2008-12-18 2:22 UTC (permalink / raw)
changelog:
Marker ID and its data must be permanent for text output, this patch
introduce permanent eID.
1) per channel permanent eID
Basic struct for store eID's data is struct marker_entry. When a channel
is activated, all the struct marker_entry for this channel are permanent
until this channel is deactivated. In this period, event if the marker
is leaved or other things are happened, the eID is permanent.
This patch introduces 4 API for permanent eID:
int start_trace_channel(const char *channel); - activate a channel
int stop_trace_channel(const char *channel); - deactivate a channel
const char *get_name_from_eid(const char *channel, u16 eID); - get name from eID
const char *get_fmt_from_eid(const char *channel, u16 eID); - get fmt from eID
See ltt/ltt-tracer.c for the using of start/stop_trace_channel().
When a channel is deactivated(its trace_count come to 0), we will defrag eID
for this channel. AND we use "trace_mark(metadata, core_marker_id," to tell
userspace that id is reset.
2) per channel struct marker_entry(permanent eID' data) management
All struct marker_entry of a channel are threaded by struct list_head.
It will help us remove a struct marker_entry quickly and help us manage
entries quickly(defrag_id(), remove_chanel_marker_entries())
We also call marker_update_probes() in defrag_id().
Since struct marker_entry is permanent eID' data. We move the definition
of struct marker_entry from marker.c to ltt-channel.h
3) as previous code, we defrag channel index when all tracer is stopped.
different: As described in 1), permanent eID will defraged when a
channel is deactivated.
It means this patch has more fine-grained ID management.
4) remove ltt_channel_mutex.
Almost all function need lock_markers(), we should remove channel_mutex.
Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>> introduce permanent eID for text output, and change
>> the ltt-channel's management.
>>
>
> Hi Lai,
>
> I'd like to review this work, but unfortunately, some documentation is
> missing. Would it be possible to describe :
>
> - The reason why each change is done.
> - How each change is done.
> - Modification done to locking (mutex changes, order of mutexes...)
> - Why the "change to ltt-channel's management" is required.
>
> Both in a complete changelog message and with added comments in the
> code.
>
> Thanks,
>
> Mathieu
>
>
>> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
>> ---
>> include/linux/ltt-channels.h | 45 ++++++-
>> include/linux/marker.h | 4
>> kernel/marker.c | 95 +++------------
>> ltt/ltt-channels.c | 271 +++++++++++++++++++++++++------------------
>> ltt/ltt-tracer.c | 9 +
>> 5 files changed, 230 insertions(+), 194 deletions(-)
>> diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
>> index ce19eda..edf959b 100644
>> --- a/include/linux/ltt-channels.h
>> +++ b/include/linux/ltt-channels.h
>> @@ -8,8 +8,10 @@
>> */
>>
>> #include <linux/limits.h>
>> +#include <linux/marker.h>
>>
>> #define EVENTS_PER_CHANNEL 65536
>> +#define EVENTS_MOST_WASTE_IDS 128
>>
>> struct ltt_trace_struct;
>> struct rchan_buf;
>> @@ -51,15 +53,51 @@ struct ltt_channel_struct {
>> struct ltt_channel_setting {
>> unsigned int subbuf_size;
>> unsigned int subbuf_cnt;
>> - struct kref kref; /* Number of references to structure content */
>> + int trace_count;
>> + int live_count;
>> struct list_head list;
>> + struct list_head marker_entry;
>> unsigned int index; /* index of channel in trace channel array */
>> u16 free_event_id; /* Next event ID to allocate */
>> char name[PATH_MAX];
>> };
>>
>> -int ltt_channels_register(const char *name);
>> -int ltt_channels_unregister(const char *name);
>> +/*
>> + * Note about RCU :
>> + * It is used to make sure every handler has finished using its private data
>> + * between two consecutive operation (add or remove) on a given marker. It is
>> + * also used to delay the free of multiple probes array until a quiescent state
>> + * is reached.
>> + * marker entries modifications are protected by the markers_mutex.
>> + */
>> +struct marker_entry {
>> + struct hlist_node hlist;
>> + char *format;
>> + char *name;
>> + /* Probe wrapper */
>> + void (*call)(const struct marker *mdata, void *call_private, ...);
>> + struct marker_probe_closure single;
>> + struct marker_probe_closure *multi;
>> + int refcount; /* Number of times armed. 0 if disarmed. */
>> + struct rcu_head rcu;
>> + void *oldptr;
>> + int rcu_pending;
>> + u16 channel_id;
>> + u16 event_id;
>> + unsigned char ptype:1;
>> + unsigned char format_allocated:1;
>> + unsigned char used:1;
>> + struct list_head sibling; /* sibling of channel's maker_entry */
>> + struct ltt_channel_setting *lcs;
>> + char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
>> +};
>> +int __makrer_set_channel(struct marker_entry *entry, const char *channel);
>> +
>> +int start_trace_channel(const char *channel);
>> +int stop_trace_channel(const char *channel);
>> +const char *get_name_from_eid(const char *channel, u16 eID);
>> +const char *get_fmt_from_eid(const char *channel, u16 eID);
>> +
>> int ltt_channels_set_default(const char *name,
>> unsigned int subbuf_size,
>> unsigned int subbuf_cnt);
>> @@ -69,7 +107,6 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
>> int overwrite,
>> int active);
>> void ltt_channels_trace_free(struct ltt_channel_struct *channels);
>> -int _ltt_channels_get_event_id(const char *channel, const char *name);
>> int ltt_channels_get_event_id(const char *channel, const char *name);
>>
>> #endif /* _LTT_CHANNELS_H */
>> diff --git a/include/linux/marker.h b/include/linux/marker.h
>> index 32387e8..82e4f7c 100644
>> --- a/include/linux/marker.h
>> +++ b/include/linux/marker.h
>> @@ -15,7 +15,6 @@
>> #include <stdarg.h>
>> #include <linux/types.h>
>> #include <linux/immediate.h>
>> -#include <linux/ltt-channels.h>
>>
>> struct module;
>> struct task_struct;
>> @@ -126,6 +125,7 @@ struct marker {
>>
>> extern void marker_update_probe_range(struct marker *begin,
>> struct marker *end);
>> +extern void marker_update_probes(void);
>>
>> #define GET_MARKER(channel, name) (__mark_##channel##_##name)
>>
>> @@ -199,8 +199,6 @@ static inline void marker_update_probe_range(struct marker *begin,
>> extern void lock_markers(void);
>> extern void unlock_markers(void);
>>
>> -extern void markers_compact_event_ids(void);
>> -
>> /* To be used for string format validity checking with gcc */
>> static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
>> {
>> diff --git a/kernel/marker.c b/kernel/marker.c
>> index b11a82a..cd38ba0 100644
>> --- a/kernel/marker.c
>> +++ b/kernel/marker.c
>> @@ -22,6 +22,7 @@
>> #include <linux/list.h>
>> #include <linux/rcupdate.h>
>> #include <linux/marker.h>
>> +#include <linux/ltt-channels.h>
>> #include <linux/err.h>
>> #include <linux/slab.h>
>> #include <linux/immediate.h>
>> @@ -60,33 +61,6 @@ void unlock_markers(void)
>> #define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
>> static struct hlist_head marker_table[MARKER_TABLE_SIZE];
>>
>> -/*
>> - * Note about RCU :
>> - * It is used to make sure every handler has finished using its private data
>> - * between two consecutive operation (add or remove) on a given marker. It is
>> - * also used to delay the free of multiple probes array until a quiescent state
>> - * is reached.
>> - * marker entries modifications are protected by the markers_mutex.
>> - */
>> -struct marker_entry {
>> - struct hlist_node hlist;
>> - char *format;
>> - char *name;
>> - /* Probe wrapper */
>> - void (*call)(const struct marker *mdata, void *call_private, ...);
>> - struct marker_probe_closure single;
>> - struct marker_probe_closure *multi;
>> - int refcount; /* Number of times armed. 0 if disarmed. */
>> - struct rcu_head rcu;
>> - void *oldptr;
>> - int rcu_pending;
>> - u16 channel_id;
>> - u16 event_id;
>> - unsigned char ptype:1;
>> - unsigned char format_allocated:1;
>> - char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
>> -};
>> -
>> #ifdef CONFIG_MARKERS_USERSPACE
>> static void marker_update_processes(void);
>> #else
>> @@ -474,7 +448,6 @@ static int remove_marker(const char *channel, const char *name)
>> size_t channel_len = strlen(channel) + 1;
>> size_t name_len = strlen(name) + 1;
>> u32 hash;
>> - int ret;
>>
>> hash = jhash(channel, channel_len-1, 0) ^ jhash(name, name_len-1, 0);
>> head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
>> @@ -488,11 +461,26 @@ static int remove_marker(const char *channel, const char *name)
>> return -ENOENT;
>> if (e->single.func != __mark_empty_function)
>> return -EBUSY;
>> +
>> + if (e->lcs) {
>> + e->lcs->live_count--;
>> + if (!e->lcs->live_count && !e->lcs->trace_count) {
>> + BUG_ON(!list_empty(&e->lcs->marker_entry));
>> + list_del(&e->lcs->list);
>> + kfree(e->lcs);
>> + e->lcs = NULL;
>> + }
>> + }
>> +
>> + /* It will be released in remove_chanel_marker_entries() */
>> + if (e->used && e->lcs && e->lcs->trace_count)
>> + return 0;
>> +
>> hlist_del(&e->hlist);
>> if (e->format_allocated)
>> kfree(e->format);
>> - ret = ltt_channels_unregister(e->channel);
>> - WARN_ON(ret);
>> + if (e->lcs)
>> + list_del(&e->sibling);
>> /* Make sure the call_rcu has been executed */
>> if (e->rcu_pending)
>> rcu_barrier_sched();
>> @@ -540,6 +528,7 @@ static int set_marker(struct marker_entry *entry, struct marker *elem,
>> if (ret)
>> return ret;
>> }
>> + entry->used = 1;
>>
>> /*
>> * probe_cb setup (statically known) is done here. It is
>> @@ -695,7 +684,7 @@ void marker_update_probe_range(struct marker *begin,
>> * Site effect : marker_set_format may delete the marker entry (creating a
>> * replacement).
>> */
>> -static void marker_update_probes(void)
>> +void marker_update_probes(void)
>> {
>> /* Core kernel markers */
>> marker_update_probe_range(__start___markers, __stop___markers);
>> @@ -738,18 +727,9 @@ int marker_probe_register(const char *channel, const char *name,
>> ret = PTR_ERR(entry);
>> if (ret)
>> goto end;
>> - ret = ltt_channels_register(channel);
>> + ret = __makrer_set_channel(entry, channel);
>> if (ret)
>> goto error_remove_marker;
>> - ret = ltt_channels_get_index_from_name(channel);
>> - if (ret < 0)
>> - goto error_unregister_channel;
>> - entry->channel_id = ret;
>> - ret = ltt_channels_get_event_id(channel, name);
>> - if (ret < 0)
>> - goto error_unregister_channel;
>> - entry->event_id = ret;
>> - ret = 0;
>> trace_mark(metadata, core_marker_id,
>> "channel %s name %s event_id %hu "
>> "int #1u%zu long #1u%zu pointer #1u%zu "
>> @@ -776,7 +756,7 @@ int marker_probe_register(const char *channel, const char *name,
>> if (IS_ERR(old)) {
>> ret = PTR_ERR(old);
>> if (first_probe)
>> - goto error_unregister_channel;
>> + goto error_remove_marker;
>> else
>> goto end;
>> }
>> @@ -797,9 +777,6 @@ int marker_probe_register(const char *channel, const char *name,
>> call_rcu_sched(&entry->rcu, free_old_closure);
>> goto end;
>>
>> -error_unregister_channel:
>> - ret_err = ltt_channels_unregister(channel);
>> - WARN_ON(ret_err);
>> error_remove_marker:
>> ret_err = remove_marker(channel, name);
>> WARN_ON(ret_err);
>> @@ -997,34 +974,6 @@ void *marker_get_private_data(const char *channel, const char *name,
>> }
>> EXPORT_SYMBOL_GPL(marker_get_private_data);
>>
>> -/**
>> - * markers_compact_event_ids: Compact markers event IDs and reassign channels
>> - *
>> - * Called when no channel users are active by the channel infrastructure.
>> - * Called with lock_markers() and channel mutex held.
>> - */
>> -void markers_compact_event_ids(void)
>> -{
>> - struct marker_entry *entry;
>> - unsigned int i;
>> - struct hlist_head *head;
>> - struct hlist_node *node;
>> - int ret;
>> -
>> - for (i = 0; i < MARKER_TABLE_SIZE; i++) {
>> - head = &marker_table[i];
>> - hlist_for_each_entry(entry, node, head, hlist) {
>> - ret = ltt_channels_get_index_from_name(entry->channel);
>> - WARN_ON(ret < 0);
>> - entry->channel_id = ret;
>> - ret = _ltt_channels_get_event_id(entry->channel,
>> - entry->name);
>> - WARN_ON(ret < 0);
>> - entry->event_id = ret;
>> - }
>> - }
>> -}
>> -
>> #ifdef CONFIG_MODULES
>>
>> /**
>> diff --git a/ltt/ltt-channels.c b/ltt/ltt-channels.c
>> index c57e737..e876f04 100644
>> --- a/ltt/ltt-channels.c
>> +++ b/ltt/ltt-channels.c
>> @@ -13,13 +13,12 @@
>> #include <linux/ltt-channels.h>
>> #include <linux/mutex.h>
>> #include <linux/vmalloc.h>
>> +#include <linux/marker.h>
>> +#include <linux/err.h>
>> +#include <linux/ltt-tracer.h>
>> +
>> +LIST_HEAD(ltt_channels);
>>
>> -/*
>> - * ltt_channel_mutex may be nested inside the LTT trace mutex.
>> - * ltt_channel_mutex mutex may be nested inside markers mutex.
>> - */
>> -static DEFINE_MUTEX(ltt_channel_mutex);
>> -static LIST_HEAD(ltt_channels);
>> /*
>> * Index of next channel in array. Makes sure that as long as a trace channel is
>> * allocated, no array index will be re-used when a channel is freed and then
>> @@ -40,104 +39,165 @@ static struct ltt_channel_setting *lookup_channel(const char *name)
>> return NULL;
>> }
>>
>> -/*
>> - * 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
>> - * event IDs when no users are active.
>> - *
>> - * Called with lock_markers() and channels mutex held.
>> - */
>> -static void release_channel_setting(struct kref *kref)
>> -{
>> - struct ltt_channel_setting *setting = container_of(kref,
>> - struct ltt_channel_setting, kref);
>> - struct ltt_channel_setting *iter;
>> +static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
>> + const char *name);
>>
>> - if (atomic_read(&index_kref.refcount) == 0
>> - && atomic_read(&setting->kref.refcount) == 0) {
>> - list_del(&setting->list);
>> - kfree(setting);
>> +int __makrer_set_channel(struct marker_entry *entry, const char *channel)
>> +{
>> + struct ltt_channel_setting *setting = lookup_channel(channel);
>> + if (!setting) {
>> + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
>> + if (!setting)
>> + return -ENOMEM;
>> + list_add_tail(&setting->list, <t_channels);
>> + strncpy(setting->name, channel, PATH_MAX-1);
>> + setting->index = free_index++;
>> + INIT_LIST_HEAD(&setting->marker_entry);
>> + }else if (setting->free_event_id == EVENTS_PER_CHANNEL - 1)
>> + return -ENOSPC;
>> + setting->live_count++;
>> + list_add(&entry->sibling, &setting->marker_entry);
>> + entry->channel_id = setting->index;
>> + entry->event_id = _ltt_channels_get_event_id(setting, entry->name);
>> + return 0;
>> +}
>>
>> - free_index = 0;
>> - list_for_each_entry(iter, <t_channels, list) {
>> - iter->index = free_index++;
>> - iter->free_event_id = 0;
>> +static void defrag_id(struct ltt_channel_setting *setting)
>> +{
>> + struct marker_entry *e;
>> + int free_count = setting->free_event_id;
>> + if (setting->live_count + EVENTS_MOST_WASTE_IDS > free_count)
>> + return;
>> + free_count = 0;
>> + list_for_each_entry(e, &setting->marker_entry, sibling) {
>> + if (e->event_id != free_count) {
>> + trace_mark(metadata, core_marker_id,
>> + "channel %s name %s event_id %hu "
>> + "int #1u%zu long #1u%zu pointer #1u%zu "
>> + "size_t #1u%zu alignment #1u%u",
>> + e->channel, e->name, (u16)free_count,
>> + sizeof(int), sizeof(long), sizeof(void *),
>> + sizeof(size_t), ltt_get_alignment());
>> }
>> - markers_compact_event_ids();
>> + e->event_id = free_count++;
>> }
>> + setting->free_event_id = (u16)free_count;
>> + marker_update_probes();
>> }
>>
>> -/*
>> - * Perform channel index compaction when the last trace channel is freed.
>> - *
>> - * Called with lock_markers() and channels mutex held.
>> - */
>> -static void release_trace_channel(struct kref *kref)
>> +static void remove_chanel_marker_entries(struct ltt_channel_setting *setting)
>> {
>> - struct ltt_channel_setting *iter, *n;
>> + struct marker_entry *e, *n;
>> +
>> + list_for_each_entry_safe(e, n, &setting->marker_entry, sibling) {
>> + if (e->single.func != __mark_empty_function)
>> + continue;
>>
>> - list_for_each_entry_safe(iter, n, <t_channels, list)
>> - release_channel_setting(&iter->kref);
>> + hlist_del(&e->hlist);
>> + if (e->format_allocated)
>> + kfree(e->format);
>> + list_del(&e->sibling);
>> + /* Make sure the call_rcu has been executed */
>> + if (e->rcu_pending)
>> + rcu_barrier_sched();
>> + kfree(e);
>> + }
>> }
>>
>> -/**
>> - * ltt_channels_register: Register a trace channel.
>> - *
>> - * Uses refcounting.
>> - */
>> -int ltt_channels_register(const char *name)
>> +int start_trace_channel(const char *channel)
>> {
>> + int ret = -ENOENT;
>> struct ltt_channel_setting *setting;
>> - int ret = 0;
>>
>> - mutex_lock(<t_channel_mutex);
>> - setting = lookup_channel(name);
>> + lock_markers();
>> + setting = lookup_channel(channel);
>> + if (setting) {
>> + if (!setting->trace_count)
>> + defrag_id(setting);
>> + setting->trace_count++;
>> + ret = 0;
>> + }
>> + unlock_markers();
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(start_trace_channel);
>> +
>> +int stop_trace_channel(const char *channel)
>> +{
>> + int ret = -ENOENT;
>> + struct ltt_channel_setting *setting;
>> +
>> + lock_markers();
>> + setting = lookup_channel(channel);
>> if (setting) {
>> - if (atomic_read(&setting->kref.refcount) == 0)
>> - goto init_kref;
>> - else {
>> - kref_get(&setting->kref);
>> - goto end;
>> + setting->trace_count--;
>> + if (!setting->trace_count)
>> + remove_chanel_marker_entries(setting);
>> + ret = 0;
>> + if (!setting->live_count && !setting->trace_count) {
>> + BUG_ON(!list_empty(&setting->marker_entry));
>> + list_del(&setting->list);
>> + kfree(setting);
>> }
>> }
>> - setting = kzalloc(sizeof(*setting), GFP_KERNEL);
>> - if (!setting) {
>> - ret = -ENOMEM;
>> - goto end;
>> + unlock_markers();
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(stop_trace_channel);
>> +
>> +/* permanent! the return value is valid before stop_strace_channel() */
>> +
>> +const char *get_name_from_eid(const char *channel, u16 eID)
>> +{
>> + char *ret = ERR_PTR(-ENOENT);
>> + struct ltt_channel_setting *setting;
>> + struct marker_entry *e;
>> +
>> + lock_markers();
>> + setting = lookup_channel(channel);
>> + list_for_each_entry(e, &setting->marker_entry, sibling) {
>> + if (e->event_id == eID) {
>> + ret = e->name;
>> + break;
>> + }
>> }
>> - list_add(&setting->list, <t_channels);
>> - strncpy(setting->name, name, PATH_MAX-1);
>> - setting->index = free_index++;
>> -init_kref:
>> - kref_init(&setting->kref);
>> -end:
>> - mutex_unlock(<t_channel_mutex);
>> + unlock_markers();
>> return ret;
>> }
>> -EXPORT_SYMBOL_GPL(ltt_channels_register);
>> +EXPORT_SYMBOL_GPL(get_name_from_eid);
>>
>> -/**
>> - * ltt_channels_unregister(): Unregister a trace channel.
>> - *
>> - * Must be called with markers mutex held.
>> - */
>> -int ltt_channels_unregister(const char *name)
>> +const char *get_fmt_from_eid(const char *channel, u16 eID)
>> {
>> + char *ret = ERR_PTR(-ENOENT);
>> struct ltt_channel_setting *setting;
>> - int ret = 0;
>> + struct marker_entry *e;
>>
>> - mutex_lock(<t_channel_mutex);
>> - setting = lookup_channel(name);
>> - if (!setting || atomic_read(&setting->kref.refcount) == 0) {
>> - ret = -ENOENT;
>> - goto end;
>> + lock_markers();
>> + setting = lookup_channel(channel);
>> + list_for_each_entry(e, &setting->marker_entry, sibling) {
>> + if (e->event_id == eID) {
>> + if (e->used)
>> + ret = e->format;
>> + break;
>> + }
>> }
>> - kref_put(&setting->kref, release_channel_setting);
>> -end:
>> - mutex_unlock(<t_channel_mutex);
>> + unlock_markers();
>> return ret;
>> }
>> -EXPORT_SYMBOL_GPL(ltt_channels_unregister);
>> +EXPORT_SYMBOL_GPL(get_fmt_from_eid);
>> +
>> +/*
>> + * 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
>> + * index when no users are active.
>> + */
>> +static void release_channel_index(struct kref *kref)
>> +{
>> + struct ltt_channel_setting *setting;
>> + free_index = 0;
>> + list_for_each_entry(setting, <t_channels, list)
>> + setting->index = free_index++;
>> +}
>>
>> /**
>> * ltt_channels_set_default: Set channel default behavior.
>> @@ -149,16 +209,16 @@ int ltt_channels_set_default(const char *name,
>> struct ltt_channel_setting *setting;
>> int ret = 0;
>>
>> - mutex_lock(<t_channel_mutex);
>> + lock_markers();
>> setting = lookup_channel(name);
>> - if (!setting || atomic_read(&setting->kref.refcount) == 0) {
>> + if (!setting) {
>> ret = -ENOENT;
>> goto end;
>> }
>> setting->subbuf_size = subbuf_size;
>> setting->subbuf_cnt = subbuf_cnt;
>> end:
>> - mutex_unlock(<t_channel_mutex);
>> + unlock_markers();
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(ltt_channels_set_default);
>> @@ -174,7 +234,7 @@ const char *ltt_channels_get_name_from_index(unsigned int index)
>> struct ltt_channel_setting *iter;
>>
>> list_for_each_entry(iter, <t_channels, list)
>> - if (iter->index == index && atomic_read(&iter->kref.refcount))
>> + if (iter->index == index)
>> return iter->name;
>> return NULL;
>> }
>> @@ -186,8 +246,7 @@ ltt_channels_get_setting_from_name(const char *name)
>> struct ltt_channel_setting *iter;
>>
>> list_for_each_entry(iter, <t_channels, list)
>> - if (!strcmp(iter->name, name)
>> - && atomic_read(&iter->kref.refcount))
>> + if (!strcmp(iter->name, name))
>> return iter;
>> return NULL;
>> }
>> @@ -228,7 +287,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
>> struct ltt_channel_struct *channel = NULL;
>> struct ltt_channel_setting *iter;
>>
>> - mutex_lock(<t_channel_mutex);
>> + lock_markers();
>> if (!free_index)
>> goto end;
>> if (!atomic_read(&index_kref.refcount))
>> @@ -241,7 +300,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
>> if (!channel)
>> goto end;
>> list_for_each_entry(iter, <t_channels, list) {
>> - if (!atomic_read(&iter->kref.refcount))
>> + if (!iter->live_count)
>> continue;
>> channel[iter->index].subbuf_size = iter->subbuf_size;
>> channel[iter->index].subbuf_cnt = iter->subbuf_cnt;
>> @@ -250,7 +309,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
>> channel[iter->index].channel_name = iter->name;
>> }
>> end:
>> - mutex_unlock(<t_channel_mutex);
>> + unlock_markers();
>> return channel;
>> }
>> EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
>> @@ -264,33 +323,18 @@ EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
>> void ltt_channels_trace_free(struct ltt_channel_struct *channels)
>> {
>> lock_markers();
>> - mutex_lock(<t_channel_mutex);
>> - kfree(channels);
>> - kref_put(&index_kref, release_trace_channel);
>> - mutex_unlock(<t_channel_mutex);
>> + kref_put(&index_kref, release_channel_index);
>> unlock_markers();
>> + kfree(channels);
>> }
>> EXPORT_SYMBOL_GPL(ltt_channels_trace_free);
>>
>> -/**
>> - * _ltt_channels_get_event_id: get next event ID for a marker
>> - * @channel: channel name
>> - * @name: event name
>> - *
>> - * Returns a unique event ID (for this channel) or < 0 on error.
>> - * Must be called with channels mutex held.
>> - */
>> -int _ltt_channels_get_event_id(const char *channel, const char *name)
>> +static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
>> + const char *name)
>> {
>> - struct ltt_channel_setting *setting;
>> int ret;
>>
>> - setting = ltt_channels_get_setting_from_name(channel);
>> - if (!setting) {
>> - ret = -ENOENT;
>> - goto end;
>> - }
>> - if (strcmp(channel, "metadata") == 0) {
>> + if (strcmp(setting->name, "metadata") == 0) {
>> if (strcmp(name, "core_marker_id") == 0)
>> ret = 0;
>> else if (strcmp(name, "core_marker_format") == 0)
>> @@ -299,7 +343,7 @@ int _ltt_channels_get_event_id(const char *channel, const char *name)
>> ret = -ENOENT;
>> goto end;
>> }
>> - if (setting->free_event_id == EVENTS_PER_CHANNEL) {
>> + if (setting->free_event_id == EVENTS_PER_CHANNEL - 1) {
>> ret = -ENOSPC;
>> goto end;
>> }
>> @@ -316,11 +360,14 @@ end:
>> */
>> int ltt_channels_get_event_id(const char *channel, const char *name)
>> {
>> - int ret;
>> + int ret = -ENOENT;
>> + struct ltt_channel_setting *setting;
>>
>> - mutex_lock(<t_channel_mutex);
>> - ret = _ltt_channels_get_event_id(channel, name);
>> - mutex_unlock(<t_channel_mutex);
>> + lock_markers();
>> + setting = ltt_channels_get_setting_from_name(channel);
>> + if (setting)
>> + ret = _ltt_channels_get_event_id(setting, name);
>> + unlock_markers();
>> return ret;
>> }
>>
>> diff --git a/ltt/ltt-tracer.c b/ltt/ltt-tracer.c
>> index af14135..71289f9 100644
>> --- a/ltt/ltt-tracer.c
>> +++ b/ltt/ltt-tracer.c
>> @@ -772,6 +772,7 @@ int ltt_trace_alloc(const char *trace_name)
>> subbuf_size = trace->channels[chan].subbuf_size;
>> subbuf_cnt = trace->channels[chan].subbuf_cnt;
>> prepare_chan_size_num(&subbuf_size, &subbuf_cnt);
>> + start_trace_channel(channel_name);
>> err = trace->ops->create_channel(trace_name, trace,
>> trace->dentry.trace_root,
>> channel_name,
>> @@ -801,8 +802,10 @@ int ltt_trace_alloc(const char *trace_name)
>>
>> create_channel_error:
>> for (chan--; chan >= 0; chan--)
>> - if (trace->channels[chan].active)
>> + if (trace->channels[chan].active) {
>> trace->ops->remove_channel(&trace->channels[chan]);
>> + stop_trace_channel(trace->channels[chan].channel_name);
>> + }
>>
>> dirs_error:
>> module_put(trace->transport->owner);
>> @@ -901,8 +904,10 @@ static void __ltt_trace_destroy(struct ltt_trace_struct *trace)
>>
>> for (i = 0; i < trace->nr_channels; i++) {
>> chan = &trace->channels[i];
>> - if (chan->active)
>> + if (chan->active) {
>> trace->ops->remove_channel(chan);
>> + stop_trace_channel(chan->channel_name);
>> + }
>> }
>>
>> kref_put(&trace->ltt_transport_kref, ltt_release_transport);
>>
>>
>>
>> _______________________________________________
>> 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] 6+ messages in thread
* [ltt-dev] [PATCH] ltt-channel: permanent eID
2008-12-18 2:22 ` Lai Jiangshan
@ 2008-12-18 4:42 ` Mathieu Desnoyers
2008-12-18 6:07 ` Lai Jiangshan
0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Desnoyers @ 2008-12-18 4:42 UTC (permalink / raw)
* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>
Thanks for this description, comments below,
> changelog:
>
> Marker ID and its data must be permanent for text output, this patch
> introduce permanent eID.
>
> 1) per channel permanent eID
>
> Basic struct for store eID's data is struct marker_entry. When a channel
> is activated, all the struct marker_entry for this channel are permanent
> until this channel is deactivated. In this period, event if the marker
> is leaved or other things are happened, the eID is permanent.
>
Ok, so you keep the struct marker_entry around until the channel is
unused (all traces are destroyed, including that all in-kernel ascii
converters have released the trace) ?
> This patch introduces 4 API for permanent eID:
> int start_trace_channel(const char *channel); - activate a channel
> int stop_trace_channel(const char *channel); - deactivate a channel
Hrm, can we change the names for
get_trace_channel()
put_trace_channel() ?
start and stop sounds a bit misleading with the tracing start/stop
semantics.
Also, why do we have "waste" ids ? This is bad, because whenever
possible, we should fit IDs in ~28 IDs allowed for "compact" event ids.
If the IDs are larger than this, the tracer will have to generate an
extended header and waste trace space. Do we really _need_ such id waste?
> const char *get_name_from_eid(const char *channel, u16 eID); - get name from eID
> const char *get_fmt_from_eid(const char *channel, u16 eID); - get fmt from eID
>
> See ltt/ltt-tracer.c for the using of start/stop_trace_channel().
>
hrm, stop_trace_channel (or put_trace_channel) is in __destroy, but
should probably be in the kref desctructor, so we are sure that it's
only called with all references to the trace data, including the
in-kernel ascii converters, have released their reference.
> When a channel is deactivated(its trace_count come to 0), we will defrag eID
> for this channel. AND we use "trace_mark(metadata, core_marker_id," to tell
> userspace that id is reset.
Sounds good.
>
> 2) per channel struct marker_entry(permanent eID' data) management
>
> All struct marker_entry of a channel are threaded by struct list_head.
>
> It will help us remove a struct marker_entry quickly and help us manage
> entries quickly(defrag_id(), remove_chanel_marker_entries())
>
> We also call marker_update_probes() in defrag_id().
>
Hrm, aren't we taking the marker mutex twice here ? I am probably
missing something.
> Since struct marker_entry is permanent eID' data. We move the definition
> of struct marker_entry from marker.c to ltt-channel.h
>
BTW, I would not use the term "permanent" for this. Because permanent,
to me, really means "we allocate something that we will only free when
the machine reboots". We are simply extending the life of the the eIDs
for the duration of all references to those IDs.
Let's say that we extend the event ID lifetime to the overall event ID
references, which now includes trace references, which includes ascii
converters.
> 3) as previous code, we defrag channel index when all tracer is stopped.
>
> different: As described in 1), permanent eID will defraged when a
> channel is deactivated.
>
> It means this patch has more fine-grained ID management.
>
There are two cases here :
Tracing starts
add/remove markers many times
Tracing stops -> defrag
And
No tracing
add/remove markers many times -> defrag each time
You don't say much about what is done at marker add/remove when tracing
is inactive. Is there some compaction done ?
> 4) remove ltt_channel_mutex.
>
> Almost all function need lock_markers(), we should remove channel_mutex.
>
Yep, sounds simpler and those two are pretty much coupled anyway.
Mathieu
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
>
> Mathieu Desnoyers wrote:
> > * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> >> introduce permanent eID for text output, and change
> >> the ltt-channel's management.
> >>
> >
> > Hi Lai,
> >
> > I'd like to review this work, but unfortunately, some documentation is
> > missing. Would it be possible to describe :
> >
> > - The reason why each change is done.
> > - How each change is done.
> > - Modification done to locking (mutex changes, order of mutexes...)
> > - Why the "change to ltt-channel's management" is required.
> >
> > Both in a complete changelog message and with added comments in the
> > code.
> >
> > Thanks,
> >
> > Mathieu
> >
> >
> >> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> >> ---
> >> include/linux/ltt-channels.h | 45 ++++++-
> >> include/linux/marker.h | 4
> >> kernel/marker.c | 95 +++------------
> >> ltt/ltt-channels.c | 271 +++++++++++++++++++++++++------------------
> >> ltt/ltt-tracer.c | 9 +
> >> 5 files changed, 230 insertions(+), 194 deletions(-)
> >> diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
> >> index ce19eda..edf959b 100644
> >> --- a/include/linux/ltt-channels.h
> >> +++ b/include/linux/ltt-channels.h
> >> @@ -8,8 +8,10 @@
> >> */
> >>
> >> #include <linux/limits.h>
> >> +#include <linux/marker.h>
> >>
> >> #define EVENTS_PER_CHANNEL 65536
> >> +#define EVENTS_MOST_WASTE_IDS 128
> >>
> >> struct ltt_trace_struct;
> >> struct rchan_buf;
> >> @@ -51,15 +53,51 @@ struct ltt_channel_struct {
> >> struct ltt_channel_setting {
> >> unsigned int subbuf_size;
> >> unsigned int subbuf_cnt;
> >> - struct kref kref; /* Number of references to structure content */
> >> + int trace_count;
> >> + int live_count;
> >> struct list_head list;
> >> + struct list_head marker_entry;
> >> unsigned int index; /* index of channel in trace channel array */
> >> u16 free_event_id; /* Next event ID to allocate */
> >> char name[PATH_MAX];
> >> };
> >>
> >> -int ltt_channels_register(const char *name);
> >> -int ltt_channels_unregister(const char *name);
> >> +/*
> >> + * Note about RCU :
> >> + * It is used to make sure every handler has finished using its private data
> >> + * between two consecutive operation (add or remove) on a given marker. It is
> >> + * also used to delay the free of multiple probes array until a quiescent state
> >> + * is reached.
> >> + * marker entries modifications are protected by the markers_mutex.
> >> + */
> >> +struct marker_entry {
> >> + struct hlist_node hlist;
> >> + char *format;
> >> + char *name;
> >> + /* Probe wrapper */
> >> + void (*call)(const struct marker *mdata, void *call_private, ...);
> >> + struct marker_probe_closure single;
> >> + struct marker_probe_closure *multi;
> >> + int refcount; /* Number of times armed. 0 if disarmed. */
> >> + struct rcu_head rcu;
> >> + void *oldptr;
> >> + int rcu_pending;
> >> + u16 channel_id;
> >> + u16 event_id;
> >> + unsigned char ptype:1;
> >> + unsigned char format_allocated:1;
> >> + unsigned char used:1;
> >> + struct list_head sibling; /* sibling of channel's maker_entry */
> >> + struct ltt_channel_setting *lcs;
> >> + char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
> >> +};
> >> +int __makrer_set_channel(struct marker_entry *entry, const char *channel);
> >> +
> >> +int start_trace_channel(const char *channel);
> >> +int stop_trace_channel(const char *channel);
> >> +const char *get_name_from_eid(const char *channel, u16 eID);
> >> +const char *get_fmt_from_eid(const char *channel, u16 eID);
> >> +
> >> int ltt_channels_set_default(const char *name,
> >> unsigned int subbuf_size,
> >> unsigned int subbuf_cnt);
> >> @@ -69,7 +107,6 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> >> int overwrite,
> >> int active);
> >> void ltt_channels_trace_free(struct ltt_channel_struct *channels);
> >> -int _ltt_channels_get_event_id(const char *channel, const char *name);
> >> int ltt_channels_get_event_id(const char *channel, const char *name);
> >>
> >> #endif /* _LTT_CHANNELS_H */
> >> diff --git a/include/linux/marker.h b/include/linux/marker.h
> >> index 32387e8..82e4f7c 100644
> >> --- a/include/linux/marker.h
> >> +++ b/include/linux/marker.h
> >> @@ -15,7 +15,6 @@
> >> #include <stdarg.h>
> >> #include <linux/types.h>
> >> #include <linux/immediate.h>
> >> -#include <linux/ltt-channels.h>
> >>
> >> struct module;
> >> struct task_struct;
> >> @@ -126,6 +125,7 @@ struct marker {
> >>
> >> extern void marker_update_probe_range(struct marker *begin,
> >> struct marker *end);
> >> +extern void marker_update_probes(void);
> >>
> >> #define GET_MARKER(channel, name) (__mark_##channel##_##name)
> >>
> >> @@ -199,8 +199,6 @@ static inline void marker_update_probe_range(struct marker *begin,
> >> extern void lock_markers(void);
> >> extern void unlock_markers(void);
> >>
> >> -extern void markers_compact_event_ids(void);
> >> -
> >> /* To be used for string format validity checking with gcc */
> >> static inline void __printf(1, 2) ___mark_check_format(const char *fmt, ...)
> >> {
> >> diff --git a/kernel/marker.c b/kernel/marker.c
> >> index b11a82a..cd38ba0 100644
> >> --- a/kernel/marker.c
> >> +++ b/kernel/marker.c
> >> @@ -22,6 +22,7 @@
> >> #include <linux/list.h>
> >> #include <linux/rcupdate.h>
> >> #include <linux/marker.h>
> >> +#include <linux/ltt-channels.h>
> >> #include <linux/err.h>
> >> #include <linux/slab.h>
> >> #include <linux/immediate.h>
> >> @@ -60,33 +61,6 @@ void unlock_markers(void)
> >> #define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
> >> static struct hlist_head marker_table[MARKER_TABLE_SIZE];
> >>
> >> -/*
> >> - * Note about RCU :
> >> - * It is used to make sure every handler has finished using its private data
> >> - * between two consecutive operation (add or remove) on a given marker. It is
> >> - * also used to delay the free of multiple probes array until a quiescent state
> >> - * is reached.
> >> - * marker entries modifications are protected by the markers_mutex.
> >> - */
> >> -struct marker_entry {
> >> - struct hlist_node hlist;
> >> - char *format;
> >> - char *name;
> >> - /* Probe wrapper */
> >> - void (*call)(const struct marker *mdata, void *call_private, ...);
> >> - struct marker_probe_closure single;
> >> - struct marker_probe_closure *multi;
> >> - int refcount; /* Number of times armed. 0 if disarmed. */
> >> - struct rcu_head rcu;
> >> - void *oldptr;
> >> - int rcu_pending;
> >> - u16 channel_id;
> >> - u16 event_id;
> >> - unsigned char ptype:1;
> >> - unsigned char format_allocated:1;
> >> - char channel[0]; /* Contains channel'\0'name'\0'format'\0' */
> >> -};
> >> -
> >> #ifdef CONFIG_MARKERS_USERSPACE
> >> static void marker_update_processes(void);
> >> #else
> >> @@ -474,7 +448,6 @@ static int remove_marker(const char *channel, const char *name)
> >> size_t channel_len = strlen(channel) + 1;
> >> size_t name_len = strlen(name) + 1;
> >> u32 hash;
> >> - int ret;
> >>
> >> hash = jhash(channel, channel_len-1, 0) ^ jhash(name, name_len-1, 0);
> >> head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
> >> @@ -488,11 +461,26 @@ static int remove_marker(const char *channel, const char *name)
> >> return -ENOENT;
> >> if (e->single.func != __mark_empty_function)
> >> return -EBUSY;
> >> +
> >> + if (e->lcs) {
> >> + e->lcs->live_count--;
> >> + if (!e->lcs->live_count && !e->lcs->trace_count) {
> >> + BUG_ON(!list_empty(&e->lcs->marker_entry));
> >> + list_del(&e->lcs->list);
> >> + kfree(e->lcs);
> >> + e->lcs = NULL;
> >> + }
> >> + }
> >> +
> >> + /* It will be released in remove_chanel_marker_entries() */
> >> + if (e->used && e->lcs && e->lcs->trace_count)
> >> + return 0;
> >> +
> >> hlist_del(&e->hlist);
> >> if (e->format_allocated)
> >> kfree(e->format);
> >> - ret = ltt_channels_unregister(e->channel);
> >> - WARN_ON(ret);
> >> + if (e->lcs)
> >> + list_del(&e->sibling);
> >> /* Make sure the call_rcu has been executed */
> >> if (e->rcu_pending)
> >> rcu_barrier_sched();
> >> @@ -540,6 +528,7 @@ static int set_marker(struct marker_entry *entry, struct marker *elem,
> >> if (ret)
> >> return ret;
> >> }
> >> + entry->used = 1;
> >>
> >> /*
> >> * probe_cb setup (statically known) is done here. It is
> >> @@ -695,7 +684,7 @@ void marker_update_probe_range(struct marker *begin,
> >> * Site effect : marker_set_format may delete the marker entry (creating a
> >> * replacement).
> >> */
> >> -static void marker_update_probes(void)
> >> +void marker_update_probes(void)
> >> {
> >> /* Core kernel markers */
> >> marker_update_probe_range(__start___markers, __stop___markers);
> >> @@ -738,18 +727,9 @@ int marker_probe_register(const char *channel, const char *name,
> >> ret = PTR_ERR(entry);
> >> if (ret)
> >> goto end;
> >> - ret = ltt_channels_register(channel);
> >> + ret = __makrer_set_channel(entry, channel);
> >> if (ret)
> >> goto error_remove_marker;
> >> - ret = ltt_channels_get_index_from_name(channel);
> >> - if (ret < 0)
> >> - goto error_unregister_channel;
> >> - entry->channel_id = ret;
> >> - ret = ltt_channels_get_event_id(channel, name);
> >> - if (ret < 0)
> >> - goto error_unregister_channel;
> >> - entry->event_id = ret;
> >> - ret = 0;
> >> trace_mark(metadata, core_marker_id,
> >> "channel %s name %s event_id %hu "
> >> "int #1u%zu long #1u%zu pointer #1u%zu "
> >> @@ -776,7 +756,7 @@ int marker_probe_register(const char *channel, const char *name,
> >> if (IS_ERR(old)) {
> >> ret = PTR_ERR(old);
> >> if (first_probe)
> >> - goto error_unregister_channel;
> >> + goto error_remove_marker;
> >> else
> >> goto end;
> >> }
> >> @@ -797,9 +777,6 @@ int marker_probe_register(const char *channel, const char *name,
> >> call_rcu_sched(&entry->rcu, free_old_closure);
> >> goto end;
> >>
> >> -error_unregister_channel:
> >> - ret_err = ltt_channels_unregister(channel);
> >> - WARN_ON(ret_err);
> >> error_remove_marker:
> >> ret_err = remove_marker(channel, name);
> >> WARN_ON(ret_err);
> >> @@ -997,34 +974,6 @@ void *marker_get_private_data(const char *channel, const char *name,
> >> }
> >> EXPORT_SYMBOL_GPL(marker_get_private_data);
> >>
> >> -/**
> >> - * markers_compact_event_ids: Compact markers event IDs and reassign channels
> >> - *
> >> - * Called when no channel users are active by the channel infrastructure.
> >> - * Called with lock_markers() and channel mutex held.
> >> - */
> >> -void markers_compact_event_ids(void)
> >> -{
> >> - struct marker_entry *entry;
> >> - unsigned int i;
> >> - struct hlist_head *head;
> >> - struct hlist_node *node;
> >> - int ret;
> >> -
> >> - for (i = 0; i < MARKER_TABLE_SIZE; i++) {
> >> - head = &marker_table[i];
> >> - hlist_for_each_entry(entry, node, head, hlist) {
> >> - ret = ltt_channels_get_index_from_name(entry->channel);
> >> - WARN_ON(ret < 0);
> >> - entry->channel_id = ret;
> >> - ret = _ltt_channels_get_event_id(entry->channel,
> >> - entry->name);
> >> - WARN_ON(ret < 0);
> >> - entry->event_id = ret;
> >> - }
> >> - }
> >> -}
> >> -
> >> #ifdef CONFIG_MODULES
> >>
> >> /**
> >> diff --git a/ltt/ltt-channels.c b/ltt/ltt-channels.c
> >> index c57e737..e876f04 100644
> >> --- a/ltt/ltt-channels.c
> >> +++ b/ltt/ltt-channels.c
> >> @@ -13,13 +13,12 @@
> >> #include <linux/ltt-channels.h>
> >> #include <linux/mutex.h>
> >> #include <linux/vmalloc.h>
> >> +#include <linux/marker.h>
> >> +#include <linux/err.h>
> >> +#include <linux/ltt-tracer.h>
> >> +
> >> +LIST_HEAD(ltt_channels);
> >>
> >> -/*
> >> - * ltt_channel_mutex may be nested inside the LTT trace mutex.
> >> - * ltt_channel_mutex mutex may be nested inside markers mutex.
> >> - */
> >> -static DEFINE_MUTEX(ltt_channel_mutex);
> >> -static LIST_HEAD(ltt_channels);
> >> /*
> >> * Index of next channel in array. Makes sure that as long as a trace channel is
> >> * allocated, no array index will be re-used when a channel is freed and then
> >> @@ -40,104 +39,165 @@ static struct ltt_channel_setting *lookup_channel(const char *name)
> >> return NULL;
> >> }
> >>
> >> -/*
> >> - * 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
> >> - * event IDs when no users are active.
> >> - *
> >> - * Called with lock_markers() and channels mutex held.
> >> - */
> >> -static void release_channel_setting(struct kref *kref)
> >> -{
> >> - struct ltt_channel_setting *setting = container_of(kref,
> >> - struct ltt_channel_setting, kref);
> >> - struct ltt_channel_setting *iter;
> >> +static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
> >> + const char *name);
> >>
> >> - if (atomic_read(&index_kref.refcount) == 0
> >> - && atomic_read(&setting->kref.refcount) == 0) {
> >> - list_del(&setting->list);
> >> - kfree(setting);
> >> +int __makrer_set_channel(struct marker_entry *entry, const char *channel)
> >> +{
> >> + struct ltt_channel_setting *setting = lookup_channel(channel);
> >> + if (!setting) {
> >> + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> >> + if (!setting)
> >> + return -ENOMEM;
> >> + list_add_tail(&setting->list, <t_channels);
> >> + strncpy(setting->name, channel, PATH_MAX-1);
> >> + setting->index = free_index++;
> >> + INIT_LIST_HEAD(&setting->marker_entry);
> >> + }else if (setting->free_event_id == EVENTS_PER_CHANNEL - 1)
> >> + return -ENOSPC;
> >> + setting->live_count++;
> >> + list_add(&entry->sibling, &setting->marker_entry);
> >> + entry->channel_id = setting->index;
> >> + entry->event_id = _ltt_channels_get_event_id(setting, entry->name);
> >> + return 0;
> >> +}
> >>
> >> - free_index = 0;
> >> - list_for_each_entry(iter, <t_channels, list) {
> >> - iter->index = free_index++;
> >> - iter->free_event_id = 0;
> >> +static void defrag_id(struct ltt_channel_setting *setting)
> >> +{
> >> + struct marker_entry *e;
> >> + int free_count = setting->free_event_id;
> >> + if (setting->live_count + EVENTS_MOST_WASTE_IDS > free_count)
> >> + return;
> >> + free_count = 0;
> >> + list_for_each_entry(e, &setting->marker_entry, sibling) {
> >> + if (e->event_id != free_count) {
> >> + trace_mark(metadata, core_marker_id,
> >> + "channel %s name %s event_id %hu "
> >> + "int #1u%zu long #1u%zu pointer #1u%zu "
> >> + "size_t #1u%zu alignment #1u%u",
> >> + e->channel, e->name, (u16)free_count,
> >> + sizeof(int), sizeof(long), sizeof(void *),
> >> + sizeof(size_t), ltt_get_alignment());
> >> }
> >> - markers_compact_event_ids();
> >> + e->event_id = free_count++;
> >> }
> >> + setting->free_event_id = (u16)free_count;
> >> + marker_update_probes();
> >> }
> >>
> >> -/*
> >> - * Perform channel index compaction when the last trace channel is freed.
> >> - *
> >> - * Called with lock_markers() and channels mutex held.
> >> - */
> >> -static void release_trace_channel(struct kref *kref)
> >> +static void remove_chanel_marker_entries(struct ltt_channel_setting *setting)
> >> {
> >> - struct ltt_channel_setting *iter, *n;
> >> + struct marker_entry *e, *n;
> >> +
> >> + list_for_each_entry_safe(e, n, &setting->marker_entry, sibling) {
> >> + if (e->single.func != __mark_empty_function)
> >> + continue;
> >>
> >> - list_for_each_entry_safe(iter, n, <t_channels, list)
> >> - release_channel_setting(&iter->kref);
> >> + hlist_del(&e->hlist);
> >> + if (e->format_allocated)
> >> + kfree(e->format);
> >> + list_del(&e->sibling);
> >> + /* Make sure the call_rcu has been executed */
> >> + if (e->rcu_pending)
> >> + rcu_barrier_sched();
> >> + kfree(e);
> >> + }
> >> }
> >>
> >> -/**
> >> - * ltt_channels_register: Register a trace channel.
> >> - *
> >> - * Uses refcounting.
> >> - */
> >> -int ltt_channels_register(const char *name)
> >> +int start_trace_channel(const char *channel)
> >> {
> >> + int ret = -ENOENT;
> >> struct ltt_channel_setting *setting;
> >> - int ret = 0;
> >>
> >> - mutex_lock(<t_channel_mutex);
> >> - setting = lookup_channel(name);
> >> + lock_markers();
> >> + setting = lookup_channel(channel);
> >> + if (setting) {
> >> + if (!setting->trace_count)
> >> + defrag_id(setting);
> >> + setting->trace_count++;
> >> + ret = 0;
> >> + }
> >> + unlock_markers();
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(start_trace_channel);
> >> +
> >> +int stop_trace_channel(const char *channel)
> >> +{
> >> + int ret = -ENOENT;
> >> + struct ltt_channel_setting *setting;
> >> +
> >> + lock_markers();
> >> + setting = lookup_channel(channel);
> >> if (setting) {
> >> - if (atomic_read(&setting->kref.refcount) == 0)
> >> - goto init_kref;
> >> - else {
> >> - kref_get(&setting->kref);
> >> - goto end;
> >> + setting->trace_count--;
> >> + if (!setting->trace_count)
> >> + remove_chanel_marker_entries(setting);
> >> + ret = 0;
> >> + if (!setting->live_count && !setting->trace_count) {
> >> + BUG_ON(!list_empty(&setting->marker_entry));
> >> + list_del(&setting->list);
> >> + kfree(setting);
> >> }
> >> }
> >> - setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> >> - if (!setting) {
> >> - ret = -ENOMEM;
> >> - goto end;
> >> + unlock_markers();
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(stop_trace_channel);
> >> +
> >> +/* permanent! the return value is valid before stop_strace_channel() */
> >> +
> >> +const char *get_name_from_eid(const char *channel, u16 eID)
> >> +{
> >> + char *ret = ERR_PTR(-ENOENT);
> >> + struct ltt_channel_setting *setting;
> >> + struct marker_entry *e;
> >> +
> >> + lock_markers();
> >> + setting = lookup_channel(channel);
> >> + list_for_each_entry(e, &setting->marker_entry, sibling) {
> >> + if (e->event_id == eID) {
> >> + ret = e->name;
> >> + break;
> >> + }
> >> }
> >> - list_add(&setting->list, <t_channels);
> >> - strncpy(setting->name, name, PATH_MAX-1);
> >> - setting->index = free_index++;
> >> -init_kref:
> >> - kref_init(&setting->kref);
> >> -end:
> >> - mutex_unlock(<t_channel_mutex);
> >> + unlock_markers();
> >> return ret;
> >> }
> >> -EXPORT_SYMBOL_GPL(ltt_channels_register);
> >> +EXPORT_SYMBOL_GPL(get_name_from_eid);
> >>
> >> -/**
> >> - * ltt_channels_unregister(): Unregister a trace channel.
> >> - *
> >> - * Must be called with markers mutex held.
> >> - */
> >> -int ltt_channels_unregister(const char *name)
> >> +const char *get_fmt_from_eid(const char *channel, u16 eID)
> >> {
> >> + char *ret = ERR_PTR(-ENOENT);
> >> struct ltt_channel_setting *setting;
> >> - int ret = 0;
> >> + struct marker_entry *e;
> >>
> >> - mutex_lock(<t_channel_mutex);
> >> - setting = lookup_channel(name);
> >> - if (!setting || atomic_read(&setting->kref.refcount) == 0) {
> >> - ret = -ENOENT;
> >> - goto end;
> >> + lock_markers();
> >> + setting = lookup_channel(channel);
> >> + list_for_each_entry(e, &setting->marker_entry, sibling) {
> >> + if (e->event_id == eID) {
> >> + if (e->used)
> >> + ret = e->format;
> >> + break;
> >> + }
> >> }
> >> - kref_put(&setting->kref, release_channel_setting);
> >> -end:
> >> - mutex_unlock(<t_channel_mutex);
> >> + unlock_markers();
> >> return ret;
> >> }
> >> -EXPORT_SYMBOL_GPL(ltt_channels_unregister);
> >> +EXPORT_SYMBOL_GPL(get_fmt_from_eid);
> >> +
> >> +/*
> >> + * 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
> >> + * index when no users are active.
> >> + */
> >> +static void release_channel_index(struct kref *kref)
> >> +{
> >> + struct ltt_channel_setting *setting;
> >> + free_index = 0;
> >> + list_for_each_entry(setting, <t_channels, list)
> >> + setting->index = free_index++;
> >> +}
> >>
> >> /**
> >> * ltt_channels_set_default: Set channel default behavior.
> >> @@ -149,16 +209,16 @@ int ltt_channels_set_default(const char *name,
> >> struct ltt_channel_setting *setting;
> >> int ret = 0;
> >>
> >> - mutex_lock(<t_channel_mutex);
> >> + lock_markers();
> >> setting = lookup_channel(name);
> >> - if (!setting || atomic_read(&setting->kref.refcount) == 0) {
> >> + if (!setting) {
> >> ret = -ENOENT;
> >> goto end;
> >> }
> >> setting->subbuf_size = subbuf_size;
> >> setting->subbuf_cnt = subbuf_cnt;
> >> end:
> >> - mutex_unlock(<t_channel_mutex);
> >> + unlock_markers();
> >> return ret;
> >> }
> >> EXPORT_SYMBOL_GPL(ltt_channels_set_default);
> >> @@ -174,7 +234,7 @@ const char *ltt_channels_get_name_from_index(unsigned int index)
> >> struct ltt_channel_setting *iter;
> >>
> >> list_for_each_entry(iter, <t_channels, list)
> >> - if (iter->index == index && atomic_read(&iter->kref.refcount))
> >> + if (iter->index == index)
> >> return iter->name;
> >> return NULL;
> >> }
> >> @@ -186,8 +246,7 @@ ltt_channels_get_setting_from_name(const char *name)
> >> struct ltt_channel_setting *iter;
> >>
> >> list_for_each_entry(iter, <t_channels, list)
> >> - if (!strcmp(iter->name, name)
> >> - && atomic_read(&iter->kref.refcount))
> >> + if (!strcmp(iter->name, name))
> >> return iter;
> >> return NULL;
> >> }
> >> @@ -228,7 +287,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> >> struct ltt_channel_struct *channel = NULL;
> >> struct ltt_channel_setting *iter;
> >>
> >> - mutex_lock(<t_channel_mutex);
> >> + lock_markers();
> >> if (!free_index)
> >> goto end;
> >> if (!atomic_read(&index_kref.refcount))
> >> @@ -241,7 +300,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> >> if (!channel)
> >> goto end;
> >> list_for_each_entry(iter, <t_channels, list) {
> >> - if (!atomic_read(&iter->kref.refcount))
> >> + if (!iter->live_count)
> >> continue;
> >> channel[iter->index].subbuf_size = iter->subbuf_size;
> >> channel[iter->index].subbuf_cnt = iter->subbuf_cnt;
> >> @@ -250,7 +309,7 @@ struct ltt_channel_struct *ltt_channels_trace_alloc(unsigned int *nr_channels,
> >> channel[iter->index].channel_name = iter->name;
> >> }
> >> end:
> >> - mutex_unlock(<t_channel_mutex);
> >> + unlock_markers();
> >> return channel;
> >> }
> >> EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
> >> @@ -264,33 +323,18 @@ EXPORT_SYMBOL_GPL(ltt_channels_trace_alloc);
> >> void ltt_channels_trace_free(struct ltt_channel_struct *channels)
> >> {
> >> lock_markers();
> >> - mutex_lock(<t_channel_mutex);
> >> - kfree(channels);
> >> - kref_put(&index_kref, release_trace_channel);
> >> - mutex_unlock(<t_channel_mutex);
> >> + kref_put(&index_kref, release_channel_index);
> >> unlock_markers();
> >> + kfree(channels);
> >> }
> >> EXPORT_SYMBOL_GPL(ltt_channels_trace_free);
> >>
> >> -/**
> >> - * _ltt_channels_get_event_id: get next event ID for a marker
> >> - * @channel: channel name
> >> - * @name: event name
> >> - *
> >> - * Returns a unique event ID (for this channel) or < 0 on error.
> >> - * Must be called with channels mutex held.
> >> - */
> >> -int _ltt_channels_get_event_id(const char *channel, const char *name)
> >> +static int _ltt_channels_get_event_id(struct ltt_channel_setting *setting,
> >> + const char *name)
> >> {
> >> - struct ltt_channel_setting *setting;
> >> int ret;
> >>
> >> - setting = ltt_channels_get_setting_from_name(channel);
> >> - if (!setting) {
> >> - ret = -ENOENT;
> >> - goto end;
> >> - }
> >> - if (strcmp(channel, "metadata") == 0) {
> >> + if (strcmp(setting->name, "metadata") == 0) {
> >> if (strcmp(name, "core_marker_id") == 0)
> >> ret = 0;
> >> else if (strcmp(name, "core_marker_format") == 0)
> >> @@ -299,7 +343,7 @@ int _ltt_channels_get_event_id(const char *channel, const char *name)
> >> ret = -ENOENT;
> >> goto end;
> >> }
> >> - if (setting->free_event_id == EVENTS_PER_CHANNEL) {
> >> + if (setting->free_event_id == EVENTS_PER_CHANNEL - 1) {
> >> ret = -ENOSPC;
> >> goto end;
> >> }
> >> @@ -316,11 +360,14 @@ end:
> >> */
> >> int ltt_channels_get_event_id(const char *channel, const char *name)
> >> {
> >> - int ret;
> >> + int ret = -ENOENT;
> >> + struct ltt_channel_setting *setting;
> >>
> >> - mutex_lock(<t_channel_mutex);
> >> - ret = _ltt_channels_get_event_id(channel, name);
> >> - mutex_unlock(<t_channel_mutex);
> >> + lock_markers();
> >> + setting = ltt_channels_get_setting_from_name(channel);
> >> + if (setting)
> >> + ret = _ltt_channels_get_event_id(setting, name);
> >> + unlock_markers();
> >> return ret;
> >> }
> >>
> >> diff --git a/ltt/ltt-tracer.c b/ltt/ltt-tracer.c
> >> index af14135..71289f9 100644
> >> --- a/ltt/ltt-tracer.c
> >> +++ b/ltt/ltt-tracer.c
> >> @@ -772,6 +772,7 @@ int ltt_trace_alloc(const char *trace_name)
> >> subbuf_size = trace->channels[chan].subbuf_size;
> >> subbuf_cnt = trace->channels[chan].subbuf_cnt;
> >> prepare_chan_size_num(&subbuf_size, &subbuf_cnt);
> >> + start_trace_channel(channel_name);
> >> err = trace->ops->create_channel(trace_name, trace,
> >> trace->dentry.trace_root,
> >> channel_name,
> >> @@ -801,8 +802,10 @@ int ltt_trace_alloc(const char *trace_name)
> >>
> >> create_channel_error:
> >> for (chan--; chan >= 0; chan--)
> >> - if (trace->channels[chan].active)
> >> + if (trace->channels[chan].active) {
> >> trace->ops->remove_channel(&trace->channels[chan]);
> >> + stop_trace_channel(trace->channels[chan].channel_name);
> >> + }
> >>
> >> dirs_error:
> >> module_put(trace->transport->owner);
> >> @@ -901,8 +904,10 @@ static void __ltt_trace_destroy(struct ltt_trace_struct *trace)
> >>
> >> for (i = 0; i < trace->nr_channels; i++) {
> >> chan = &trace->channels[i];
> >> - if (chan->active)
> >> + if (chan->active) {
> >> trace->ops->remove_channel(chan);
> >> + stop_trace_channel(chan->channel_name);
> >> + }
> >> }
> >>
> >> kref_put(&trace->ltt_transport_kref, ltt_release_transport);
> >>
> >>
> >>
> >> _______________________________________________
> >> 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] 6+ messages in thread
* [ltt-dev] [PATCH] ltt-channel: permanent eID
2008-12-18 4:42 ` Mathieu Desnoyers
@ 2008-12-18 6:07 ` Lai Jiangshan
2008-12-18 15:05 ` Mathieu Desnoyers
0 siblings, 1 reply; 6+ messages in thread
From: Lai Jiangshan @ 2008-12-18 6:07 UTC (permalink / raw)
Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>
> Thanks for this description, comments below,
>
>> changelog:
>>
>> Marker ID and its data must be permanent for text output, this patch
>> introduce permanent eID.
>>
>> 1) per channel permanent eID
>>
>> Basic struct for store eID's data is struct marker_entry. When a channel
>> is activated, all the struct marker_entry for this channel are permanent
>> until this channel is deactivated. In this period, event if the marker
>> is leaved or other things are happened, the eID is permanent.
>>
>
> Ok, so you keep the struct marker_entry around until the channel is
> unused (all traces are destroyed, including that all in-kernel ascii
> converters have released the trace) ?
When "this channel is deactivated", the unused struct marker_entry for
this channel will be removed, event if this channel is still live.
all traces are destroyed means all channel are deactivated.
when we trace a channel, we say this channel is activated.
>
>> This patch introduces 4 API for permanent eID:
>> int start_trace_channel(const char *channel); - activate a channel
>> int stop_trace_channel(const char *channel); - deactivate a channel
>
> Hrm, can we change the names for
> get_trace_channel()
> put_trace_channel() ?
>
> start and stop sounds a bit misleading with the tracing start/stop
> semantics.
>
> Also, why do we have "waste" ids ? This is bad, because whenever
> possible, we should fit IDs in ~28 IDs allowed for "compact" event ids.
> If the IDs are larger than this, the tracer will have to generate an
> extended header and waste trace space. Do we really _need_ such id waste?
The previous code has a degression: when eID for a marker is changed,
userspace tool do not know the ID changed. this patch make eIDs are
not changed too frequent.
>
>> const char *get_name_from_eid(const char *channel, u16 eID); - get name from eID
>> const char *get_fmt_from_eid(const char *channel, u16 eID); - get fmt from eID
>>
>> See ltt/ltt-tracer.c for the using of start/stop_trace_channel().
>>
> hrm, stop_trace_channel (or put_trace_channel) is in __destroy, but
> should probably be in the kref desctructor, so we are sure that it's
> only called with all references to the trace data, including the
> in-kernel ascii converters, have released their reference.
>
start_trace_channel()
create ltt relay channel
ID's life time
destory ltt relay channel
stop_trace_channel()
Is it incorrect?
>
>
>> When a channel is deactivated(its trace_count come to 0), we will defrag eID
>> for this channel. AND we use "trace_mark(metadata, core_marker_id," to tell
>> userspace that id is reset.
>
> Sounds good.
>
>> 2) per channel struct marker_entry(permanent eID' data) management
>>
>> All struct marker_entry of a channel are threaded by struct list_head.
>>
>> It will help us remove a struct marker_entry quickly and help us manage
>> entries quickly(defrag_id(), remove_chanel_marker_entries())
>>
>> We also call marker_update_probes() in defrag_id().
>>
>
> Hrm, aren't we taking the marker mutex twice here ? I am probably
> missing something.
Oh, I forgot marker_update_probes() needs marker.
This patch have been tested, but without marker_update_probes() in defrag_id().
It was added when I saw my review note about markers_compact_event_ids()
when I was going to send patch.
marker_update_probes() is affirmatively need after defrag_id() or
markers_compact_event_ids()(removed function).
>
>> Since struct marker_entry is permanent eID' data. We move the definition
>> of struct marker_entry from marker.c to ltt-channel.h
>>
>
> BTW, I would not use the term "permanent" for this. Because permanent,
> to me, really means "we allocate something that we will only free when
> the machine reboots". We are simply extending the life of the the eIDs
> for the duration of all references to those IDs.
>
> Let's say that we extend the event ID lifetime to the overall event ID
> references, which now includes trace references, which includes ascii
> converters.
>
>> 3) as previous code, we defrag channel index when all tracer is stopped.
>>
>> different: As described in 1), permanent eID will defraged when a
>> channel is deactivated.
>>
>> It means this patch has more fine-grained ID management.
>>
>
> There are two cases here :
>
> Tracing starts
> add/remove markers many times
> Tracing stops -> defrag
>
> And
>
> No tracing
> add/remove markers many times -> defrag each time
>
> You don't say much about what is done at marker add/remove when tracing
> is inactive. Is there some compaction done ?
start/stop_trace_channel() is very short. Reviewing the code is better than
ugly English comment:
stop_trace_channel()
remove_chanel_marker_entries() ------- remove unused marker_entry
start_trace_channel()
defrag_id() ----Usually, defrag_id() should in stop_trace_channel()
but delay defrag_id() until start_trace_channel() is better.
if no tracing
removing marker_entry is done quickly. not defrag id
if tracing
removing marker_entry will be delayed.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH] ltt-channel: permanent eID
2008-12-18 6:07 ` Lai Jiangshan
@ 2008-12-18 15:05 ` Mathieu Desnoyers
0 siblings, 0 replies; 6+ messages in thread
From: Mathieu Desnoyers @ 2008-12-18 15:05 UTC (permalink / raw)
* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> >
> > Thanks for this description, comments below,
> >
> >> changelog:
> >>
> >> Marker ID and its data must be permanent for text output, this patch
> >> introduce permanent eID.
> >>
> >> 1) per channel permanent eID
> >>
> >> Basic struct for store eID's data is struct marker_entry. When a channel
> >> is activated, all the struct marker_entry for this channel are permanent
> >> until this channel is deactivated. In this period, event if the marker
> >> is leaved or other things are happened, the eID is permanent.
> >>
> >
> > Ok, so you keep the struct marker_entry around until the channel is
> > unused (all traces are destroyed, including that all in-kernel ascii
> > converters have released the trace) ?
>
> When "this channel is deactivated", the unused struct marker_entry for
> this channel will be removed, event if this channel is still live.
> all traces are destroyed means all channel are deactivated.
>
> when we trace a channel, we say this channel is activated.
>
OK
> >
> >> This patch introduces 4 API for permanent eID:
> >> int start_trace_channel(const char *channel); - activate a channel
> >> int stop_trace_channel(const char *channel); - deactivate a channel
> >
> > Hrm, can we change the names for
> > get_trace_channel()
> > put_trace_channel() ?
> >
> > start and stop sounds a bit misleading with the tracing start/stop
> > semantics.
> >
> > Also, why do we have "waste" ids ? This is bad, because whenever
> > possible, we should fit IDs in ~28 IDs allowed for "compact" event ids.
> > If the IDs are larger than this, the tracer will have to generate an
> > extended header and waste trace space. Do we really _need_ such id waste?
>
> The previous code has a degression: when eID for a marker is changed,
> userspace tool do not know the ID changed. this patch make eIDs are
> not changed too frequent.
>
An event ID should _never_ be re-used for a different marker when a trace
is active. But when tracing is activated, we defragment the ID space (as
you do). Actually, we could postpone the ID allocation to tracing
activation rather than doing it at marker registration.
> >
> >> const char *get_name_from_eid(const char *channel, u16 eID); - get name from eID
> >> const char *get_fmt_from_eid(const char *channel, u16 eID); - get fmt from eID
> >>
> >> See ltt/ltt-tracer.c for the using of start/stop_trace_channel().
> >>
> > hrm, stop_trace_channel (or put_trace_channel) is in __destroy, but
> > should probably be in the kref desctructor, so we are sure that it's
> > only called with all references to the trace data, including the
> > in-kernel ascii converters, have released their reference.
> >
>
> start_trace_channel()
> create ltt relay channel
>
> ID's life time
>
> destory ltt relay channel
> stop_trace_channel()
>
> Is it incorrect?
>
This is correct, but the "destroy ltt relay channel" does not happen
exactly in
for (i = 0; i < trace->nr_channels; i++) {
chan = &trace->channels[i];
if (chan->active)
trace->ops->remove_channel(chan);
}
Because all it calls is :
static void ltt_relay_remove_channel(struct ltt_channel_struct *channel)
{
struct rchan *rchan = channel->trans_channel_data;
ltt_relay_close(rchan);
-> does a kref_put(&buf->kref, relay_remove_buf);
kref_put(&channel->kref, ltt_relay_release_channel);
}
But relay_file_open (in ltt-relay-alloc.c) does :
kref_get(&buf->kref);
And relay_file_release in ltt-relay-alloc.c does :
kref_put(&buf->kref, relay_remove_buf);
__ltt_trace_destroy
trace->ops->remove_channel -> ltt-relay.c:ltt_relay_remove_channel
ltt-relay-alloc.c:ltt_relay_close
ltt-relay-alloc.c:relay_close_buf
kref_put(&buf->kref, relay_remove_buf);
So we basically have to :
- Have __ltt_trace_destroy releasing the kref
- Have any file opener releasing the kref
Before the following gets called :
ltt-relay-alloc.c:relay_remove_buf
buf->chan->cb->remove_buf_file -> ltt-relay.c:ltt_remove_buf_file_callback
ltt_relay_destroy_buffer
kref_put(<t_chan->trace->ltt_transport_kref,
ltt_release_transport);
kref_put(<t_chan->kref, ltt_relay_release_channel);
kref_put(&trace->kref, ltt_release_trace);
relay_destroy_buf
kref_put(&chan->kref, relay_destroy_channel);
If we look below in __ltt_trace_destroy, we see :
/*
* Wait for lttd readers to release the files, therefore making sure
* the last subbuffers have been read.
*/
if (atomic_read(&trace->kref.refcount) > 1) {
int ret = 0;
__wait_event_interruptible(trace->kref_wq,
(atomic_read(&trace->kref.refcount) == 1), ret);
}
kref_put(&trace->kref, ltt_release_trace);
This lets us wait for any file opener to exit before we continue.
So, the correct moment to do the stop_trace_channel(chan->channel_name);
is when all references to the traces are done. The easiest spot to put
it is in ltt_release_trace() just before the ltt_channels_trace_free
call.
> >
> >
> >> When a channel is deactivated(its trace_count come to 0), we will defrag eID
> >> for this channel. AND we use "trace_mark(metadata, core_marker_id," to tell
> >> userspace that id is reset.
> >
> > Sounds good.
> >
> >> 2) per channel struct marker_entry(permanent eID' data) management
> >>
> >> All struct marker_entry of a channel are threaded by struct list_head.
> >>
> >> It will help us remove a struct marker_entry quickly and help us manage
> >> entries quickly(defrag_id(), remove_chanel_marker_entries())
> >>
> >> We also call marker_update_probes() in defrag_id().
> >>
> >
> > Hrm, aren't we taking the marker mutex twice here ? I am probably
> > missing something.
>
> Oh, I forgot marker_update_probes() needs marker.
> This patch have been tested, but without marker_update_probes() in defrag_id().
> It was added when I saw my review note about markers_compact_event_ids()
> when I was going to send patch.
> marker_update_probes() is affirmatively need after defrag_id() or
> markers_compact_event_ids()(removed function).
>
Yep.
> >
> >> Since struct marker_entry is permanent eID' data. We move the definition
> >> of struct marker_entry from marker.c to ltt-channel.h
> >>
> >
> > BTW, I would not use the term "permanent" for this. Because permanent,
> > to me, really means "we allocate something that we will only free when
> > the machine reboots". We are simply extending the life of the the eIDs
> > for the duration of all references to those IDs.
> >
> > Let's say that we extend the event ID lifetime to the overall event ID
> > references, which now includes trace references, which includes ascii
> > converters.
> >
> >> 3) as previous code, we defrag channel index when all tracer is stopped.
> >>
> >> different: As described in 1), permanent eID will defraged when a
> >> channel is deactivated.
> >>
> >> It means this patch has more fine-grained ID management.
> >>
> >
> > There are two cases here :
> >
> > Tracing starts
> > add/remove markers many times
> > Tracing stops -> defrag
> >
> > And
> >
> > No tracing
> > add/remove markers many times -> defrag each time
> >
> > You don't say much about what is done at marker add/remove when tracing
> > is inactive. Is there some compaction done ?
>
> start/stop_trace_channel() is very short. Reviewing the code is better than
> ugly English comment:
>
> stop_trace_channel()
> remove_chanel_marker_entries() ------- remove unused marker_entry
>
> start_trace_channel()
> defrag_id() ----Usually, defrag_id() should in stop_trace_channel()
> but delay defrag_id() until start_trace_channel() is better.
>
> if no tracing
> removing marker_entry is done quickly. not defrag id
>
> if tracing
> removing marker_entry will be delayed.
>
That makes sense.
Mathieu
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-12-18 15:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-12-16 13:28 [ltt-dev] [PATCH] ltt-channel: permanent eID Lai Jiangshan
2008-12-17 19:40 ` Mathieu Desnoyers
2008-12-18 2:22 ` Lai Jiangshan
2008-12-18 4:42 ` Mathieu Desnoyers
2008-12-18 6:07 ` Lai Jiangshan
2008-12-18 15:05 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox