From: Rob Herring <robh@kernel.org> To: Will Deacon <will@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@redhat.com>, Mark Rutland <mark.rutland@arm.com>, Ian Rogers <irogers@google.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>, Zachary.Leaf@arm.com, Raphael Gault <raphael.gault@arm.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, Namhyung Kim <namhyung@kernel.org>, Itaru Kitayama <itaru.kitayama@gmail.com>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v6 02/10] arm64: perf: Enable PMU counter direct access for perf event Date: Tue, 30 Mar 2021 16:08:11 -0500 [thread overview] Message-ID: <CAL_JsqKqKKb8uXSxQKT4ZMqMv8dt3ABpP+T8x+A1-zb2RKjCNA@mail.gmail.com> (raw) In-Reply-To: <CAL_JsqKN4=T4tHofEoBoWVEZSQEj_m=561_kEdEEkz5szHszhQ@mail.gmail.com> On Tue, Mar 30, 2021 at 12:09 PM Rob Herring <robh@kernel.org> wrote: > > On Tue, Mar 30, 2021 at 10:31 AM Will Deacon <will@kernel.org> wrote: > > > > On Wed, Mar 10, 2021 at 05:08:29PM -0700, Rob Herring wrote: > > > From: Raphael Gault <raphael.gault@arm.com> > > > > > > Keep track of event opened with direct access to the hardware counters > > > and modify permissions while they are open. > > > > > > The strategy used here is the same which x86 uses: every time an event > > > is mapped, the permissions are set if required. The atomic field added > > > in the mm_context helps keep track of the different event opened and > > > de-activate the permissions when all are unmapped. > > > We also need to update the permissions in the context switch code so > > > that tasks keep the right permissions. > > > > > > In order to enable 64-bit counters for userspace when available, a new > > > config1 bit is added for userspace to indicate it wants userspace counter > > > access. This bit allows the kernel to decide if chaining should be > > > disabled and chaining and userspace access are incompatible. > > > The modes for config1 are as follows: > > > > > > config1 = 0 or 2 : user access enabled and always 32-bit > > > config1 = 1 : user access disabled and always 64-bit (using chaining if needed) > > > config1 = 3 : user access enabled and counter size matches underlying counter. [...] > > > @@ -980,9 +1032,23 @@ static int __armv8_pmuv3_map_event(struct perf_event *event, > > > &armv8_pmuv3_perf_cache_map, > > > ARMV8_PMU_EVTYPE_EVENT); > > > > > > - if (armv8pmu_event_is_64bit(event)) > > > + if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) { > > > + event->hw.flags |= ARMPMU_EL0_RD_CNTR; > > > > Why do you set this for all 32-bit events? > > It goes back to the config1 bits as explained in the commit msg. We > can always support user access for 32-bit counters, but for 64-bit > counters the user has to request both user access and 64-bit counters. > We could require explicit user access request for 32-bit access, but I > thought it was better to not require userspace to do something Arm > specific on open. > > > The logic here feels like it > > could with a bit of untangling. > > Yes, I don't love it, but couldn't come up with anything better. It is > complicated by the fact that flags have to be set before we assign the > counter and can't set/change them when we assign the counter. It would > take a lot of refactoring with armpmu code to fix that. How's this instead?: if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) event->hw.flags |= ARMPMU_EL0_RD_CNTR; /* * At this point, the counter is not assigned. If a 64-bit counter is * requested, we must make sure the h/w has 64-bit counters if we set * the event size to 64-bit because chaining is not supported with * userspace access. This may still fail later on if the CPU cycle * counter is in use. */ if (armv8pmu_event_is_64bit(event) && (!armv8pmu_event_want_user_access(event) || armv8pmu_has_long_event(cpu_pmu) || (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES))) event->hw.flags |= ARMPMU_EVT_64BIT; > > > + /* > > > + * At this point, the counter is not assigned. If a 64-bit > > > + * counter is requested, we must make sure the h/w has 64-bit > > > + * counters if we set the event size to 64-bit because chaining > > > + * is not supported with userspace access. This may still fail > > > + * later on if the CPU cycle counter is in use. > > > + */ > > > + if (armv8pmu_event_is_64bit(event) && > > > + (armv8pmu_has_long_event(armpmu) || > > > + hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES)) > > > + event->hw.flags |= ARMPMU_EVT_64BIT; > > > + } else if (armv8pmu_event_is_64bit(event)) > > > event->hw.flags |= ARMPMU_EVT_64BIT;
WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robh@kernel.org> To: Will Deacon <will@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@redhat.com>, Mark Rutland <mark.rutland@arm.com>, Ian Rogers <irogers@google.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>, Zachary.Leaf@arm.com, Raphael Gault <raphael.gault@arm.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, Namhyung Kim <namhyung@kernel.org>, Itaru Kitayama <itaru.kitayama@gmail.com>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v6 02/10] arm64: perf: Enable PMU counter direct access for perf event Date: Tue, 30 Mar 2021 16:08:11 -0500 [thread overview] Message-ID: <CAL_JsqKqKKb8uXSxQKT4ZMqMv8dt3ABpP+T8x+A1-zb2RKjCNA@mail.gmail.com> (raw) In-Reply-To: <CAL_JsqKN4=T4tHofEoBoWVEZSQEj_m=561_kEdEEkz5szHszhQ@mail.gmail.com> On Tue, Mar 30, 2021 at 12:09 PM Rob Herring <robh@kernel.org> wrote: > > On Tue, Mar 30, 2021 at 10:31 AM Will Deacon <will@kernel.org> wrote: > > > > On Wed, Mar 10, 2021 at 05:08:29PM -0700, Rob Herring wrote: > > > From: Raphael Gault <raphael.gault@arm.com> > > > > > > Keep track of event opened with direct access to the hardware counters > > > and modify permissions while they are open. > > > > > > The strategy used here is the same which x86 uses: every time an event > > > is mapped, the permissions are set if required. The atomic field added > > > in the mm_context helps keep track of the different event opened and > > > de-activate the permissions when all are unmapped. > > > We also need to update the permissions in the context switch code so > > > that tasks keep the right permissions. > > > > > > In order to enable 64-bit counters for userspace when available, a new > > > config1 bit is added for userspace to indicate it wants userspace counter > > > access. This bit allows the kernel to decide if chaining should be > > > disabled and chaining and userspace access are incompatible. > > > The modes for config1 are as follows: > > > > > > config1 = 0 or 2 : user access enabled and always 32-bit > > > config1 = 1 : user access disabled and always 64-bit (using chaining if needed) > > > config1 = 3 : user access enabled and counter size matches underlying counter. [...] > > > @@ -980,9 +1032,23 @@ static int __armv8_pmuv3_map_event(struct perf_event *event, > > > &armv8_pmuv3_perf_cache_map, > > > ARMV8_PMU_EVTYPE_EVENT); > > > > > > - if (armv8pmu_event_is_64bit(event)) > > > + if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) { > > > + event->hw.flags |= ARMPMU_EL0_RD_CNTR; > > > > Why do you set this for all 32-bit events? > > It goes back to the config1 bits as explained in the commit msg. We > can always support user access for 32-bit counters, but for 64-bit > counters the user has to request both user access and 64-bit counters. > We could require explicit user access request for 32-bit access, but I > thought it was better to not require userspace to do something Arm > specific on open. > > > The logic here feels like it > > could with a bit of untangling. > > Yes, I don't love it, but couldn't come up with anything better. It is > complicated by the fact that flags have to be set before we assign the > counter and can't set/change them when we assign the counter. It would > take a lot of refactoring with armpmu code to fix that. How's this instead?: if (armv8pmu_event_want_user_access(event) || !armv8pmu_event_is_64bit(event)) event->hw.flags |= ARMPMU_EL0_RD_CNTR; /* * At this point, the counter is not assigned. If a 64-bit counter is * requested, we must make sure the h/w has 64-bit counters if we set * the event size to 64-bit because chaining is not supported with * userspace access. This may still fail later on if the CPU cycle * counter is in use. */ if (armv8pmu_event_is_64bit(event) && (!armv8pmu_event_want_user_access(event) || armv8pmu_has_long_event(cpu_pmu) || (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES))) event->hw.flags |= ARMPMU_EVT_64BIT; > > > + /* > > > + * At this point, the counter is not assigned. If a 64-bit > > > + * counter is requested, we must make sure the h/w has 64-bit > > > + * counters if we set the event size to 64-bit because chaining > > > + * is not supported with userspace access. This may still fail > > > + * later on if the CPU cycle counter is in use. > > > + */ > > > + if (armv8pmu_event_is_64bit(event) && > > > + (armv8pmu_has_long_event(armpmu) || > > > + hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES)) > > > + event->hw.flags |= ARMPMU_EVT_64BIT; > > > + } else if (armv8pmu_event_is_64bit(event)) > > > event->hw.flags |= ARMPMU_EVT_64BIT; _______________________________________________ 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:[~2021-03-30 21:09 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-11 0:08 [PATCH v6 00/10] libperf and arm64 userspace counter access support Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 01/10] arm64: pmu: Add function implementation to update event index in userpage Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-30 15:30 ` Will Deacon 2021-03-30 15:30 ` Will Deacon 2021-03-11 0:08 ` [PATCH v6 02/10] arm64: perf: Enable PMU counter direct access for perf event Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-30 11:30 ` Zachary Leaf 2021-03-30 11:30 ` Zachary Leaf 2021-03-30 15:31 ` Will Deacon 2021-03-30 15:31 ` Will Deacon 2021-03-30 17:09 ` Rob Herring 2021-03-30 17:09 ` Rob Herring 2021-03-30 21:08 ` Rob Herring [this message] 2021-03-30 21:08 ` Rob Herring 2021-03-31 15:38 ` Will Deacon 2021-03-31 15:38 ` Will Deacon 2021-03-31 17:52 ` Rob Herring 2021-03-31 17:52 ` Rob Herring 2021-04-01 9:04 ` Will Deacon 2021-04-01 9:04 ` Will Deacon 2021-03-31 16:00 ` Will Deacon 2021-03-31 16:00 ` Will Deacon 2021-04-01 19:45 ` Rob Herring 2021-04-01 19:45 ` Rob Herring 2021-04-07 12:44 ` Will Deacon 2021-04-07 12:44 ` Will Deacon 2021-04-08 11:08 ` Mark Rutland 2021-04-08 11:08 ` Mark Rutland 2021-04-08 18:38 ` Rob Herring 2021-04-08 18:38 ` Rob Herring 2021-04-19 16:14 ` Will Deacon 2021-04-19 16:14 ` Will Deacon 2021-04-19 19:00 ` Rob Herring 2021-04-19 19:00 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 03/10] tools/include: Add an initial math64.h Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 04/10] libperf: Add evsel mmap support Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-12 13:58 ` Jiri Olsa 2021-03-12 13:58 ` Jiri Olsa 2021-03-12 14:34 ` Rob Herring 2021-03-12 14:34 ` Rob Herring 2021-03-12 18:29 ` Jiri Olsa 2021-03-12 18:29 ` Jiri Olsa 2021-03-31 22:06 ` Rob Herring 2021-03-31 22:06 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 05/10] libperf: tests: Add support for verbose printing Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 06/10] libperf: Add support for user space counter access Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-05-04 21:40 ` Ian Rogers 2021-05-04 21:40 ` Ian Rogers 2021-05-05 2:12 ` Rob Herring 2021-05-05 2:12 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 07/10] libperf: Add arm64 support to perf_mmap__read_self() Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 08/10] perf: arm64: Add test for userspace counter access on heterogeneous systems Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-15 16:09 ` Masayoshi Mizuma 2021-03-15 16:09 ` Masayoshi Mizuma 2021-03-11 0:08 ` [PATCH v6 09/10] perf: arm64: Add tests for 32-bit and 64-bit counter size userspace access Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-11 0:08 ` [PATCH v6 10/10] Documentation: arm64: Document PMU counters access from userspace Rob Herring 2021-03-11 0:08 ` Rob Herring 2021-03-31 16:00 ` Will Deacon 2021-03-31 16:00 ` Will Deacon 2021-03-30 11:31 ` [PATCH v6 00/10] libperf and arm64 userspace counter access support Zachary Leaf 2021-03-30 11:31 ` Zachary Leaf
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=CAL_JsqKqKKb8uXSxQKT4ZMqMv8dt3ABpP+T8x+A1-zb2RKjCNA@mail.gmail.com \ --to=robh@kernel.org \ --cc=Jonathan.Cameron@huawei.com \ --cc=Zachary.Leaf@arm.com \ --cc=acme@kernel.org \ --cc=alexander.shishkin@linux.intel.com \ --cc=catalin.marinas@arm.com \ --cc=honnappa.nagarahalli@arm.com \ --cc=irogers@google.com \ --cc=itaru.kitayama@gmail.com \ --cc=jolsa@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mingo@redhat.com \ --cc=namhyung@kernel.org \ --cc=peterz@infradead.org \ --cc=raphael.gault@arm.com \ --cc=will@kernel.org \ /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.