lkml.org 
[lkml]   [2012]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/3] KVM: perf: kvm events analysis tool


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


\
 
 \ /
  Last update: 2012-02-08 15:29    [from the cache]
©2003-2011 Jasper Spaans