tracing: 'hist' triggers

Add tracing_map, a special-purpose lock-free map for tracing.

tracing_map is designed to aggregate or 'sum' one or more values associated with a specific object of type tracing_map_elt, which is associated by the map to a given key.

It provides various hooks allowing per-tracer customization and is separated out into a separate file in order to allow it to be shared between multiple tracers, but isn't meant to be generally used outside of that context.

The tracing_map implementation was inspired by lock-free map algorithms originated by Dr. Cliff Click:

http://www.azulsystems.com/blog/cliff/2007-03-26-non-blocking-hashtable
http://www.azulsystems.com/events/javaone_2007/2007_LockFreeHash.pdf

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

kernel/trace/tracing_map.c new file +22 -0

+
+	for (i = 0; i < a->n_pages; i++)
+		free_page((unsigned long)a->pages[i]);
+
+	kfree(a);
+}
+
+struct tracing_map_array *tracing_map_array_alloc(unsigned int n_elts,
+						  unsigned int entry_size)
+{
+	struct tracing_map_array *a;
+	unsigned int i;
+
+	a = kzalloc(sizeof(*a), GFP_KERNEL);
+	if (!a)
+		return NULL;
+
+	a->entry_size_shift = fls(roundup_pow_of_two(entry_size) - 1);
+	a->entries_per_page = PAGE_SIZE / (1 << a->entry_size_shift);
+	a->n_pages = n_elts / a->entries_per_page;
+	if (!a->n_pages)
+		a->n_pages = 1;

kernel/trace/tracing_map.c new file +11 -0

+			goto free;
+	}
+ out:
+	return a;
+ free:
+	tracing_map_array_free(a);
+	a = NULL;
+
+	goto out;
+}
+

kernel/trace/tracing_map.c new file +58 -0

