From patchwork Wed Apr 26 18:19:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ganapatrao Kulkarni X-Patchwork-Id: 9701791 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 DFF0F60245 for ; Wed, 26 Apr 2017 18:20:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D3844268AE for ; Wed, 26 Apr 2017 18:20:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C6ECA28418; Wed, 26 Apr 2017 18:20:28 +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 C222F268AE for ; Wed, 26 Apr 2017 18:20:27 +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=MDvmcaplj075Z8pqasnfkoJX0seX6JXNwWeG0gA8KaU=; b=LD3ozDzNGBHJmC lXjG7QFRKMrAV3IIM6/gMfrQkI665xvFy2nnZ0R1lAPCNutEBe1b+AeuexVvbZsT51NIcuIRH6OQq 3XCQB1KYrEq52ncZAL0PgC4rDVEUd97L1KLYVaMV6gVU00VGOQYRWxPmFKe257pzLSvtUWv71VAUo sgKoQjB2rwkLTrnEtbn3wfgXTI6kBiJHAbKcn5EFfMxWhZESMTq3OdqYR3iecyC7fvQ6phGzDYJNN HbB4SUN5JE9XK+e3xCJrU4OhQIUOMqxwcaKqldupSzpv4i8r+ExdLQSejbHsBe+aqhOLp9nQnBr1/ Vf45tfjBKElUf2U6oqGw==; 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 1d3RY7-0007bv-PH; Wed, 26 Apr 2017 18:20:23 +0000 Received: from mail-oi0-x242.google.com ([2607:f8b0:4003:c06::242]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1d3RXt-0006CM-0z for linux-arm-kernel@lists.infradead.org; Wed, 26 Apr 2017 18:20:11 +0000 Received: by mail-oi0-x242.google.com with SMTP id w12so2242965oiw.0 for ; Wed, 26 Apr 2017 11:19:48 -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=CxYNjyXHQ1IvC7Y8m5KvE59SgMsPF5IlS+zGDqK6rcA=; b=jlvH2r5kZvlxpdBtHu1Rg/9ShZ7V0vWKJ+Ra3V3lkbhPoQq+232Se4G06md/GHdRxl 0bFgPUpqqewebzAjSL3MnhNMPN82dAGKx5naNtbBgbD9FD32ho/A5ZK98X6S5e0NPiM+ i5mdbJCchjrAgCC6/kdU6tMOz56wWqPtNt6rYRwvpPoP/s/ErbSG6XngmqYBFuH1QF9j PH3i/uyT+rR6XoaaETaP6kjRnT9nSTk33j0eyjKz2Kmjz6eHELTNCv0rmOpfyzVMcr4A KjvBGZEfdO07ltS9K0A2C8umQRgFsI1JwEkOayeRQKfajPxP55o2svMMmY1RkFNi3VBk Xpiw== 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=CxYNjyXHQ1IvC7Y8m5KvE59SgMsPF5IlS+zGDqK6rcA=; b=kQX1ioP+RJQ5CaKC/S2FzvLhVxv2uRjT44B6CPsf01RcUXk+ysZo+nGitHeBNmIP0h InrbtX3/wuTonqgkn3DdOKwxwBT4kDWLGHvv8O7vE2RYsorVy2R+pDEx9nUBJYdIURlL DKoMJOAzdSLh1Ur52G+U41O950X1a4wtIFHM7mUqHVWDr8N8IRDKvfF+/aM6OA0Y6/c2 Z8/NR1T16ByNmBHGnIBXIAoYLkEt9gyFRfGS/1MuD3cT2Rql78js0Ke+IJEQLkaIkror 3zGKnTS7j42gYSJe8M3s0XlDY1M/H/9d1zj0th1KlwBoNs2KSrtjfgupUsCISOsPLUmD c6gQ== X-Gm-Message-State: AN3rC/4m+QJeii7GpHDkJck0MuFydjUnbTIgIfVJvyiuCszaTwZU/Ec+ DVkvWAuUA3vEFjZlEtqZLoJLgGlgVA== X-Received: by 10.157.43.116 with SMTP id f49mr591420otd.216.1493230787272; Wed, 26 Apr 2017 11:19:47 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.236.38 with HTTP; Wed, 26 Apr 2017 11:19:46 -0700 (PDT) In-Reply-To: <20170426171208.GC23669@leverpostej> References: <1493198780-25415-1-git-send-email-ganapatrao.kulkarni@cavium.com> <20170426145057.GK27156@leverpostej> <20170426171208.GC23669@leverpostej> From: Ganapatrao Kulkarni Date: Wed, 26 Apr 2017 23:49:46 +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-20170426_112009_357603_48436EB9 X-CRM114-Status: GOOD ( 27.65 ) 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 Hi Mark, On Wed, Apr 26, 2017 at 10:42 PM, Mark Rutland wrote: > On Wed, Apr 26, 2017 at 03:50:57PM +0100, Mark Rutland wrote: >> Hi Ganapatrao, >> >> Thanks for tracking this down. >> >> On Wed, Apr 26, 2017 at 02:56:20PM +0530, Ganapatrao Kulkarni wrote: >> > In some cases, ncpus used for perf_evsel__alloc_fd and for >> > perf_evsel__close are not the same, this is causing memory >> > overwrite/corruption. >> >> It would be good if we could enumerate when this occurs. >> >> From what I can tell, the problem occurs when opening a thread-bound >> event on PMU with a cpus/cpumask in sysfs. >> >> For perf-stat we create events using create_perf_stat_counter(). There >> we see !target_has_cpu(), so we call perf_evsel__open_per_thread(). Thus >> perf_evsel__open() is passed NULL cpus, and creates an empty cpu_map. As >> cpus->nr = 1, we get 1 * nthreads fds allocated, and open events for >> each of these. >> >> Later, we try to close events using perf_evlist__close(). This doesn't >> take target_has_cpu() into account, but sees evsel->cpus is non-NULL >> (since the PMU had a cpus/cpumask file), and tries to close events for >> cpus->nr * nthreads, and goes out-of-bounds of the fd array. > > Looking further, I introduced this inconsistency in commit: > > 3df33eff2ba96be4 ("perf stat: Avoid skew when reading events ") > > To fix that, perf-stat needs to control the closing of its events, to > match what it does when opening them. I've spun the below to do that, > introducing new helpers to make it clear. > > Does that work for you? > > Thanks, > Mark. > > ---->8---- > 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. 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! if (threads == NULL) { gkulkarni@localhost>perf>> thanks Ganapat 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; }