From: Leo Yan <leo.yan@linaro.org> To: James Clark <james.clark@arm.com> Cc: Timothy Hayes <timothy.hayes@arm.com>, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, acme@kernel.org, John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>, Mathieu Poirier <mathieu.poirier@linaro.org>, Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH 2/3] perf: arm-spe: Fix SPE events with phys addresses Date: Tue, 26 Apr 2022 21:19:52 +0800 [thread overview] Message-ID: <20220426131952.GA1923836@leoy-ThinkPad-X240s> (raw) In-Reply-To: <322009d2-330c-22d4-4075-eca2042f64e1@arm.com> On Mon, Apr 25, 2022 at 10:12:36AM +0100, James Clark wrote: [...] > >> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c > >> index 151cc38a171c..1a80151baed9 100644 > >> --- a/tools/perf/util/arm-spe.c > >> +++ b/tools/perf/util/arm-spe.c > >> @@ -1033,7 +1033,8 @@ arm_spe_synth_events(struct arm_spe *spe, struct perf_session *session) > >> memset(&attr, 0, sizeof(struct perf_event_attr)); > >> attr.size = sizeof(struct perf_event_attr); > >> attr.type = PERF_TYPE_HARDWARE; > >> - attr.sample_type = evsel->core.attr.sample_type & PERF_SAMPLE_MASK; > >> + attr.sample_type = evsel->core.attr.sample_type & > >> + (PERF_SAMPLE_MASK | PERF_SAMPLE_PHYS_ADDR); > > > > I verified this patch and I can confirm the physical address can be > > dumped successfully. > > > > I have a more general question, seems to me, we need to change the > > macro PERF_SAMPLE_MASK in the file util/event.h as below, so > > here doesn't need to 'or' the flag PERF_SAMPLE_PHYS_ADDR anymore. > > > > @Arnaldo, @Jiri, could you confirm if this is the right way to move > > forward? I am not sure why PERF_SAMPLE_MASK doesn't contain the bit > > PERF_SAMPLE_PHYS_ADDR in current code. > > I think there is a reason that PERF_SAMPLE_MASK is a subset of all the > bits. This comment below suggests it. Is it so the mask only includes fields > that are 64bits? That makes the __evsel__sample_size() function a simple > multiplication of a count of all the fields that are 64bits. After reading code, seems the conclusion "a count of all the fields that are 64bits" is not valid. PERF_SAMPLE_MASK contains bits PERF_SAMPLE_IP and PERF_SAMPLE_TID, the corresponding fields 'pid' and 'tid' in the structure perf_sample are u32 type. > static int > perf_event__check_size(union perf_event *event, unsigned int sample_size) > { > /* > * The evsel's sample_size is based on PERF_SAMPLE_MASK which includes > * up to PERF_SAMPLE_PERIOD. After that overflow() must be used to > * check the format does not go past the end of the event. > */ > if (sample_size + sizeof(event->header) > event->header.size) > return -EFAULT; Okay, thanks for sharing the info, it does show that it's deliberately to not contain all fields in PERF_SAMPLE_MASK. If so, this patch is fine for me: Reviewed-by: Leo Yan <leo.yan@linaro.org> > return 0; > } > > Having said that, the mask was updated once to add PERF_SAMPLE_IDENTIFIER to > it, so that comment is slightly out of date now. > > > > > > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h > > index cdd72e05fd28..c905ac32ebad 100644 > > --- a/tools/perf/util/event.h > > +++ b/tools/perf/util/event.h > > @@ -39,7 +39,7 @@ struct perf_event_attr; > > PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR | \ > > PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID | \ > > PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD | \ > > - PERF_SAMPLE_IDENTIFIER) > > + PERF_SAMPLE_IDENTIFIER | PERF_SAMPLE_PHYS_ADDR) > > > > Thanks, > > Leo > > > >> attr.sample_type |= PERF_SAMPLE_IP | PERF_SAMPLE_TID | > >> PERF_SAMPLE_PERIOD | PERF_SAMPLE_DATA_SRC | > >> PERF_SAMPLE_WEIGHT | PERF_SAMPLE_ADDR; > >> -- > >> 2.25.1 > >>
WARNING: multiple messages have this Message-ID (diff)
From: Leo Yan <leo.yan@linaro.org> To: James Clark <james.clark@arm.com> Cc: Timothy Hayes <timothy.hayes@arm.com>, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, acme@kernel.org, John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>, Mathieu Poirier <mathieu.poirier@linaro.org>, Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH 2/3] perf: arm-spe: Fix SPE events with phys addresses Date: Tue, 26 Apr 2022 21:19:52 +0800 [thread overview] Message-ID: <20220426131952.GA1923836@leoy-ThinkPad-X240s> (raw) In-Reply-To: <322009d2-330c-22d4-4075-eca2042f64e1@arm.com> On Mon, Apr 25, 2022 at 10:12:36AM +0100, James Clark wrote: [...] > >> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c > >> index 151cc38a171c..1a80151baed9 100644 > >> --- a/tools/perf/util/arm-spe.c > >> +++ b/tools/perf/util/arm-spe.c > >> @@ -1033,7 +1033,8 @@ arm_spe_synth_events(struct arm_spe *spe, struct perf_session *session) > >> memset(&attr, 0, sizeof(struct perf_event_attr)); > >> attr.size = sizeof(struct perf_event_attr); > >> attr.type = PERF_TYPE_HARDWARE; > >> - attr.sample_type = evsel->core.attr.sample_type & PERF_SAMPLE_MASK; > >> + attr.sample_type = evsel->core.attr.sample_type & > >> + (PERF_SAMPLE_MASK | PERF_SAMPLE_PHYS_ADDR); > > > > I verified this patch and I can confirm the physical address can be > > dumped successfully. > > > > I have a more general question, seems to me, we need to change the > > macro PERF_SAMPLE_MASK in the file util/event.h as below, so > > here doesn't need to 'or' the flag PERF_SAMPLE_PHYS_ADDR anymore. > > > > @Arnaldo, @Jiri, could you confirm if this is the right way to move > > forward? I am not sure why PERF_SAMPLE_MASK doesn't contain the bit > > PERF_SAMPLE_PHYS_ADDR in current code. > > I think there is a reason that PERF_SAMPLE_MASK is a subset of all the > bits. This comment below suggests it. Is it so the mask only includes fields > that are 64bits? That makes the __evsel__sample_size() function a simple > multiplication of a count of all the fields that are 64bits. After reading code, seems the conclusion "a count of all the fields that are 64bits" is not valid. PERF_SAMPLE_MASK contains bits PERF_SAMPLE_IP and PERF_SAMPLE_TID, the corresponding fields 'pid' and 'tid' in the structure perf_sample are u32 type. > static int > perf_event__check_size(union perf_event *event, unsigned int sample_size) > { > /* > * The evsel's sample_size is based on PERF_SAMPLE_MASK which includes > * up to PERF_SAMPLE_PERIOD. After that overflow() must be used to > * check the format does not go past the end of the event. > */ > if (sample_size + sizeof(event->header) > event->header.size) > return -EFAULT; Okay, thanks for sharing the info, it does show that it's deliberately to not contain all fields in PERF_SAMPLE_MASK. If so, this patch is fine for me: Reviewed-by: Leo Yan <leo.yan@linaro.org> > return 0; > } > > Having said that, the mask was updated once to add PERF_SAMPLE_IDENTIFIER to > it, so that comment is slightly out of date now. > > > > > > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h > > index cdd72e05fd28..c905ac32ebad 100644 > > --- a/tools/perf/util/event.h > > +++ b/tools/perf/util/event.h > > @@ -39,7 +39,7 @@ struct perf_event_attr; > > PERF_SAMPLE_TIME | PERF_SAMPLE_ADDR | \ > > PERF_SAMPLE_ID | PERF_SAMPLE_STREAM_ID | \ > > PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD | \ > > - PERF_SAMPLE_IDENTIFIER) > > + PERF_SAMPLE_IDENTIFIER | PERF_SAMPLE_PHYS_ADDR) > > > > Thanks, > > Leo > > > >> attr.sample_type |= PERF_SAMPLE_IP | PERF_SAMPLE_TID | > >> PERF_SAMPLE_PERIOD | PERF_SAMPLE_DATA_SRC | > >> PERF_SAMPLE_WEIGHT | PERF_SAMPLE_ADDR; > >> -- > >> 2.25.1 > >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-04-26 13:20 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-21 16:52 [PATCH 0/3] perf: arm-spe: Fix addresses of synthesized Arm SPE events Timothy Hayes 2022-04-21 16:52 ` Timothy Hayes 2022-04-21 16:52 ` [PATCH 1/3] perf: arm-spe: Fix addresses of synthesized " Timothy Hayes 2022-04-21 16:52 ` Timothy Hayes 2022-04-24 12:28 ` Leo Yan 2022-04-24 12:28 ` Leo Yan 2022-04-24 15:22 ` Leo Yan 2022-04-24 15:22 ` Leo Yan 2022-04-21 16:52 ` [PATCH 2/3] perf: arm-spe: Fix SPE events with phys addresses Timothy Hayes 2022-04-21 16:52 ` Timothy Hayes 2022-04-24 12:59 ` Leo Yan 2022-04-24 12:59 ` Leo Yan 2022-04-25 9:12 ` James Clark 2022-04-25 9:12 ` James Clark 2022-04-26 13:19 ` Leo Yan [this message] 2022-04-26 13:19 ` Leo Yan 2022-04-21 16:52 ` [PATCH 3/3] perf test: Add perf_event_attr test for Arm SPE Timothy Hayes 2022-04-21 16:52 ` Timothy Hayes 2022-04-24 14:53 ` Leo Yan 2022-04-24 14:53 ` Leo Yan 2022-04-28 13:41 ` Arnaldo Carvalho de Melo 2022-04-28 13:41 ` Arnaldo Carvalho de Melo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220426131952.GA1923836@leoy-ThinkPad-X240s \ --to=leo.yan@linaro.org \ --cc=acme@kernel.org \ --cc=alexander.shishkin@linux.intel.com \ --cc=bpf@vger.kernel.org \ --cc=james.clark@arm.com \ --cc=john.fastabend@gmail.com \ --cc=john.garry@huawei.com \ --cc=jolsa@kernel.org \ --cc=kafai@fb.com \ --cc=kpsingh@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mathieu.poirier@linaro.org \ --cc=namhyung@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=songliubraving@fb.com \ --cc=timothy.hayes@arm.com \ --cc=will@kernel.org \ --cc=yhs@fb.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.