Re: [PATCH, v4] perf: Fix capabilities bitfield compatibility in'struct perf_event_mmap_page'

[Ingo Molnar]

* Vince Weaver <vince@xxxxxxxxxx> wrote:

> On Thu, 19 Sep 2013, Ingo Molnar wrote:
> 
> > To solve all that make this change explicit, detectable and self-contained,
> > by iterating the ABI the following way:
> > 
> >  - Always clear bit 0, and rename it to usrpage->cap_bit0, to at least not
> >    confuse old user-space binaries. RDPMC will be marked as unavailable
> >    to old binaries but that's within the ABI, this is a capability bit.
> > 
> >  - Rename bit 1 to ->cap_bit0_is_deprecated and always set it to 1, so new
> >    libraries can reliably detect that bit 0 is deprecated and perma-zero
> >    without having to check the kernel version.
> > 
> >  - Use bits 2, 3, 4 for the newly defined, correct functionality:
> > 
> > 	cap_user_rdpmc		: 1, /* The RDPMC instruction can be used to read counts */
> > 	cap_user_time		: 1, /* The time_* fields are used */
> > 	cap_user_time_zero	: 1, /* The time_zero field is used */
> > 
> 
> So let me think through the possible combinations:
> 
> OLD-KERNEL OLD-HEADER
>    + cap_usr_rdpmc and cap_usr_time map to the same bit
>    + In general for kernels from 3.4 to 3.11 the bit will be set
>      for all x86
> 
> NEW-KERNEL OLD-HEADER
>    + cap_usr_rdpmc (cap_bit0) and cap_usr_time (cap_bit0) both are 0
>    + code detecting cap_usr_rdpmc will probably fall back to non-rdpmc
>      even through rdpmc code is probably there and functioning
> 
> OLD-KERNEL NEW-HEADER
>    + cap_user_rdpmc and cap_user_time both set to 0
>    + cap_bit0_is_deprecated can be read; if it is 0 you can
>      read cap_bit0, and if it is 1, assume rdpmc is available
>      with the same likelyhood you could with the old header

Yes - this is the enabling vector to support old kernels (to the extent 
it's possible), in newly built libraries - without having to test for some 
explicit kernel version in the quirk code. (which is really a fragile 
concept when features/fixes get backported.)

> NEW-KERNEL NEW-HEADER
>    + Use cap_user_rdpmc and cap_user_time and everything is OK

Yes.

> >  - Rename all the bitfield names in perf_event.h to be different from the
> >    old names, to make sure it's not possible to mis-compile it
> >    accidentally with old assumptions.
> 
> Well this breaks that API, though I guess there are no guarantees there.

Not just that there are no guarantees, but without the rename old code 
with new headers could also result in hard to debug "it seems to work but 
is really broken" code.

> I guess that's intentional since it will force users to fix their code, 
> but a pain if you aren't expecting it and suddenly your project doesn't 
> compile anymore after a kernel-headers update.  Most of my code carries 
> around its own perf_event.h so I guess I'll be unaffected.

I'd expect mainly libraries to ever run into the build failure, not end 
users.

> In any case this seems to be about as reasonable a solution to this 
> problem as we can get.

Great, thanks for the review!

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/