From patchwork Thu Apr 27 15:46:41 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ganapatrao Kulkarni X-Patchwork-Id: 9703075 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 17710602CC for ; Thu, 27 Apr 2017 15:47:10 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CFAE422B27 for ; Thu, 27 Apr 2017 15:47:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C441E2862A; Thu, 27 Apr 2017 15:47:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, DKIM_VALID, FREEMAIL_FROM autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 178FC22B27 for ; Thu, 27 Apr 2017 15:47:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: References:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=lh2sRUnO4fLywtTpyCsDnt4VviT76088fwol/9YC5NI=; b=k+H4cMS+IOdhTX uy5I4Uag+5clQN/jC/bL1GMT7QccEPg05Gp7g/wWVDgxMdeToDXPpuweF5gzezl/3Qmnd/qY3LPW2 YMBlZl9WW9Diq8qn0uCHx6Uswwnx3cTWiIfXfbFhDuwY8ryD15ykGb8NzXMo9ZXc/Or41wb6rRtfH 3B8k0E2COHgeBvBI79vOu9bfACoQpMA34ngscIuwj4y7Rku6w15Nu16WeuTVrXoNmEQTfpevroskj gdK4473zKEuFbBDFzVWR11C1GhmAdIE2TkuGFrJo+VQx60fF6HvueaiTgrFXdrXSs/EHvwpDZQN+5 qiLNCbFy/TVYVufaNquQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1d3ldK-0003lP-CU; Thu, 27 Apr 2017 15:47:06 +0000 Received: from mail-io0-x242.google.com ([2607:f8b0:4001:c06::242]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1d3ldG-0003hx-VZ for linux-arm-kernel@lists.infradead.org; Thu, 27 Apr 2017 15:47:04 +0000 Received: by mail-io0-x242.google.com with SMTP id x86so4266980ioe.3 for ; Thu, 27 Apr 2017 08:46:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=tXDQki25yHoKiYMweiTaLiXrA0XfyYLFCjLt9XC6zSw=; b=cmNTfAxi3vuUhuz3zIshIW4Mc5xUKTdQVEaE55BUUsuLgzpD2T9QGOUiFswAwU268T bP3AZhuUmz0HmelDjUYc1zjrHIq2SHcQ80+Ij1Z2kgoMWWL+8AV7hv3M9JLJU307aivT 1+ItAk0xOMASTA+3Uas+F7tbGleL2u2iCPP8vRqq0w9R2gMkzqM3PwijjXnlO2RZSXd7 naQeWOZF9oxOckWpeYMyTHt2gT+6IeS8h4a61FmJ0e8u1+ds1gEgux+HAcse/4PczV39 TFUCPbi1FfC4XPWLQ+9to9wmP68fbyruBNwYGSWFm8Wi76jwtiW2ks1e6Fe5ZT2Dsmor aR3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=tXDQki25yHoKiYMweiTaLiXrA0XfyYLFCjLt9XC6zSw=; b=JekC2Xz6ABMjbcAUX2XyYpJvUxSS6LxJmlmvTkRqkTMc6fqnykDmrj9dkae7ag2Aon B333dUjEOat3D/x+IZRHyZcy+Anb/Tgg/FS2GhdYzTMLIvvAvoftOZfhrY3QuFvKwP2E FEPIRuGje26rnaMCRutQGTbJu2lN5NOthNHAwdc0FpkQWOk1AfndFZaD+A6WhiKMs29C F+871BiqgpVruHlTJd1BYgJlw1O7Yrx/UDYYRHsnu17Ld1hDXVKSKuMdjjFvWC6bkfsZ bxJrXk6ohpKzNxP6OwmdnfhFHGutDhDIATXZ4vLmyhHDp0v+JSYLpD3cRMLPtgTV8A+G ZmAg== X-Gm-Message-State: AN3rC/5zG6IRbowcdeFhYAEuHomwu0LS0NBTUZ5dDrEHA2oXPjTkPi/z DSbxbhzfZu5El0Yv6AFrtVzo9ie4nw== X-Received: by 10.157.31.20 with SMTP id x20mr2973699otd.53.1493308001511; Thu, 27 Apr 2017 08:46:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.236.38 with HTTP; Thu, 27 Apr 2017 08:46:41 -0700 (PDT) In-Reply-To: <20170427143438.GE31337@leverpostej> References: <1493198780-25415-1-git-send-email-ganapatrao.kulkarni@cavium.com> <20170426145057.GK27156@leverpostej> <20170426171208.GC23669@leverpostej> <20170427143438.GE31337@leverpostej> From: Ganapatrao Kulkarni Date: Thu, 27 Apr 2017 21:16:41 +0530 Message-ID: Subject: Re: [PATCH] perf evsel: Fix to perf-stat malloc corruption on arm64 platforms To: Mark Rutland X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170427_084703_123044_4C3DE941 X-CRM114-Status: GOOD ( 26.54 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alexander.shishkin@linux.intel.com, Catalin Marinas , Will Deacon , "linux-kernel@vger.kernel.org" , acme@kernel.org, peterz@infradead.org, Ingo Molnar , jnair@caviumnetworks.com, Ganapatrao Kulkarni , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Apr 27, 2017 at 8:04 PM, Mark Rutland wrote: > On Wed, Apr 26, 2017 at 11:49:46PM +0530, Ganapatrao Kulkarni wrote: >> On Wed, Apr 26, 2017 at 10:42 PM, Mark Rutland wrote: >> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c >> > index 13b5499..638aefa 100644 >> > --- a/tools/perf/builtin-stat.c >> > +++ b/tools/perf/builtin-stat.c >> > @@ -346,6 +346,28 @@ static void read_counters(void) >> > } >> > } >> > >> > +/* >> > + * Close all evnt FDs we open in __run_perf_stat() and >> > + * create_perf_stat_counter(), taking care to match the number of threads and CPUs. >> > + * >> > + * Note that perf_evlist__close(evsel_list) is not equivalent, as it doesn't >> > + * take the target into account. >> > + */ >> > +static void close_counters(void) >> > +{ >> > + bool per_cpu = target__has_cpu(&target); >> > + struct perf_evsel *evsel; >> > + >> > + evlist__for_each_entry(evsel_list, evsel) { >> > + if (per_cpu) >> > + perf_evsel__close_per_cpu(evsel, >> > + perf_evsel__cpus(evsel)); >> > + else >> > + perf_evsel__close_per_thread(evsel, >> > + evsel_list->threads); >> > + } >> > +} >> > + >> > static void process_interval(void) >> > { >> > struct timespec ts, rs; >> > @@ -686,7 +708,7 @@ static int __run_perf_stat(int argc, const char **argv) >> > * group leaders. >> > */ >> > read_counters(); >> > - perf_evlist__close(evsel_list); >> > + close_counters(); >> > >> > return WEXITSTATUS(status); >> > } >> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >> > index ac59710..726ceca 100644 >> > --- a/tools/perf/util/evsel.c >> > +++ b/tools/perf/util/evsel.c >> > @@ -1670,6 +1670,18 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus, >> > return err; >> > } >> > >> > +int perf_evsel__open_per_cpu(struct perf_evsel *evsel, >> > + struct cpu_map *cpus) >> > +{ >> > + return perf_evsel__open(evsel, cpus, NULL); >> > +} >> > + >> > +int perf_evsel__open_per_thread(struct perf_evsel *evsel, >> > + struct thread_map *threads) >> > +{ >> > + return perf_evsel__open(evsel, NULL, threads); >> > +} >> > + >> > void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads) >> > { >> > if (evsel->fd == NULL) >> > @@ -1679,16 +1691,18 @@ void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads) >> > perf_evsel__free_fd(evsel); >> > } >> > >> > -int perf_evsel__open_per_cpu(struct perf_evsel *evsel, >> > - struct cpu_map *cpus) >> > +void perf_evsel__close_per_cpu(struct perf_evsel *evsel, >> > + struct cpu_map *cpus) >> > { >> > - return perf_evsel__open(evsel, cpus, NULL); >> > + int ncpus = cpus ? cpus->nr : 1; >> > + perf_evsel__close(evsel, ncpus, 1); >> > } >> > >> > -int perf_evsel__open_per_thread(struct perf_evsel *evsel, >> > - struct thread_map *threads) >> > +void perf_evsel__close_per_thread(struct perf_evsel *evsel, >> > + struct thread_map *threads) >> > { >> > - return perf_evsel__open(evsel, NULL, threads); >> > + int nthreads = threads ? threads->nr : 1; >> > + perf_evsel__close(evsel, 1, nthreads); >> > } >> > >> > static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel, >> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h >> > index 06ef6f2..02bea43 100644 >> > --- a/tools/perf/util/evsel.h >> > +++ b/tools/perf/util/evsel.h >> > @@ -252,6 +252,10 @@ int perf_evsel__open_per_thread(struct perf_evsel *evsel, >> > struct thread_map *threads); >> > int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus, >> > struct thread_map *threads); >> > +void perf_evsel__close_per_cpu(struct perf_evsel *evsel, >> > + struct cpu_map *cpus); >> > +void perf_evsel__close_per_thread(struct perf_evsel *evsel, >> > + struct thread_map *threads); >> > void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads); >> > >> > struct perf_sample; >> >> this diff looks to me doing same as mine. > > Be careful when reading the diff above; the open functions have been > moved, but have not changed. > > I've only changed the close path, whereas your proposal changed the open > path. Those are not equivalent changes. > >> i think below diff should be more appropriate fix to this issue? >> >> when open allocates and uses dummy cpus, there is no point in holding >> old unused one. instead it should free and link to dummy cpus which >> is created with 1 CPU. same will be used by close. >> >> i did quick testing on both x86 and arm64. testing looks ok, may need >> more testing! >> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >> index ac59710..b1aab0a 100644 >> --- a/tools/perf/util/evsel.c >> +++ b/tools/perf/util/evsel.c >> @@ -1466,9 +1466,13 @@ int perf_evsel__open(struct perf_evsel *evsel, >> struct cpu_map *cpus, >> empty_cpu_map = cpu_map__dummy_new(); >> if (empty_cpu_map == NULL) >> return -ENOMEM; >> + } else { >> + cpu_map__get(empty_cpu_map); >> } >> >> cpus = empty_cpu_map; >> + cpu_map__put(evsel->cpus); >> + evsel->cpus = cpus; >> } >> >> if (threads == NULL) { > > Unfortunately, I believe that might break the logic added in commit: > > 9f21b815be863218 ("perf evlist: Only open events on CPUs an evsel permits") > > ... since the evsel->cpus would now not represent the PMUs CPUs. > > As I'd mentioned in my prior reply, I think in order to use the cpu_maps > consistently we need to do a bigger rework of the way cpu_maps are used, > in order to separate the PMU CPUs from the requested event CPUs, etc. > while taking all of these into account. > > Could you please give my diff a go? i tried your diff, and testing looks ok. below is the cleanly merged diff on top of latest commit f832460 Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc > > Thanks, > Mark. thanks Ganapat diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index 13b5499..4be2980 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -346,6 +346,28 @@ static void read_counters(void) } } +/* + * Close all evnt FDs we open in __run_perf_stat() and + * create_perf_stat_counter(), taking care to match the number of threads and CPUs. + * + * Note that perf_evlist__close(evsel_list) is not equivalent, as it doesn't + * take the target into account. + */ +static void close_counters(void) +{ + bool per_cpu = target__has_cpu(&target); + struct perf_evsel *evsel; + + evlist__for_each_entry(evsel_list, evsel) { + if (per_cpu) + perf_evsel__close_per_cpu(evsel, + perf_evsel__cpus(evsel)); + else + perf_evsel__close_per_thread(evsel, + evsel_list->threads); + } +} + static void process_interval(void) { struct timespec ts, rs; @@ -686,7 +708,7 @@ static int __run_perf_stat(int argc, const char **argv) * group leaders. */ read_counters(); - perf_evlist__close(evsel_list); + close_counters(); return WEXITSTATUS(status); } diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index ac59710..ecd9778 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -1691,6 +1691,20 @@ int perf_evsel__open_per_thread(struct perf_evsel *evsel, return perf_evsel__open(evsel, NULL, threads); } +void perf_evsel__close_per_cpu(struct perf_evsel *evsel, + struct cpu_map *cpus) + { + int ncpus = cpus ? cpus->nr : 1; + perf_evsel__close(evsel, ncpus, 1); + } + +void perf_evsel__close_per_thread(struct perf_evsel *evsel, + struct thread_map *threads) + { + int nthreads = threads ? threads->nr : 1; + perf_evsel__close(evsel, 1, nthreads); + } + static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel, const union perf_event *event, struct perf_sample *sample) diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 06ef6f2..6779bd2 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -250,6 +250,10 @@ int perf_evsel__open_per_cpu(struct perf_evsel *evsel, struct cpu_map *cpus); int perf_evsel__open_per_thread(struct perf_evsel *evsel, struct thread_map *threads); +void perf_evsel__close_per_cpu(struct perf_evsel *evsel, + struct cpu_map *cpus); +void perf_evsel__close_per_thread(struct perf_evsel *evsel, + struct thread_map *threads); int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus, struct thread_map *threads); void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads);