On Wed, 2017-09-06 at 14:47 -0400, Steven Rostedt wrote:

On Tue,  5 Sep 2017 16:57:14 -0500
Tom Zanussi [off-list ref] wrote:

quoted

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-

quoted

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-

quoted

key_size)) {

+				atomic64_inc(&map->hits);
+				return entry->val;
+			} else if (unlikely(!entry->val)) {

I'm thinking we need a READ_ONCE() here.

		val = READ_ONCE(entry->val);

then use "val" instead of entry->val. Otherwise, wont it be possible
if two tasks are inserting at the same time, to have this:

(Using reg as when the value is read into a register from memory)

	CPU0			CPU1
	----			----
 reg = entry->val
 (reg == zero)

			   entry->val = elt;

 keys_match(key, reg)
 (false)

 reg = entry->val
 (reg = elt)

 if (unlikely(!reg))

Causes the if to fail.

A READ_ONCE(), would make sure the entry->val used to test against
key
would also be the same value used to test if it is zero.

Hi Steve, 

Thanks for the input. 

I agree with your change. Adding READ_ONCE will avoid a race condition
which might result in adding duplicates. Will add it in the next
version.

-Vedang

-- Steve

quoted

+				/*
+				 * 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;
 			}
 		}