Re: [PATCH v3 3/3] KVM: perf: kvm events analysis tool

[David Ahern <dsahern@gmail.com>]

On 02/07/2012 11:14 PM, Xiao Guangrong wrote:
> On 02/08/2012 01:55 AM, David Ahern wrote:
> 
>> On 02/07/2012 06:08 AM, Xiao Guangrong wrote:
>>> Add 'perf kvm-events' support to analyze kvm vmexit/mmio/ioport smartly
>>
>> example output?
>>
> 
> 
> You can find a example output at this website:
> 	http://lwn.net/Articles/479069/
> 
> [ Sorry, i did not put it into changlog in the v3 ]

Right, I meant add an example output to the changelog.

> 
>>> +--------
>>> +[verse]
>>> +'perf kvm-events' {record|report}
>>
>> command name is very generic -- kvm-events; but command focus is rather
>> narrow -- I/O events.
>>
> 
> 
> It is not only analysing IO events but also for other events(like vmexit,
> and other events will be added). I prefer to kvm-events :)

Same. kvm-events should be the name of the command. But from the
admittedly not thorough review of the code it seemed to be expecting I/O
(and entry/exit tracepoints as a part of that) and not easily amendable
to add new tracepoints.

---8<---

>>> +
>>> +	vcpu_event_record = zalloc(sizeof(*vcpu_event_record) * kvm_max_vcpus);
>>> +	if (!vcpu_event_record)
>>> +		die("zalloc.\n");
>>> +
>>> +	for (i = 0; i < (int)EVENTS_CACHE_SIZE; i++)
>>> +		INIT_LIST_HEAD(&kvm_events_cache[i]);
>>> +}
>>
>> Really, the caches could be tied to thread structs, and then you don't
>> need a max vcpu style allocation. more below.
>>
> 
> 
> Hmm, this cache is used to cache "event struct", this is the common resources
> for all vcpus.

But consecutive events that are analyzed together happen in the context
of a thread -- the vcpu task.

---8<---

>>> +static int kvm_events_report(int vcpu)
>>> +{
>>> +	init_kvm_event_record();
>>> +	init_kvm_tid_to_pid();
>>> +	verify_vcpu(vcpu);
>>> +	select_key();
>>> +	register_kvm_events_ops();
>>> +	setup_pager();
>>
>> I believe setup_pager is handled by perf.c
>>
> 
> 
> Hmm, i did not find it, could you please tell me where is it?
> And, setup_pager is also used in other tools such 'perf sched',
> 'perf lock'...
> 

run_builtin() --> commit_pager_choice() --> setup_pager()

It could be that the other commands need to be updated.

---8<---

>>> +	if (!strncmp(argv[0], "rec", 3))
>>> +		return kvm_events_record(argc, argv);
>>> +
>>> +	if (!strncmp(argv[0], "report", 6)) {
>>
>> exact match for 'report'?
>>
> 
> 
> It is the style in other tools, what is your suggestion?

Right, I see that. builtin-lock should be fixed.

rec is ok for record; is report123 ok for report? if not that string
should be an exact match.

David