Re: [PATCH RFC 2/3] x86: Add Intel PT logger

[Takao Indoh <indou.takao@jp.fujitsu.com>]

On 2015/07/29 15:08, Alexander Shishkin wrote:
> Takao Indoh <indou.takao@jp.fujitsu.com> writes:
> 
>> This patch provides Intel PT logging feature. When system boots with a
>> parameter "intel_pt_log", log buffers for Intel PT are allocated and
>> logging starts, then processor flow information is written in the log
>> buffer by hardware like flight recorder. This is very helpful to
>> investigate a cause of kernel panic.
>>
>> The log buffer size is specified by the parameter
>> "intel_pt_log_buf_len=<size>". This buffer is used as circular buffer,
>> therefore old events are overwritten by new events.
> 
> [skip]
> 
>> +static void enable_pt(int enable)
>> +{
>> +	u64 ctl;
>> +
>> +	rdmsrl(MSR_IA32_RTIT_CTL, ctl);
> 
> Ideally, you shouldn't need this rdmsr(), because in this code you
> should know exactly which ctl bits you need set when you enable.
> 
>> +
>> +	if (enable)
>> +		ctl |= RTIT_CTL_TRACEEN;
>> +	else
>> +		ctl &= ~RTIT_CTL_TRACEEN;
>> +
>> +	wrmsrl(MSR_IA32_RTIT_CTL, ctl);
>> +}
> 
> But the bigger problem with this approach is that it duplicates the
> existing driver's functionality and some of the code, which just makes
> it harder to maintain amoung other things.
> 
> Instead, we should be able to do use the existing perf functionality to
> enable the system-wide tracing, so that it goes through the
> driver. Another thing to remember is that you'd also need some of the
> sideband data (vm mappings, context switches) to be able to properly
> decode the trace, which also can come from perf. And it'd also be much
> less code. The only missing piece is the code that would allocate the
> ring buffer for such events.

Alexander,

I checked perf code to find out what kinds of information are needed as
side-band data. It seems that the following two events are used.
 - sched:sched_switch
 - dummy(PERF_COUNT_SW_DUMMY)

So, what I need to do is adding kernel counter for three events
(intel_pt, sched:sched_switch, dummy). My understanding is correct?

Thanks,
Takao Indoh

> 
> Something like:
> 
> static DEFINE_PER_CPU(struct perf_event *, perf_kdump_event);
> 
> static struct perf_event_attr perf_kdump_attr;
> 
> ...
> 
> static int perf_kdump_init(void)
> {
>          struct perf_event *event;
>          int cpu;
> 
>          get_online_cpus();
>          for_each_possible_cpu(cpu) {
>                  event = perf_create_kernel_counter(&perf_kdump_attr,
>          					   cpu, NULL,
> 					           NULL, NULL);
> 
> 		...
> 
>                  ret = rb_alloc_kernel(event, perf_kdump_data_size, perf_kdump_aux_size);
> 
>                  ...
>                  
>                  per_cpu(perf_kdump_event, cpu) = event;
>          }
>          put_online_cpus();
> }
>