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

[Xiao Guangrong]

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. :(

> +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.

> +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?

Others look good to me. :)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/