| Message ID | 20231009043724.175100-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: perf: arm_pmuv3: Update 'pmc_width' based on actual HW event width | expand |
On 09/10/2023 05:37, Anshuman Khandual wrote: > This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's > width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. > Might be worth adding why this is needed or what the actual effect is. > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > This applies on v6.6-rc5. > > drivers/perf/arm_pmuv3.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index fe4db1831662..94723d00548e 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, > if (userpg->cap_user_rdpmc) { > if (event->hw.flags & ARMPMU_EVT_64BIT) > userpg->pmc_width = 64; > + else if (event->hw.flags & ARMPMU_EVT_63BIT) > + userpg->pmc_width = 63; > + else if (event->hw.flags & ARMPMU_EVT_47BIT) > + userpg->pmc_width = 47; Although it doesn't explicitly say it, the bit of the docs about pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this is always either 64 or 32. Now that this isn't the case it could mislead someone in userspace that they don't have to handle the now arbitrary bit widths rather than just whole bytes/ints. I think the fix is as simple as adding something like "the width may not match the requested value or necessarily be a multiple of 8". Unless we think this is already widely known and I suppose we could leave it as is. (The existing bit in perf that uses it already handles it correctly). > else > userpg->pmc_width = 32; > }
Hi James, On 10/9/23 15:13, James Clark wrote: > > > On 09/10/2023 05:37, Anshuman Khandual wrote: >> This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's >> width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. >> > > Might be worth adding why this is needed or what the actual effect is. To have correct 'pmc_width' visible to the user space ? > >> Cc: Will Deacon <will@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> This applies on v6.6-rc5. >> >> drivers/perf/arm_pmuv3.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c >> index fe4db1831662..94723d00548e 100644 >> --- a/drivers/perf/arm_pmuv3.c >> +++ b/drivers/perf/arm_pmuv3.c >> @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, >> if (userpg->cap_user_rdpmc) { >> if (event->hw.flags & ARMPMU_EVT_64BIT) >> userpg->pmc_width = 64; >> + else if (event->hw.flags & ARMPMU_EVT_63BIT) >> + userpg->pmc_width = 63; >> + else if (event->hw.flags & ARMPMU_EVT_47BIT) >> + userpg->pmc_width = 47; > > Although it doesn't explicitly say it, the bit of the docs about > pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this > is always either 64 or 32. Now that this isn't the case it could mislead > someone in userspace that they don't have to handle the now arbitrary > bit widths rather than just whole bytes/ints. Are you suggesting that the user space would not handle pmc_width correctly , once it deviates from a whole bytes/ints format ? In that case user space handling might need some fixing. > > I think the fix is as simple as adding something like "the width may not > match the requested value or necessarily be a multiple of 8". Unless we > think this is already widely known and I suppose we could leave it as > is. (The existing bit in perf that uses it already handles it correctly). This is from perf_event_mmap_page definition where it does not assert the width to be multiple of bytes or ints. Hence the assumption should not be made into the user space tools. /* * If cap_user_rdpmc this field provides the bit-width of the value * read using the rdpmc() or equivalent instruction. This can be used * to sign extend the result like: * * pmc <<= 64 - width; * pmc >>= 64 - width; // signed shift right * count += pmc; */ __u16 pmc_width; Moreover, on x86 too 'userpg->pmc_width' gets assigned to different values although multiple of 8. userpg->pmc_width = x86_pmu.cntval_bits arch/x86/events/amd/core.c: .cntval_bits = 48 arch/x86/events/intel/knc.c: .cntval_bits = 40 arch/x86/events/intel/p6.c: .cntval_bits = 32 > >> else >> userpg->pmc_width = 32; >> }
On 10/10/2023 04:03, Anshuman Khandual wrote: > Hi James, > > On 10/9/23 15:13, James Clark wrote: >> >> >> On 09/10/2023 05:37, Anshuman Khandual wrote: >>> This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's >>> width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. >>> >> >> Might be worth adding why this is needed or what the actual effect is. > > To have correct 'pmc_width' visible to the user space ? Well yeah, but for example I didn't know what that was. And it's not clear why it needs updating at this point in time without a link to any other commit or relevant section from the Arm ARM. So I had a kind of a "why now" question. "To have correct 'pmc_width' visible to the user space" is definitely more of a what than a why. > >> >>> Cc: Will Deacon <will@kernel.org> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>> --- >>> This applies on v6.6-rc5. >>> >>> drivers/perf/arm_pmuv3.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c >>> index fe4db1831662..94723d00548e 100644 >>> --- a/drivers/perf/arm_pmuv3.c >>> +++ b/drivers/perf/arm_pmuv3.c >>> @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, >>> if (userpg->cap_user_rdpmc) { >>> if (event->hw.flags & ARMPMU_EVT_64BIT) >>> userpg->pmc_width = 64; >>> + else if (event->hw.flags & ARMPMU_EVT_63BIT) >>> + userpg->pmc_width = 63; >>> + else if (event->hw.flags & ARMPMU_EVT_47BIT) >>> + userpg->pmc_width = 47; >> >> Although it doesn't explicitly say it, the bit of the docs about >> pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this >> is always either 64 or 32. Now that this isn't the case it could mislead >> someone in userspace that they don't have to handle the now arbitrary >> bit widths rather than just whole bytes/ints. > > Are you suggesting that the user space would not handle pmc_width correctly > , once it deviates from a whole bytes/ints format ? In that case user space > handling might need some fixing. > Not really, I'm just suggesting that anyone writing a new tool and only reading the docs could make that assumption. Seeing as only 32 and 64 bit options are mentioned. So it's more to avoid misleading someone in the future than about fixing any existing code, as updating the docs wouldn't have that effect. >> >> I think the fix is as simple as adding something like "the width may not >> match the requested value or necessarily be a multiple of 8". Unless we >> think this is already widely known and I suppose we could leave it as >> is. (The existing bit in perf that uses it already handles it correctly). > > This is from perf_event_mmap_page definition where it does not assert the > width to be multiple of bytes or ints. Hence the assumption should not be > made into the user space tools. > Yeah I know its already ok for Perf which is why I mentioned it. But there are more tools out there than Perf, and ones that don't even exist yet, which people would normally read the documentation before writing. > /* > * If cap_user_rdpmc this field provides the bit-width of the value > * read using the rdpmc() or equivalent instruction. This can be used > * to sign extend the result like: > * > * pmc <<= 64 - width; > * pmc >>= 64 - width; // signed shift right > * count += pmc; > */ > __u16 pmc_width; > > Moreover, on x86 too 'userpg->pmc_width' gets assigned to different values > although multiple of 8. > > userpg->pmc_width = x86_pmu.cntval_bits > arch/x86/events/amd/core.c: .cntval_bits = 48 > arch/x86/events/intel/knc.c: .cntval_bits = 40 > arch/x86/events/intel/p6.c: .cntval_bits = 32 > >> >>> else >>> userpg->pmc_width = 32; >>> }
On 10/10/2023 09:28, James Clark wrote: > > > On 10/10/2023 04:03, Anshuman Khandual wrote: >> Hi James, >> >> On 10/9/23 15:13, James Clark wrote: >>> >>> >>> On 09/10/2023 05:37, Anshuman Khandual wrote: >>>> This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's >>>> width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. >>>> >>> >>> Might be worth adding why this is needed or what the actual effect is. >> >> To have correct 'pmc_width' visible to the user space ? > > Well yeah, but for example I didn't know what that was. And it's not > clear why it needs updating at this point in time without a link to any > other commit or relevant section from the Arm ARM. So I had a kind of a > "why now" question. > > "To have correct 'pmc_width' visible to the user space" is definitely > more of a what than a why. > If anything this should at least have a fixes: tag on it. If you're saying that it's now correct. >> >>> >>>> Cc: Will Deacon <will@kernel.org> >>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Cc: linux-kernel@vger.kernel.org >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>> --- >>>> This applies on v6.6-rc5. >>>> >>>> drivers/perf/arm_pmuv3.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c >>>> index fe4db1831662..94723d00548e 100644 >>>> --- a/drivers/perf/arm_pmuv3.c >>>> +++ b/drivers/perf/arm_pmuv3.c >>>> @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, >>>> if (userpg->cap_user_rdpmc) { >>>> if (event->hw.flags & ARMPMU_EVT_64BIT) >>>> userpg->pmc_width = 64; >>>> + else if (event->hw.flags & ARMPMU_EVT_63BIT) >>>> + userpg->pmc_width = 63; >>>> + else if (event->hw.flags & ARMPMU_EVT_47BIT) >>>> + userpg->pmc_width = 47; >>> >>> Although it doesn't explicitly say it, the bit of the docs about >>> pmc_width in Documentation/arch/arm64/perf.rst loosely implies that this >>> is always either 64 or 32. Now that this isn't the case it could mislead >>> someone in userspace that they don't have to handle the now arbitrary >>> bit widths rather than just whole bytes/ints. >> >> Are you suggesting that the user space would not handle pmc_width correctly >> , once it deviates from a whole bytes/ints format ? In that case user space >> handling might need some fixing. >> > > Not really, I'm just suggesting that anyone writing a new tool and only > reading the docs could make that assumption. Seeing as only 32 and 64 > bit options are mentioned. So it's more to avoid misleading someone in > the future than about fixing any existing code, as updating the docs > wouldn't have that effect. > >>> >>> I think the fix is as simple as adding something like "the width may not >>> match the requested value or necessarily be a multiple of 8". Unless we >>> think this is already widely known and I suppose we could leave it as >>> is. (The existing bit in perf that uses it already handles it correctly). >> >> This is from perf_event_mmap_page definition where it does not assert the >> width to be multiple of bytes or ints. Hence the assumption should not be >> made into the user space tools. >> > > Yeah I know its already ok for Perf which is why I mentioned it. But > there are more tools out there than Perf, and ones that don't even exist > yet, which people would normally read the documentation before writing. > >> /* >> * If cap_user_rdpmc this field provides the bit-width of the value >> * read using the rdpmc() or equivalent instruction. This can be used >> * to sign extend the result like: >> * >> * pmc <<= 64 - width; >> * pmc >>= 64 - width; // signed shift right >> * count += pmc; >> */ >> __u16 pmc_width; >> >> Moreover, on x86 too 'userpg->pmc_width' gets assigned to different values >> although multiple of 8. >> >> userpg->pmc_width = x86_pmu.cntval_bits >> arch/x86/events/amd/core.c: .cntval_bits = 48 >> arch/x86/events/intel/knc.c: .cntval_bits = 40 >> arch/x86/events/intel/p6.c: .cntval_bits = 32 >> >>> >>>> else >>>> userpg->pmc_width = 32; >>>> } > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 09/10/2023 5:37 am, Anshuman Khandual wrote: > This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's > width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. > > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > This applies on v6.6-rc5. > > drivers/perf/arm_pmuv3.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index fe4db1831662..94723d00548e 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, > if (userpg->cap_user_rdpmc) { > if (event->hw.flags & ARMPMU_EVT_64BIT) > userpg->pmc_width = 64; > + else if (event->hw.flags & ARMPMU_EVT_63BIT) > + userpg->pmc_width = 63; > + else if (event->hw.flags & ARMPMU_EVT_47BIT) > + userpg->pmc_width = 47; I think this will give the wrong behaviour in at least some cases - if the user has not requested a "long" event which would lead to ARMPMU_EVT_64BIT being set, we will bias the values to only count 32 effective bits regardless of how wide the physical counters are. I believe this is what "the valid width of the counter" in the documentation is trying to refer to. Thanks, Robin. > else > userpg->pmc_width = 32; > }
On Mon, Oct 09, 2023 at 10:07:24AM +0530, Anshuman Khandual wrote: > This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's > width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. > > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > This applies on v6.6-rc5. > > drivers/perf/arm_pmuv3.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index fe4db1831662..94723d00548e 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, > if (userpg->cap_user_rdpmc) { > if (event->hw.flags & ARMPMU_EVT_64BIT) > userpg->pmc_width = 64; > + else if (event->hw.flags & ARMPMU_EVT_63BIT) > + userpg->pmc_width = 63; > + else if (event->hw.flags & ARMPMU_EVT_47BIT) > + userpg->pmc_width = 47; The PMUv3 driver *never* uses 63-bit or 47-bit counters, so the PMUv3 driver doesn't need this. The ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT flags are only used by the Apple PMU driver, which doesn't support user access, and similarly doesn't need to handle this. This code is not necessary as the two new branches can never run. It will also confuse people into thinking the PMUv3 driver can use these widths when it cannot. NAK to this patch for the reasons above. Mark > else > userpg->pmc_width = 32; > } > -- > 2.25.1 >
diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c index fe4db1831662..94723d00548e 100644 --- a/drivers/perf/arm_pmuv3.c +++ b/drivers/perf/arm_pmuv3.c @@ -1375,6 +1375,10 @@ void arch_perf_update_userpage(struct perf_event *event, if (userpg->cap_user_rdpmc) { if (event->hw.flags & ARMPMU_EVT_64BIT) userpg->pmc_width = 64; + else if (event->hw.flags & ARMPMU_EVT_63BIT) + userpg->pmc_width = 63; + else if (event->hw.flags & ARMPMU_EVT_47BIT) + userpg->pmc_width = 47; else userpg->pmc_width = 32; }
This updates 'perf_event_mmap_page->pmc_width' based on actual HW event's width that are currently missing i.e ARMPMU_EVT_63BIT and ARMPMU_EVT_47BIT. Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- This applies on v6.6-rc5. drivers/perf/arm_pmuv3.c | 4 ++++ 1 file changed, 4 insertions(+)