Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753076AbdIFU6H (ORCPT ); Wed, 6 Sep 2017 16:58:07 -0400 Received: from mga14.intel.com ([192.55.52.115]:2693 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752619AbdIFU6F (ORCPT ); Wed, 6 Sep 2017 16:58:05 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,355,1500966000"; d="scan'208";a="1169764845" From: "Patel, Vedang" To: "rostedt@goodmis.org" , "tom.zanussi@linux.intel.com" CC: "bigeasy@linutronix.de" , "linux-kernel@vger.kernel.org" , "tglx@linutronix.de" , "namhyung@kernel.org" , "joelaf@google.com" , "mhiramat@kernel.org" , "linux-rt-users@vger.kernel.org" , "mathieu.desnoyers@efficios.com" , "joel.opensrc@gmail.com" , "Liu, Baohong" Subject: Re: [PATCH v2 02/40] tracing: Add support to detect and avoid duplicates Thread-Topic: [PATCH v2 02/40] tracing: Add support to detect and avoid duplicates Thread-Index: AQHTJpDqeiPXntLUXUKw7dbH39IWEKKoqVwAgAAkggA= Date: Wed, 6 Sep 2017 20:58:03 +0000 Message-ID: <1504731482.21216.8.camel@intel.com> References: <7f3ca68455bdac47285ed45d608075a4af287362.1504642143.git.tom.zanussi@linux.intel.com> <20170906144722.6eb71312@gandalf.local.home> In-Reply-To: <20170906144722.6eb71312@gandalf.local.home> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.7.159.64] Content-Type: text/plain; charset="utf-8" Content-ID: <3C3ED21A51293349B8B15E52EDD5DF78@intel.com> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v86KwD36031323 Content-Length: 3258 Lines: 128 On Wed, 2017-09-06 at 14:47 -0400, Steven Rostedt wrote: > On Tue,  5 Sep 2017 16:57:14 -0500 > Tom Zanussi 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; > >   } > >   } > >