+/**
+ * tracing_map_insert - Insert key and/or retrieve val from a tracing_map
+ * @map: The tracing_map to insert into
+ * @key: The key to insert
+ *
+ * Inserts a key into a tracing_map and creates and returns a new
+ * tracing_map_elt for it, or if the key has already been inserted by
+ * a previous call, returns the tracing_map_elt already associated
+ * with it.  When the map was created, the number of elements to be
+ * allocated for the map was specified (internally maintained as
+ * 'max_elts' in struct tracing_map), and that number of
+ * tracing_map_elts was created by tracing_map_init().  This is the
+ * pre-allocated pool of tracing_map_elts that tracing_map_insert()
+ * will allocate from when adding new keys.  Once that pool is
+ * exhausted, tracing_map_insert() is useless and will return NULL to
+ * signal that state.
+ *
+ * This is a lock-free tracing map insertion function implementing a
+ * modified form of Cliff Click's basic insertion algorithm.  It
+ * requires the table size be a power of two.  To prevent any
+ * possibility of an infinite loop we always make the internal table
+ * size double the size of the requested table size (max_elts * 2).
+ * Likewise, we never reuse a slot or resize or delete elements - when
+ * we've reached max_elts entries, we simply return NULL once we've
+ * run out of entries.  Readers can at any point in time traverse the
+ * tracing map and safely access the key/val pairs.
+ *
+ * Return: the tracing_map_elt pointer val associated with the key.
+ * If this was a newly inserted key, the val will be a newly allocated
+ * and associated tracing_map_elt pointer val.  If the key wasn't
+ * found and the pool of tracing_map_elts has been exhausted, NULL is
+ * returned and no further insertions will succeed.
+ */
+struct tracing_map_elt *tracing_map_insert(struct tracing_map *map, void *key)
+{
+	u32 idx, key_hash, test_key;
+	struct tracing_map_entry *entry;
+
+	key_hash = jhash(key, map->key_size, 0);
+	if (key_hash == 0)
+		key_hash = 1;
+	idx = key_hash >> (32 - (map->map_bits + 1));
+
+	while (1) {
+		idx &= (map->map_size - 1);
+		entry = TRACING_MAP_ENTRY(map->map, idx);
+		test_key = entry->key;
+
+		if (test_key && test_key == key_hash && entry->val &&
+		    keys_match(key, entry->val->key, map->key_size))
+			return entry->val;
+
+		if (!test_key && !cmpxchg(&entry->key, 0, key_hash)) {
+			struct tracing_map_elt *elt;
+
+			elt = get_free_elt(map);
+			if (!elt)
+				break;

Hi Tom,

Quoted 20 lines · Tom Zanussi

Add tracing_map, a special-purpose lock-free map for tracing.

tracing_map is designed to aggregate or 'sum' one or more values associated with a specific object of type tracing_map_elt, which is associated by the map to a given key.

It provides various hooks allowing per-tracer customization and is separated out into a separate file in order to allow it to be shared between multiple tracers, but isn't meant to be generally used outside of that context.

The tracing_map implementation was inspired by lock-free map algorithms originated by Dr. Cliff Click:

http://www.azulsystems.com/blog/cliff/2007-03-26-non-blocking-hashtable
http://www.azulsystems.com/events/javaone_2007/2007_LockFreeHash.pdf

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

[SNIP]

kernel/trace/tracing_map.c line 222

Quoted 6 lines

kernel/trace/tracing_map.c new file +6 -0

+void tracing_map_array_free(struct tracing_map_array *a)
+{
+	unsigned int i;
+
+	if (!a->pages)
+		return;

You need to free 'a' anyway.. Or did you want to check 'a' being NULL?

Quoted 27 lines

kernel/trace/tracing_map.c new file +26 -0

+
+	for (i = 0; i < a->n_pages; i++)
+		free_page((unsigned long)a->pages[i]);
+
+	kfree(a);
+}
+
+struct tracing_map_array *tracing_map_array_alloc(unsigned int n_elts,
+						  unsigned int entry_size)
+{
+	struct tracing_map_array *a;
+	unsigned int i;
+
+	a = kzalloc(sizeof(*a), GFP_KERNEL);
+	if (!a)
+		return NULL;
+
+	a->entry_size_shift = fls(roundup_pow_of_two(entry_size) - 1);
+	a->entries_per_page = PAGE_SIZE / (1 << a->entry_size_shift);
+	a->n_pages = n_elts / a->entries_per_page;
+	if (!a->n_pages)
+		a->n_pages = 1;
+	a->entry_shift = fls(a->entries_per_page) - 1;
+	a->entry_mask = (1 << a->entry_shift) - 1;
+
+	a->pages = kcalloc(a->n_pages, sizeof(void *), GFP_KERNEL);

kernel/trace/tracing_map.c line 254

Quoted 6 lines

kernel/trace/tracing_map.c new file +6 -0

+	if (!a->pages)
+		goto free;
+
+	for (i = 0; i < a->n_pages; i++) {
+		a->pages[i] = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!a->pages)

		if (!a->pages[i])

Quoted 12 lines

kernel/trace/tracing_map.c new file +11 -0

+			goto free;
+	}
+ out:
+	return a;
+ free:
+	tracing_map_array_free(a);
+	a = NULL;
+
+	goto out;
+}
+

[SNIP]

Quoted 58 lines

kernel/trace/tracing_map.c new file +58 -0

+/**
+ * tracing_map_insert - Insert key and/or retrieve val from a tracing_map
+ * @map: The tracing_map to insert into
+ * @key: The key to insert
+ *
+ * Inserts a key into a tracing_map and creates and returns a new
+ * tracing_map_elt for it, or if the key has already been inserted by
+ * a previous call, returns the tracing_map_elt already associated
+ * with it.  When the map was created, the number of elements to be
+ * allocated for the map was specified (internally maintained as
+ * 'max_elts' in struct tracing_map), and that number of
+ * tracing_map_elts was created by tracing_map_init().  This is the
+ * pre-allocated pool of tracing_map_elts that tracing_map_insert()
+ * will allocate from when adding new keys.  Once that pool is
+ * exhausted, tracing_map_insert() is useless and will return NULL to
+ * signal that state.
+ *
+ * This is a lock-free tracing map insertion function implementing a
+ * modified form of Cliff Click's basic insertion algorithm.  It
+ * requires the table size be a power of two.  To prevent any
+ * possibility of an infinite loop we always make the internal table
+ * size double the size of the requested table size (max_elts * 2).
+ * Likewise, we never reuse a slot or resize or delete elements - when
+ * we've reached max_elts entries, we simply return NULL once we've
+ * run out of entries.  Readers can at any point in time traverse the
+ * tracing map and safely access the key/val pairs.
+ *
+ * Return: the tracing_map_elt pointer val associated with the key.
+ * If this was a newly inserted key, the val will be a newly allocated
+ * and associated tracing_map_elt pointer val.  If the key wasn't
+ * found and the pool of tracing_map_elts has been exhausted, NULL is
+ * returned and no further insertions will succeed.
+ */
+struct tracing_map_elt *tracing_map_insert(struct tracing_map *map, void *key)
+{
+	u32 idx, key_hash, test_key;
+	struct tracing_map_entry *entry;
+
+	key_hash = jhash(key, map->key_size, 0);
+	if (key_hash == 0)
+		key_hash = 1;
+	idx = key_hash >> (32 - (map->map_bits + 1));
+
+	while (1) {
+		idx &= (map->map_size - 1);
+		entry = TRACING_MAP_ENTRY(map->map, idx);
+		test_key = entry->key;
+
+		if (test_key && test_key == key_hash && entry->val &&
+		    keys_match(key, entry->val->key, map->key_size))
+			return entry->val;
+
+		if (!test_key && !cmpxchg(&entry->key, 0, key_hash)) {
+			struct tracing_map_elt *elt;
+
+			elt = get_free_elt(map);
+			if (!elt)
+				break;

kernel/trace/tracing_map.c line 467

Quoted 10 lines

kernel/trace/tracing_map.c new file +10 -0

+			memcpy(elt->key, key, map->key_size);
+			entry->val = elt;
+
+			return entry->val;
+		}
+		idx++;
+	}
+
+	return NULL;
+}

IIUC this always insert new entry if no matching key found. And if the map is full, it only fails after walking through the entries to find an empty one, mark the entry with the key and call to get_free_elt() returns NULL. As more key added, it worsenes the problem since more entries will be marked with no value IMHO.

I can see you checked hist_data->drops in the next patch to work around this problem. But IMHO it's suboptimal since it cannot update the existing entries too. I think it'd be better having lookup-only version of this function and use it after it sees drops. The lookup function can bail out from the loop if the insert doesn't mark empty entry anymore IMHO.

Thoughts?

Thanks,
Namhyung

Hi Namhyung,

Quoted 98 lines · Namhyung Kim

Hi Tom,

Quoted 78 lines · Tom Zanussi

Add tracing_map, a special-purpose lock-free map for tracing.

tracing_map is designed to aggregate or 'sum' one or more values associated with a specific object of type tracing_map_elt, which is associated by the map to a given key.

It provides various hooks allowing per-tracer customization and is separated out into a separate file in order to allow it to be shared between multiple tracers, but isn't meant to be generally used outside of that context.

The tracing_map implementation was inspired by lock-free map algorithms originated by Dr. Cliff Click:

http://www.azulsystems.com/blog/cliff/2007-03-26-non-blocking-hashtable
http://www.azulsystems.com/events/javaone_2007/2007_LockFreeHash.pdf

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

kernel/trace/tracing_map.c new file +58 -0

+/**
+ * tracing_map_insert - Insert key and/or retrieve val from a tracing_map
+ * @map: The tracing_map to insert into
+ * @key: The key to insert
+ *
+ * Inserts a key into a tracing_map and creates and returns a new
+ * tracing_map_elt for it, or if the key has already been inserted by
+ * a previous call, returns the tracing_map_elt already associated
+ * with it.  When the map was created, the number of elements to be
+ * allocated for the map was specified (internally maintained as
+ * 'max_elts' in struct tracing_map), and that number of
+ * tracing_map_elts was created by tracing_map_init().  This is the
+ * pre-allocated pool of tracing_map_elts that tracing_map_insert()
+ * will allocate from when adding new keys.  Once that pool is
+ * exhausted, tracing_map_insert() is useless and will return NULL to
+ * signal that state.
+ *
+ * This is a lock-free tracing map insertion function implementing a
+ * modified form of Cliff Click's basic insertion algorithm.  It
+ * requires the table size be a power of two.  To prevent any
+ * possibility of an infinite loop we always make the internal table
+ * size double the size of the requested table size (max_elts * 2).
+ * Likewise, we never reuse a slot or resize or delete elements - when
+ * we've reached max_elts entries, we simply return NULL once we've
+ * run out of entries.  Readers can at any point in time traverse the
+ * tracing map and safely access the key/val pairs.
+ *
+ * Return: the tracing_map_elt pointer val associated with the key.
+ * If this was a newly inserted key, the val will be a newly allocated
+ * and associated tracing_map_elt pointer val.  If the key wasn't
+ * found and the pool of tracing_map_elts has been exhausted, NULL is
+ * returned and no further insertions will succeed.
+ */
+struct tracing_map_elt *tracing_map_insert(struct tracing_map *map, void *key)
+{
+	u32 idx, key_hash, test_key;
+	struct tracing_map_entry *entry;
+
+	key_hash = jhash(key, map->key_size, 0);
+	if (key_hash == 0)
+		key_hash = 1;
+	idx = key_hash >> (32 - (map->map_bits + 1));
+
+	while (1) {
+		idx &= (map->map_size - 1);
+		entry = TRACING_MAP_ENTRY(map->map, idx);
+		test_key = entry->key;
+
+		if (test_key && test_key == key_hash && entry->val &&
+		    keys_match(key, entry->val->key, map->key_size))
+			return entry->val;
+
+		if (!test_key && !cmpxchg(&entry->key, 0, key_hash)) {
+			struct tracing_map_elt *elt;
+
+			elt = get_free_elt(map);
+			if (!elt)
+				break;

kernel/trace/tracing_map.c line 467

Quoted 10 lines

kernel/trace/tracing_map.c new file +10 -0

+			memcpy(elt->key, key, map->key_size);
+			entry->val = elt;
+
+			return entry->val;
+		}
+		idx++;
+	}
+
+	return NULL;
+}

IIUC this always insert new entry if no matching key found. And if the map is full, it only fails after walking through the entries to find an empty one, mark the entry with the key and call to get_free_elt() returns NULL. As more key added, it worsenes the problem since more entries will be marked with no value IMHO.

Quoted 9 lines

I can see you checked hist_data->drops in the next patch to work around this problem. But IMHO it's suboptimal since it cannot update the existing entries too. I think it'd be better having lookup-only version of this function and use it after it sees drops. The lookup function can bail out from the loop if the insert doesn't mark empty entry anymore IMHO.

Thoughts?

The assumption has always been that once you have drops (i.e. tracing_map_insert() returns NULL), the data can't really be trusted any more and tracing should just stop (and presumably be restarted with a bigger table). It doesn't mean that the data is completely useless, just that it no longer can be assumed to have captured all the events over the tracing run. Having a lookup-only version for the purpose of updating only existing entries sort of illustrates the problem even better - in that case only the events that already have entries in the table will be included while the events that don't yet have entries will be ignored, skewing the value of the data.

On the other hand, if users do end up calling this even after it's returned NULL, we should make sure it doesn't result in an infinite loop, and cap the number of iterations through the loop. tracing_map wasn't really meant to be generally reusable - it was separated out so it could be shared between two tracers - but it wouldn't hurt to add that check just in case...

Hi Tom,

Quoted 111 lines · Tom Zanussi

Hi Namhyung,

Quoted 98 lines · Namhyung Kim

Hi Tom,

Quoted 78 lines · Tom Zanussi

Add tracing_map, a special-purpose lock-free map for tracing.

tracing_map is designed to aggregate or 'sum' one or more values associated with a specific object of type tracing_map_elt, which is associated by the map to a given key.

It provides various hooks allowing per-tracer customization and is separated out into a separate file in order to allow it to be shared between multiple tracers, but isn't meant to be generally used outside of that context.

The tracing_map implementation was inspired by lock-free map algorithms originated by Dr. Cliff Click:

http://www.azulsystems.com/blog/cliff/2007-03-26-non-blocking-hashtable
http://www.azulsystems.com/events/javaone_2007/2007_LockFreeHash.pdf

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

kernel/trace/tracing_map.c new file +58 -0

+/**
+ * tracing_map_insert - Insert key and/or retrieve val from a tracing_map
+ * @map: The tracing_map to insert into
+ * @key: The key to insert
+ *
+ * Inserts a key into a tracing_map and creates and returns a new
+ * tracing_map_elt for it, or if the key has already been inserted by
+ * a previous call, returns the tracing_map_elt already associated
+ * with it.  When the map was created, the number of elements to be
+ * allocated for the map was specified (internally maintained as
+ * 'max_elts' in struct tracing_map), and that number of
+ * tracing_map_elts was created by tracing_map_init().  This is the
+ * pre-allocated pool of tracing_map_elts that tracing_map_insert()
+ * will allocate from when adding new keys.  Once that pool is
+ * exhausted, tracing_map_insert() is useless and will return NULL to
+ * signal that state.
+ *
+ * This is a lock-free tracing map insertion function implementing a
+ * modified form of Cliff Click's basic insertion algorithm.  It
+ * requires the table size be a power of two.  To prevent any
+ * possibility of an infinite loop we always make the internal table
+ * size double the size of the requested table size (max_elts * 2).
+ * Likewise, we never reuse a slot or resize or delete elements - when
+ * we've reached max_elts entries, we simply return NULL once we've
+ * run out of entries.  Readers can at any point in time traverse the
+ * tracing map and safely access the key/val pairs.
+ *
+ * Return: the tracing_map_elt pointer val associated with the key.
+ * If this was a newly inserted key, the val will be a newly allocated
+ * and associated tracing_map_elt pointer val.  If the key wasn't
+ * found and the pool of tracing_map_elts has been exhausted, NULL is
+ * returned and no further insertions will succeed.
+ */
+struct tracing_map_elt *tracing_map_insert(struct tracing_map *map, void *key)
+{
+	u32 idx, key_hash, test_key;
+	struct tracing_map_entry *entry;
+
+	key_hash = jhash(key, map->key_size, 0);
+	if (key_hash == 0)
+		key_hash = 1;
+	idx = key_hash >> (32 - (map->map_bits + 1));
+
+	while (1) {
+		idx &= (map->map_size - 1);
+		entry = TRACING_MAP_ENTRY(map->map, idx);
+		test_key = entry->key;
+
+		if (test_key && test_key == key_hash && entry->val &&
+		    keys_match(key, entry->val->key, map->key_size))
+			return entry->val;
+
+		if (!test_key && !cmpxchg(&entry->key, 0, key_hash)) {
+			struct tracing_map_elt *elt;
+
+			elt = get_free_elt(map);
+			if (!elt)
+				break;

kernel/trace/tracing_map.c line 467

Quoted 10 lines

kernel/trace/tracing_map.c new file +10 -0

+			memcpy(elt->key, key, map->key_size);
+			entry->val = elt;
+
+			return entry->val;
+		}
+		idx++;
+	}
+
+	return NULL;
+}

IIUC this always insert new entry if no matching key found. And if the map is full, it only fails after walking through the entries to find an empty one, mark the entry with the key and call to get_free_elt() returns NULL. As more key added, it worsenes the problem since more entries will be marked with no value IMHO.

Quoted 9 lines

I can see you checked hist_data->drops in the next patch to work around this problem. But IMHO it's suboptimal since it cannot update the existing entries too. I think it'd be better having lookup-only version of this function and use it after it sees drops. The lookup function can bail out from the loop if the insert doesn't mark empty entry anymore IMHO.

Thoughts?

Quoted 10 lines

The assumption has always been that once you have drops (i.e. tracing_map_insert() returns NULL), the data can't really be trusted any more and tracing should just stop (and presumably be restarted with a bigger table). It doesn't mean that the data is completely useless, just that it no longer can be assumed to have captured all the events over the tracing run. Having a lookup-only version for the purpose of updating only existing entries sort of illustrates the problem even better - in that case only the events that already have entries in the table will be included while the events that don't yet have entries will be ignored, skewing the value of the data.

I thought it'd be better if users can see which one is the real drop or not. IOW if drop count is much smaller than the normal event count, [s]he might want to ignore the occasional drops. Otherwise, [s]he should restart with a bigger table. This requires accurate counts of events and drops though.

Quoted 8 lines

On the other hand, if users do end up calling this even after it's returned NULL, we should make sure it doesn't result in an infinite loop, and cap the number of iterations through the loop. tracing_map wasn't really meant to be generally reusable - it was separated out so it could be shared between two tracers - but it wouldn't hurt to add that check just in case...

Right.

Thanks,
Namhyung

Hi Namhyung,

Quoted 6 lines · Namhyung Kim

I thought it'd be better if users can see which one is the real drop or not. IOW if drop count is much smaller than the normal event count, [s]he might want to ignore the occasional drops. Otherwise, [s]he should restart with a bigger table. This requires accurate counts of events and drops though.

OK, how about the below - it basically moves the drops set/test/inc into tracing_map_insert(), as well as a total hits count. So those values will be available for users to use in deciding whether to use the data or restart with a bigger table, and the loop is bailed out of only if no matching keys are found and there are drops, so callers can continue updating existing entries.

Users who want the original behavior still get the NULL return and can stop calling tracing_map_insert() as before:

struct tracing_map_elt *tracing_map_insert(struct tracing_map *map, void *key)

{
        u32 idx, key_hash, test_key;
        struct tracing_map_entry *entry;

        key_hash = jhash(key, map->key_size, 0);
        if (key_hash == 0)
                key_hash = 1;
        idx = key_hash >> (32 - (map->map_bits + 1));

        while (1) {
                idx &= (map->map_size - 1);
                entry = TRACING_MAP_ENTRY(map->map, idx);
                test_key = entry->key;

                if (test_key && test_key == key_hash && entry->val &&
                    keys_match(key, entry->val->key, map->key_size)) {

                        atomic64_inc(&map->hits);
                        return entry->val;
                }

                if (atomic64_read(&map->drops)) {
                        atomic64_inc(&map->drops);
                        break;
                }

                if (!test_key && !cmpxchg(&entry->key, 0, key_hash)) {
                        struct tracing_map_elt *elt;

                        elt = get_free_elt(map);
                        if (!elt) {
                                atomic64_inc(&map->drops);

                                break;
                        }
                        memcpy(elt->key, key, map->key_size);
                        entry->val = elt;

                        atomic64_inc(&map->hits);
                        return entry->val;
                }
                idx++;
        }

        return NULL;
}

Hi Tom,

Quoted 10 lines · Tom Zanussi

Hi Namhyung,

Quoted 6 lines · Namhyung Kim

I thought it'd be better if users can see which one is the real drop or not. IOW if drop count is much smaller than the normal event count, [s]he might want to ignore the occasional drops. Otherwise, [s]he should restart with a bigger table. This requires accurate counts of events and drops though.

Quoted 6 lines

OK, how about the below - it basically moves the drops set/test/inc into tracing_map_insert(), as well as a total hits count. So those values will be available for users to use in deciding whether to use the data or restart with a bigger table, and the loop is bailed out of only if no matching keys are found and there are drops, so callers can continue updating existing entries.

But if a key didn't get a desired index, it'd still fail to update..

Quoted 21 lines

Users who want the original behavior still get the NULL return and can stop calling tracing_map_insert() as before:

struct tracing_map_elt *tracing_map_insert(struct tracing_map *map, void *key)

{
        u32 idx, key_hash, test_key;
        struct tracing_map_entry *entry;

        key_hash = jhash(key, map->key_size, 0);
        if (key_hash == 0)
                key_hash = 1;
        idx = key_hash >> (32 - (map->map_bits + 1));

        while (1) {
                idx &= (map->map_size - 1);
                entry = TRACING_MAP_ENTRY(map->map, idx);
                test_key = entry->key;

                if (test_key && test_key == key_hash && entry->val &&
                    keys_match(key, entry->val->key, map->key_size)) {

Quoted 8 lines

                        atomic64_inc(&map->hits);
                        return entry->val;
                }

                if (atomic64_read(&map->drops)) {
                        atomic64_inc(&map->drops);
                        break;
                }

IMHO it should be removed.

Quoted 7 lines

                if (!test_key && !cmpxchg(&entry->key, 0, key_hash)) {
                        struct tracing_map_elt *elt;

                        elt = get_free_elt(map);
                        if (!elt) {
                                atomic64_inc(&map->drops);

And reset entry->key here..

Quoted 5 lines

                                break;
                        }
                        memcpy(elt->key, key, map->key_size);
                        entry->val = elt;

Quoted 8 lines

                        atomic64_inc(&map->hits);
                        return entry->val;
                }
                idx++;
        }

        return NULL;
}

Then tracing_map_lookup() can be implemented like *_insert but bail out from the loop if test_key is 0. The caller might do like this:

	if (!atomic64_read(&map->drops))
		elt = tracing_map_insert(...);
	else
		elt = tracing_map_lookup(...);

Thanks,
Namhyung

'hist' triggers allow users to continually aggregate trace events, which can then be viewed afterwards by simply reading a 'hist' file containing the aggregation in a human-readable format.

The basic idea is very simple and boils down to a mechanism whereby trace events, rather than being exhaustively dumped in raw form and viewed directly, are automatically 'compressed' into meaningful tables completely defined by the user.

This is done strictly via single-line command-line commands and without the aid of any kind of programming language or interpreter.

A surprising number of typical use cases can be accomplished by users via this simple mechanism. In fact, a large number of the tasks that users typically do using the more complicated script-based tracing tools, at least during the initial stages of an investigation, can be accomplished by simply specifying a set of keys and values to be used in the creation of a hash table.

The Linux kernel trace event subsystem happens to provide an extensive list of keys and values ready-made for such a purpose in the form of the event format files associated with each trace event. By simply consulting the format file for field names of interest and by plugging them into the hist trigger command, users can create an endless number of useful aggregations to help with investigating various properties of the system. See Documentation/trace/events.txt for examples.

hist triggers are implemented on top of the existing event trigger infrastructure, and as such are consistent with the existing triggers from a user's perspective as well.

The basic syntax follows the existing trigger syntax. Users start an aggregation by writing a 'hist' trigger to the event of interest's trigger file:

  # echo hist:keys=xxx [ if filter] > event/trigger

Once a hist trigger has been set up, by default it continually aggregates every matching event into a hash table using the event key and a value field named 'hitcount'.

To view the aggregation at any point in time, simply read the 'hist' file in the same directory as the 'trigger' file:

  # cat event/hist

The detailed syntax provides additional options for user control, and is described exhaustively in Documentation/trace/events.txt and in the virtual tracing/README file in the tracing subsystem.

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

kernel/trace/trace_events_hist.c new file +16 -0

+static int event_hist_trigger_func(struct event_command *cmd_ops,
+				   struct trace_event_file *file,
+				   char *glob, char *cmd, char *param)
+{
+	unsigned int hist_trigger_bits = TRACING_MAP_BITS_DEFAULT;
+	struct event_trigger_data *trigger_data;
+	struct hist_trigger_attrs *attrs;
+	struct event_trigger_ops *trigger_ops;
+	struct hist_trigger_data *hist_data;
+	char *trigger;
+	int ret = 0;
+
+	if (!param)
+		return -EINVAL;
+
+	/* separate the trigger from the filter (k:v [if filter]) */

kernel/trace/trace_events_hist.c new file +50 -0

+
+	trigger_data->count = -1;
+	trigger_data->ops = trigger_ops;
+	trigger_data->cmd_ops = cmd_ops;
+
+	INIT_LIST_HEAD(&trigger_data->list);
+	RCU_INIT_POINTER(trigger_data->filter, NULL);
+
+	trigger_data->private_data = hist_data;
+
+	if (glob[0] == '!') {
+		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
+		ret = 0;
+		goto out_free;
+	}
+
+	if (!param) /* if param is non-empty, it's supposed to be a filter */
+		goto out_reg;
+
+	if (!cmd_ops->set_filter)
+		goto out_reg;
+
+	ret = cmd_ops->set_filter(param, trigger_data, file);
+	if (ret < 0)
+		goto out_free;
+ out_reg:
+	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
+	/*
+	 * The above returns on success the # of triggers registered,
+	 * but if it didn't register any it returns zero.  Consider no
+	 * triggers registered a failure too.
+	 */
+	if (!ret) {
+		ret = -ENOENT;
+		goto out_free;
+	} else if (ret < 0)
+		goto out_free;
+	/* Just return zero, not the number of registered triggers */
+	ret = 0;
+ out:
+	return ret;
+ out_free:
+	if (cmd_ops->set_filter)
+		cmd_ops->set_filter(NULL, trigger_data, NULL);
+
+	kfree(trigger_data);
+
+	destroy_hist_data(hist_data);
+	goto out;
+}

Quoted 53 lines · Tom Zanussi

'hist' triggers allow users to continually aggregate trace events, which can then be viewed afterwards by simply reading a 'hist' file containing the aggregation in a human-readable format.

The basic idea is very simple and boils down to a mechanism whereby trace events, rather than being exhaustively dumped in raw form and viewed directly, are automatically 'compressed' into meaningful tables completely defined by the user.

This is done strictly via single-line command-line commands and without the aid of any kind of programming language or interpreter.

A surprising number of typical use cases can be accomplished by users via this simple mechanism. In fact, a large number of the tasks that users typically do using the more complicated script-based tracing tools, at least during the initial stages of an investigation, can be accomplished by simply specifying a set of keys and values to be used in the creation of a hash table.

The Linux kernel trace event subsystem happens to provide an extensive list of keys and values ready-made for such a purpose in the form of the event format files associated with each trace event. By simply consulting the format file for field names of interest and by plugging them into the hist trigger command, users can create an endless number of useful aggregations to help with investigating various properties of the system. See Documentation/trace/events.txt for examples.

hist triggers are implemented on top of the existing event trigger infrastructure, and as such are consistent with the existing triggers from a user's perspective as well.

The basic syntax follows the existing trigger syntax. Users start an aggregation by writing a 'hist' trigger to the event of interest's trigger file:

  # echo hist:keys=xxx [ if filter] > event/trigger

Once a hist trigger has been set up, by default it continually aggregates every matching event into a hash table using the event key and a value field named 'hitcount'.

To view the aggregation at any point in time, simply read the 'hist' file in the same directory as the 'trigger' file:

  # cat event/hist

The detailed syntax provides additional options for user control, and is described exhaustively in Documentation/trace/events.txt and in the virtual tracing/README file in the tracing subsystem.

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

[SNIP]

Quoted 16 lines

kernel/trace/trace_events_hist.c new file +16 -0

+static int event_hist_trigger_func(struct event_command *cmd_ops,
+				   struct trace_event_file *file,
+				   char *glob, char *cmd, char *param)
+{
+	unsigned int hist_trigger_bits = TRACING_MAP_BITS_DEFAULT;
+	struct event_trigger_data *trigger_data;
+	struct hist_trigger_attrs *attrs;
+	struct event_trigger_ops *trigger_ops;
+	struct hist_trigger_data *hist_data;
+	char *trigger;
+	int ret = 0;
+
+	if (!param)
+		return -EINVAL;
+
+	/* separate the trigger from the filter (k:v [if filter]) */

kernel/trace/trace_events_hist.c line 747

Quoted 10 lines

kernel/trace/trace_events_hist.c new file +10 -0

+	trigger = strsep(&param, " \t");
+	if (!trigger)
+		return -EINVAL;
+
+	attrs = parse_hist_trigger_attrs(trigger);
+	if (IS_ERR(attrs))
+		return PTR_ERR(attrs);
+
+	if (!attrs->keys_str)
+		return -EINVAL;

Wouldn't it leak the attrs?

kernel/trace/trace_events_hist.c line 754

Quoted 7 lines

kernel/trace/trace_events_hist.c new file +7 -0

+
+	if (attrs->map_bits)
+		hist_trigger_bits = attrs->map_bits;
+
+	hist_data = create_hist_data(hist_trigger_bits, attrs, file);
+	if (IS_ERR(hist_data))
+		return PTR_ERR(hist_data);

It also can leak the attrs IMHO.

kernel/trace/trace_events_hist.c line 761

Quoted 7 lines

kernel/trace/trace_events_hist.c new file +7 -0

+
+	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
+
+	ret = -ENOMEM;
+	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	if (!trigger_data)
+		goto out;

Here, the hist_data and the attr can be leaked.

Thanks,
Namhyung

Quoted 50 lines

kernel/trace/trace_events_hist.c new file +50 -0

+
+	trigger_data->count = -1;
+	trigger_data->ops = trigger_ops;
+	trigger_data->cmd_ops = cmd_ops;
+
+	INIT_LIST_HEAD(&trigger_data->list);
+	RCU_INIT_POINTER(trigger_data->filter, NULL);
+
+	trigger_data->private_data = hist_data;
+
+	if (glob[0] == '!') {
+		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
+		ret = 0;
+		goto out_free;
+	}
+
+	if (!param) /* if param is non-empty, it's supposed to be a filter */
+		goto out_reg;
+
+	if (!cmd_ops->set_filter)
+		goto out_reg;
+
+	ret = cmd_ops->set_filter(param, trigger_data, file);
+	if (ret < 0)
+		goto out_free;
+ out_reg:
+	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
+	/*
+	 * The above returns on success the # of triggers registered,
+	 * but if it didn't register any it returns zero.  Consider no
+	 * triggers registered a failure too.
+	 */
+	if (!ret) {
+		ret = -ENOENT;
+		goto out_free;
+	} else if (ret < 0)
+		goto out_free;
+	/* Just return zero, not the number of registered triggers */
+	ret = 0;
+ out:
+	return ret;
+ out_free:
+	if (cmd_ops->set_filter)
+		cmd_ops->set_filter(NULL, trigger_data, NULL);
+
+	kfree(trigger_data);
+
+	destroy_hist_data(hist_data);
+	goto out;
+}

Allow users to specify trace event fields to use in aggregated sums via a new 'vals=' keyword. Before this addition, the only aggregated sum supported was the implied value 'hitcount'. With this addition, 'hitcount' is also supported as an explicit value field, as is any numeric trace event field.

This expands the hist trigger syntax from this:

  # echo hist:keys=xxx [ if filter] > event/trigger

to this:

  # echo hist:keys=xxx:vals=yyy [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

kernel/trace/trace.c +6 -5

 	"\t            [:size=#entries]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
-	"\t    table using the key named, and the value of a sum called\n"
-	"\t    'hitcount' is incremented.  Keys correspond to fields in the\n"
-	"\t    event's format description.  Keys can be any field.  The\n"
-	"\t    'size' parameter can be  used to specify more or fewer than\n"
-	"\t    the default 2048 entries for the hashtable size.\n\n"
+	"\t    table using the key(s) and value(s) named, and the value of a\n"
+	"\t    sum called 'hitcount' is incremented.  Keys and values\n"
+	"\t    correspond to fields in the event's format description.  Keys\n"
+	"\t    can be any field.  Values must correspond to numeric fields.\n"
+	"\t    The 'size' parameter can be  used to specify more or fewer\n"
+	"\t    than the default 2048 entries for the hashtable size.\n\n"
 	"\t    Reading the 'hist' file for the event will dump the hash\n"
 	"\t    table in its entirety to stdout."
 #endif

kernel/trace/trace.c +6 -0

@@ -534,6 +593,12 @@ hist_trigger_entry_print(struct seq_file *m,
 	seq_printf(m, " hitcount: %10llu",
 		   tracing_map_read_sum(elt, HITCOUNT_IDX));
 
+	for (i = 1; i < hist_data->n_vals; i++) {
+		seq_printf(m, "  %s: %10llu",
+			   hist_data->fields[i]->field->name,
+			   tracing_map_read_sum(elt, i));
+	}
+
 	seq_puts(m, "\n");
 }
 

kernel/trace/trace.c +6 -0

+			seq_puts(m, "hitcount");
+		else {
+			seq_puts(m, ",");
+			hist_field_print(m, hist_data->fields[i]);
+		}
+	}
 
 	seq_puts(m, ":sort=");
 	seq_puts(m, "hitcount");

Signature · 1 line

1.9.3

Allow users to specify keys and/or values to sort on. With this addition, keys and values specified using the 'keys=' and 'vals=' keywords can be used to sort the hist trigger output via a new 'sort=' keyword. If multiple sort keys are specified, the output will be sorted using the second key as a secondary sort key, etc. The default sort order is ascending; if the user wants a different sort order, '.descending' can be appended to the specific sort key. Before this addition, output was always sorted by 'hitcount' in ascending order.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy \
          [ if filter] > event/trigger

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---

kernel/trace/trace_events_hist.c +78 -1

+	if (!fields_str)
+		goto out;
+
+	strsep(&fields_str, "=");
+	if (!fields_str) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	for (i = 0; i < TRACING_MAP_SORT_KEYS_MAX; i++) {
+		char *field_str, *field_name;
+
+		sort_key = &hist_data->sort_keys[i];
+
+		field_str = strsep(&fields_str, ",");
+		if (!field_str) {
+			if (i == 0)
+				ret = -EINVAL;
+			break;
+		}
+
+		if ((i == TRACING_MAP_SORT_KEYS_MAX - 1) && fields_str) {
+			ret = -EINVAL;
+			break;
+		}
+
+		field_name = strsep(&field_str, ".");
+		if (!field_name) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (!strcmp(field_name, "hitcount")) {
+			ret = is_descending(field_str);
+			if (ret < 0)
+				break;
+			sort_key->descending = ret;
+			continue;
+		}
+
+		for (j = 1; j < hist_data->n_fields; j++) {
+			field = hist_data->fields[j]->field;
+			if (field && !strcmp(field_name, field->name)) {
+				sort_key->field_idx = j;
+				ret = is_descending(field_str);
+				if (ret < 0)
+					goto out;
+				sort_key->descending = ret;
+				break;
+			}
+		}
+		if (j == hist_data->n_fields) {
+			ret = -EINVAL;
+			break;
+		}
+	}
+	hist_data->n_sort_keys = i;
+ out:
 	return ret;
 }

