All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tom Zanussi <tom.zanussi@linux.intel.com>
To: rostedt@goodmis.org
Cc: tglx@linutronix.de, mhiramat@kernel.org, namhyung@kernel.org,
	vedang.patel@intel.com, bigeasy@linutronix.de,
	joel.opensrc@gmail.com, joelaf@google.com,
	mathieu.desnoyers@efficios.com, baohong.liu@intel.com,
	linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org
Subject: [PATCH v2 02/40] tracing: Add support to detect and avoid duplicates
Date: Tue,  5 Sep 2017 16:57:14 -0500	[thread overview]
Message-ID: <7f3ca68455bdac47285ed45d608075a4af287362.1504642143.git.tom.zanussi@linux.intel.com> (raw)
In-Reply-To: <cover.1504642143.git.tom.zanussi@linux.intel.com>
In-Reply-To: <cover.1504642143.git.tom.zanussi@linux.intel.com>

From: Vedang Patel <vedang.patel@intel.com>

A duplicate in the tracing_map hash table is when 2 different entries
have the same key and, as a result, the key_hash. This is possible due
to a race condition in the algorithm. This race condition is inherent to
the algorithm and not a bug. This was fine because, until now, we were
only interested in the sum of all the values related to a particular
key (the duplicates are dealt with in tracing_map_sort_entries()). But,
with the inclusion of variables[1], we are interested in individual
values. So, it will not be clear what value to choose when
there are duplicates. So, the duplicates need to be removed.

The duplicates can occur in the code in the following scenarios:

- A thread is in the process of adding a new element. It has
successfully executed cmpxchg() and inserted the key. But, it is still
not done acquiring the trace_map_elt struct, populating it and storing
the pointer to the struct in the value field of tracing_map hash table.
If another thread comes in at this time and wants to add an element with
the same key, it will not see the current element and add a new one.

- There are multiple threads trying to execute cmpxchg at the same time,
one of the threads will succeed and the others will fail. The ones which
fail will go ahead increment 'idx' and add a new element there creating
a duplicate.

This patch detects and avoids the first condition by asking the thread
which detects the duplicate to loop one more time. There is also a
possibility of infinite loop if the thread which is trying to insert
goes to sleep indefinitely and the one which is trying to insert a new
element detects a duplicate. Which is why, the thread loops for
map_size iterations before returning NULL.

The second scenario is avoided by preventing the threads which failed
cmpxchg() from incrementing idx. This way, they will loop
around and check if the thread which succeeded in executing cmpxchg()
had the same key.

[1] - https://lkml.org/lkml/2017/6/26/751

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
---
 kernel/trace/tracing_map.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/tracing_map.c b/kernel/trace/tracing_map.c
index 305039b..437b490 100644
--- a/kernel/trace/tracing_map.c
+++ b/kernel/trace/tracing_map.c
@@ -414,6 +414,7 @@ static inline bool keys_match(void *key, void *test_key, unsigned key_size)
 __tracing_map_insert(struct tracing_map *map, void *key, bool lookup_only)
 {
 	u32 idx, key_hash, test_key;
+	int dup_try = 0;
 	struct tracing_map_entry *entry;
 
 	key_hash = jhash(key, map->key_size, 0);
@@ -426,10 +427,31 @@ static inline bool keys_match(void *key, void *test_key, unsigned key_size)
 		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 (test_key && test_key == key_hash) {
+			if (entry->val &&
+			    keys_match(key, entry->val->key, map->key_size)) {
+				atomic64_inc(&map->hits);
+				return entry->val;
+			} else if (unlikely(!entry->val)) {
+				/*
+				 * The key is present. But, val (pointer to elt
+				 * struct) is still NULL. which means some other
+				 * thread is in the process of inserting an
+				 * element.
+				 *
+				 * On top of that, it's key_hash is same as the
+				 * one being inserted right now. So, it's
+				 * possible that the element has the same
+				 * key as well.
+				 */
+
+				dup_try++;
+				if (dup_try > map->map_size) {
+					atomic64_inc(&map->drops);
+					break;
+				}
+				continue;
+			}
 		}
 
 		if (!test_key) {
@@ -451,6 +473,13 @@ static inline bool keys_match(void *key, void *test_key, unsigned key_size)
 				atomic64_inc(&map->hits);
 
 				return entry->val;
+			} else {
+				/*
+				 * cmpxchg() failed. Loop around once
+				 * more to check what key was inserted.
+				 */
+				dup_try++;
+				continue;
 			}
 		}
 
