[PW_SID:991172] perf: Rework event_init checks - #771
Conversation
In the hypothetical case where a CMN event is opened with a software group leader that already has some other hardware sibling, currently arm_cmn_val_add_event() could try to interpret the other event's data as an arm_cmn_hw_event, which is not great since we dereference a pointer from there... Thankfully the way to be more robust is to be less clever - stop trying to special-case software events and simply skip any event that isn't for our PMU. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation logic shared by the HiSilicon HNS3/PCIe drivers is a bit off, in that given a software group leader, it will consider that event *in place of* the actual new event being opened. At worst this could theoretically allow an unschedulable group if the software event config happens to look like one of the hardware siblings. The uncore framework avoids that particular issue, but all 3 also share the common issue of not preventing racy access to the sibling list, and some redundant checks which can be cleaned up. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation here is erroneously inspecting software events, as well as other hardware siblings, which are only checked for *after* they've already been misinterpreted. Once again, just ignore events which don't belong to our PMU, and don't duplicate what perf_event_open() will already check for us. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation code here is superficially the right shape, but is failing to count the group leader, while also erroneously counting software siblings. Just correctly count the events which belong to our PMU, and let perf core worry about the rest. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation here has a few issues to fix: firstly, failing to count the group leader or the event being opened itself. Secondly it appears wrong not to count disabled sibling events given that they could be enabled later. Finally there's the subtlety that we should avoid racy access to the sibling list when the event is its own group leader. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation here is almost right, but fails to count the new event itself. While we fix that, also adopt the standard pattern to avoid racy access the sibling list and drop checks that are redundant with core code. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation here gets the event and its group leader mixed up, such that if the group leader belongs to a different PMU, the set_bit() may go wildly out of bounds. While we fix that, also adopt the standard pattern to avoid racy access the sibling list and drop checks that are redundant with core code. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The group validation here is OK, except for the benign issue that it will double-count an event that is its own group leader. Even though it's highly unlikely we'd ever have PMCG hardware with only one counter, let's sort that out, cleaning up some reudundant checks in the process. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The L3 driver's group validation is almost right, except for erroneously counting a software group leader - which is benign other than artificially limiting the maximum size of such a group to one less than it could be. Correct that with the now-established pattern of simply ignoring all events which do not belong to our PMU. The L2 driver gets a cleanup of some slightly suspicious logic, and both can have the same overall simplification to not duplicate things that perf core will already do, and avoid racy access to the sibling list of group leader events. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Although it is entirely benign for arm_ni_val_count_event() to count any old hardware leader/sibling as an NI event (perf core will still ultimately reject the cross-PMU group), it would still be nicer if it didn't. Stop trying to special-case software events and simply skip any event which doesn't belong to our PMU. Similarly drop the early return paths since they can almost never actually return early. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
The CCI driver only accepts events of its own type, so it is pointless to re-check the event type again further into validation. Conversely, if an event *is* for CCI but has a nonsense config, we should not return -ENOENT to potentially offer it to other PMUs. Finally it seems wrong not to count disabled events which may be enabled later. These are all artefacts left over from the original attempt to fit CCI into the arm_pmu framework; clean them up, along with the now-redundant checks for cross-PMU groups which core code will already handle (albeit not quite as the out-of-date comment says). Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
It may have been different long ago, but today it seems wrong for these drivers to skip counting disabled sibling events in group validation, given that perf_event_enable() could make them schedulable again, and thus increase the effective size of the group later. Conversely, if a sibling event is truly dead then it stands to reason that the whole group is dead, so it's not worth going to any special effort to try to squeeze in a new event that's never going to run anyway. Thus, we can simply remove all these checks. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Several drivers cannot support groups, but enforce this inconsistently (including not at all) in their event_init routines. Add a helper so that such drivers can simply and robustly check for the acceptable conditions that their event is either standalone, or the first one being added to a software-only group. In particular it took a while to see that marvell_cn10k_tad_pmu was seemingly trying to rely on the empirical behaviour of perf tool creating group leader events with disabled=1 and subsequent siblings with disabled=0. Down with this sort of thing! Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
None of these drivers are doing anything that perf_event_open() doesn't inherently do as of commit bf480f9 ("perf/core: Don't allow grouping events from different hw pmus"). While it's quite possible that they should be doing some actual validation of the schedulability of their own events within the given group, for now at least removing this redundant code makes it even clearer that they are not. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
All of these drivers copy a pattern of actively policing cross-PMU groups, which is redundant since commit bf480f9 ("perf/core: Don't allow grouping events from different hw pmus"). Clean up these checks to simplfy matters, especially for thunderx2 which can reduce right down to trivial counting. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Sampling is inherently a feature for CPU PMUs, given that the thing to be sampled is a CPU context. These days, we have many more uncore/system PMUs than CPU PMUs, so it no longer makes much sense to assume sampling support by default and force the ever-growing majority of drivers to opt out of it (or erroneously fail to). Instead, let's introduce a positive opt-in capability that's more obvious and easier to maintain. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Now that we have a well-defined cap for sampling support, clean up the remains of the mildly unintuitive and inconsistently-applied PERF_PMU_CAP_NO_INTERRUPT. Not to mention the obvious redundancy of some of these drivers still checking for sampling in event_init too. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
Only a handful of CPU PMUs accept PERF_TYPE_{RAW,HARDWARE,HW_CACHE}
events without registering themselves as PERF_TYPE_RAW in the first
place. Add an explicit opt-in for these special cases, so that we can
make life easier for every other driver (and probably also speed up the
slow-path search) by having perf_try_init_event() do the basic type
checking to cover the majority of cases.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
All these boilerplate event_init checks are now redundant. Of course many of them were already redundant, or done in the wrong order so as to be pointless, and what we don't see here is all the ones which were missing, but have now been implicitly gained thanks to some of these new core code behaviours. In summary: - event->attr.type Now only relevant to PERF_TYPE_RAW PMUs or those advertising PERF_PMU_CAP_RAW_EVENTS. - event->cpu < 0 Already rejected by perf_event_alloc() unless a task is passed, wherein that will also set PERF_ATTACH_TASK prior to reaching perf_init_event(), so is always redundant with... - PERF_ATTACH_TASK Since at least commit bd27568 ("perf: Rewrite core context handling"), only relevant to PMUs using perf_hw_context or perf_sw_context; for uncore PMUs this is covered by perf_event_alloc() again, right after perf_init_event() returns, by virtue of the same non-NULL task which caused attach_state to be set in the first place. - is_sampling_event() (and variations) Now only relevant to PMUs advertising PERF_PMU_CAP_SAMPLING. - has_branch_stack() Now doubly-illogical for PMUs which never supported sampling anyway. Signed-off-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Linux RISC-V bot <linux.riscv.bot@gmail.com>
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 1: "[01/19] perf/arm-cmn: Fix event validation" |
|
Patch 17: "[17/19] perf: Retire PERF_PMU_CAP_NO_INTERRUPT" |
|
Patch 17: "[17/19] perf: Retire PERF_PMU_CAP_NO_INTERRUPT" |
|
Patch 17: "[17/19] perf: Retire PERF_PMU_CAP_NO_INTERRUPT" |
|
Patch 17: "[17/19] perf: Retire PERF_PMU_CAP_NO_INTERRUPT" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 18: "[18/19] perf: Introduce positive capability for raw events" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
|
Patch 19: "[19/19] perf: Garbage-collect event_init checks" |
2 participants
PR for series 991172 applied to workflow__riscv__fixes
Name: perf: Rework event_init checks
URL: https://patchwork.kernel.org/project/linux-riscv/list/?series=991172
Version: 1