@@ -737,7 +817,26 @@ static int event_hist_trigger_print(struct seq_file *m,
 	}
 
 	seq_puts(m, ":sort=");
-	seq_puts(m, "hitcount");
+
+	for (i = 0; i < hist_data->n_sort_keys; i++) {
+		struct tracing_map_sort_key *sort_key;
+
+		sort_key = &hist_data->sort_keys[i];
+
+		if (i > 0)
+			seq_puts(m, ",");
+
+		if (sort_key->field_idx == HITCOUNT_IDX)
+			seq_puts(m, "hitcount");
+		else {
+			unsigned int idx = sort_key->field_idx;
+
+			hist_field_print(m, hist_data->fields[idx]);
+		}
+
+		if (sort_key->descending)
+			seq_puts(m, ".descending");
+	}
 
 	seq_printf(m, ":size=%u", (1 << hist_data->map->map_bits));
 

Signature · 1 line

1.9.3

Allow users to append 'pause' or 'continue' to an existing trigger in order to have it paused or to have a paused trace continue.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
          [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace.c             |  7 ++++++-
 kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

kernel/trace/trace.c +6 -1

index efa8d05..a223959 100644
@@ -3802,6 +3802,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
+	"\t            [:pause][:continue]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named, and the value of a\n"

@@ -3816,7 +3817,11 @@ static const char readme_msg[] =
 	"\t    used to specify more or fewer than the default 2048 entries\n"
 	"\t    for the hashtable size.\n\n"
 	"\t    Reading the 'hist' file for the event will dump the hash\n"
-	"\t    table in its entirety to stdout."
+	"\t    table in its entirety to stdout.\n\n"
+	"\t    The 'pause' parameter can be used to pause an existing hist\n"
+	"\t    trigger or to start a hist trigger but not log any events\n"
+	"\t    until told to do so.  'continue' can be used to start or\n"
+	"\t    restart a paused hist trigger.\n\n"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +23 -3

index 4fc3136..8753c3b 100644
@@ -78,6 +78,8 @@ struct hist_trigger_attrs {
 	char		*keys_str;
 	char		*vals_str;
 	char		*sort_key_str;
+	bool		pause;
+	bool		cont;
 	unsigned int	map_bits;
 };
 

@@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "sort", strlen("sort")))
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
+		else if (!strncmp(str, "pause", strlen("pause")))
+			attrs->pause = true;
+		else if (!strncmp(str, "continue", strlen("continue")) ||
+			 !strncmp(str, "cont", strlen("cont")))
+			attrs->cont = true;
 		else if (!strncmp(str, "size", strlen("size"))) {
 			int map_bits = parse_map_size(str);
 

@@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
 	if (data->filter_str)
 		seq_printf(m, " if %s", data->filter_str);
 
-	seq_puts(m, " [active]");
+	if (data->paused)
+		seq_puts(m, " [paused]");
+	else
+		seq_puts(m, " [active]");
 
 	seq_putc(m, '\n');
 

@@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)
 {
+	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test;
 	int ret = 0;
 
 	list_for_each_entry_rcu(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			ret = -EEXIST;
+			if (hist_data->attrs->pause)
+				test->paused = true;
+			else if (hist_data->attrs->cont)
+				test->paused = false;
+			else
+				ret = -EEXIST;
 			goto out;
 		}
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)

