Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964860AbeALQqV convert rfc822-to-8bit (ORCPT + 1 other); Fri, 12 Jan 2018 11:46:21 -0500 Received: from mga14.intel.com ([192.55.52.115]:3945 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934027AbeALQqT (ORCPT ); Fri, 12 Jan 2018 11:46:19 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,349,1511856000"; d="scan'208";a="10298746" From: "Liang, Kan" To: Jiri Olsa CC: "acme@kernel.org" , "peterz@infradead.org" , "mingo@redhat.com" , "linux-kernel@vger.kernel.org" , "wangnan0@huawei.com" , "jolsa@kernel.org" , "namhyung@kernel.org" , "ak@linux.intel.com" , "yao.jin@linux.intel.com" Subject: RE: [PATCH V3 08/12] perf top: check per event overwrite term Thread-Topic: [PATCH V3 08/12] perf top: check per event overwrite term Thread-Index: AQHTeohZVR6bK+Uv8UC/ikJPKdDyPaNuVPqAgADsXwCAAHg4AIAAsIRggAAov0A= Date: Fri, 12 Jan 2018 16:46:14 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07753800042@SHSMSX103.ccr.corp.intel.com> References: <1513879734-237492-1-git-send-email-kan.liang@intel.com> <1513879734-237492-9-git-send-email-kan.liang@intel.com> <20180111142549.GD16655@krava> <37D7C6CF3E00A74B8858931C1DB2F077537FFC1F@SHSMSX103.ccr.corp.intel.com> <20180112114206.GA25413@krava> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2FmMTFjNzQtNjdlNC00N2Q2LTgyOTEtZjhmZjQzYmJmYjA0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjVVV2FCOVwvc2s4S2pzUUNnY3paOFJGQTRcL2NwVmZKZTBLWmJ4ajRIWjdIVT0ifQ== x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: > > On Thu, Jan 11, 2018 at 09:29:21PM +0000, Liang, Kan wrote: > > > > On Thu, Dec 21, 2017 at 10:08:50AM -0800, kan.liang@intel.com wrote: > > > > > From: Kan Liang > > > > > > > > > > Per event overwrite term is not forbidden in perf top, which can > > > > > bring problems. Because perf top only support non-overwrite mode. > > > > > > > > > > Check and forbid inconsistent per event overwrite term in the evlist. > > > > > Make it possible to support either non-overwrite or overwrite mode. > > > > > The overwrite mode is forbidden now, which will be removed when > > > > > the overwrite mode is supported later. > > > > > > > > > > Signed-off-by: Kan Liang > > > > > --- > > > > > tools/perf/builtin-top.c | 60 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 60 insertions(+) > > > > > > > > > > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > > > > > index c6ccda5..4b85e7b 100644 > > > > > --- a/tools/perf/builtin-top.c > > > > > +++ b/tools/perf/builtin-top.c > > > > > @@ -881,6 +881,56 @@ static void perf_top__mmap_read(struct > > > > > perf_top > > > > *top) > > > > > perf_top__mmap_read_idx(top, i); } > > > > > > > > > > +/* > > > > > + * Check per event overwrite term. > > > > > + * perf top supports consistent mode for all events. > > > > > + * Return -1 if the per event terms set but not consistent. > > > > > > > > please list the rules for overwrite in the comment are they just > > > > top specific? > > > > > > Yes, it's only for perf top. > > > It doesn't support that some events are overwrite mode and other > > > events are non-overwrite mode. > > > I will refine the comments. > > > > > > > > > > > SNIP > > > > > > > > > + if (evsel == perf_evlist__first(evlist)) > > > > > + overwrite = set; > > > > > + else > > > > > + return -1; > > > > > + } > > > > > + } > > > > > + > > > > > + if ((overwrite >= 0) && (opts->overwrite != overwrite)) > > > > > + opts->overwrite = overwrite; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static int perf_top__start_counters(struct perf_top *top) { > > > > > char msg[BUFSIZ]; > > > > > @@ -890,6 +940,16 @@ static int perf_top__start_counters(struct > > > > perf_top *top) > > > > > > > > > > perf_evlist__config(evlist, opts, &callchain_param); > > > > > > > > so perf_evlist__config uses opts->overwrite, which you set in your > > > > perf_top_overwrite_check call.. I'd think you need to call it > > > > sooner > > > > > > > > > > User may set per-event mode, which is processed in > > > perf_evlist__config It's possible that the per-event mode is not same as > opts->overwrite. > > > If so, perf_evlist__config actually uses per-event mode. > > > > > > perf_top_overwrite_check will do the check. If the per-event mode is > > > inconsistent as opts->overwrite, updating the opts->overwrite. > > > > > > It has to be called after the perf_evlist__config. > > > > let's see the rules then first, also wrt opts->overwrite, which is > > used later on I assume > > > > The rules for perf_top_overwrite_check is quite simple. > - All events have same per-event mode. > E.g. "cpu/cpu-cycles,no-overwrite/,cpu/instructions,no-overwrite/ > Using the per-event setting to replace the opts->overwrite if they are > different. > - Events have different per-event mode. > E.g. "cpu/cpu-cycles,overwrite/,cpu/instructions,no-overwrite/" > Error out. > - Some of the event set per-event mode, but some not. > E.g. "cpu/cpu-cycles, /,cpu/instructions,no-overwrite/" > Error out. > > There is no --overwrite option support for perf top. > We have a default opts->overwrite. > In perf_evlist__config, attr->write_backward will be set according to > opts->overwrite at first. But if there is per-event mode, the > attr->write_backward will be updated accordingly. > There may be inconsistent between opts->overwrite and > attr->write_backward. > > perf_top_overwrite_check will be called right after perf_evlist__config. > It can fix the inconsistence by updating the opts->overwrite or error out. > The opts->overwrite will be used by fallback and _mmap_read() later. > Check the code again. Perf_top_overwrite_check() reply on evsel->config_terms to check the terms, which is already parsed before perf_evlist__config(). You are right, we can call it earlier. I will move the codes right before the perf_evlist__config() in V4. Thanks, Kan