Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756051AbdIHO1u (ORCPT ); Fri, 8 Sep 2017 10:27:50 -0400 Received: from mail.kernel.org ([198.145.29.99]:40542 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752679AbdIHO1s (ORCPT ); Fri, 8 Sep 2017 10:27:48 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 00B7822B24 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org Date: Fri, 8 Sep 2017 11:27:45 -0300 From: Arnaldo Carvalho de Melo To: kan.liang@intel.com Cc: peterz@infradead.org, mingo@redhat.com, linux-kernel@vger.kernel.org, jolsa@kernel.org, namhyung@kernel.org, adrian.hunter@intel.com, lukasz.odzioba@intel.com, ak@linux.intel.com Subject: Re: [PATCH RFC 06/10] perf tools: lock to protect comm_str rb tree Message-ID: <20170908142745.GG11725@kernel.org> References: <1504806954-150842-1-git-send-email-kan.liang@intel.com> <1504806954-150842-7-git-send-email-kan.liang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1504806954-150842-7-git-send-email-kan.liang@intel.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3102 Lines: 113 Em Thu, Sep 07, 2017 at 10:55:50AM -0700, kan.liang@intel.com escreveu: > From: Kan Liang > > Add comm_str_lock to protect comm_str rb tree. > > Signed-off-by: Kan Liang > --- > tools/perf/util/comm.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c > index 7bc981b..1bdfef1 100644 > --- a/tools/perf/util/comm.c > +++ b/tools/perf/util/comm.c > @@ -5,6 +5,7 @@ > #include > #include > #include > +#include > > struct comm_str { > char *str; > @@ -14,6 +15,7 @@ struct comm_str { > > /* Should perhaps be moved to struct machine */ > static struct rb_root comm_str_root; > +static pthread_mutex_t comm_str_lock = PTHREAD_MUTEX_INITIALIZER; > > static struct comm_str *comm_str__get(struct comm_str *cs) > { > @@ -24,11 +26,13 @@ static struct comm_str *comm_str__get(struct comm_str *cs) > > static void comm_str__put(struct comm_str *cs) > { > + pthread_mutex_lock(&comm_str_lock); > if (cs && refcount_dec_and_test(&cs->refcnt)) { > rb_erase(&cs->rb_node, &comm_str_root); > zfree(&cs->str); > free(cs); > } > + pthread_mutex_unlock(&comm_str_lock); > } The above should use a smaller locked section, i.e.: static void comm_str__put(struct comm_str *cs) { if (cs && refcount_dec_and_test(&cs->refcnt)) { + pthread_mutex_lock(&comm_str_lock); rb_erase(&cs->rb_node, &comm_str_root); + pthread_mutex_unlock(&comm_str_lock); zfree(&cs->str); free(cs); } } > static struct comm_str *comm_str__alloc(const char *str) > @@ -52,18 +56,22 @@ static struct comm_str *comm_str__alloc(const char *str) > > static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root) The usual way is to just rename the above to __comm_str__findnew(), leaving it unlocked, and then add a locked wrapper: static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root) { struct comm_str *cs; pthread_mutex_lock(&comm_str_lock); cs = __comm_str__findnew(str, root); pthread_mutex_unlock(&comm_str_lock); return cs; } > { > - struct rb_node **p = &root->rb_node; > struct rb_node *parent = NULL; > struct comm_str *iter, *new; > + struct rb_node **p; > int cmp; > > + pthread_mutex_lock(&comm_str_lock); > + p = &root->rb_node; > while (*p != NULL) { > parent = *p; > iter = rb_entry(parent, struct comm_str, rb_node); > > cmp = strcmp(str, iter->str); > - if (!cmp) > - return comm_str__get(iter); > + if (!cmp) { > + new = comm_str__get(iter); > + goto unlock; > + } > > if (cmp < 0) > p = &(*p)->rb_left; > @@ -73,11 +81,13 @@ static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root) > > new = comm_str__alloc(str); > if (!new) > - return NULL; > + goto unlock; > > rb_link_node(&new->rb_node, parent, p); > rb_insert_color(&new->rb_node, root); > > +unlock: > + pthread_mutex_unlock(&comm_str_lock); > return new; > } > > -- > 2.5.5