@@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */
 	if (!ret) {
-		ret = -ENOENT;
+		if (!(attrs->pause || attrs->cont))
+			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)
 		goto out_free;

Signature · 1 line

1.9.3

Quoted 7 lines · Tom Zanussi

Allow users to append 'pause' or 'continue' to an existing trigger in order to have it paused or to have a paused trace continue.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

Quoted 4 lines

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
          [ if filter] > event/trigger

What if 'cont' is used for a non-existing hist? I think it should fail with -ENOENT but it seems that it'd create a new hist trigger..

Thanks,
Namhyung

Quoted 110 lines

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace.c             |  7 ++++++-
 kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

kernel/trace/trace.c +6 -1

index efa8d05..a223959 100644
@@ -3802,6 +3802,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
+	"\t            [:pause][:continue]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named, and the value of a\n"

@@ -3816,7 +3817,11 @@ static const char readme_msg[] =
 	"\t    used to specify more or fewer than the default 2048 entries\n"
 	"\t    for the hashtable size.\n\n"
 	"\t    Reading the 'hist' file for the event will dump the hash\n"
-	"\t    table in its entirety to stdout."
+	"\t    table in its entirety to stdout.\n\n"
+	"\t    The 'pause' parameter can be used to pause an existing hist\n"
+	"\t    trigger or to start a hist trigger but not log any events\n"
+	"\t    until told to do so.  'continue' can be used to start or\n"
+	"\t    restart a paused hist trigger.\n\n"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +23 -3

