Re: [PATCH 01/10] perf/core, x86: Add PERF_SAMPLE_LBR_TOS

["Liang, Kan" <kan.liang@linux.intel.com>]

On 10/8/2019 10:38 AM, Peter Zijlstra wrote:
> On Tue, Oct 08, 2019 at 09:53:24AM -0400, Liang, Kan wrote:
>>
>>
>> On 10/8/2019 4:31 AM, Peter Zijlstra wrote:
>>> On Mon, Oct 07, 2019 at 10:59:01AM -0700, kan.liang@linux.intel.com wrote:
>>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>>> index 61448c19a132..ee9ef0c4cb08 100644
>>>> --- a/include/linux/perf_event.h
>>>> +++ b/include/linux/perf_event.h
>>>> @@ -100,6 +100,7 @@ struct perf_raw_record {
>>>>     */
>>>>    struct perf_branch_stack {
>>>>    	__u64				nr;
>>>> +	__u64				tos;
>>>>    	struct perf_branch_entry	entries[0];
>>>>    };
>>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>>>> index bb7b271397a6..fe36ebb7dc2e 100644
>>>> --- a/include/uapi/linux/perf_event.h
>>>> +++ b/include/uapi/linux/perf_event.h
>>>> @@ -141,8 +141,9 @@ enum perf_event_sample_format {
>>>>    	PERF_SAMPLE_TRANSACTION			= 1U << 17,
>>>>    	PERF_SAMPLE_REGS_INTR			= 1U << 18,
>>>>    	PERF_SAMPLE_PHYS_ADDR			= 1U << 19,
>>>> +	PERF_SAMPLE_LBR_TOS			= 1U << 20,
>>>> -	PERF_SAMPLE_MAX = 1U << 20,		/* non-ABI */
>>>> +	PERF_SAMPLE_MAX = 1U << 21,		/* non-ABI */
>>>>    	__PERF_SAMPLE_CALLCHAIN_EARLY		= 1ULL << 63, /* non-ABI; internal use */
>>>>    };
>>>> @@ -864,6 +865,7 @@ enum perf_event_type {
>>>>    	 *	{ u64			abi; # enum perf_sample_regs_abi
>>>>    	 *	  u64			regs[weight(mask)]; } && PERF_SAMPLE_REGS_INTR
>>>>    	 *	{ u64			phys_addr;} && PERF_SAMPLE_PHYS_ADDR
>>>> +	 *	{ u64			tos;} && PERF_SAMPLE_LBR_TOS
>>>>    	 * };
>>>>    	 */
>>>>    	PERF_RECORD_SAMPLE			= 9,
>>>
>>> I have problems with the API.. You're introducing the intel specific LBR
>>> naming, and adding a whole new sample type vs extending the existing
>>> BRANCH_STACK (like you really already do with struct perf_branch_stack). >
>>> So why not add a bit to PERF_SAMPLE_BRANCH_* to request the presence of
>>> the TOS field in the PERF_SAMPLE_BRANCH_STACK output?
>>
>> We never store PERF_SAMPLE_BRANCH_* in a sample. The perf tool cannot tell
>> if the sample includes TOS field.
> 
> The perf tool bloody sets the perf_event_attr::branch_sample_type value!
> Of course it knows to expect the TOS field when it asks for it in the
> first place.
>

Users may generate the perf.data on one machine, and parse the data on 
another machine.
If the perf.data is from a new kernel with a new perf tool on one 
machine, but users have an old perf tool on another machine to parse it. 
The old perf tool doesn't know the exists of TOS field.


Thanks,
Kan

>> There will be a problem when a new perf tool parsing the data generated by
>> an old kernel.
> 
> ISTR perf stores the full perf_event_attr in the .data file these days,
> and therefore such confusion should never happen.
>