From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 60BE5C433F5 for ; Mon, 13 Dec 2021 16:17:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240596AbhLMQRV (ORCPT ); Mon, 13 Dec 2021 11:17:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37980 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235246AbhLMQRT (ORCPT ); Mon, 13 Dec 2021 11:17:19 -0500 Received: from mail-il1-x12b.google.com (mail-il1-x12b.google.com [IPv6:2607:f8b0:4864:20::12b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B868EC06173F for ; Mon, 13 Dec 2021 08:17:19 -0800 (PST) Received: by mail-il1-x12b.google.com with SMTP id 15so15493689ilq.2 for ; Mon, 13 Dec 2021 08:17:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qPM6FbxijuW1dfYjxg6dbH9bQm3NIxcRZrN/l42tCuQ=; b=ZEl1/ovprzGp/GYpdAAmLu+DWUyD7pS05OFjLKDA7RuLxn4AXG9i0PNZwE5aZkKNAB vCuHmSR0dlxNer4Om3YrecdvFioWvF/sHX+yJx/GmXUxv+tTA7xrvgXJYSFZvoqBuSRz fcJ3HgayoVGqLnGyhrcDLNpy5JLE1fYz0xS3FTxFD1VEKWzOzCZ1R2pF6qZxFiLnxiMT PD7FKPle1d4Z+0kx8hDAQxAKZnC+Y9RzHPSODeCmPcNqNSDd2fF6edmGUL28f6SrXgSD 697LmZFwKtm4rbQ2GjaLWF+AZ2n7XwgQMD3MNv82jRNq/MK6Plq2mo8Ruqn3veCTDWoH 1SDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qPM6FbxijuW1dfYjxg6dbH9bQm3NIxcRZrN/l42tCuQ=; b=t66yy1+cGlVMDxOqHJSNZIYLi0Y+U2X17ppyxNEUV2wXn+Ozits0+vBu+O/dN6LKMM yiLiVeNzxNhFDhPUsYaeu4xUyFTO9k58Fww0DZoy7PLEHM5+3bKkYD2YRVQN5yWUdNmF HMPwmhVnE6D7yrciBhpiKEx9fPiwtWzz2agASfq9NU4rkWxBiDreeC9KgEi4Ih2N2wvB JinlL+LDdruKK11Bm8t3b64nWWK1GBDLxFRigpIgFHGLk8h9Mtd+MtWpafT/23gu5AKA VXEwA0rQ8UZZOk1ymbtElKgBlQCuTyIXDUejFqndNcpzcSOVxmBY6kVwH6sJQeM+8Qhf QMxQ== X-Gm-Message-State: AOAM532ZvOGR+G3ERtxXBzwSpBxanG4djkZs+aki/ovXXFDiSxRXWpu2 jfkGHLx+WDJthu/OOHJX61ISABB1LfoVeXILcZCoTFBt7R0aWA== X-Google-Smtp-Source: ABdhPJyKsZDVqqIu1ZdJcA0ZUVjlcTaQL/Rz4Xw8Qf3rUVwazwtxJc/6cQaxlh9cs3Hpac7Wd/NJ0hMuHcmiQOlBqWw= X-Received: by 2002:a05:6e02:148c:: with SMTP id n12mr33731744ilk.89.1639412238938; Mon, 13 Dec 2021 08:17:18 -0800 (PST) MIME-Version: 1.0 References: <20211208024607.1784932-1-irogers@google.com> <20211208024607.1784932-4-irogers@google.com> In-Reply-To: From: Ian Rogers Date: Mon, 13 Dec 2021 08:17:07 -0800 Message-ID: Subject: Re: [PATCH 03/22] perf stat: Switch aggregation to use for_each loop To: Jiri Olsa Cc: Andi Kleen , Namhyung Kim , John Garry , Kajol Jain , "Paul A . Clarke" , Arnaldo Carvalho de Melo , Riccardo Mancini , Kan Liang , Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, Vineet Singh , James Clark , Mathieu Poirier , Suzuki K Poulose , Mike Leach , Leo Yan , coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, eranian@google.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Dec 11, 2021 at 11:25 AM Jiri Olsa wrote: > > On Tue, Dec 07, 2021 at 06:45:48PM -0800, Ian Rogers wrote: > > Tidy up the use of cpu and index to hopefully make the code less error > > prone. Avoid unused warnings with (void) which will be removed in a > > later patch. > > > > In aggr_update_shadow, the perf_cpu_map is switched from > > the evlist to the counter's cpu map, so the index is appropriate. This > > addresses a problem where uncore counts, with a cpumap like: > > $ cat /sys/devices/uncore_imc_0/cpumask > > 0,18 > > Don't aggregate counts in CPUs based on the index of those values in the > > cpumap (0 and 1) but on the actual CPU (0 and 18). Thereby correcting > > metric calculations in per-socket mode for counters with without a full > > cpumask. > > > > Signed-off-by: Ian Rogers > > --- > > tools/perf/util/stat-display.c | 48 +++++++++++++++++++--------------- > > 1 file changed, 27 insertions(+), 21 deletions(-) > > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > > index 588601000f3f..efab39a759ff 100644 > > --- a/tools/perf/util/stat-display.c > > +++ b/tools/perf/util/stat-display.c > > @@ -330,8 +330,8 @@ static void print_metric_header(struct perf_stat_config *config, > > static int first_shadow_cpu(struct perf_stat_config *config, > > struct evsel *evsel, struct aggr_cpu_id id) > > { > > - struct evlist *evlist = evsel->evlist; > > - int i; > > + struct perf_cpu_map *cpus; > > + int cpu, idx; > > > > if (config->aggr_mode == AGGR_NONE) > > return id.core; > > @@ -339,14 +339,11 @@ static int first_shadow_cpu(struct perf_stat_config *config, > > if (!config->aggr_get_id) > > return 0; > > > > - for (i = 0; i < evsel__nr_cpus(evsel); i++) { > > - int cpu2 = evsel__cpus(evsel)->map[i]; > > - > > - if (cpu_map__compare_aggr_cpu_id( > > - config->aggr_get_id(config, evlist->core.cpus, cpu2), > > - id)) { > > - return cpu2; > > - } > > + cpus = evsel__cpus(evsel); > > + perf_cpu_map__for_each_cpu(cpu, idx, cpus) { > > + if (cpu_map__compare_aggr_cpu_id(config->aggr_get_id(config, cpus, idx), > > + id)) > > + return cpu; > > so this looks strange, you pass idx instead of cpu2 to aggr_get_id, > which takes idx as 3rd argument, so it looks like it was broken now, > should this be a separate fix? Yep, I tried to cover this in the commit message, but agree a separate patch would be clearer. The aggregation is currently broken on anything other than CPU 0 or when the CPU mask covers every CPU - the case for something like topdown, hence this not being spotted. > also the original code for some reason passed evlist->core.cpus > to aggr_get_id, which might differ rom evsel's cpus Part of the same fix. > same for aggr_update_shadow change In this case the cpu is really an index and so the change is just renaming one to the other for the sake of clarity. Thanks, Ian > jirka > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2569FC433F5 for ; Mon, 13 Dec 2021 16:18:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=R+y9ry4L1IRhHhYh16twetcvnXjYAaVe3MhOn3QCFwg=; b=jtJ8byM5P+MS3V +28/qah36je0CemphoXI8RwM1VsPQEmAJrf3lqDNvtU32xpQGRptO5UTWb0iXFWymaE3oxSUO7W67 nHpZe5rb4okjPEdpQKnGlCPPs2a5aejBiPJMoT/Gzr42+AjDSz91LVrl0+XxToR4pboLhcpho4lJ+ Izlg/ZQ02OKWczLgkTG4Cm+ZHj8BgNOmI9s6oVDbH8sf4Z0KIrrFnKwl5W5ZoBz1DQ7VOiRtC0BGh pzfcAL1Pncf3Ko8w3dyq5/fnrwkdNllk/6w5IZklm6Yy7R9X6iuhyFC/EQmWRttYYGJW8F3EOmmLb zWtf7oYdwe/68BE9K94w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mwo0y-00AXfs-B6; Mon, 13 Dec 2021 16:17:24 +0000 Received: from mail-il1-x136.google.com ([2607:f8b0:4864:20::136]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mwo0u-00AXea-Ck for linux-arm-kernel@lists.infradead.org; Mon, 13 Dec 2021 16:17:21 +0000 Received: by mail-il1-x136.google.com with SMTP id o13so155399ilt.12 for ; Mon, 13 Dec 2021 08:17:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qPM6FbxijuW1dfYjxg6dbH9bQm3NIxcRZrN/l42tCuQ=; b=ZEl1/ovprzGp/GYpdAAmLu+DWUyD7pS05OFjLKDA7RuLxn4AXG9i0PNZwE5aZkKNAB vCuHmSR0dlxNer4Om3YrecdvFioWvF/sHX+yJx/GmXUxv+tTA7xrvgXJYSFZvoqBuSRz fcJ3HgayoVGqLnGyhrcDLNpy5JLE1fYz0xS3FTxFD1VEKWzOzCZ1R2pF6qZxFiLnxiMT PD7FKPle1d4Z+0kx8hDAQxAKZnC+Y9RzHPSODeCmPcNqNSDd2fF6edmGUL28f6SrXgSD 697LmZFwKtm4rbQ2GjaLWF+AZ2n7XwgQMD3MNv82jRNq/MK6Plq2mo8Ruqn3veCTDWoH 1SDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qPM6FbxijuW1dfYjxg6dbH9bQm3NIxcRZrN/l42tCuQ=; b=H25cmLti4nb+9kQJ9ldKRPi6ylVuWNK7fRQ21t7FkL2U96q5o9Xe7vaGYGJQVDC2Zj +xMQviMWinATAbi1b6U9Ce0Rti7EPbcrel5USQLVgibFH/Wae+diUfltb9ZsYpiwNoWS s0zaICPbxv1hImTNoEEs/5s+/dytXVujEBCQ9C3RwSGLEPwKD3TlWlET9kw5NPpARdYX /ljdBvtSpXeEwis0pcF5qca4LXXN1V0ZWqnWAfF6YslfXpj5ivhdhaQomXoevmp1LKAu 8k89JzMnOzIn4ENMKwSGpyqlNTPLXjMrij22J1EbSn4GwyiNDozgrFT0jjV4bFz+7s2W 5jFQ== X-Gm-Message-State: AOAM533uIC/dXfoecVeSSzKZuXlMityPmP4zV3fRtLByMup1RDXXQjjQ yJ1NPF0MP98XbQ8o41c4586fKqGx7AqNNaRfXkkzmg== X-Google-Smtp-Source: ABdhPJyKsZDVqqIu1ZdJcA0ZUVjlcTaQL/Rz4Xw8Qf3rUVwazwtxJc/6cQaxlh9cs3Hpac7Wd/NJ0hMuHcmiQOlBqWw= X-Received: by 2002:a05:6e02:148c:: with SMTP id n12mr33731744ilk.89.1639412238938; Mon, 13 Dec 2021 08:17:18 -0800 (PST) MIME-Version: 1.0 References: <20211208024607.1784932-1-irogers@google.com> <20211208024607.1784932-4-irogers@google.com> In-Reply-To: From: Ian Rogers Date: Mon, 13 Dec 2021 08:17:07 -0800 Message-ID: Subject: Re: [PATCH 03/22] perf stat: Switch aggregation to use for_each loop To: Jiri Olsa Cc: Andi Kleen , Namhyung Kim , John Garry , Kajol Jain , "Paul A . Clarke" , Arnaldo Carvalho de Melo , Riccardo Mancini , Kan Liang , Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, Vineet Singh , James Clark , Mathieu Poirier , Suzuki K Poulose , Mike Leach , Leo Yan , coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, eranian@google.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211213_081720_457633_B6385071 X-CRM114-Status: GOOD ( 32.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sat, Dec 11, 2021 at 11:25 AM Jiri Olsa wrote: > > On Tue, Dec 07, 2021 at 06:45:48PM -0800, Ian Rogers wrote: > > Tidy up the use of cpu and index to hopefully make the code less error > > prone. Avoid unused warnings with (void) which will be removed in a > > later patch. > > > > In aggr_update_shadow, the perf_cpu_map is switched from > > the evlist to the counter's cpu map, so the index is appropriate. This > > addresses a problem where uncore counts, with a cpumap like: > > $ cat /sys/devices/uncore_imc_0/cpumask > > 0,18 > > Don't aggregate counts in CPUs based on the index of those values in the > > cpumap (0 and 1) but on the actual CPU (0 and 18). Thereby correcting > > metric calculations in per-socket mode for counters with without a full > > cpumask. > > > > Signed-off-by: Ian Rogers > > --- > > tools/perf/util/stat-display.c | 48 +++++++++++++++++++--------------- > > 1 file changed, 27 insertions(+), 21 deletions(-) > > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > > index 588601000f3f..efab39a759ff 100644 > > --- a/tools/perf/util/stat-display.c > > +++ b/tools/perf/util/stat-display.c > > @@ -330,8 +330,8 @@ static void print_metric_header(struct perf_stat_config *config, > > static int first_shadow_cpu(struct perf_stat_config *config, > > struct evsel *evsel, struct aggr_cpu_id id) > > { > > - struct evlist *evlist = evsel->evlist; > > - int i; > > + struct perf_cpu_map *cpus; > > + int cpu, idx; > > > > if (config->aggr_mode == AGGR_NONE) > > return id.core; > > @@ -339,14 +339,11 @@ static int first_shadow_cpu(struct perf_stat_config *config, > > if (!config->aggr_get_id) > > return 0; > > > > - for (i = 0; i < evsel__nr_cpus(evsel); i++) { > > - int cpu2 = evsel__cpus(evsel)->map[i]; > > - > > - if (cpu_map__compare_aggr_cpu_id( > > - config->aggr_get_id(config, evlist->core.cpus, cpu2), > > - id)) { > > - return cpu2; > > - } > > + cpus = evsel__cpus(evsel); > > + perf_cpu_map__for_each_cpu(cpu, idx, cpus) { > > + if (cpu_map__compare_aggr_cpu_id(config->aggr_get_id(config, cpus, idx), > > + id)) > > + return cpu; > > so this looks strange, you pass idx instead of cpu2 to aggr_get_id, > which takes idx as 3rd argument, so it looks like it was broken now, > should this be a separate fix? Yep, I tried to cover this in the commit message, but agree a separate patch would be clearer. The aggregation is currently broken on anything other than CPU 0 or when the CPU mask covers every CPU - the case for something like topdown, hence this not being spotted. > also the original code for some reason passed evlist->core.cpus > to aggr_get_id, which might differ rom evsel's cpus Part of the same fix. > same for aggr_update_shadow change In this case the cpu is really an index and so the change is just renaming one to the other for the sake of clarity. Thanks, Ian > jirka > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel