[PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[Eduardo Habkost]

If kvmtrace crashes or gets killed while collecting trace data, stale
trace setup information may be enabled on the kernel, and currently
there is no way to force the previous trace setup to be overwritten
or disabled.

The following two patches against kvm-userspace will add a new
command-line option to kvmtrace (-f), that will allow the user to forcibly
clean up an existing trace setup before enabling trace.

-- 
Eduardo

[PATCH 1/2] Extract disable_trace() from cleanup_trace()

[Eduardo Habkost]

Extract the KVM_TRACE_DISABLE ioctl() call from cleanup_trace(), as
other parts of the code will use it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 user/kvmtrace.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/user/kvmtrace.c b/user/kvmtrace.c
index de3c189..5d154c1 100644
--- a/user/kvmtrace.c
+++ b/user/kvmtrace.c
@@ -460,6 +460,12 @@ static void stop_threads()
 	}
 }
 
+static void disable_trace(void)
+{
+	if (ioctl(trace_information.fd, KVM_TRACE_DISABLE) < 0)
+		perror("KVM_TRACE_DISABLE");
+}
+
 static int start_trace(void)
 {
 	int fd;
@@ -494,8 +500,7 @@ static void cleanup_trace(void)
 
 	if (trace_information.trace_started) {
 		trace_information.trace_started = 0;
-		if (ioctl(trace_information.fd, KVM_TRACE_DISABLE) < 0)
-			perror("KVM_TRACE_DISABLE");
+		disable_trace();
 	}
 
 	close(trace_information.fd);
-- 
1.5.6.rc3.11.g5f54d

[PATCH 2/2] kvmtrace: Add -f option to clean up existing trace setup

[Eduardo Habkost]

If kvmtrace crashes or gets killed while collecting trace data, stale
trace setup information may be enabled, and currently there is no way
to force the previous trace setup to be overwritten or disabled.

The -f option will allow the user to forcibly clean up an existing trace
setup before enabling trace, on those cases.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 user/kvmtrace.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/user/kvmtrace.c b/user/kvmtrace.c
index 5d154c1..f06574f 100644
--- a/user/kvmtrace.c
+++ b/user/kvmtrace.c
@@ -54,7 +54,7 @@ static char kvmtrace_version[] = "0.1";
 
 #define max(a, b)	((a) > (b) ? (a) : (b))
 
-#define S_OPTS	"r:o:w:?Vb:n:D:"
+#define S_OPTS	"r:o:w:?Vb:n:D:f"
 static struct option l_opts[] = {
 	{
 		.name = "relay",
@@ -155,6 +155,7 @@ static int stop_watch;
 static unsigned long buf_size = BUF_SIZE;
 static unsigned long buf_nr = BUF_NR;
 static unsigned int page_size;
+static int force_disable;
 
 #define for_each_cpu_online(cpu) \
 	for (cpu = 0; cpu < ncpus; cpu++)
@@ -481,6 +482,9 @@ static int start_trace(void)
 	kuts.buf_size = trace_information.buf_size = buf_size;
 	kuts.buf_nr = trace_information.buf_nr = buf_nr;
 
+	if (force_disable)
+		disable_trace();
+
 	if (ioctl(trace_information.fd , KVM_TRACE_ENABLE, &kuts) < 0) {
 		perror("KVM_TRACE_ENABLE");
 		close(fd);
@@ -600,6 +604,7 @@ static char usage_str[] = \
 	"\t-w Stop after defined time, in seconds\n" \
 	"\t-b Sub buffer size in KiB\n" \
 	"\t-n Number of sub buffers\n" \
+	"\t-f Force: cleanup previous tracing setup before enabling tracing\n" \
 	"\t-V Print program version info\n\n";
 
 static void show_usage(char *prog)
@@ -614,6 +619,9 @@ void parse_args(int argc, char **argv)
 
 	while ((c = getopt_long(argc, argv, S_OPTS, l_opts, NULL)) >= 0) {
 		switch (c) {
+		case 'f':
+			force_disable = 1;
+			break;
 		case 'r':
 			debugfs_path = optarg;
 			break;
-- 
1.5.6.rc3.11.g5f54d

Re: [PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[Glauber Costa]

On Wed, Oct 22, 2008 at 3:35 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> If kvmtrace crashes or gets killed while collecting trace data, stale
> trace setup information may be enabled on the kernel, and currently
> there is no way to force the previous trace setup to be overwritten
> or disabled.
>
> The following two patches against kvm-userspace will add a new
> command-line option to kvmtrace (-f), that will allow the user to forcibly
> clean up an existing trace setup before enabling trace.

Why can't we simply disable potential stale setups everytime, before we start?
The option would then not be needed
>
> --
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[Eduardo Habkost]

On Wed, Oct 22, 2008 at 03:42:27PM -0200, Glauber Costa wrote:
> On Wed, Oct 22, 2008 at 3:35 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > If kvmtrace crashes or gets killed while collecting trace data, stale
> > trace setup information may be enabled on the kernel, and currently
> > there is no way to force the previous trace setup to be overwritten
> > or disabled.
> >
> > The following two patches against kvm-userspace will add a new
> > command-line option to kvmtrace (-f), that will allow the user to forcibly
> > clean up an existing trace setup before enabling trace.
> 
> Why can't we simply disable potential stale setups everytime, before we start?
> The option would then not be needed

Another kvmtrace instance may be already running. Do we want to kill
its tracing setup without asking, by default?

-- 
Eduardo

Re: [PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[Glauber Costa]

On Wed, Oct 22, 2008 at 3:54 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Wed, Oct 22, 2008 at 03:42:27PM -0200, Glauber Costa wrote:
>> On Wed, Oct 22, 2008 at 3:35 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > If kvmtrace crashes or gets killed while collecting trace data, stale
>> > trace setup information may be enabled on the kernel, and currently
>> > there is no way to force the previous trace setup to be overwritten
>> > or disabled.
>> >
>> > The following two patches against kvm-userspace will add a new
>> > command-line option to kvmtrace (-f), that will allow the user to forcibly
>> > clean up an existing trace setup before enabling trace.
>>
>> Why can't we simply disable potential stale setups everytime, before we start?
>> The option would then not be needed
>
> Another kvmtrace instance may be already running. Do we want to kill
> its tracing setup without asking, by default?
do we allow more than one instance?

Note that if the answer is "yes", we also have the problem in which we
have two or three instances in flight.
Which one of them does the command kill ?

>
> --
> Eduardo
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[Eduardo Habkost]

On Wed, Oct 22, 2008 at 04:05:01PM -0200, Glauber Costa wrote:
> On Wed, Oct 22, 2008 at 3:54 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Wed, Oct 22, 2008 at 03:42:27PM -0200, Glauber Costa wrote:
> >> On Wed, Oct 22, 2008 at 3:35 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > If kvmtrace crashes or gets killed while collecting trace data, stale
> >> > trace setup information may be enabled on the kernel, and currently
> >> > there is no way to force the previous trace setup to be overwritten
> >> > or disabled.
> >> >
> >> > The following two patches against kvm-userspace will add a new
> >> > command-line option to kvmtrace (-f), that will allow the user to forcibly
> >> > clean up an existing trace setup before enabling trace.
> >>
> >> Why can't we simply disable potential stale setups everytime, before we start?
> >> The option would then not be needed
> >
> > Another kvmtrace instance may be already running. Do we want to kill
> > its tracing setup without asking, by default?
> do we allow more than one instance?

No. That's why the current behaviour is to fail when there is another
instance running.

> 
> Note that if the answer is "yes", we also have the problem in which we
> have two or three instances in flight.
> Which one of them does the command kill ?

Currently, the behaviour of ioctl(KVM_TRACE_DISABLE) when another kvmtrace
instance is still running will be:

- the previous instance will be still running, but getting no data
- the new instance will fail when trying to enable tracing (because the
  old instance is keeping the old debugfs files open)

IMO, aborting by default (and making the user aware that something is
wrong if he needs to use -f) looks better than breaking both instances.

-- 
Eduardo

Re: [PATCH 0/2] kvmtrace: Allow user to clean up stale trace setup

[Avi Kivity]

Eduardo Habkost wrote:
> If kvmtrace crashes or gets killed while collecting trace data, stale
> trace setup information may be enabled on the kernel, and currently
> there is no way to force the previous trace setup to be overwritten
> or disabled.
>
> The following two patches against kvm-userspace will add a new
> command-line option to kvmtrace (-f), that will allow the user to forcibly
> clean up an existing trace setup before enabling trace.
>   

The long term plan is to drop kvmtrace in favor of the more generic 
frameworks (which provide a common trace transport, IIRC).  Can you 
check if this has been merged (or if enough of it is in mainline)?

This is exactly the sort of issues that are best handled by a generic 
implementation.

-- 
error compiling committee.c: too many arguments to function