Messages in this thread | | | Date | Tue, 21 Nov 2023 14:43:54 +0530 | Subject | Re: [V14 3/8] drivers: perf: arm_pmuv3: Enable branch stack sampling framework | From | Anshuman Khandual <> |
| |
On 11/15/23 15:07, James Clark wrote: > > > On 15/11/2023 05:44, Anshuman Khandual wrote: >> On 11/14/23 15:28, James Clark wrote: >>> >>> >>> On 14/11/2023 05:13, Anshuman Khandual wrote: >>>> Branch stack sampling support i.e capturing branch records during execution >>>> in core perf, rides along with normal HW events being scheduled on the PMU. >>>> This prepares ARMV8 PMU framework for branch stack support on relevant PMUs >>>> with required HW implementation. >>>> >>> >>> [...] >>> >>>> - All armv8pmu_branch_xxxx() stub definitions have been moved inside >>>> include/linux/perf/arm_pmuv3.h for easy access from both arm32 and >>>> arm64 platforms >>>> >>> >>> This causes lots of W=1 build errors because the prototypes are in >>> arm_pmuv3.h, but arm_brbe.c doesn't include it. >> >> I guess these are the W=1 warnings you mentioned above. >> >> drivers/perf/arm_brbe.c:11:6: warning: no previous prototype for ‘armv8pmu_branch_reset’ [-Wmissing-prototypes] >> 11 | void armv8pmu_branch_reset(void) >> | ^~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:190:6: warning: no previous prototype for ‘armv8pmu_branch_save’ [-Wmissing-prototypes] >> 190 | void armv8pmu_branch_save(struct arm_pmu *arm_pmu, void *ctx) >> | ^~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:236:6: warning: no previous prototype for ‘armv8pmu_branch_attr_valid’ [-Wmissing-prototypes] >> 236 | bool armv8pmu_branch_attr_valid(struct perf_event *event) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:269:5: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_alloc’ [-Wmissing-prototypes] >> 269 | int armv8pmu_task_ctx_cache_alloc(struct arm_pmu *arm_pmu) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:279:6: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_free’ [-Wmissing-prototypes] >> 279 | void armv8pmu_task_ctx_cache_free(struct arm_pmu *arm_pmu) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:303:6: warning: no previous prototype for ‘armv8pmu_branch_probe’ [-Wmissing-prototypes] >> 303 | void armv8pmu_branch_probe(struct arm_pmu *armpmu) >> | ^~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:449:6: warning: no previous prototype for ‘armv8pmu_branch_enable’ [-Wmissing-prototypes] >> 449 | void armv8pmu_branch_enable(struct arm_pmu *arm_pmu) >> | ^~~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:474:6: warning: no previous prototype for ‘armv8pmu_branch_disable’ [-Wmissing-prototypes] >> 474 | void armv8pmu_branch_disable(void) >> | ^~~~~~~~~~~~~~~~~~~~~~~ >> drivers/perf/arm_brbe.c:717:6: warning: no previous prototype for ‘armv8pmu_branch_read’ [-Wmissing-prototypes] >> 717 | void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event) >> >> Branch helpers are used in ARM PMU V3 driver i.e drivers/perf/arm_pmuv3.c. >> Whether the actual BRBE helper definitions, or their fallback stubs (when >> CONFIG_ARM64_BRBE is not enabled), need to be accessible from arm_pmuv3.c >> driver not from brbe.c implementations itself. >> >>> >>> It seems like the main reason you can't include arm_brbe.h in arm32 code >>> is because there are a load of inline functions and references to >>> registers in there. But these are only used in arm_brbe.c, so they don't >> >> Right, arm32 should not be exposed to BRBE internals via arm_brbe.h header. >> >>> need to be in the header anyway. >> >> Right, these are only used in arm_brbe.c >> >>> >>> If you removed the code from the header and moved it to the source file >>> you could move the brbe prototypes to the brbe header and it would be a >>> bit cleaner and more idiomatic. >> >> Alight, how about the following changes - build tested on arm32 and arm64. >> >> - Move BRBE helpers from arm_brbe.h into arm_brbe.c >> - Move armv8_pmu_xxx() declaration inside arm_brbe.h for arm64 (CONFIG_ARM64_BRBE) >> - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 (!CONFIG_ARM64_BRBE) >> - Include arm_brbe.h header both in arm_pmuv3.c and arm_brbe.c > > Agree to them all except: > > - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 > (!CONFIG_ARM64_BRBE) > > Normally you put the stubs right next to the prototypes with #else, so > in this case both would be in arm_brbe.h. Not sure what the reason for > splitting them here is? You already said "include arm_brbe.h in > arm_pmuv3.c", so that covers arm32 too.
Not any particular strong reason for the split as such, will move these stubs to the header as well. BRBE header includes <linux/perf/arm_pmu.h> which causes the following redefinition warning for the pr_fmt().
drivers/perf/arm_brbe.c:11: warning: "pr_fmt" redefined 11 | #define pr_fmt(fmt) "brbe: " fmt | In file included from ./include/linux/kernel.h:31, from ./include/linux/interrupt.h:6, from ./include/linux/perf/arm_pmu.h:11, from drivers/perf/arm_brbe.h:10, from drivers/perf/arm_brbe.c:9: ./include/linux/printk.h:345: note: this is the location of the previous definition 345 | #define pr_fmt(fmt) fmt
Although it should be okay to just drop this custom pr_fmt() from BRBE.
| |