On 02/21/2012 07:47 AM, David Ahern wrote: - show the result: >> perf kvm-events report > > It would be nice to have example reports in this commit message. >
Okay. >> +DESCRIPTION >> +----------- >> +You can analyze some crucial kvm events and statistics with this >> +'perf kvm-events' command. Currently, vmexit, mmio and ioport events >> +are supported. > First sentence should be written in the 3rd person. eg., > This command generates a statistical analysis of KVM events. > Okay. >> +--events=<value>:: >> + events to be analyzed. Possible values: vmexit, mmio, ioport. > > Add a comment stating which event type is the default. > OK, will fix. >> >> +-k:: >> +--key=<value>:: >> + Sorting key. Possible values: sample(default, sort by samples >> number), >> +time(sort by average time). > Space before both of the '('. > Yes, will fix. >> +static const char *get_exit_reason(u64 exit_code, int isa) >> +{ >> + int table_size = ARRAY_SIZE(svm_exit_reasons); >> + struct exit_reasons_table *table = svm_exit_reasons; >> + >> + if (isa == 1) { >> + table = vmx_exit_reasons; >> + table_size = ARRAY_SIZE(vmx_exit_reasons); >> + } > Why not use globals that are set once in read_events() after looking up > cpu_isa? Then you don't have to state a preference on default init here -- > AMD or Intel. And the isa argument will not be needed here. > I agree. >> + #define DEFAULT_VCPU_NUM 32 > Why 32 for the default number of vcpus in a guest? Seems like a lot for the > typical VM. Versus something like 4 or 8. >> Hmm, since 32 is the default vcpu number in the old kernel (IIRC), but your suggestion sounds good. >> + /* Both begin and end events did not get the key. */ >> + if (!event&& key->key == INVALID_KEY) >> + return; >> + > Should not be able to get here with event unset, so the next 2 lines should > not be needed. ie., you only want to process events where the begin event was > seen in which case event is defined. In some case, the 'begin event' just records the start timestamp, the actually event is recognised in the 'end event'. Take mmio-read for example, in the old kernel, we use kvm-exit as the 'begin event' and kvm_mmio(KVM_TRACE_MMIO_READ...) is the 'end event'. >> + "perf kvm events report [<options>]", > missing '-' between kvm and events > ... >> +static const char * const kvm_events_usage[] = { >> + "perf kvm events [<options>] {record|report}", > missing '-' between kvm and events Sorry for my careless, these will be fixed in the next version. Thanks very much for your review, David! :) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html