lkml.org 
[lkml]   [2022]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] perf: Expand perf_branch_entry.type
On Tue, Feb 15, 2022 at 01:01:06PM -0600, Rob Herring wrote:
> On Fri, Feb 04, 2022 at 04:02:04PM +0000, Mark Rutland wrote:
> > On Fri, Feb 04, 2022 at 10:25:24AM +0530, Anshuman Khandual wrote:
> > > On 2/2/22 5:27 PM, Mark Rutland wrote:
> > > > On Fri, Jan 28, 2022 at 11:14:13AM +0530, Anshuman Khandual wrote:
> > > >> @@ -1370,8 +1376,8 @@ struct perf_branch_entry {
> > > >> in_tx:1, /* in transaction */
> > > >> abort:1, /* transaction abort */
> > > >> cycles:16, /* cycle count to last branch */
> > > >> - type:4, /* branch type */
> > > >> - reserved:40;
> > > >> + type:6, /* branch type */
> > > >
> > > > As above, is this a safe-change ABI-wise?
> > >
> > > If the bit fields here cannot be expanded without breaking ABI, then
> > > there is a fundamental problem. Only remaining option will be to add
> > > new fields (with new width value) which could accommodate these new
> > > required branch types.
> >
> > Unfortunately, I think expanding this does break ABI, and is a fundamental
> > problem, as:
> >
> > (a) Any new values in the expanded field will be truncated when read by old
> > userspace, and so those may be mis-reported. Maybe we're not too worried
> > about this case.
>
> 'type' or specfically branch stack is not currently supported on arm64.
> Do we expect an old userspace which this didn't work on to start working
> with a new kernel?

I agree for arm64 specifically this probably doesn't matter; I just wanted to
have a clear explanation of why this *could* be a problem, since this could
affect other architectures.

> Given at least some of the new types are arch specific, perhaps
> the existing type field should get a new 'PERF_BR_ARCH_SPECIFIC' or
> 'PERF_BR_EXTENDED' value (or use PERF_BR_UNKNOWN?) which means read a
> new 'arch_type' field.

Yup; something of that shape sounds good to me -- that was roughly what I had
suggested elsewhere.

> Another option is maybe some of these additional types just shouldn't be
> exposed to userspace? For example, are branches to FIQ useful or leaking
> any info about secure world? Debug mode branches also seem minimally
> useful to me (though I'm no expert in how this is used).

I agree; this wasn't clear to me, and regardless I think many of the types
added in the prior patch should not be generic since they're very specific to
the Arm architecture.

> > (b) Depending on how the field is placed, existing values might get stored
> > differently. This could break any mismatched combination of
> > {old,new}-kernel and {old,new}-userspace.
> >
> > In practice, I think this means that this is broken for BE, and happens to
> > work for LE, but I don't know how bitfields are defined for each
> > architecture, so there could be other brokenness.
> >
> > Consider the test case below:
>
> [...]
>
> > ... where the low bits of the field have moved, and so this is broken even for
> > existing values!
>
> So that is a separate issue to be fixed and not directly related to the
> size of 'type'.

I agree if you moved the entire field that's broken everywhere, but in this
case it *is* directly related to the size changing. In my example the meaning
of specific bits changed *because* the size of the field changed and in BE that
meant the low bits of the field moved, even though the field started at the
same position.

> Looks like it needs similar '#if
> defined(__LITTLE_ENDIAN_BITFIELD)' treatment as some of the other struct
> bitfields. Though somehow BE PPC hasn't had issues?

IIRC there were recent problems in this area, and I think historically we've
broken ABI and people only noticed much later.

Thanks,
Mark.

\
 
 \ /
  Last update: 2022-02-15 20:47    [W:0.051 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site