* [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
@ 2019-10-30 3:46 Shaokun Zhang
2019-10-30 13:34 ` Richard Henderson
2019-10-31 16:08 ` Will Deacon
0 siblings, 2 replies; 18+ messages in thread
From: Shaokun Zhang @ 2019-10-30 3:46 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Shaokun Zhang, Mark Rutland, Will Deacon
For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
&armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
to simplify the armv8_pmuv3_event_attrs.
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
---
arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
1 file changed, 65 insertions(+), 124 deletions(-)
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index a0b4f1bca491..d0f084939bcf 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
}
#define ARMV8_EVENT_ATTR(name, config) \
- PMU_EVENT_ATTR(name, armv8_event_attr_##name, \
- config, armv8pmu_events_sysfs_show)
-
-ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR);
-ARMV8_EVENT_ATTR(l1i_cache_refill, ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL);
-ARMV8_EVENT_ATTR(l1i_tlb_refill, ARMV8_PMUV3_PERFCTR_L1I_TLB_REFILL);
-ARMV8_EVENT_ATTR(l1d_cache_refill, ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL);
-ARMV8_EVENT_ATTR(l1d_cache, ARMV8_PMUV3_PERFCTR_L1D_CACHE);
-ARMV8_EVENT_ATTR(l1d_tlb_refill, ARMV8_PMUV3_PERFCTR_L1D_TLB_REFILL);
-ARMV8_EVENT_ATTR(ld_retired, ARMV8_PMUV3_PERFCTR_LD_RETIRED);
-ARMV8_EVENT_ATTR(st_retired, ARMV8_PMUV3_PERFCTR_ST_RETIRED);
-ARMV8_EVENT_ATTR(inst_retired, ARMV8_PMUV3_PERFCTR_INST_RETIRED);
-ARMV8_EVENT_ATTR(exc_taken, ARMV8_PMUV3_PERFCTR_EXC_TAKEN);
-ARMV8_EVENT_ATTR(exc_return, ARMV8_PMUV3_PERFCTR_EXC_RETURN);
-ARMV8_EVENT_ATTR(cid_write_retired, ARMV8_PMUV3_PERFCTR_CID_WRITE_RETIRED);
-ARMV8_EVENT_ATTR(pc_write_retired, ARMV8_PMUV3_PERFCTR_PC_WRITE_RETIRED);
-ARMV8_EVENT_ATTR(br_immed_retired, ARMV8_PMUV3_PERFCTR_BR_IMMED_RETIRED);
-ARMV8_EVENT_ATTR(br_return_retired, ARMV8_PMUV3_PERFCTR_BR_RETURN_RETIRED);
-ARMV8_EVENT_ATTR(unaligned_ldst_retired, ARMV8_PMUV3_PERFCTR_UNALIGNED_LDST_RETIRED);
-ARMV8_EVENT_ATTR(br_mis_pred, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED);
-ARMV8_EVENT_ATTR(cpu_cycles, ARMV8_PMUV3_PERFCTR_CPU_CYCLES);
-ARMV8_EVENT_ATTR(br_pred, ARMV8_PMUV3_PERFCTR_BR_PRED);
-ARMV8_EVENT_ATTR(mem_access, ARMV8_PMUV3_PERFCTR_MEM_ACCESS);
-ARMV8_EVENT_ATTR(l1i_cache, ARMV8_PMUV3_PERFCTR_L1I_CACHE);
-ARMV8_EVENT_ATTR(l1d_cache_wb, ARMV8_PMUV3_PERFCTR_L1D_CACHE_WB);
-ARMV8_EVENT_ATTR(l2d_cache, ARMV8_PMUV3_PERFCTR_L2D_CACHE);
-ARMV8_EVENT_ATTR(l2d_cache_refill, ARMV8_PMUV3_PERFCTR_L2D_CACHE_REFILL);
-ARMV8_EVENT_ATTR(l2d_cache_wb, ARMV8_PMUV3_PERFCTR_L2D_CACHE_WB);
-ARMV8_EVENT_ATTR(bus_access, ARMV8_PMUV3_PERFCTR_BUS_ACCESS);
-ARMV8_EVENT_ATTR(memory_error, ARMV8_PMUV3_PERFCTR_MEMORY_ERROR);
-ARMV8_EVENT_ATTR(inst_spec, ARMV8_PMUV3_PERFCTR_INST_SPEC);
-ARMV8_EVENT_ATTR(ttbr_write_retired, ARMV8_PMUV3_PERFCTR_TTBR_WRITE_RETIRED);
-ARMV8_EVENT_ATTR(bus_cycles, ARMV8_PMUV3_PERFCTR_BUS_CYCLES);
-/* Don't expose the chain event in /sys, since it's useless in isolation */
-ARMV8_EVENT_ATTR(l1d_cache_allocate, ARMV8_PMUV3_PERFCTR_L1D_CACHE_ALLOCATE);
-ARMV8_EVENT_ATTR(l2d_cache_allocate, ARMV8_PMUV3_PERFCTR_L2D_CACHE_ALLOCATE);
-ARMV8_EVENT_ATTR(br_retired, ARMV8_PMUV3_PERFCTR_BR_RETIRED);
-ARMV8_EVENT_ATTR(br_mis_pred_retired, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED_RETIRED);
-ARMV8_EVENT_ATTR(stall_frontend, ARMV8_PMUV3_PERFCTR_STALL_FRONTEND);
-ARMV8_EVENT_ATTR(stall_backend, ARMV8_PMUV3_PERFCTR_STALL_BACKEND);
-ARMV8_EVENT_ATTR(l1d_tlb, ARMV8_PMUV3_PERFCTR_L1D_TLB);
-ARMV8_EVENT_ATTR(l1i_tlb, ARMV8_PMUV3_PERFCTR_L1I_TLB);
-ARMV8_EVENT_ATTR(l2i_cache, ARMV8_PMUV3_PERFCTR_L2I_CACHE);
-ARMV8_EVENT_ATTR(l2i_cache_refill, ARMV8_PMUV3_PERFCTR_L2I_CACHE_REFILL);
-ARMV8_EVENT_ATTR(l3d_cache_allocate, ARMV8_PMUV3_PERFCTR_L3D_CACHE_ALLOCATE);
-ARMV8_EVENT_ATTR(l3d_cache_refill, ARMV8_PMUV3_PERFCTR_L3D_CACHE_REFILL);
-ARMV8_EVENT_ATTR(l3d_cache, ARMV8_PMUV3_PERFCTR_L3D_CACHE);
-ARMV8_EVENT_ATTR(l3d_cache_wb, ARMV8_PMUV3_PERFCTR_L3D_CACHE_WB);
-ARMV8_EVENT_ATTR(l2d_tlb_refill, ARMV8_PMUV3_PERFCTR_L2D_TLB_REFILL);
-ARMV8_EVENT_ATTR(l2i_tlb_refill, ARMV8_PMUV3_PERFCTR_L2I_TLB_REFILL);
-ARMV8_EVENT_ATTR(l2d_tlb, ARMV8_PMUV3_PERFCTR_L2D_TLB);
-ARMV8_EVENT_ATTR(l2i_tlb, ARMV8_PMUV3_PERFCTR_L2I_TLB);
-ARMV8_EVENT_ATTR(remote_access, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS);
-ARMV8_EVENT_ATTR(ll_cache, ARMV8_PMUV3_PERFCTR_LL_CACHE);
-ARMV8_EVENT_ATTR(ll_cache_miss, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS);
-ARMV8_EVENT_ATTR(dtlb_walk, ARMV8_PMUV3_PERFCTR_DTLB_WALK);
-ARMV8_EVENT_ATTR(itlb_walk, ARMV8_PMUV3_PERFCTR_ITLB_WALK);
-ARMV8_EVENT_ATTR(ll_cache_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_RD);
-ARMV8_EVENT_ATTR(ll_cache_miss_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS_RD);
-ARMV8_EVENT_ATTR(remote_access_rd, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS_RD);
-ARMV8_EVENT_ATTR(sample_pop, ARMV8_SPE_PERFCTR_SAMPLE_POP);
-ARMV8_EVENT_ATTR(sample_feed, ARMV8_SPE_PERFCTR_SAMPLE_FEED);
-ARMV8_EVENT_ATTR(sample_filtrate, ARMV8_SPE_PERFCTR_SAMPLE_FILTRATE);
-ARMV8_EVENT_ATTR(sample_collision, ARMV8_SPE_PERFCTR_SAMPLE_COLLISION);
+ (&((struct perf_pmu_events_attr[]) { \
+ { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
+ .id = config, } \
+ })[0].attr.attr)
static struct attribute *armv8_pmuv3_event_attrs[] = {
- &armv8_event_attr_sw_incr.attr.attr,
- &armv8_event_attr_l1i_cache_refill.attr.attr,
- &armv8_event_attr_l1i_tlb_refill.attr.attr,
- &armv8_event_attr_l1d_cache_refill.attr.attr,
- &armv8_event_attr_l1d_cache.attr.attr,
- &armv8_event_attr_l1d_tlb_refill.attr.attr,
- &armv8_event_attr_ld_retired.attr.attr,
- &armv8_event_attr_st_retired.attr.attr,
- &armv8_event_attr_inst_retired.attr.attr,
- &armv8_event_attr_exc_taken.attr.attr,
- &armv8_event_attr_exc_return.attr.attr,
- &armv8_event_attr_cid_write_retired.attr.attr,
- &armv8_event_attr_pc_write_retired.attr.attr,
- &armv8_event_attr_br_immed_retired.attr.attr,
- &armv8_event_attr_br_return_retired.attr.attr,
- &armv8_event_attr_unaligned_ldst_retired.attr.attr,
- &armv8_event_attr_br_mis_pred.attr.attr,
- &armv8_event_attr_cpu_cycles.attr.attr,
- &armv8_event_attr_br_pred.attr.attr,
- &armv8_event_attr_mem_access.attr.attr,
- &armv8_event_attr_l1i_cache.attr.attr,
- &armv8_event_attr_l1d_cache_wb.attr.attr,
- &armv8_event_attr_l2d_cache.attr.attr,
- &armv8_event_attr_l2d_cache_refill.attr.attr,
- &armv8_event_attr_l2d_cache_wb.attr.attr,
- &armv8_event_attr_bus_access.attr.attr,
- &armv8_event_attr_memory_error.attr.attr,
- &armv8_event_attr_inst_spec.attr.attr,
- &armv8_event_attr_ttbr_write_retired.attr.attr,
- &armv8_event_attr_bus_cycles.attr.attr,
- &armv8_event_attr_l1d_cache_allocate.attr.attr,
- &armv8_event_attr_l2d_cache_allocate.attr.attr,
- &armv8_event_attr_br_retired.attr.attr,
- &armv8_event_attr_br_mis_pred_retired.attr.attr,
- &armv8_event_attr_stall_frontend.attr.attr,
- &armv8_event_attr_stall_backend.attr.attr,
- &armv8_event_attr_l1d_tlb.attr.attr,
- &armv8_event_attr_l1i_tlb.attr.attr,
- &armv8_event_attr_l2i_cache.attr.attr,
- &armv8_event_attr_l2i_cache_refill.attr.attr,
- &armv8_event_attr_l3d_cache_allocate.attr.attr,
- &armv8_event_attr_l3d_cache_refill.attr.attr,
- &armv8_event_attr_l3d_cache.attr.attr,
- &armv8_event_attr_l3d_cache_wb.attr.attr,
- &armv8_event_attr_l2d_tlb_refill.attr.attr,
- &armv8_event_attr_l2i_tlb_refill.attr.attr,
- &armv8_event_attr_l2d_tlb.attr.attr,
- &armv8_event_attr_l2i_tlb.attr.attr,
- &armv8_event_attr_remote_access.attr.attr,
- &armv8_event_attr_ll_cache.attr.attr,
- &armv8_event_attr_ll_cache_miss.attr.attr,
- &armv8_event_attr_dtlb_walk.attr.attr,
- &armv8_event_attr_itlb_walk.attr.attr,
- &armv8_event_attr_ll_cache_rd.attr.attr,
- &armv8_event_attr_ll_cache_miss_rd.attr.attr,
- &armv8_event_attr_remote_access_rd.attr.attr,
- &armv8_event_attr_sample_pop.attr.attr,
- &armv8_event_attr_sample_feed.attr.attr,
- &armv8_event_attr_sample_filtrate.attr.attr,
- &armv8_event_attr_sample_collision.attr.attr,
+ ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR),
+ ARMV8_EVENT_ATTR(l1i_cache_refill, ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL),
+ ARMV8_EVENT_ATTR(l1i_tlb_refill, ARMV8_PMUV3_PERFCTR_L1I_TLB_REFILL),
+ ARMV8_EVENT_ATTR(l1d_cache_refill, ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL),
+ ARMV8_EVENT_ATTR(l1d_cache, ARMV8_PMUV3_PERFCTR_L1D_CACHE),
+ ARMV8_EVENT_ATTR(l1d_tlb_refill, ARMV8_PMUV3_PERFCTR_L1D_TLB_REFILL),
+ ARMV8_EVENT_ATTR(ld_retired, ARMV8_PMUV3_PERFCTR_LD_RETIRED),
+ ARMV8_EVENT_ATTR(st_retired, ARMV8_PMUV3_PERFCTR_ST_RETIRED),
+ ARMV8_EVENT_ATTR(inst_retired, ARMV8_PMUV3_PERFCTR_INST_RETIRED),
+ ARMV8_EVENT_ATTR(exc_taken, ARMV8_PMUV3_PERFCTR_EXC_TAKEN),
+ ARMV8_EVENT_ATTR(exc_return, ARMV8_PMUV3_PERFCTR_EXC_RETURN),
+ ARMV8_EVENT_ATTR(cid_write_retired, ARMV8_PMUV3_PERFCTR_CID_WRITE_RETIRED),
+ ARMV8_EVENT_ATTR(pc_write_retired, ARMV8_PMUV3_PERFCTR_PC_WRITE_RETIRED),
+ ARMV8_EVENT_ATTR(br_immed_retired, ARMV8_PMUV3_PERFCTR_BR_IMMED_RETIRED),
+ ARMV8_EVENT_ATTR(br_return_retired, ARMV8_PMUV3_PERFCTR_BR_RETURN_RETIRED),
+ ARMV8_EVENT_ATTR(unaligned_ldst_retired, ARMV8_PMUV3_PERFCTR_UNALIGNED_LDST_RETIRED),
+ ARMV8_EVENT_ATTR(br_mis_pred, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED),
+ ARMV8_EVENT_ATTR(cpu_cycles, ARMV8_PMUV3_PERFCTR_CPU_CYCLES),
+ ARMV8_EVENT_ATTR(br_pred, ARMV8_PMUV3_PERFCTR_BR_PRED),
+ ARMV8_EVENT_ATTR(mem_access, ARMV8_PMUV3_PERFCTR_MEM_ACCESS),
+ ARMV8_EVENT_ATTR(l1i_cache, ARMV8_PMUV3_PERFCTR_L1I_CACHE),
+ ARMV8_EVENT_ATTR(l1d_cache_wb, ARMV8_PMUV3_PERFCTR_L1D_CACHE_WB),
+ ARMV8_EVENT_ATTR(l2d_cache, ARMV8_PMUV3_PERFCTR_L2D_CACHE),
+ ARMV8_EVENT_ATTR(l2d_cache_refill, ARMV8_PMUV3_PERFCTR_L2D_CACHE_REFILL),
+ ARMV8_EVENT_ATTR(l2d_cache_wb, ARMV8_PMUV3_PERFCTR_L2D_CACHE_WB),
+ ARMV8_EVENT_ATTR(bus_access, ARMV8_PMUV3_PERFCTR_BUS_ACCESS),
+ ARMV8_EVENT_ATTR(memory_error, ARMV8_PMUV3_PERFCTR_MEMORY_ERROR),
+ ARMV8_EVENT_ATTR(inst_spec, ARMV8_PMUV3_PERFCTR_INST_SPEC),
+ ARMV8_EVENT_ATTR(ttbr_write_retired, ARMV8_PMUV3_PERFCTR_TTBR_WRITE_RETIRED),
+ ARMV8_EVENT_ATTR(bus_cycles, ARMV8_PMUV3_PERFCTR_BUS_CYCLES),
+ /* Don't expose the chain event in /sys, since it's useless in isolation */
+ ARMV8_EVENT_ATTR(l1d_cache_allocate, ARMV8_PMUV3_PERFCTR_L1D_CACHE_ALLOCATE),
+ ARMV8_EVENT_ATTR(l2d_cache_allocate, ARMV8_PMUV3_PERFCTR_L2D_CACHE_ALLOCATE),
+ ARMV8_EVENT_ATTR(br_retired, ARMV8_PMUV3_PERFCTR_BR_RETIRED),
+ ARMV8_EVENT_ATTR(br_mis_pred_retired, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED_RETIRED),
+ ARMV8_EVENT_ATTR(stall_frontend, ARMV8_PMUV3_PERFCTR_STALL_FRONTEND),
+ ARMV8_EVENT_ATTR(stall_backend, ARMV8_PMUV3_PERFCTR_STALL_BACKEND),
+ ARMV8_EVENT_ATTR(l1d_tlb, ARMV8_PMUV3_PERFCTR_L1D_TLB),
+ ARMV8_EVENT_ATTR(l1i_tlb, ARMV8_PMUV3_PERFCTR_L1I_TLB),
+ ARMV8_EVENT_ATTR(l2i_cache, ARMV8_PMUV3_PERFCTR_L2I_CACHE),
+ ARMV8_EVENT_ATTR(l2i_cache_refill, ARMV8_PMUV3_PERFCTR_L2I_CACHE_REFILL),
+ ARMV8_EVENT_ATTR(l3d_cache_allocate, ARMV8_PMUV3_PERFCTR_L3D_CACHE_ALLOCATE),
+ ARMV8_EVENT_ATTR(l3d_cache_refill, ARMV8_PMUV3_PERFCTR_L3D_CACHE_REFILL),
+ ARMV8_EVENT_ATTR(l3d_cache, ARMV8_PMUV3_PERFCTR_L3D_CACHE),
+ ARMV8_EVENT_ATTR(l3d_cache_wb, ARMV8_PMUV3_PERFCTR_L3D_CACHE_WB),
+ ARMV8_EVENT_ATTR(l2d_tlb_refill, ARMV8_PMUV3_PERFCTR_L2D_TLB_REFILL),
+ ARMV8_EVENT_ATTR(l2i_tlb_refill, ARMV8_PMUV3_PERFCTR_L2I_TLB_REFILL),
+ ARMV8_EVENT_ATTR(l2d_tlb, ARMV8_PMUV3_PERFCTR_L2D_TLB),
+ ARMV8_EVENT_ATTR(l2i_tlb, ARMV8_PMUV3_PERFCTR_L2I_TLB),
+ ARMV8_EVENT_ATTR(remote_access, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS),
+ ARMV8_EVENT_ATTR(ll_cache, ARMV8_PMUV3_PERFCTR_LL_CACHE),
+ ARMV8_EVENT_ATTR(ll_cache_miss, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS),
+ ARMV8_EVENT_ATTR(dtlb_walk, ARMV8_PMUV3_PERFCTR_DTLB_WALK),
+ ARMV8_EVENT_ATTR(itlb_walk, ARMV8_PMUV3_PERFCTR_ITLB_WALK),
+ ARMV8_EVENT_ATTR(ll_cache_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_RD),
+ ARMV8_EVENT_ATTR(ll_cache_miss_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS_RD),
+ ARMV8_EVENT_ATTR(remote_access_rd, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS_RD),
+ ARMV8_EVENT_ATTR(sample_pop, ARMV8_SPE_PERFCTR_SAMPLE_POP),
+ ARMV8_EVENT_ATTR(sample_feed, ARMV8_SPE_PERFCTR_SAMPLE_FEED),
+ ARMV8_EVENT_ATTR(sample_filtrate, ARMV8_SPE_PERFCTR_SAMPLE_FILTRATE),
+ ARMV8_EVENT_ATTR(sample_collision, ARMV8_SPE_PERFCTR_SAMPLE_COLLISION),
NULL,
};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-10-30 3:46 [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes Shaokun Zhang
@ 2019-10-30 13:34 ` Richard Henderson
2019-10-31 7:55 ` Mark Rutland
2019-10-31 8:45 ` Shaokun Zhang
2019-10-31 16:08 ` Will Deacon
1 sibling, 2 replies; 18+ messages in thread
From: Richard Henderson @ 2019-10-30 13:34 UTC (permalink / raw)
To: Shaokun Zhang, linux-arm-kernel; +Cc: Mark Rutland, Will Deacon
On 10/30/19 4:46 AM, Shaokun Zhang wrote:
> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> to simplify the armv8_pmuv3_event_attrs.
...
> #define ARMV8_EVENT_ATTR(name, config) \
> + (&((struct perf_pmu_events_attr[]) { \
> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> + .id = config, } \
> + })[0].attr.attr)
>
> static struct attribute *armv8_pmuv3_event_attrs[] = {
> + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR),
You do realize this creates complete perf_pmu_events_attr structures, most of
which is unused and unreachable, right?
Also, why not take the opportunity to assert that the armv8_pmuv3_event_attrs
array cannot get out of sync with the ARMV8_PMUV3_* defines?
Slightly better would seem to be
#define ARMV8_EVENT_ATTR(name, config) \
[config] = &((struct device_attribute) \
__ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL)).attr
though I'm not sure why __ATTR is particularly desired above
#define ARMV8_EVENT_ATTR(name, config) \
[config] = &(struct attribute){ \
.name = __stringify(name), \
.mode = 0444, \
}
r~
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-10-30 13:34 ` Richard Henderson
@ 2019-10-31 7:55 ` Mark Rutland
2019-10-31 8:45 ` Shaokun Zhang
1 sibling, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2019-10-31 7:55 UTC (permalink / raw)
To: Richard Henderson; +Cc: Shaokun Zhang, Will Deacon, linux-arm-kernel
On Wed, Oct 30, 2019 at 02:34:37PM +0100, Richard Henderson wrote:
> On 10/30/19 4:46 AM, Shaokun Zhang wrote:
> > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> > to simplify the armv8_pmuv3_event_attrs.
> ...
> > #define ARMV8_EVENT_ATTR(name, config) \
> > + (&((struct perf_pmu_events_attr[]) { \
> > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > + .id = config, } \
> > + })[0].attr.attr)
> >
> > static struct attribute *armv8_pmuv3_event_attrs[] = {
> > + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR),
>
> You do realize this creates complete perf_pmu_events_attr structures, most of
> which is unused and unreachable, right?
In armv8pmu_events_sysfs_show() we use container_of() to access the
perf_pmu_events_attr, and extracts the id field:
| static ssize_t
| armv8pmu_events_sysfs_show(struct device *dev,
| struct device_attribute *attr, char *page)
| {
| struct perf_pmu_events_attr *pmu_attr;
|
| pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
|
| return sprintf(page, "event=0x%03llx\n", pmu_attr->id);
| }
> Also, why not take the opportunity to assert that the armv8_pmuv3_event_attrs
> array cannot get out of sync with the ARMV8_PMUV3_* defines?
>
> Slightly better would seem to be
>
> #define ARMV8_EVENT_ATTR(name, config) \
> [config] = &((struct device_attribute) \
> __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL)).attr
I'm not sure I follow. This is not equivalent, and you're using the config
field in a very different way -- that's not an index in the parent array in the
current code. How do you expect armv8pmu_events_sysfs_show to get the config
value in this case?
>
> though I'm not sure why __ATTR is particularly desired above
>
> #define ARMV8_EVENT_ATTR(name, config) \
> [config] = &(struct attribute){ \
> .name = __stringify(name), \
> .mode = 0444, \
> }
Using __ATTR is consistent with other drivers, so I don't see a reason to
change that unless there's a significant simplification, or a functional
improvement
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-10-30 13:34 ` Richard Henderson
2019-10-31 7:55 ` Mark Rutland
@ 2019-10-31 8:45 ` Shaokun Zhang
1 sibling, 0 replies; 18+ messages in thread
From: Shaokun Zhang @ 2019-10-31 8:45 UTC (permalink / raw)
To: Richard Henderson, linux-arm-kernel; +Cc: Mark Rutland, Will Deacon
Hi Richard,
Thanks your comments and Mark has helped to reply some.
On 2019/10/30 21:34, Richard Henderson wrote:
> On 10/30/19 4:46 AM, Shaokun Zhang wrote:
>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>> to simplify the armv8_pmuv3_event_attrs.
> ...
>> #define ARMV8_EVENT_ATTR(name, config) \
>> + (&((struct perf_pmu_events_attr[]) { \
>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>> + .id = config, } \
>> + })[0].attr.attr)
>>
>> static struct attribute *armv8_pmuv3_event_attrs[] = {
>> + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR),
>
> You do realize this creates complete perf_pmu_events_attr structures, most of
> which is unused and unreachable, right?
>
> Also, why not take the opportunity to assert that the armv8_pmuv3_event_attrs
> array cannot get out of sync with the ARMV8_PMUV3_* defines?
>
For my initial purpose: remove the &armv8_event_attr_xx.attr.attr and only
maintain the armv8_pmuv3_event_attrs array directly when we want to add one new
PMU event. For example:
#define ARMV8_PMUV3_PERFCTR_OP_RETIRED 0x3A
.....
static struct attribute *armv8_pmuv3_event_attrs[] = {
......
ARMV8_EVENT_ATTR(op_retired, ARMV8_PMUV3_PERFCTR_OP_RETIRED),
NULL,
};
Thanks,
Shaokun
> Slightly better would seem to be
>
> #define ARMV8_EVENT_ATTR(name, config) \
> [config] = &((struct device_attribute) \
> __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL)).attr
>
> though I'm not sure why __ATTR is particularly desired above
>
> #define ARMV8_EVENT_ATTR(name, config) \
> [config] = &(struct attribute){ \
> .name = __stringify(name), \
> .mode = 0444, \
> }
>
>
> r~
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-10-30 3:46 [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes Shaokun Zhang
2019-10-30 13:34 ` Richard Henderson
@ 2019-10-31 16:08 ` Will Deacon
2019-11-01 3:45 ` Shaokun Zhang
2019-11-01 8:53 ` Mark Rutland
1 sibling, 2 replies; 18+ messages in thread
From: Will Deacon @ 2019-10-31 16:08 UTC (permalink / raw)
To: Shaokun Zhang; +Cc: Mark Rutland, linux-arm-kernel
On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> to simplify the armv8_pmuv3_event_attrs.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> ---
> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
> 1 file changed, 65 insertions(+), 124 deletions(-)
>
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index a0b4f1bca491..d0f084939bcf 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
> }
[...]
> + (&((struct perf_pmu_events_attr[]) { \
> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> + .id = config, } \
> + })[0].attr.attr)
I don't get the need for the array here. Why can't you do:
(&((struct perf_pmu_events_attr) {
.attr = ...,
.id = ...,
}).attr.attr)
?
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-10-31 16:08 ` Will Deacon
@ 2019-11-01 3:45 ` Shaokun Zhang
2019-11-01 8:53 ` Mark Rutland
1 sibling, 0 replies; 18+ messages in thread
From: Shaokun Zhang @ 2019-11-01 3:45 UTC (permalink / raw)
To: Will Deacon; +Cc: Mark Rutland, linux-arm-kernel
Hi Will,
On 2019/11/1 0:08, Will Deacon wrote:
> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>> to simplify the armv8_pmuv3_event_attrs.
>>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> ---
>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index a0b4f1bca491..d0f084939bcf 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>> }
>
> [...]
>
>> + (&((struct perf_pmu_events_attr[]) { \
>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>> + .id = config, } \
>> + })[0].attr.attr)
>
> I don't get the need for the array here. Why can't you do:
>
> (&((struct perf_pmu_events_attr) {
> .attr = ...,
> .id = ...,
> }).attr.attr)
>
> ?
I try it and there is a compiler error:
zhangshaokun@ubuntu:~/linux$ make Image -j64
CALL scripts/atomic/check-atomics.sh
CALL scripts/checksyscalls.sh
CHK include/generated/compile.h
CC arch/arm64/kernel/perf_event.o
arch/arm64/kernel/perf_event.c:162:13: error: unknown field ‘attr’ specified in initializer
(&((struct perf_pmu_events_attr) { \
^
arch/arm64/kernel/perf_event.c:168:2: note: in expansion of macro ‘ARMV8_EVENT_ATTR’
ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR),
^
arch/arm64/kernel/perf_event.c:162:13: warning: braces around scalar initializer
(&((struct perf_pmu_events_attr) { \
^
arch/arm64/kernel/perf_event.c:168:2: note: in expansion of macro ‘ARMV8_EVENT_ATTR’
ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR),
It seems that it became completely anonymous and the compiler can't find its member.
Thanks,
Shaokun
>
> Will
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-10-31 16:08 ` Will Deacon
2019-11-01 3:45 ` Shaokun Zhang
@ 2019-11-01 8:53 ` Mark Rutland
2019-11-01 10:36 ` Will Deacon
1 sibling, 1 reply; 18+ messages in thread
From: Mark Rutland @ 2019-11-01 8:53 UTC (permalink / raw)
To: Will Deacon; +Cc: Shaokun Zhang, linux-arm-kernel
On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
> > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> > to simplify the armv8_pmuv3_event_attrs.
> >
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> > ---
> > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
> > 1 file changed, 65 insertions(+), 124 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > index a0b4f1bca491..d0f084939bcf 100644
> > --- a/arch/arm64/kernel/perf_event.c
> > +++ b/arch/arm64/kernel/perf_event.c
> > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
> > }
>
> [...]
>
> > + (&((struct perf_pmu_events_attr[]) { \
> > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > + .id = config, } \
> > + })[0].attr.attr)
>
> I don't get the need for the array here. Why can't you do:
>
> (&((struct perf_pmu_events_attr) {
> .attr = ...,
> .id = ...,
> }).attr.attr)
You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
#define ARMV8_EVENT_ATTR(name, config) \
(&((struct perf_pmu_events_attr) { \
.attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
.id = config, \
}.attr.attr))
... which compiles for me.
It'd be worth checking that yields a working data structure at runtime.
I'm not sure why I did the array hack in the other PMU drivers -- looks like we
can simplify those assuming this works. :)
THanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 8:53 ` Mark Rutland
@ 2019-11-01 10:36 ` Will Deacon
2019-11-01 10:54 ` Robin Murphy
2019-11-01 14:32 ` Mark Rutland
0 siblings, 2 replies; 18+ messages in thread
From: Will Deacon @ 2019-11-01 10:36 UTC (permalink / raw)
To: Mark Rutland; +Cc: Shaokun Zhang, linux-arm-kernel
On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
> > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
> > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> > > to simplify the armv8_pmuv3_event_attrs.
> > >
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> > > ---
> > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
> > > 1 file changed, 65 insertions(+), 124 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > > index a0b4f1bca491..d0f084939bcf 100644
> > > --- a/arch/arm64/kernel/perf_event.c
> > > +++ b/arch/arm64/kernel/perf_event.c
> > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
> > > }
> >
> > [...]
> >
> > > + (&((struct perf_pmu_events_attr[]) { \
> > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > + .id = config, } \
> > > + })[0].attr.attr)
> >
> > I don't get the need for the array here. Why can't you do:
> >
> > (&((struct perf_pmu_events_attr) {
> > .attr = ...,
> > .id = ...,
> > }).attr.attr)
>
> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>
> #define ARMV8_EVENT_ATTR(name, config) \
> (&((struct perf_pmu_events_attr) { \
> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> .id = config, \
> }.attr.attr))
>
> ... which compiles for me.
Weird, the following compiles fine for me with both GCC and clang:
#define ARMV8_EVENT_ATTR(name, config) \
(&((struct perf_pmu_events_attr) { \
.attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
.id = config, \
}).attr.attr)
> It'd be worth checking that yields a working data structure at runtime.
...and perf list works as expected.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 10:36 ` Will Deacon
@ 2019-11-01 10:54 ` Robin Murphy
2019-11-01 10:55 ` Will Deacon
2019-11-01 14:32 ` Mark Rutland
1 sibling, 1 reply; 18+ messages in thread
From: Robin Murphy @ 2019-11-01 10:54 UTC (permalink / raw)
To: Will Deacon, Mark Rutland; +Cc: Shaokun Zhang, linux-arm-kernel
On 2019-11-01 10:36 am, Will Deacon wrote:
> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>>>> to simplify the armv8_pmuv3_event_attrs.
>>>>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>> ---
>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>>>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>>> index a0b4f1bca491..d0f084939bcf 100644
>>>> --- a/arch/arm64/kernel/perf_event.c
>>>> +++ b/arch/arm64/kernel/perf_event.c
>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>>>> }
>>>
>>> [...]
>>>
>>>> + (&((struct perf_pmu_events_attr[]) { \
>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>> + .id = config, } \
>>>> + })[0].attr.attr)
>>>
>>> I don't get the need for the array here. Why can't you do:
>>>
>>> (&((struct perf_pmu_events_attr) {
>>> .attr = ...,
>>> .id = ...,
>>> }).attr.attr)
>>
>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>>
>> #define ARMV8_EVENT_ATTR(name, config) \
>> (&((struct perf_pmu_events_attr) { \
>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>> .id = config, \
>> }.attr.attr))
>>
>> ... which compiles for me.
>
> Weird, the following compiles fine for me with both GCC and clang:
>
> #define ARMV8_EVENT_ATTR(name, config) \
> (&((struct perf_pmu_events_attr) { \
> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> .id = config, \
> }).attr.attr)
You know that the expressions are equivalent because unary "&" has lower
precedence than ".", right? ;)
Robin.
>> It'd be worth checking that yields a working data structure at runtime.
>
> ...and perf list works as expected.
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 10:54 ` Robin Murphy
@ 2019-11-01 10:55 ` Will Deacon
2019-11-01 11:11 ` Robin Murphy
2019-11-04 1:02 ` Shaokun Zhang
0 siblings, 2 replies; 18+ messages in thread
From: Will Deacon @ 2019-11-01 10:55 UTC (permalink / raw)
To: Robin Murphy; +Cc: Mark Rutland, Shaokun Zhang, linux-arm-kernel
On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
> On 2019-11-01 10:36 am, Will Deacon wrote:
> > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
> > > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
> > > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
> > > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> > > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> > > > > to simplify the armv8_pmuv3_event_attrs.
> > > > >
> > > > > Cc: Will Deacon <will@kernel.org>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> > > > > ---
> > > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
> > > > > 1 file changed, 65 insertions(+), 124 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > > > > index a0b4f1bca491..d0f084939bcf 100644
> > > > > --- a/arch/arm64/kernel/perf_event.c
> > > > > +++ b/arch/arm64/kernel/perf_event.c
> > > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
> > > > > }
> > > >
> > > > [...]
> > > >
> > > > > + (&((struct perf_pmu_events_attr[]) { \
> > > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > > + .id = config, } \
> > > > > + })[0].attr.attr)
> > > >
> > > > I don't get the need for the array here. Why can't you do:
> > > >
> > > > (&((struct perf_pmu_events_attr) {
> > > > .attr = ...,
> > > > .id = ...,
> > > > }).attr.attr)
> > >
> > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
> > >
> > > #define ARMV8_EVENT_ATTR(name, config) \
> > > (&((struct perf_pmu_events_attr) { \
> > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > .id = config, \
> > > }.attr.attr))
> > > ... which compiles for me.
> >
> > Weird, the following compiles fine for me with both GCC and clang:
> >
> > #define ARMV8_EVENT_ATTR(name, config) \
> > (&((struct perf_pmu_events_attr) { \
> > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > .id = config, \
> > }).attr.attr)
>
> You know that the expressions are equivalent because unary "&" has lower
> precedence than ".", right? ;)
Right, which is why it's weird that Shaokun claims that the version I posted
doesn't compile. I assume it didn't build for Mark either, hence his extra
brackets.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 10:55 ` Will Deacon
@ 2019-11-01 11:11 ` Robin Murphy
2019-11-01 14:36 ` Mark Rutland
2019-11-04 2:02 ` Shaokun Zhang
2019-11-04 1:02 ` Shaokun Zhang
1 sibling, 2 replies; 18+ messages in thread
From: Robin Murphy @ 2019-11-01 11:11 UTC (permalink / raw)
To: Will Deacon; +Cc: Mark Rutland, Shaokun Zhang, linux-arm-kernel
On 2019-11-01 10:55 am, Will Deacon wrote:
> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
>> On 2019-11-01 10:36 am, Will Deacon wrote:
>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>>>>>> to simplify the armv8_pmuv3_event_attrs.
>>>>>>
>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>>>> ---
>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>>>>> index a0b4f1bca491..d0f084939bcf 100644
>>>>>> --- a/arch/arm64/kernel/perf_event.c
>>>>>> +++ b/arch/arm64/kernel/perf_event.c
>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>>>>>> }
>>>>>
>>>>> [...]
>>>>>
>>>>>> + (&((struct perf_pmu_events_attr[]) { \
>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>>> + .id = config, } \
>>>>>> + })[0].attr.attr)
>>>>>
>>>>> I don't get the need for the array here. Why can't you do:
>>>>>
>>>>> (&((struct perf_pmu_events_attr) {
>>>>> .attr = ...,
>>>>> .id = ...,
>>>>> }).attr.attr)
>>>>
>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>>>>
>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>> (&((struct perf_pmu_events_attr) { \
>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>> .id = config, \
>>>> }.attr.attr))
>>>> ... which compiles for me.
>>>
>>> Weird, the following compiles fine for me with both GCC and clang:
>>>
>>> #define ARMV8_EVENT_ATTR(name, config) \
>>> (&((struct perf_pmu_events_attr) { \
>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>> .id = config, \
>>> }).attr.attr)
>>
>> You know that the expressions are equivalent because unary "&" has lower
>> precedence than ".", right? ;)
>
> Right, which is why it's weird that Shaokun claims that the version I posted
> doesn't compile. I assume it didn't build for Mark either, hence his extra
> brackets.
Because different compilers have different ideas of whether "obj" is a
valid thing to dereference at all, regardless of where you put
parentheses. From what I remember, the array trick was the only way to
convince older GCCs to treat the floating struct initialiser as an
actual object definition. I guess newer versions are a bit more lenient.
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 11:11 ` Robin Murphy
@ 2019-11-01 14:36 ` Mark Rutland
2019-11-01 16:05 ` Will Deacon
2019-11-04 2:02 ` Shaokun Zhang
1 sibling, 1 reply; 18+ messages in thread
From: Mark Rutland @ 2019-11-01 14:36 UTC (permalink / raw)
To: Robin Murphy; +Cc: Shaokun Zhang, Will Deacon, linux-arm-kernel
On Fri, Nov 01, 2019 at 11:11:49AM +0000, Robin Murphy wrote:
> On 2019-11-01 10:55 am, Will Deacon wrote:
> > On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
> > > On 2019-11-01 10:36 am, Will Deacon wrote:
> > > > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
> > > > > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
> > > > > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
> > > > > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> > > > > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> > > > > > > to simplify the armv8_pmuv3_event_attrs.
> > > > > > >
> > > > > > > Cc: Will Deacon <will@kernel.org>
> > > > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> > > > > > > ---
> > > > > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
> > > > > > > 1 file changed, 65 insertions(+), 124 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > > > > > > index a0b4f1bca491..d0f084939bcf 100644
> > > > > > > --- a/arch/arm64/kernel/perf_event.c
> > > > > > > +++ b/arch/arm64/kernel/perf_event.c
> > > > > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
> > > > > > > }
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > + (&((struct perf_pmu_events_attr[]) { \
> > > > > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > > > > + .id = config, } \
> > > > > > > + })[0].attr.attr)
> > > > > >
> > > > > > I don't get the need for the array here. Why can't you do:
> > > > > >
> > > > > > (&((struct perf_pmu_events_attr) {
> > > > > > .attr = ...,
> > > > > > .id = ...,
> > > > > > }).attr.attr)
> > > > >
> > > > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
> > > > >
> > > > > #define ARMV8_EVENT_ATTR(name, config) \
> > > > > (&((struct perf_pmu_events_attr) { \
> > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > > .id = config, \
> > > > > }.attr.attr))
> > > > > ... which compiles for me.
> > > >
> > > > Weird, the following compiles fine for me with both GCC and clang:
> > > >
> > > > #define ARMV8_EVENT_ATTR(name, config) \
> > > > (&((struct perf_pmu_events_attr) { \
> > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > .id = config, \
> > > > }).attr.attr)
> > >
> > > You know that the expressions are equivalent because unary "&" has lower
> > > precedence than ".", right? ;)
> >
> > Right, which is why it's weird that Shaokun claims that the version I posted
> > doesn't compile. I assume it didn't build for Mark either, hence his extra
> > brackets.
I must've meessed up locally -- sorry for the noise.
> Because different compilers have different ideas of whether "obj" is a valid
> thing to dereference at all, regardless of where you put parentheses. From
> what I remember, the array trick was the only way to convince older GCCs to
> treat the floating struct initialiser as an actual object definition. I
> guess newer versions are a bit more lenient.
I strongly suspect Will's (much cleaner) version would work with those older
compilers too, and I just didn't know what I was doing ~8 years ago when I came
up with the trick.
I can have a go with my toolchain museum on Monday; if old GCCs are happy we
can clean up the other instances of the trick to be much more legible.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 14:36 ` Mark Rutland
@ 2019-11-01 16:05 ` Will Deacon
0 siblings, 0 replies; 18+ messages in thread
From: Will Deacon @ 2019-11-01 16:05 UTC (permalink / raw)
To: Mark Rutland; +Cc: Shaokun Zhang, Robin Murphy, linux-arm-kernel
On Fri, Nov 01, 2019 at 02:36:03PM +0000, Mark Rutland wrote:
> On Fri, Nov 01, 2019 at 11:11:49AM +0000, Robin Murphy wrote:
> > On 2019-11-01 10:55 am, Will Deacon wrote:
> > > On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
> > > > On 2019-11-01 10:36 am, Will Deacon wrote:
> > > > > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
> > > > > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
> > > > > >
> > > > > > #define ARMV8_EVENT_ATTR(name, config) \
> > > > > > (&((struct perf_pmu_events_attr) { \
> > > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > > > .id = config, \
> > > > > > }.attr.attr))
> > > > > > ... which compiles for me.
> > > > >
> > > > > Weird, the following compiles fine for me with both GCC and clang:
> > > > >
> > > > > #define ARMV8_EVENT_ATTR(name, config) \
> > > > > (&((struct perf_pmu_events_attr) { \
> > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > > .id = config, \
> > > > > }).attr.attr)
> > > >
> > > > You know that the expressions are equivalent because unary "&" has lower
> > > > precedence than ".", right? ;)
> > >
> > > Right, which is why it's weird that Shaokun claims that the version I posted
> > > doesn't compile. I assume it didn't build for Mark either, hence his extra
> > > brackets.
>
> I must've meessed up locally -- sorry for the noise.
>
> > Because different compilers have different ideas of whether "obj" is a valid
> > thing to dereference at all, regardless of where you put parentheses. From
> > what I remember, the array trick was the only way to convince older GCCs to
> > treat the floating struct initialiser as an actual object definition. I
> > guess newer versions are a bit more lenient.
>
> I strongly suspect Will's (much cleaner) version would work with those older
> compilers too, and I just didn't know what I was doing ~8 years ago when I came
> up with the trick.
>
> I can have a go with my toolchain museum on Monday; if old GCCs are happy we
> can clean up the other instances of the trick to be much more legible.
I've thrown it into -next to see how it gets on.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 11:11 ` Robin Murphy
2019-11-01 14:36 ` Mark Rutland
@ 2019-11-04 2:02 ` Shaokun Zhang
2019-11-04 3:25 ` Shaokun Zhang
1 sibling, 1 reply; 18+ messages in thread
From: Shaokun Zhang @ 2019-11-04 2:02 UTC (permalink / raw)
To: Robin Murphy, Will Deacon; +Cc: Mark Rutland, linux-arm-kernel
Hi Robin,
On 2019/11/1 19:11, Robin Murphy wrote:
> On 2019-11-01 10:55 am, Will Deacon wrote:
>> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
>>> On 2019-11-01 10:36 am, Will Deacon wrote:
>>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
>>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
>>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>>>>>>> to simplify the armv8_pmuv3_event_attrs.
>>>>>>>
>>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>>>>> ---
>>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>>>>>> index a0b4f1bca491..d0f084939bcf 100644
>>>>>>> --- a/arch/arm64/kernel/perf_event.c
>>>>>>> +++ b/arch/arm64/kernel/perf_event.c
>>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>>>>>>> }
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> + (&((struct perf_pmu_events_attr[]) { \
>>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>>>> + .id = config, } \
>>>>>>> + })[0].attr.attr)
>>>>>>
>>>>>> I don't get the need for the array here. Why can't you do:
>>>>>>
>>>>>> (&((struct perf_pmu_events_attr) {
>>>>>> .attr = ...,
>>>>>> .id = ...,
>>>>>> }).attr.attr)
>>>>>
>>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>>>>>
>>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>>> (&((struct perf_pmu_events_attr) { \
>>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>> .id = config, \
>>>>> }.attr.attr))
>>>>> ... which compiles for me.
>>>>
>>>> Weird, the following compiles fine for me with both GCC and clang:
>>>>
>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>> (&((struct perf_pmu_events_attr) { \
>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>> .id = config, \
>>>> }).attr.attr)
>>>
>>> You know that the expressions are equivalent because unary "&" has lower
>>> precedence than ".", right? ;)
>>
>> Right, which is why it's weird that Shaokun claims that the version I posted
>> doesn't compile. I assume it didn't build for Mark either, hence his extra
>> brackets.
>
> Because different compilers have different ideas of whether "obj" is a valid thing to dereference at all, regardless of where you put parentheses. From what I remember, the array trick was the only way to convince older GCCs to treat the floating struct initialiser as an actual object definition. I guess newer versions are a bit more lenient.
>
Thanks for your detailed explanations, sounds great! Both GCC 5.4 and 7.3 are
unhappy without the array trick.
Thanks,
Shaokun
> Robin.
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-04 2:02 ` Shaokun Zhang
@ 2019-11-04 3:25 ` Shaokun Zhang
0 siblings, 0 replies; 18+ messages in thread
From: Shaokun Zhang @ 2019-11-04 3:25 UTC (permalink / raw)
To: Robin Murphy, Will Deacon; +Cc: Mark Rutland, linux-arm-kernel
Hi Robin,
Sorry for the noise, it works on both GCC 5.4 and 7.3 using Will's method.
Thanks,
Shaokun
On 2019/11/4 10:02, Shaokun Zhang wrote:
> Hi Robin,
>
> On 2019/11/1 19:11, Robin Murphy wrote:
>> On 2019-11-01 10:55 am, Will Deacon wrote:
>>> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
>>>> On 2019-11-01 10:36 am, Will Deacon wrote:
>>>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
>>>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
>>>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>>>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>>>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>>>>>>>> to simplify the armv8_pmuv3_event_attrs.
>>>>>>>>
>>>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>>>>>> ---
>>>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>>>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>>>>>>> index a0b4f1bca491..d0f084939bcf 100644
>>>>>>>> --- a/arch/arm64/kernel/perf_event.c
>>>>>>>> +++ b/arch/arm64/kernel/perf_event.c
>>>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>>>>>>>> }
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> + (&((struct perf_pmu_events_attr[]) { \
>>>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>>>>> + .id = config, } \
>>>>>>>> + })[0].attr.attr)
>>>>>>>
>>>>>>> I don't get the need for the array here. Why can't you do:
>>>>>>>
>>>>>>> (&((struct perf_pmu_events_attr) {
>>>>>>> .attr = ...,
>>>>>>> .id = ...,
>>>>>>> }).attr.attr)
>>>>>>
>>>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>>>>>>
>>>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>>>> (&((struct perf_pmu_events_attr) { \
>>>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>>> .id = config, \
>>>>>> }.attr.attr))
>>>>>> ... which compiles for me.
>>>>>
>>>>> Weird, the following compiles fine for me with both GCC and clang:
>>>>>
>>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>>> (&((struct perf_pmu_events_attr) { \
>>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>> .id = config, \
>>>>> }).attr.attr)
>>>>
>>>> You know that the expressions are equivalent because unary "&" has lower
>>>> precedence than ".", right? ;)
>>>
>>> Right, which is why it's weird that Shaokun claims that the version I posted
>>> doesn't compile. I assume it didn't build for Mark either, hence his extra
>>> brackets.
>>
>> Because different compilers have different ideas of whether "obj" is a valid thing to dereference at all, regardless of where you put parentheses. From what I remember, the array trick was the only way to convince older GCCs to treat the floating struct initialiser as an actual object definition. I guess newer versions are a bit more lenient.
>>
>
> Thanks for your detailed explanations, sounds great! Both GCC 5.4 and 7.3 are
> unhappy without the array trick.
>
> Thanks,
> Shaokun
>
>> Robin.
>>
>> .
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 10:55 ` Will Deacon
2019-11-01 11:11 ` Robin Murphy
@ 2019-11-04 1:02 ` Shaokun Zhang
2019-11-04 3:18 ` Shaokun Zhang
1 sibling, 1 reply; 18+ messages in thread
From: Shaokun Zhang @ 2019-11-04 1:02 UTC (permalink / raw)
To: Will Deacon, Robin Murphy; +Cc: Mark Rutland, linux-arm-kernel
Hi Will,
On 2019/11/1 18:55, Will Deacon wrote:
> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
>> On 2019-11-01 10:36 am, Will Deacon wrote:
>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>>>>>> to simplify the armv8_pmuv3_event_attrs.
>>>>>>
>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>>>> ---
>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>>>>> index a0b4f1bca491..d0f084939bcf 100644
>>>>>> --- a/arch/arm64/kernel/perf_event.c
>>>>>> +++ b/arch/arm64/kernel/perf_event.c
>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>>>>>> }
>>>>>
>>>>> [...]
>>>>>
>>>>>> + (&((struct perf_pmu_events_attr[]) { \
>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>>> + .id = config, } \
>>>>>> + })[0].attr.attr)
>>>>>
>>>>> I don't get the need for the array here. Why can't you do:
>>>>>
>>>>> (&((struct perf_pmu_events_attr) {
>>>>> .attr = ...,
>>>>> .id = ...,
>>>>> }).attr.attr)
>>>>
>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>>>>
>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>> (&((struct perf_pmu_events_attr) { \
>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>> .id = config, \
>>>> }.attr.attr))
>>>> ... which compiles for me.
>>>
>>> Weird, the following compiles fine for me with both GCC and clang:
>>>
>>> #define ARMV8_EVENT_ATTR(name, config) \
>>> (&((struct perf_pmu_events_attr) { \
>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>> .id = config, \
>>> }).attr.attr)
>>
>> You know that the expressions are equivalent because unary "&" has lower
>> precedence than ".", right? ;)
>
> Right, which is why it's weird that Shaokun claims that the version I posted
> doesn't compile. I assume it didn't build for Mark either, hence his extra
The GCC version is gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609.
It's a little old ;-)
Thanks,
Shaokun
> brackets.
>
> Will
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-04 1:02 ` Shaokun Zhang
@ 2019-11-04 3:18 ` Shaokun Zhang
0 siblings, 0 replies; 18+ messages in thread
From: Shaokun Zhang @ 2019-11-04 3:18 UTC (permalink / raw)
To: Will Deacon, Robin Murphy; +Cc: Mark Rutland, linux-arm-kernel
Hi Will,
On 2019/11/4 9:02, Shaokun Zhang wrote:
> Hi Will,
>
> On 2019/11/1 18:55, Will Deacon wrote:
>> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote:
>>> On 2019-11-01 10:36 am, Will Deacon wrote:
>>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
>>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
>>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
>>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
>>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
>>>>>>> to simplify the armv8_pmuv3_event_attrs.
>>>>>>>
>>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>>>>> ---
>>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
>>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>>>>>>> index a0b4f1bca491..d0f084939bcf 100644
>>>>>>> --- a/arch/arm64/kernel/perf_event.c
>>>>>>> +++ b/arch/arm64/kernel/perf_event.c
>>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
>>>>>>> }
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> + (&((struct perf_pmu_events_attr[]) { \
>>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>>>> + .id = config, } \
>>>>>>> + })[0].attr.attr)
>>>>>>
>>>>>> I don't get the need for the array here. Why can't you do:
>>>>>>
>>>>>> (&((struct perf_pmu_events_attr) {
>>>>>> .attr = ...,
>>>>>> .id = ...,
>>>>>> }).attr.attr)
>>>>>
>>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
>>>>>
>>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>>> (&((struct perf_pmu_events_attr) { \
>>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>>> .id = config, \
>>>>> }.attr.attr))
>>>>> ... which compiles for me.
>>>>
>>>> Weird, the following compiles fine for me with both GCC and clang:
>>>>
>>>> #define ARMV8_EVENT_ATTR(name, config) \
>>>> (&((struct perf_pmu_events_attr) { \
>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
>>>> .id = config, \
>>>> }).attr.attr)
>>>
>>> You know that the expressions are equivalent because unary "&" has lower
>>> precedence than ".", right? ;)
>>
>> Right, which is why it's weird that Shaokun claims that the version I posted
>> doesn't compile. I assume it didn't build for Mark either, hence his extra
>
> The GCC version is gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609.
> It's a little old ;-)
Sorry for my noise, it's my mistake that I forgot to remove the brackets when deleted the array.
#define ARMV8_EVENT_ATTR(name, config) \
- (&((struct perf_pmu_events_attr[]) { \
+ (&((struct perf_pmu_events_attr) { \
{ .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
.id = config, } \
- })[0].attr.attr)
+ }).attr.attr)
It works now if I follow your proposal completely.:-P
Thanks,
Shaokun
>
> Thanks,
> Shaokun
>
>> brackets.
>>
>> Will
>>
>> .
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
2019-11-01 10:36 ` Will Deacon
2019-11-01 10:54 ` Robin Murphy
@ 2019-11-01 14:32 ` Mark Rutland
1 sibling, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2019-11-01 14:32 UTC (permalink / raw)
To: Will Deacon; +Cc: Shaokun Zhang, linux-arm-kernel
On Fri, Nov 01, 2019 at 10:36:17AM +0000, Will Deacon wrote:
> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote:
> > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote:
> > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote:
> > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and
> > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR
> > > > to simplify the armv8_pmuv3_event_attrs.
> > > >
> > > > Cc: Will Deacon <will@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> > > > ---
> > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++---------------------------
> > > > 1 file changed, 65 insertions(+), 124 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> > > > index a0b4f1bca491..d0f084939bcf 100644
> > > > --- a/arch/arm64/kernel/perf_event.c
> > > > +++ b/arch/arm64/kernel/perf_event.c
> > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev,
> > > > }
> > >
> > > [...]
> > >
> > > > + (&((struct perf_pmu_events_attr[]) { \
> > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > > > + .id = config, } \
> > > > + })[0].attr.attr)
> > >
> > > I don't get the need for the array here. Why can't you do:
> > >
> > > (&((struct perf_pmu_events_attr) {
> > > .attr = ...,
> > > .id = ...,
> > > }).attr.attr)
> >
> > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e.
> >
> > #define ARMV8_EVENT_ATTR(name, config) \
> > (&((struct perf_pmu_events_attr) { \
> > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> > .id = config, \
> > }.attr.attr))
> >
> > ... which compiles for me.
>
> Weird, the following compiles fine for me with both GCC and clang:
>
> #define ARMV8_EVENT_ATTR(name, config) \
> (&((struct perf_pmu_events_attr) { \
> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \
> .id = config, \
> }).attr.attr)
Works for me, too (I'm using the kernel.org crosstool GCC 8.1.0).
I must've messed up locally such that I had (&obj).attr.attr.
> > It'd be worth checking that yields a working data structure at runtime.
>
> ...and perf list works as expected.
Perfect.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-11-04 3:25 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-30 3:46 [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes Shaokun Zhang
2019-10-30 13:34 ` Richard Henderson
2019-10-31 7:55 ` Mark Rutland
2019-10-31 8:45 ` Shaokun Zhang
2019-10-31 16:08 ` Will Deacon
2019-11-01 3:45 ` Shaokun Zhang
2019-11-01 8:53 ` Mark Rutland
2019-11-01 10:36 ` Will Deacon
2019-11-01 10:54 ` Robin Murphy
2019-11-01 10:55 ` Will Deacon
2019-11-01 11:11 ` Robin Murphy
2019-11-01 14:36 ` Mark Rutland
2019-11-01 16:05 ` Will Deacon
2019-11-04 2:02 ` Shaokun Zhang
2019-11-04 3:25 ` Shaokun Zhang
2019-11-04 1:02 ` Shaokun Zhang
2019-11-04 3:18 ` Shaokun Zhang
2019-11-01 14:32 ` Mark Rutland
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.