Message ID | 20230715032915.97146-3-yangjihong1@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp224vqt; Fri, 14 Jul 2023 20:36:48 -0700 (PDT) X-Google-Smtp-Source: APBJJlGPgYBoBgSGMVm0AGQADzPJmQYXLckcwiWbBPUyDh5WaJ8dvVKCVci3Dg3yGRkvFnIP2lC/ X-Received: by 2002:a05:6512:210a:b0:4f6:6b:a43b with SMTP id q10-20020a056512210a00b004f6006ba43bmr4273409lfr.52.1689392207779; Fri, 14 Jul 2023 20:36:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689392207; cv=none; d=google.com; s=arc-20160816; b=VYvW3e5wztEh7E25WEc6qkr1gzJUuILOsFBUZv++RMPqBc4WJgJOOMknrkCAD+o1/H dAnaZj8Yn/s4qWhz9xa7ml7FH79EM0vSRm48g+32uVLjXzLqVvxfu85q6Q4KKsZb1TVW fmK3+D0gGnB7ZP/rjKXMi6YAZDEulp9wIWBqTUELoXWlBBOo0BSc5l86U0xeLDKjUFnz E9mT3kxwioF8aoX6KMVwheDY3msBSV6Q23TUlbj5c4wEYZ4FQR4CMZPaQeQRFW3K6bI/ pJpgm75TlTE1wnfuN9j2EoVtAxx2wMTdHwUt2iX9P2+E7drYDb4Qirv7IWJ9pvivs5WG vttA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=Op/wOwUQ6kgLn1wSpQN6Nxcm0PTNIgrnEOInp+6SHFk=; fh=Ib1wi7jRbo+eNDLEErvWCJx+cnUm87PkGFthhdFf08Y=; b=NIJCLj9QjuePMop8meeN1dlrVcDRYf5ul2G/tFQbakU7FPhyJL/Gf/A1R7NqGIY/Pj QnqMAUFfOb0AOY41YGyI5RpJzrYm3mUg4p82IqLZqARPhtbxdhkXBLnYCV+crwuUepmg I8mikJWzg/8BdV5A/Hvd44CJr33yk2bv22dCzpBXE4RlVVZLCuGGjx2LRPAOpNCNyd9B 4lXln7xfrTmitrc61Du0R1uM5mOzcrp87HlfNn18JLJJZwJh4GgtweH+fgLbtgcvtVHz piYoX7YHXaXVofKvsUcSPnnZCZB567lbrBBqo8jyBrqO0SV2t13UhWABC89ggYzvkoi6 0eBA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b3-20020aa7c6c3000000b0051e2936779fsi4141298eds.380.2023.07.14.20.36.24; Fri, 14 Jul 2023 20:36:47 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230109AbjGODbn (ORCPT <rfc822;tebrre53rla2o@gmail.com> + 99 others); Fri, 14 Jul 2023 23:31:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33216 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229737AbjGODbf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 14 Jul 2023 23:31:35 -0400 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 35C0B3A99; Fri, 14 Jul 2023 20:31:34 -0700 (PDT) Received: from kwepemm600003.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4R2v25500VztR9j; Sat, 15 Jul 2023 11:28:29 +0800 (CST) Received: from localhost.localdomain (10.67.174.95) by kwepemm600003.china.huawei.com (7.193.23.202) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Sat, 15 Jul 2023 11:31:31 +0800 From: Yang Jihong <yangjihong1@huawei.com> To: <peterz@infradead.org>, <mingo@redhat.com>, <acme@kernel.org>, <mark.rutland@arm.com>, <alexander.shishkin@linux.intel.com>, <jolsa@kernel.org>, <namhyung@kernel.org>, <irogers@google.com>, <adrian.hunter@intel.com>, <kan.liang@linux.intel.com>, <james.clark@arm.com>, <tmricht@linux.ibm.com>, <ak@linux.intel.com>, <anshuman.khandual@arm.com>, <linux-kernel@vger.kernel.org>, <linux-perf-users@vger.kernel.org> CC: <yangjihong1@huawei.com> Subject: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper Date: Sat, 15 Jul 2023 03:29:10 +0000 Message-ID: <20230715032915.97146-3-yangjihong1@huawei.com> X-Mailer: git-send-email 2.30.GIT In-Reply-To: <20230715032915.97146-1-yangjihong1@huawei.com> References: <20230715032915.97146-1-yangjihong1@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.67.174.95] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To kwepemm600003.china.huawei.com (7.193.23.202) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771456123528246571 X-GMAIL-MSGID: 1771456123528246571 |
Series |
perf record: Track sideband events for all CPUs when tracing selected CPUs
|
|
Commit Message
Yang Jihong
July 15, 2023, 3:29 a.m. UTC
Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
tracking to the evlist. We may need to search for the dummy event for
some settings. Therefore, add evlist__findnew_tracking_event() helper.
evlist__findnew_tracking_event() also deal with system_wide maps if
system_wide is true.
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
tools/perf/builtin-record.c | 11 +++--------
tools/perf/util/evlist.c | 18 ++++++++++++++++++
tools/perf/util/evlist.h | 1 +
3 files changed, 22 insertions(+), 8 deletions(-)
Comments
On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: > > Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for > tracking to the evlist. We may need to search for the dummy event for > some settings. Therefore, add evlist__findnew_tracking_event() helper. > > evlist__findnew_tracking_event() also deal with system_wide maps if > system_wide is true. I'm wondering if we can simplify the naming in the API, we have "dummy event" which makes sense as we literally call the event "dummy", "sideband" which refers to the kind of samples/events the dummy event will record but "tracking" I think tends to get used as a verb rather than a noun. So I think evlist__findnew_tracking_event should be evlist__findnew_dummy_event. Thanks, Ian > Signed-off-by: Yang Jihong <yangjihong1@huawei.com> > --- > tools/perf/builtin-record.c | 11 +++-------- > tools/perf/util/evlist.c | 18 ++++++++++++++++++ > tools/perf/util/evlist.h | 1 + > 3 files changed, 22 insertions(+), 8 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index aec18db7ff23..ca83599cc50c 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec) > */ > if (opts->target.initial_delay || target__has_cpu(&opts->target) || > perf_pmus__num_core_pmus() > 1) { > - pos = evlist__get_tracking_event(evlist); > - if (!evsel__is_dummy_event(pos)) { > - /* Set up dummy event. */ > - if (evlist__add_dummy(evlist)) > - return -ENOMEM; > - pos = evlist__last(evlist); > - evlist__set_tracking_event(evlist, pos); > - } > + pos = evlist__findnew_tracking_event(evlist, false); > + if (!pos) > + return -ENOMEM; > > /* > * Enable the dummy event when the process is forked for > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 7ef43f72098e..25c3ebe2c2f5 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev > tracking_evsel->tracking = true; > } > > +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide) > +{ > + struct evsel *evsel; > + > + evsel = evlist__get_tracking_event(evlist); > + if (!evsel__is_dummy_event(evsel)) { > + evsel = evlist__add_aux_dummy(evlist, system_wide); > + if (!evsel) > + return NULL; > + > + evlist__set_tracking_event(evlist, evsel); > + } else if (system_wide) { > + perf_evlist__go_system_wide(&evlist->core, &evsel->core); > + } > + > + return evsel; > +} > + > struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str) > { > struct evsel *evsel; > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h > index 664c6bf7b3e0..98e7ddb2bd30 100644 > --- a/tools/perf/util/evlist.h > +++ b/tools/perf/util/evlist.h > @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr); > > struct evsel *evlist__get_tracking_event(struct evlist *evlist); > void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel); > +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide); > > struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str); > > -- > 2.30.GIT >
On 19/07/23 19:44, Ian Rogers wrote: > On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: >> >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for >> tracking to the evlist. We may need to search for the dummy event for >> some settings. Therefore, add evlist__findnew_tracking_event() helper. >> >> evlist__findnew_tracking_event() also deal with system_wide maps if >> system_wide is true. > > I'm wondering if we can simplify the naming in the API, we have "dummy > event" which makes sense as we literally call the event "dummy", > "sideband" which refers to the kind of samples/events the dummy event > will record but "tracking" I think tends to get used as a verb rather > than a noun. So I think evlist__findnew_tracking_event should be > evlist__findnew_dummy_event. Except the tracking event has "tracking" set but there can be other dummy events that do not. > > Thanks, > Ian > >> Signed-off-by: Yang Jihong <yangjihong1@huawei.com> >> --- >> tools/perf/builtin-record.c | 11 +++-------- >> tools/perf/util/evlist.c | 18 ++++++++++++++++++ >> tools/perf/util/evlist.h | 1 + >> 3 files changed, 22 insertions(+), 8 deletions(-) >> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >> index aec18db7ff23..ca83599cc50c 100644 >> --- a/tools/perf/builtin-record.c >> +++ b/tools/perf/builtin-record.c >> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec) >> */ >> if (opts->target.initial_delay || target__has_cpu(&opts->target) || >> perf_pmus__num_core_pmus() > 1) { >> - pos = evlist__get_tracking_event(evlist); >> - if (!evsel__is_dummy_event(pos)) { >> - /* Set up dummy event. */ >> - if (evlist__add_dummy(evlist)) >> - return -ENOMEM; >> - pos = evlist__last(evlist); >> - evlist__set_tracking_event(evlist, pos); >> - } >> + pos = evlist__findnew_tracking_event(evlist, false); >> + if (!pos) >> + return -ENOMEM; >> >> /* >> * Enable the dummy event when the process is forked for >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >> index 7ef43f72098e..25c3ebe2c2f5 100644 >> --- a/tools/perf/util/evlist.c >> +++ b/tools/perf/util/evlist.c >> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev >> tracking_evsel->tracking = true; >> } >> >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide) >> +{ >> + struct evsel *evsel; >> + >> + evsel = evlist__get_tracking_event(evlist); >> + if (!evsel__is_dummy_event(evsel)) { >> + evsel = evlist__add_aux_dummy(evlist, system_wide); >> + if (!evsel) >> + return NULL; >> + >> + evlist__set_tracking_event(evlist, evsel); >> + } else if (system_wide) { >> + perf_evlist__go_system_wide(&evlist->core, &evsel->core); >> + } >> + >> + return evsel; >> +} >> + >> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str) >> { >> struct evsel *evsel; >> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h >> index 664c6bf7b3e0..98e7ddb2bd30 100644 >> --- a/tools/perf/util/evlist.h >> +++ b/tools/perf/util/evlist.h >> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr); >> >> struct evsel *evlist__get_tracking_event(struct evlist *evlist); >> void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel); >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide); >> >> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str); >> >> -- >> 2.30.GIT >>
On Wed, Jul 19, 2023 at 9:59 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 19/07/23 19:44, Ian Rogers wrote: > > On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: > >> > >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for > >> tracking to the evlist. We may need to search for the dummy event for > >> some settings. Therefore, add evlist__findnew_tracking_event() helper. > >> > >> evlist__findnew_tracking_event() also deal with system_wide maps if > >> system_wide is true. > > > > I'm wondering if we can simplify the naming in the API, we have "dummy > > event" which makes sense as we literally call the event "dummy", > > "sideband" which refers to the kind of samples/events the dummy event > > will record but "tracking" I think tends to get used as a verb rather > > than a noun. So I think evlist__findnew_tracking_event should be > > evlist__findnew_dummy_event. > > Except the tracking event has "tracking" set but there can be other dummy > events that do not. Thanks! I'm wondering what a dummy event without tracking would be used for - do you have a reference? I don't see anything in perf_event.h calling things like mmap2/comm in perf_event_attr tracking. I'm not a fan of dummy due to it not being intention revealing. Perhaps if we could go back in time we could call the event "sideband", maybe we should migrate to this. We have other non-obvious uses of the term dummy like in cpumap: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/perf/include/perf/cpumap.h?h=perf-tools-next#n24 If tracking can be on any event then does that make the functions behavior more ambiguous if it means dummy+tracking not <any event>+tracking? Thanks, Ian > > > > Thanks, > > Ian > > > >> Signed-off-by: Yang Jihong <yangjihong1@huawei.com> > >> --- > >> tools/perf/builtin-record.c | 11 +++-------- > >> tools/perf/util/evlist.c | 18 ++++++++++++++++++ > >> tools/perf/util/evlist.h | 1 + > >> 3 files changed, 22 insertions(+), 8 deletions(-) > >> > >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > >> index aec18db7ff23..ca83599cc50c 100644 > >> --- a/tools/perf/builtin-record.c > >> +++ b/tools/perf/builtin-record.c > >> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec) > >> */ > >> if (opts->target.initial_delay || target__has_cpu(&opts->target) || > >> perf_pmus__num_core_pmus() > 1) { > >> - pos = evlist__get_tracking_event(evlist); > >> - if (!evsel__is_dummy_event(pos)) { > >> - /* Set up dummy event. */ > >> - if (evlist__add_dummy(evlist)) > >> - return -ENOMEM; > >> - pos = evlist__last(evlist); > >> - evlist__set_tracking_event(evlist, pos); > >> - } > >> + pos = evlist__findnew_tracking_event(evlist, false); > >> + if (!pos) > >> + return -ENOMEM; > >> > >> /* > >> * Enable the dummy event when the process is forked for > >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > >> index 7ef43f72098e..25c3ebe2c2f5 100644 > >> --- a/tools/perf/util/evlist.c > >> +++ b/tools/perf/util/evlist.c > >> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev > >> tracking_evsel->tracking = true; > >> } > >> > >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide) > >> +{ > >> + struct evsel *evsel; > >> + > >> + evsel = evlist__get_tracking_event(evlist); > >> + if (!evsel__is_dummy_event(evsel)) { > >> + evsel = evlist__add_aux_dummy(evlist, system_wide); > >> + if (!evsel) > >> + return NULL; > >> + > >> + evlist__set_tracking_event(evlist, evsel); > >> + } else if (system_wide) { > >> + perf_evlist__go_system_wide(&evlist->core, &evsel->core); > >> + } > >> + > >> + return evsel; > >> +} > >> + > >> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str) > >> { > >> struct evsel *evsel; > >> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h > >> index 664c6bf7b3e0..98e7ddb2bd30 100644 > >> --- a/tools/perf/util/evlist.h > >> +++ b/tools/perf/util/evlist.h > >> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr); > >> > >> struct evsel *evlist__get_tracking_event(struct evlist *evlist); > >> void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel); > >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide); > >> > >> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str); > >> > >> -- > >> 2.30.GIT > >> >
Hello, On 2023/7/20 0:44, Ian Rogers wrote: > On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: >> >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for >> tracking to the evlist. We may need to search for the dummy event for >> some settings. Therefore, add evlist__findnew_tracking_event() helper. >> >> evlist__findnew_tracking_event() also deal with system_wide maps if >> system_wide is true. > > I'm wondering if we can simplify the naming in the API, we have "dummy > event" which makes sense as we literally call the event "dummy", > "sideband" which refers to the kind of samples/events the dummy event > will record but "tracking" I think tends to get used as a verb rather > than a noun. So I think evlist__findnew_tracking_event should be > evlist__findnew_dummy_event. > Uh, from the discussion that followed, it seems that there is no consensus yet... If there is a clear consensus on whether to use "dummy event" or "tracking event", I will change the name of the API. I think sideband event is equivalent to tracking event (refer evsel__config(), tracking events include task, mmap, mmap2, and comm sideband events, which are all sideband). tracking event are instances of dummy event. For example, we create another dummy event to record the text poke event of ksymbol (refer perf record --kcore). An evlist contains only one tracking event, but can contain multiple dummy events. Thanks, Yang
On Thu, Jul 20, 2023 at 12:24 AM Yang Jihong <yangjihong1@huawei.com> wrote: > > Hello, > > On 2023/7/20 0:44, Ian Rogers wrote: > > On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: > >> > >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for > >> tracking to the evlist. We may need to search for the dummy event for > >> some settings. Therefore, add evlist__findnew_tracking_event() helper. > >> > >> evlist__findnew_tracking_event() also deal with system_wide maps if > >> system_wide is true. > > > > I'm wondering if we can simplify the naming in the API, we have "dummy > > event" which makes sense as we literally call the event "dummy", > > "sideband" which refers to the kind of samples/events the dummy event > > will record but "tracking" I think tends to get used as a verb rather > > than a noun. So I think evlist__findnew_tracking_event should be > > evlist__findnew_dummy_event. > > > Uh, from the discussion that followed, it seems that there is no > consensus yet... > If there is a clear consensus on whether to use "dummy event" or > "tracking event", I will change the name of the API. > > I think sideband event is equivalent to tracking event (refer > evsel__config(), tracking events include task, mmap, mmap2, and comm > sideband events, which are all sideband). > > tracking event are instances of dummy event. For example, we create > another dummy event to record the text poke event of ksymbol (refer perf > record --kcore). > > An evlist contains only one tracking event, but can contain multiple > dummy events. Thanks for the feedback. So the tracking event is by definition the first dummy event in the evlist? What is the purpose of the other dummy events in this case? Perhaps we can get to an intention revealing implementation something like: /** The "tracking event" gathering sideband data is the first dummy event in the list. */ struct evsel *evlist__findnew_tracking_event(struct evlist *evlist) { struct evsel *dummy = evlist__find_first_dummy_event(evlist); if (!dummy) { dummy = evlist__add_dummy(evlist); } return dummy; } But I think the key thing for me is I'm still not sure what is going on when there are multiple dummy events for you, what are the other dummy events for other than tracking sideband data? Thanks, Ian > > Thanks, > Yang
Hello, On 2023/7/29 0:40, Ian Rogers wrote: > On Thu, Jul 20, 2023 at 12:24 AM Yang Jihong <yangjihong1@huawei.com> wrote: >> >> Hello, >> >> On 2023/7/20 0:44, Ian Rogers wrote: >>> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: >>>> >>>> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for >>>> tracking to the evlist. We may need to search for the dummy event for >>>> some settings. Therefore, add evlist__findnew_tracking_event() helper. >>>> >>>> evlist__findnew_tracking_event() also deal with system_wide maps if >>>> system_wide is true. >>> >>> I'm wondering if we can simplify the naming in the API, we have "dummy >>> event" which makes sense as we literally call the event "dummy", >>> "sideband" which refers to the kind of samples/events the dummy event >>> will record but "tracking" I think tends to get used as a verb rather >>> than a noun. So I think evlist__findnew_tracking_event should be >>> evlist__findnew_dummy_event. >>> >> Uh, from the discussion that followed, it seems that there is no >> consensus yet... >> If there is a clear consensus on whether to use "dummy event" or >> "tracking event", I will change the name of the API. >> >> I think sideband event is equivalent to tracking event (refer >> evsel__config(), tracking events include task, mmap, mmap2, and comm >> sideband events, which are all sideband). >> >> tracking event are instances of dummy event. For example, we create >> another dummy event to record the text poke event of ksymbol (refer perf >> record --kcore). >> >> An evlist contains only one tracking event, but can contain multiple >> dummy events. > > Thanks for the feedback. So the tracking event is by definition the > first dummy event in the evlist? What is the purpose of the other Uh... It may not be the first dummy event, but evsel->track must be true. Only one evsel in an evlist meets this condition. > dummy events in this case? Perhaps we can get to an intention > revealing implementation something like: > > /** The "tracking event" gathering sideband data is the first dummy > event in the list. */ > struct evsel *evlist__findnew_tracking_event(struct evlist *evlist) > { > struct evsel *dummy = evlist__find_first_dummy_event(evlist); > > if (!dummy) { > dummy = evlist__add_dummy(evlist); > } > return dummy; > } > > But I think the key thing for me is I'm still not sure what is going > on when there are multiple dummy events for you, what are the other > dummy events for other than tracking sideband data? > For other dummy events, perf record will open a dummy event to track ksymbol text_poke when "--kcore" option is used. I thinks tracking ksymbol text_poke separately it needs to be processed independently, go system_wide and enable immediately. All of the above is my understanding, may need Adrian to confirm whether it is accurate. Thanks, Yang
On 19/07/23 20:12, Ian Rogers wrote: > On Wed, Jul 19, 2023 at 9:59 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 19/07/23 19:44, Ian Rogers wrote: >>> On Fri, Jul 14, 2023 at 8:31 PM Yang Jihong <yangjihong1@huawei.com> wrote: >>>> >>>> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for >>>> tracking to the evlist. We may need to search for the dummy event for >>>> some settings. Therefore, add evlist__findnew_tracking_event() helper. >>>> >>>> evlist__findnew_tracking_event() also deal with system_wide maps if >>>> system_wide is true. >>> >>> I'm wondering if we can simplify the naming in the API, we have "dummy >>> event" which makes sense as we literally call the event "dummy", >>> "sideband" which refers to the kind of samples/events the dummy event >>> will record but "tracking" I think tends to get used as a verb rather >>> than a noun. So I think evlist__findnew_tracking_event should be >>> evlist__findnew_dummy_event. >> >> Except the tracking event has "tracking" set but there can be other dummy >> events that do not. > > Thanks! I'm wondering what a dummy event without tracking would be > used for - do you have a reference? For example, text_poke events need to be recorded on all CPUs because changes to the kernel affect every CPU. Another example. it can be interesting to capture switch events CPU-wide even if only some processes are being traced. > I don't see anything in > perf_event.h calling things like mmap2/comm in perf_event_attr > tracking. I'm not a fan of dummy due to it not being intention > revealing. Perhaps if we could go back in time we could call the > event "sideband", Auxiliary events are inband with respect to the perf buffer. Only when the "main" tracing could be considered to be the AUX area buffer, can the events in the perf buffer be considered sideband. > maybe we should migrate to this. We have other > non-obvious uses of the term dummy like in cpumap: > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/perf/include/perf/cpumap.h?h=perf-tools-next#n24 > If tracking can be on any event then does that make the functions > behavior more ambiguous if it means dummy+tracking not <any > event>+tracking? > > Thanks, > Ian > >>> >>> Thanks, >>> Ian >>> >>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com> >>>> --- >>>> tools/perf/builtin-record.c | 11 +++-------- >>>> tools/perf/util/evlist.c | 18 ++++++++++++++++++ >>>> tools/perf/util/evlist.h | 1 + >>>> 3 files changed, 22 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>>> index aec18db7ff23..ca83599cc50c 100644 >>>> --- a/tools/perf/builtin-record.c >>>> +++ b/tools/perf/builtin-record.c >>>> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec) >>>> */ >>>> if (opts->target.initial_delay || target__has_cpu(&opts->target) || >>>> perf_pmus__num_core_pmus() > 1) { >>>> - pos = evlist__get_tracking_event(evlist); >>>> - if (!evsel__is_dummy_event(pos)) { >>>> - /* Set up dummy event. */ >>>> - if (evlist__add_dummy(evlist)) >>>> - return -ENOMEM; >>>> - pos = evlist__last(evlist); >>>> - evlist__set_tracking_event(evlist, pos); >>>> - } >>>> + pos = evlist__findnew_tracking_event(evlist, false); >>>> + if (!pos) >>>> + return -ENOMEM; >>>> >>>> /* >>>> * Enable the dummy event when the process is forked for >>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >>>> index 7ef43f72098e..25c3ebe2c2f5 100644 >>>> --- a/tools/perf/util/evlist.c >>>> +++ b/tools/perf/util/evlist.c >>>> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev >>>> tracking_evsel->tracking = true; >>>> } >>>> >>>> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide) >>>> +{ >>>> + struct evsel *evsel; >>>> + >>>> + evsel = evlist__get_tracking_event(evlist); >>>> + if (!evsel__is_dummy_event(evsel)) { >>>> + evsel = evlist__add_aux_dummy(evlist, system_wide); >>>> + if (!evsel) >>>> + return NULL; >>>> + >>>> + evlist__set_tracking_event(evlist, evsel); >>>> + } else if (system_wide) { >>>> + perf_evlist__go_system_wide(&evlist->core, &evsel->core); >>>> + } >>>> + >>>> + return evsel; >>>> +} >>>> + >>>> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str) >>>> { >>>> struct evsel *evsel; >>>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h >>>> index 664c6bf7b3e0..98e7ddb2bd30 100644 >>>> --- a/tools/perf/util/evlist.h >>>> +++ b/tools/perf/util/evlist.h >>>> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr); >>>> >>>> struct evsel *evlist__get_tracking_event(struct evlist *evlist); >>>> void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel); >>>> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide); >>>> >>>> struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str); >>>> >>>> -- >>>> 2.30.GIT >>>> >>
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index aec18db7ff23..ca83599cc50c 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec) */ if (opts->target.initial_delay || target__has_cpu(&opts->target) || perf_pmus__num_core_pmus() > 1) { - pos = evlist__get_tracking_event(evlist); - if (!evsel__is_dummy_event(pos)) { - /* Set up dummy event. */ - if (evlist__add_dummy(evlist)) - return -ENOMEM; - pos = evlist__last(evlist); - evlist__set_tracking_event(evlist, pos); - } + pos = evlist__findnew_tracking_event(evlist, false); + if (!pos) + return -ENOMEM; /* * Enable the dummy event when the process is forked for diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 7ef43f72098e..25c3ebe2c2f5 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_ev tracking_evsel->tracking = true; } +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide) +{ + struct evsel *evsel; + + evsel = evlist__get_tracking_event(evlist); + if (!evsel__is_dummy_event(evsel)) { + evsel = evlist__add_aux_dummy(evlist, system_wide); + if (!evsel) + return NULL; + + evlist__set_tracking_event(evlist, evsel); + } else if (system_wide) { + perf_evlist__go_system_wide(&evlist->core, &evsel->core); + } + + return evsel; +} + struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str) { struct evsel *evsel; diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h index 664c6bf7b3e0..98e7ddb2bd30 100644 --- a/tools/perf/util/evlist.h +++ b/tools/perf/util/evlist.h @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_cpu_iterator *evlist_cpu_itr); struct evsel *evlist__get_tracking_event(struct evlist *evlist); void evlist__set_tracking_event(struct evlist *evlist, struct evsel *tracking_evsel); +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, bool system_wide); struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str);