-- 
1.9.3

  parent reply	other threads:[~2017-09-05 21:58 UTC|newest]

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-05 21:57 [PATCH v2 00/40] tracing: Inter-event (e.g. latency) support Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 01/40] tracing: Exclude 'generic fields' from histograms Tom Zanussi
2017-09-05 21:57 ` Tom Zanussi [this message]
2017-09-06 18:32   ` [PATCH v2 02/40] tracing: Add support to detect and avoid duplicates Steven Rostedt
2017-09-06 18:47   ` Steven Rostedt
2017-09-06 20:58     ` Patel, Vedang
2017-09-05 21:57 ` [PATCH v2 03/40] tracing: Remove code which merges duplicates Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 04/40] tracing: Add hist_field_name() accessor Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 05/40] tracing: Reimplement log2 Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 06/40] ring-buffer: Add interface for setting absolute time stamps Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 07/40] tracing: Apply absolute timestamps to instance max buffer Tom Zanussi
2017-09-06 19:57   ` Steven Rostedt
2017-09-07  0:49   ` Steven Rostedt
2017-09-07  1:15     ` Liu, Baohong
2017-09-05 21:57 ` [PATCH v2 08/40] ring-buffer: Redefine the unimplemented RINGBUF_TIME_TIME_STAMP Tom Zanussi
2017-09-07 14:35   ` Steven Rostedt
2017-09-07 15:05     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 09/40] tracing: Give event triggers access to ring_buffer_event Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 10/40] tracing: Add ring buffer event param to hist field functions Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 11/40] tracing: Increase tracing map KEYS_MAX size Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 12/40] tracing: Break out hist trigger assignment parsing Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 13/40] tracing: Make traceprobe parsing code reusable Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 14/40] tracing: Add hist trigger timestamp support Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 15/40] tracing: Add per-element variable support to tracing_map Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 16/40] tracing: Add hist_data member to hist_field Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 17/40] tracing: Add usecs modifier for hist trigger timestamps Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 18/40] tracing: Add variable support to hist triggers Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 19/40] tracing: Account for variables in named trigger compatibility Tom Zanussi
2017-09-07 16:40   ` Steven Rostedt
2017-09-07 17:00     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 20/40] tracing: Add simple expression support to hist triggers Tom Zanussi
2017-09-07 16:46   ` Steven Rostedt
2017-09-07 17:01     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 21/40] tracing: Generalize per-element hist trigger data Tom Zanussi
2017-09-07 17:56   ` Steven Rostedt
2017-09-07 18:14     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 22/40] tracing: Pass tracing_map_elt to hist_field accessor functions Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 23/40] tracing: Add hist_field 'type' field Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 24/40] tracing: Add variable reference handling to hist triggers Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 25/40] tracing: Add support for dynamic tracepoints Tom Zanussi
2017-09-05 23:29   ` Mathieu Desnoyers
2017-09-06  2:35     ` Tom Zanussi
2017-09-07 22:02   ` Steven Rostedt
2017-09-08 14:18     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 26/40] tracing: Add hist trigger action hook Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 27/40] tracing: Add support for 'synthetic' events Tom Zanussi
2017-09-07 23:40   ` Steven Rostedt
2017-09-08 14:30     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 28/40] tracing: Add support for 'field variables' Tom Zanussi
2017-09-07 23:43   ` Steven Rostedt
2017-09-08 15:37     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 29/40] tracing: Add 'onmatch' hist trigger action support Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 30/40] tracing: Add 'onmax' " Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 31/40] tracing: Allow whitespace to surround hist trigger filter Tom Zanussi
2017-09-08 18:50   ` Steven Rostedt
2017-09-08 19:08     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 32/40] tracing: Add cpu field for hist triggers Tom Zanussi
2017-09-08 19:08   ` Steven Rostedt
2017-09-08 19:35     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 33/40] tracing: Add hist trigger support for variable reference aliases Tom Zanussi
2017-09-08 19:09   ` Steven Rostedt
2017-09-08 19:41     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 34/40] tracing: Add 'last error' error facility for hist triggers Tom Zanussi
2017-09-08 19:25   ` Steven Rostedt
2017-09-08 19:44     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 35/40] tracing: Reverse the order event_mutex/trace_types_lock are taken Tom Zanussi
2017-09-08 19:31   ` Steven Rostedt
2017-09-08 19:41     ` Steven Rostedt
2017-09-08 20:00       ` Steven Rostedt
2017-09-05 21:57 ` [PATCH v2 36/40] tracing: Remove lookups from tracing_map hitcount Tom Zanussi
2017-09-12  2:16   ` Masami Hiramatsu
2017-09-12 14:16     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 37/40] tracing: Add inter-event hist trigger Documentation Tom Zanussi
2017-09-20 14:44   ` Julia Cartwright
2017-09-20 17:15     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 38/40] tracing: Make tracing_set_clock() non-static Tom Zanussi
2017-09-12  2:18   ` Masami Hiramatsu
2017-09-12 14:18     ` Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 39/40] tracing: Add a clock attribute for hist triggers Tom Zanussi
2017-09-05 21:57 ` [PATCH v2 40/40] tracing: Add trace_event_buffer_reserve() variant that allows recursion Tom Zanussi
2017-09-07 22:29   ` kbuild test robot
2017-09-07 22:35   ` kbuild test robot
2017-09-08 20:27   ` Steven Rostedt
2017-09-08 20:41     ` Tom Zanussi
2017-09-12  1:50 ` [PATCH v2 00/40] tracing: Inter-event (e.g. latency) support Masami Hiramatsu
2017-09-12 14:14   ` Tom Zanussi
2017-09-19 16:31 ` Steven Rostedt
2017-09-19 18:44   ` Tom Zanussi
2017-09-21 20:20     ` Steven Rostedt
2017-09-21 21:11       ` Tom Zanussi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7f3ca68455bdac47285ed45d608075a4af287362.1504642143.git.tom.zanussi@linux.intel.com \
    --to=tom.zanussi@linux.intel.com \
    --cc=baohong.liu@intel.com \
    --cc=bigeasy@linutronix.de \
    --cc=joel.opensrc@gmail.com \
    --cc=joelaf@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=namhyung@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=vedang.patel@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.