Re: [PATCH 5/9] perf kvm: add live mode - v3

[David Ahern <dsahern@gmail.com>]

On 8/5/13 1:53 AM, Xiao Guangrong wrote:
> Hi David,
>
> Thanks for your nice job! I got some questions.
>
> On 08/03/2013 04:05 AM, David Ahern wrote:
>
>>   static int kvm_events_hash_fn(u64 key)
>>   {
>>   	return key & (EVENTS_CACHE_SIZE - 1);
>> @@ -472,7 +501,11 @@ static bool handle_end_event(struct perf_kvm_stat *kvm,
>>   	vcpu_record->last_event = NULL;
>>   	vcpu_record->start_time = 0;
>>
>> -	BUG_ON(timestamp < time_begin);
>> +	/* seems to happen once in a while during live mode */
>> +	if (timestamp < time_begin) {
>> +		pr_debug("End time before begin time; skipping event.\n");
>> +		return true;
>> +	}
>
> No idea why it can happen. :(

I saw it triggering quite often early on (last Fall when I started this) 
and I changed the BUG_ON to keep going and get the command working. It 
needs to be revisited and figured out why an end event comes before a 
begin event - might be a start up problem only. But as a start point did 
not seem to hurt to just ignore the sample.

>
>> +static bool verify_vcpu(int vcpu)
>> +{
>> +	int nr_cpus;
>> +
>> +	if (vcpu != -1 && vcpu < 0) {
>> +		pr_err("Invalid vcpu:%d.\n", vcpu);
>> +		return false;
>> +	}
>> +
>> +	nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>> +	if ((nr_cpus > 0) && (vcpu > nr_cpus - 1)) {
>> +		pr_err("Invalid vcpu:%d.\n", vcpu);
>> +		return false;
>> +	}
>
> Hmm, kvm can use more vcpus than the cpus on host.

Good point. I'll fix.

>
>> +static int kvm_events_live(struct perf_kvm_stat *kvm,
>> +			   int argc, const char **argv)
>> +{
>> +	char errbuf[BUFSIZ];
>> +	int err;
>> +
>> +	const struct option live_options[] = {
>> +		OPT_STRING('p', "pid", &kvm->opts.target.pid, "pid",
>> +			"record events on existing process id"),
>> +		OPT_STRING('t', "tid", &kvm->opts.target.tid, "tid",
>> +			"record events on existing thread id"),
>> +		OPT_STRING('C', "cpu", &kvm->opts.target.cpu_list, "cpu",
>> +			"list of host cpus to monitor"),
>> +		OPT_UINTEGER('m', "mmap-pages", &kvm->opts.mmap_pages,
>> +			"number of mmap data pages"),
>> +		OPT_INCR('v', "verbose", &verbose,
>> +			"be more verbose (show counter open errors, etc)"),
>> +		OPT_BOOLEAN('a', "all-cpus", &kvm->opts.target.system_wide,
>> +			"system-wide collection from all CPUs"),
>> +		OPT_UINTEGER('d', "display", &kvm->display_time,
>> +			"time in seconds between display updates"),
>> +		OPT_STRING(0, "event", &kvm->report_event, "report event",
>> +			"event for reporting: vmexit, mmio, ioport"),
>> +		OPT_INTEGER(0, "vcpu", &kvm->trace_vcpu,
>> +			"vcpu id to report"),
>> +		OPT_STRING('k', "key", &kvm->sort_key, "sort-key",
>> +			"key for sorting: sample(sort by samples number)"
>> +			" time (sort by avg time)"),
>
> Why we have so many parameters used for tracking. For KVM, we only need to know
> 1) which guest is tracked and 2) which vcpu in the guest is tracked and 3) what
> kind of events. no?

I should drop the cpu_list argument and tid is the same as vcpu so I can 
drop it. 'a' is implicit if pid is not given.

mmap-pages is definitely needed: a LOT of tracepoints get generated.

I'll update.

David