Re: [PATCH v2 02/40] tracing: Add support to detect and avoid duplicates

["Patel, Vedang" <vedang.patel@intel.com>]

On Wed, 2017-09-06 at 14:47 -0400, Steven Rostedt wrote:
> On Tue,  5 Sep 2017 16:57:14 -0500
> Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
> 
> 
> > 
> > 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)) {
> 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
> 
> 
> 
> > 
> > +				/*
> > +				 * 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;
> >  			}
> >  		}
> >