Re: [PATCH] perf_counter: extensible perf_counter_attr

[Corey Ashford <cjashfor@linux.vnet.ibm.com>]

Ingo Molnar wrote:
> * Corey Ashford <cjashfor@linux.vnet.ibm.com> wrote:
>
>   
>> If I understand you correctly, you would simply make 
>> perf_counter_attr larger every time you want to add a new 
>> attribute.  Users using the new attributes would call 
>> sys_perf_counter_open with a larger attr_size value.
>>     
>
> Yes, exactly. Basically ABIs in Linux only get extended (never 
> shrunk and never changed) so it's not like we ever want to (or can) 
> shrink the size of the structure or change its semantics.
>
> Each future extension gives the structure a new, unique size - which 
> also acts as an 'ABI version' identifier, in a pretty robust way. We 
> check this 'ABI version' (the structure size) in the kernel code so 
> it's not just a passive 'version field' thing.
>
> Here are the various compatibility variations:
>
>  - same-version kernel and user-space: they both use the same 
>    attr_size value and support the full set of features.
>
>  - old user-space running on new kernel: works fine, as the kernel 
>    will do a short copy and zero out the remaining attributes.
>
>  - new user-space running on old kernel: the kernel returns -ENOTSUP 
>    and user-space has a choice to refuse to run cleanly - or, if an 
>    old ABI version is widespread, might chose to utilize the old,
>    smaller attribute structure size field (at the cost of not using
>    new attribute features, obviously).
>
> ( Additional detail: in the size mismatch failure case the kernel 
>   should write back the supported size into attr_size, so that 
>   user-space knows which precise ABI variant it deals with on the 
>   kernel side. )
>
> This kind of ABI maintenance method has a number of substantial 
> advantages:
>
>  - It is very compatible (see above)
>
>  - It is extensible easily and in an unlimited way - we just extend 
>    the structure size.
>
>  - It is very clean on the kernel side and the user side as well,
>    because we just have a single attribute structure.
>
>  - It makes the ABI 'version' field an _active_ component of 
>    functionality - so there is no way for subtle breakages to slip 
>    in.
>
>  - New attributes are prime-time members of the attribute structure, 
>    not second-class citizens that first have to be read in via an
>    elaborate chaining mechanism at extra cost.
>
> [ If only all our syscall ABIs used this technique :-) It would be 
>   so much easier to extend syscalls cleanly - without having to go 
>   through the expensive and time-consuming process to add new
>   syscalls. ]
>   
Thanks for the detailed explanation :)

>> What about arch-dependent attributes?  Would you want to place 
>> them all in the perf_counter_attr struct?  I suppose this could be 
>> done by #include'ing an arch-specific .h file.
>>     
>
> What arch-dependent attributes are you thinking about? In the 
> perfcounters subsystem we want to support PMU and other performance 
> analysis features in a way that makes it possible for all 
> architectures to make use of them.
>
> So 'arch dependent attributes' per se are bad and against the 
> perfcounters design. "Generic perfcounter feature only supported by 
> a single architecture initially" is better.
>   
Well, I think Intel has PEBS and AMD has some similar mechanism.  I 
would guess that at some point you would want to provide access to those 
PMU features via these attributes.  Since these mechanisms are very 
chip-specific, I don't think you would want to try to create an 
arch-independent interface to them.  There may be future mechanisms that 
only make sense on one particular chip design, and would therefore not 
be a candidate for wider use, but would still make sense to provide some 
support for that mechanism via the attributes.

Did you have some different plan for PEBS  (etc.) ?

- Corey