Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934425AbbLQJ0i (ORCPT ); Thu, 17 Dec 2015 04:26:38 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:34890 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753240AbbLQJ0e (ORCPT ); Thu, 17 Dec 2015 04:26:34 -0500 MIME-Version: 1.0 In-Reply-To: <20151216212104.GZ15533@two.firstfloor.org> References: <1450227266-2501-1-git-send-email-andi@firstfloor.org> <1450227266-2501-6-git-send-email-andi@firstfloor.org> <20151216212104.GZ15533@two.firstfloor.org> Date: Thu, 17 Dec 2015 01:26:33 -0800 Message-ID: Subject: Re: [PATCH 05/10] perf, tools, stat: Basic support for TopDown in perf stat From: Stephane Eranian To: Andi Kleen Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Jiri Olsa , Ingo Molnar , LKML , Namhyung Kim , Andi Kleen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2486 Lines: 68 Andi, On Wed, Dec 16, 2015 at 1:21 PM, Andi Kleen wrote: >> > +/* >> > + * Check whether we can use a group for top down. >> > + * Without a group may get bad results due to multiplexing. >> > + */ >> >> That is not because you have a counter used by the NMI that >> you cannot group. If HT is off you have plenty of counters to >> do this. > > Such a heuristic wouldn't work on Atom where there are no more > counters in this case. > >> > +static bool check_group(bool *warn) >> > +{ >> > + int n; >> > + >> > + if (sysctl__read_int("kernel/nmi_watchdog", &n) < 0) >> > + return false; >> > + if (n > 0) { >> > + *warn = true; >> > + return false; >> > + } >> > + return true; >> > +} >> > + >> >> I do not like this part very much. You are pulling in x86 specific >> knowledge into >> builtin_stat.c. Why not move this into an x86 specific file? > > Done. > >> > err = parse_events(evsel_list, transaction_attrs, NULL); >> > @@ -1511,6 +1579,36 @@ static int add_default_attributes(void) >> > return 0; >> > } >> > >> > + if (topdown_run) { >> > + char *str = NULL; >> > + bool warn = false; >> > + >> > + filter_events(topdown_attrs, &str, check_group(&warn)); >> > + if (topdown_attrs[0] && str) { >> > + if (warn) >> > + fprintf(stderr, >> > + "nmi_watchdog enabled with topdown. May give wrong results.\n" >> > + "Disable with echo 0 > /proc/sys/kernel/nmi_watchdog\n"); >> >> This is x86 specific. Why not just try it out and in case of error >> suggest checking >> if pinned system-wide events exist (such as NMI watchdog on x86). that would >> be more generic. > > That's really complicated, i would have to tear down all state and then > resubmit all the events. I think just checking the NMI watchdog is good > enough. I couldn't give a sensible error message for the generic case > anyways. > What about that fancy error reporting extension that was proposed a few months back? It could help print a sensible error msg. Did it ever make it in? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/