index 4fc3136..8753c3b 100644
@@ -78,6 +78,8 @@ struct hist_trigger_attrs {
 	char		*keys_str;
 	char		*vals_str;
 	char		*sort_key_str;
+	bool		pause;
+	bool		cont;
 	unsigned int	map_bits;
 };
 

@@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "sort", strlen("sort")))
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
+		else if (!strncmp(str, "pause", strlen("pause")))
+			attrs->pause = true;
+		else if (!strncmp(str, "continue", strlen("continue")) ||
+			 !strncmp(str, "cont", strlen("cont")))
+			attrs->cont = true;
 		else if (!strncmp(str, "size", strlen("size"))) {
 			int map_bits = parse_map_size(str);
 

@@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
 	if (data->filter_str)
 		seq_printf(m, " if %s", data->filter_str);
 
-	seq_puts(m, " [active]");
+	if (data->paused)
+		seq_puts(m, " [paused]");
+	else
+		seq_puts(m, " [active]");
 
 	seq_putc(m, '\n');
 

@@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)
 {
+	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test;
 	int ret = 0;
 
 	list_for_each_entry_rcu(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			ret = -EEXIST;
+			if (hist_data->attrs->pause)
+				test->paused = true;
+			else if (hist_data->attrs->cont)
+				test->paused = false;
+			else
+				ret = -EEXIST;
 			goto out;
 		}
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)

@@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */
 	if (!ret) {
-		ret = -ENOENT;
+		if (!(attrs->pause || attrs->cont))
+			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)
 		goto out_free;

Signature · 1 line

1.9.3

Allow users to append 'clear' to an existing trigger in order to have the hash table cleared.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause/cont \
           [ if filter] > event/trigger

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause/cont/clear \
          [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

Allow users to have pid fields displayed as program names in the output by appending '.execname' to field names:

   # echo hist:keys=aaa.execname ... \
              [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace.c             |  3 +-
 kernel/trace/trace_events_hist.c | 86 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 2 deletions(-)

kernel/trace/trace.c +2 -1

index 4f0968e..a143be0 100644
@@ -3822,7 +3822,8 @@ static const char readme_msg[] =
 	"\t    following modifiers to the field name, as applicable:\n\n"
 	"\t            .hex        display a number as a hex value\n"
 	"\t            .sym        display an address as a symbol\n"
-	"\t            .sym-offset display an address as a symbol and offset\n\n"
+	"\t            .sym-offset display an address as a symbol and offset\n"
+	"\t            .execname   display a common_pid as a program name\n\n"
 	"\t    The 'pause' parameter can be used to pause an existing hist\n"
 	"\t    trigger or to start a hist trigger but not log any events\n"
 	"\t    until told to do so.  'continue' can be used to start or\n"

kernel/trace/trace_events_hist.c +7 -0

index e6e5c6e..656973a 100644
@@ -75,6 +75,7 @@ enum hist_field_flags {
 	HIST_FIELD_HEX		= 8,
 	HIST_FIELD_SYM		= 16,
 	HIST_FIELD_SYM_OFFSET	= 32,
+	HIST_FIELD_EXECNAME	= 64,
 };
 
 struct hist_trigger_attrs {

@@ -218,6 +219,78 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 	return ERR_PTR(ret);
 }
 
+static inline void save_comm(char *comm, struct task_struct *task)
+{
+	if (!task->pid) {
+		strcpy(comm, "<idle>");
+		return;
+	}

kernel/trace/trace_events_hist.c +6 -0

+
+	memcpy(comm, task->comm, TASK_COMM_LEN);
+}
+
+static void hist_trigger_elt_free(struct tracing_map_elt *elt)
+{

kernel/trace/trace_events_hist.c +29 -0

+		key_field = hist_data->fields[i];
+
+		if (key_field->flags & HIST_FIELD_EXECNAME) {
+			unsigned int size = TASK_COMM_LEN + 1;
+
+			elt->private_data = kzalloc(size, GFP_KERNEL);
+			if (!elt->private_data)
+				return -ENOMEM;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static void hist_trigger_elt_copy(struct tracing_map_elt *to,
+				  struct tracing_map_elt *from)
+{
+	char *comm_from = from->private_data;
+	char *comm_to = to->private_data;
+
+	if (comm_from)
+		memcpy(comm_to, comm_from, TASK_COMM_LEN + 1);
+}
+
+static void hist_trigger_elt_init(struct tracing_map_elt *elt)
+{
+	char *comm = elt->private_data;
+

kernel/trace/trace_events_hist.c +4 -0

+
 static void destroy_hist_field(struct hist_field *hist_field)
 {
 	kfree(hist_field);

@@ -376,6 +449,9 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 			flags |= HIST_FIELD_SYM;
 		else if (!strcmp(field_str, "sym-offset"))
 			flags |= HIST_FIELD_SYM_OFFSET;
+		else if (!strcmp(field_str, "execname") &&
+			 !strcmp(field_name, "common_pid"))
+			flags |= HIST_FIELD_EXECNAME;
 		else {
 			ret = -EINVAL;
 			goto out;

kernel/trace/trace_events_hist.c +8 -0

 	if (IS_ERR(hist_data->map)) {
 		ret = PTR_ERR(hist_data->map);
 		hist_data->map = NULL;

@@ -732,6 +808,12 @@ hist_trigger_entry_print(struct seq_file *m,
 			sprint_symbol(str, uval);
 			seq_printf(m, "%s: [%llx] %-55s",
 				   key_field->field->name, uval, str);
+		} else if (key_field->flags & HIST_FIELD_EXECNAME) {
+			char *comm = elt->private_data;
+
+			uval = *(u64 *)(key + key_field->offset);
+			seq_printf(m, "%s: %-16s[%10llu]",
+				   key_field->field->name, comm, uval);
 		} else if (key_field->flags & HIST_FIELD_STRING) {
 			seq_printf(m, "%s: %-50s", key_field->field->name,
 				   (char *)(key + key_field->offset));

@@ -851,6 +933,8 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
 		flags_str = "sym";
 	else if (hist_field->flags & HIST_FIELD_SYM_OFFSET)
 		flags_str = "sym-offset";
+	else if (hist_field->flags & HIST_FIELD_EXECNAME)
+		flags_str = "execname";
 
 	return flags_str;
 }

Signature · 1 line

1.9.3

Quoted 5 lines · Tom Zanussi

Allow users to have pid fields displayed as program names in the output by appending '.execname' to field names:

   # echo hist:keys=aaa.execname ... \
              [ if filter] > event/trigger

So in this case, 'aaa' should be 'common_pid', right?

Also, it's just for display thus tasks which have different pid but same comm name will be saved in different entries, right? If so, it'd be better to note that somewhere as it might confuse users..

Quoted 45 lines

Signed-off-by Tom Zanussi <[email protected]>

Tested-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace.c             |  3 +-
 kernel/trace/trace_events_hist.c | 86 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 2 deletions(-)

kernel/trace/trace.c +2 -1

index 4f0968e..a143be0 100644
@@ -3822,7 +3822,8 @@ static const char readme_msg[] =
 	"\t    following modifiers to the field name, as applicable:\n\n"
 	"\t            .hex        display a number as a hex value\n"
 	"\t            .sym        display an address as a symbol\n"
-	"\t            .sym-offset display an address as a symbol and offset\n\n"
+	"\t            .sym-offset display an address as a symbol and offset\n"
+	"\t            .execname   display a common_pid as a program name\n\n"
 	"\t    The 'pause' parameter can be used to pause an existing hist\n"
 	"\t    trigger or to start a hist trigger but not log any events\n"
 	"\t    until told to do so.  'continue' can be used to start or\n"

kernel/trace/trace_events_hist.c +7 -0

index e6e5c6e..656973a 100644
@@ -75,6 +75,7 @@ enum hist_field_flags {
 	HIST_FIELD_HEX		= 8,
 	HIST_FIELD_SYM		= 16,
 	HIST_FIELD_SYM_OFFSET	= 32,
+	HIST_FIELD_EXECNAME	= 64,
 };
 
 struct hist_trigger_attrs {

@@ -218,6 +219,78 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 	return ERR_PTR(ret);
 }
 
+static inline void save_comm(char *comm, struct task_struct *task)
+{
+	if (!task->pid) {
+		strcpy(comm, "<idle>");
+		return;
+	}

kernel/trace/trace_events_hist.c static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str) · line 237

Quoted 10 lines

kernel/trace/trace_events_hist.c +10 -0

+
+	if (WARN_ON_ONCE(task->pid < 0)) {
+		strcpy(comm, "<XXX>");
+		return;
+	}
+
+	if (task->pid > PID_MAX_DEFAULT) {
+		strcpy(comm, "<...>");
+		return;
+	}

I guess this comes from the saved_cmdlines code which uses a fixed array of PID_MAX_DEFAULT size. But as it doesn't use the array, we can simply copy the task->comm IMHO.

Quoted 12 lines

kernel/trace/trace_events_hist.c +12 -0

+
+	memcpy(comm, task->comm, TASK_COMM_LEN);
+}
+
+static void hist_trigger_elt_free(struct tracing_map_elt *elt)
+{
+	kfree((char *)elt->private_data);
+}
+
+static int hist_trigger_elt_alloc(struct tracing_map_elt *elt)
+{
+	struct hist_trigger_data *hist_data = elt->map->private_data;

kernel/trace/trace_events_hist.c static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str) · line 253

Quoted 4 lines

kernel/trace/trace_events_hist.c +4 -0

+	struct hist_field *key_field;
+	unsigned int i;
+
+	for (i = hist_data->n_vals; i < hist_data->n_fields; i++) {

I think it'd be better to add a list iterator macro like for_each_key_field() or something since it's easy to miss that the key fields always follows the value fields IMHO.

Quoted 34 lines

kernel/trace/trace_events_hist.c +33 -0

+		key_field = hist_data->fields[i];
+
+		if (key_field->flags & HIST_FIELD_EXECNAME) {
+			unsigned int size = TASK_COMM_LEN + 1;
+
+			elt->private_data = kzalloc(size, GFP_KERNEL);
+			if (!elt->private_data)
+				return -ENOMEM;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static void hist_trigger_elt_copy(struct tracing_map_elt *to,
+				  struct tracing_map_elt *from)
+{
+	char *comm_from = from->private_data;
+	char *comm_to = to->private_data;
+
+	if (comm_from)
+		memcpy(comm_to, comm_from, TASK_COMM_LEN + 1);
+}
+
+static void hist_trigger_elt_init(struct tracing_map_elt *elt)
+{
+	char *comm = elt->private_data;
+
+	if (comm)
+		save_comm(comm, current);
+}
+

kernel/trace/trace_events_hist.c static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str) · line 292

Quoted 6 lines

kernel/trace/trace_events_hist.c +6 -0

+static struct tracing_map_ops hist_trigger_ops = {
+	.elt_alloc	= hist_trigger_elt_alloc,
+	.elt_copy	= hist_trigger_elt_copy,
+	.elt_free	= hist_trigger_elt_free,
+	.elt_init	= hist_trigger_elt_init,
+};

How about making it const?

Quoted 18 lines

kernel/trace/trace_events_hist.c +4 -0

+
 static void destroy_hist_field(struct hist_field *hist_field)
 {
 	kfree(hist_field);

@@ -376,6 +449,9 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 			flags |= HIST_FIELD_SYM;
 		else if (!strcmp(field_str, "sym-offset"))
 			flags |= HIST_FIELD_SYM_OFFSET;
+		else if (!strcmp(field_str, "execname") &&
+			 !strcmp(field_name, "common_pid"))
+			flags |= HIST_FIELD_EXECNAME;
 		else {
 			ret = -EINVAL;
 			goto out;

@@ -612,7 +688,7 @@ create_hist_data(unsigned int map_bits,
 		goto free;
 

kernel/trace/trace_events_hist.c create_hist_data(unsigned int map_bits, · line 691

Quoted 3 lines

kernel/trace/trace_events_hist.c +1 -1

 	hist_data->map = tracing_map_create(map_bits, hist_data->key_size,
-					    NULL, hist_data);
+					    &hist_trigger_ops, hist_data);

Why not using the hist_trigger_ops only if HIST_FIELD_EXECNAME is set to one of the key fields actually?

Thanks,
Namhyung

Quoted 28 lines

kernel/trace/trace_events_hist.c +8 -0

 	if (IS_ERR(hist_data->map)) {
 		ret = PTR_ERR(hist_data->map);
 		hist_data->map = NULL;

@@ -732,6 +808,12 @@ hist_trigger_entry_print(struct seq_file *m,
 			sprint_symbol(str, uval);
 			seq_printf(m, "%s: [%llx] %-55s",
 				   key_field->field->name, uval, str);
+		} else if (key_field->flags & HIST_FIELD_EXECNAME) {
+			char *comm = elt->private_data;
+
+			uval = *(u64 *)(key + key_field->offset);
+			seq_printf(m, "%s: %-16s[%10llu]",
+				   key_field->field->name, comm, uval);
 		} else if (key_field->flags & HIST_FIELD_STRING) {
 			seq_printf(m, "%s: %-50s", key_field->field->name,
 				   (char *)(key + key_field->offset));

@@ -851,6 +933,8 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
 		flags_str = "sym";
 	else if (hist_field->flags & HIST_FIELD_SYM_OFFSET)
 		flags_str = "sym-offset";
+	else if (hist_field->flags & HIST_FIELD_EXECNAME)
+		flags_str = "execname";
 
 	return flags_str;
 }

Signature · 1 line

1.9.3

This is v11 of the 'hist triggers' patchset, which is the same as v10 but adds the 2 new ftrace/hist triggers kselftest patches from Masami.

Changes from v10:

- Rebased to latest trace/for-next.
- Added Masami's ftrace/hist triggers kselftest patches.
- Added Masami's Tested-by: to the series.

Quoted 9 lines · On Thu, 22 Oct 2015 13:14:04 -0500 Tom Zanussi <[email protected]>…

This is v11 of the 'hist triggers' patchset, which is the same as v10 but adds the 2 new ftrace/hist triggers kselftest patches from Masami.

Changes from v10:

- Rebased to latest trace/for-next.
- Added Masami's ftrace/hist triggers kselftest patches.
- Added Masami's Tested-by: to the series.

Every time you send a patch set it's always when I'm about to leave for a conference (last one was for Dubin and I think plumbers was before that). Tomorrow I leave for Kernel Summit (takes me three days to get there :-p).

Maybe, just maybe (I'm not going to hold my breath), I'll get to look at them while there. But between jet lag and concentrating on the topics (there's no keynotes to ignore), I don't think I'll get to it.

Feel free to ping me on Monday Nov 2nd, and hopefully I'll be recovered by then.

-- Steve