* [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process
@ 2013-07-15 19:41 Christian Borntraeger
2013-07-16 3:05 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Christian Borntraeger @ 2013-07-15 19:41 UTC (permalink / raw)
To: aliguori; +Cc: Christian Borntraeger, Michael Mueller, qemu-devel, Lluís
From: Michael Mueller <mimu@linux.vnet.ibm.com>
When running with trace backend e.g. "simple" the writer thread needs to be
implemented in the same process context as the trace points that will be
processed. Under libvirtd control, qemu gets first started in daemonized
mode to privide its capabilities. Creating the writer thread in the initial
process context then leads to a dead lock because the thread gets termined
together with the initial parent. (-daemonize) This results in
stale qemu processes.
Fix this by deferring trace initialization.
Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
[Christian Borntraeger: white space fixes]
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
vl.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 25b8f2f..3c24108 100644
--- a/vl.c
+++ b/vl.c
@@ -3935,8 +3935,10 @@ int main(int argc, char **argv, char **envp)
qemu_set_log(mask);
}
- if (!trace_backend_init(trace_events, trace_file)) {
- exit(1);
+ if (!is_daemonized()) {
+ if (!trace_backend_init(trace_events, trace_file)) {
+ exit(1);
+ }
}
/* If no data_dir is specified then try to find it relative to the
@@ -4429,6 +4431,12 @@ int main(int argc, char **argv, char **envp)
os_setup_post();
+ if (is_daemonized()) {
+ if (!trace_backend_init(trace_events, trace_file)) {
+ exit(1);
+ }
+ }
+
main_loop();
bdrv_close_all();
pause_all_vcpus();
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process
2013-07-15 19:41 [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process Christian Borntraeger
@ 2013-07-16 3:05 ` Stefan Hajnoczi
2013-07-16 12:17 ` Michael Mueller
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-07-16 3:05 UTC (permalink / raw)
To: Christian Borntraeger; +Cc: aliguori, Lluís, Michael Mueller, qemu-devel
On Mon, Jul 15, 2013 at 09:41:19PM +0200, Christian Borntraeger wrote:
> When running with trace backend e.g. "simple" the writer thread needs to be
> implemented in the same process context as the trace points that will be
> processed. Under libvirtd control, qemu gets first started in daemonized
> mode to privide its capabilities. Creating the writer thread in the initial
> process context then leads to a dead lock because the thread gets termined
> together with the initial parent. (-daemonize) This results in
> stale qemu processes.
> Fix this by deferring trace initialization.
I don't think this works since trace events will fill up trace_buf[] and
eventually invoke flush_trace_file().
At that point we use trace_available_cond and trace_empty_cond, which
may be NULL in Glib <2.31.0.
Perhaps this can be made safe by checking trace_writeout_enabled. It
will be false before the backend has been initialized.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process
2013-07-16 3:05 ` Stefan Hajnoczi
@ 2013-07-16 12:17 ` Michael Mueller
2013-07-17 2:08 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Michael Mueller @ 2013-07-16 12:17 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Christian Borntraeger, aliguori, qemu-devel, Lluís
On Tue, 16 Jul 2013 11:05:11 +0800
Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 15, 2013 at 09:41:19PM +0200, Christian Borntraeger wrote:
> > When running with trace backend e.g. "simple" the writer thread
> > needs to be implemented in the same process context as the trace
> > points that will be processed. Under libvirtd control, qemu gets
> > first started in daemonized mode to privide its capabilities.
> > Creating the writer thread in the initial process context then
> > leads to a dead lock because the thread gets termined together with
> > the initial parent. (-daemonize) This results in stale qemu
> > processes. Fix this by deferring trace initialization.
>
> I don't think this works since trace events will fill up trace_buf[]
> and eventually invoke flush_trace_file().
>
> At that point we use trace_available_cond and trace_empty_cond, which
> may be NULL in Glib <2.31.0.
>
> Perhaps this can be made safe by checking trace_writeout_enabled. It
> will be false before the backend has been initialized.
>
> Stefan
>
You mean something like this. I'll give it a try:
---
trace/simple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -53,7 +53,7 @@ static GCond *trace_empty_cond;
#endif
static bool trace_available;
-static bool trace_writeout_enabled;
+static bool trace_writeout_enabled = false;
enum {
TRACE_BUF_LEN = 4096 * 64,
@@ -427,5 +427,6 @@ bool trace_backend_init(const char *even
atexit(st_flush_trace_buffer);
trace_backend_init_events(events);
st_set_trace_file(file);
+ trace_writeout_enabled = false;
return true;
}
Michael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process
2013-07-16 12:17 ` Michael Mueller
@ 2013-07-17 2:08 ` Stefan Hajnoczi
2013-09-06 14:06 ` Michael Mueller
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-07-17 2:08 UTC (permalink / raw)
To: Michael Mueller; +Cc: Christian Borntraeger, aliguori, qemu-devel, Lluís
On Tue, Jul 16, 2013 at 02:17:28PM +0200, Michael Mueller wrote:
> On Tue, 16 Jul 2013 11:05:11 +0800
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> > On Mon, Jul 15, 2013 at 09:41:19PM +0200, Christian Borntraeger wrote:
> > > When running with trace backend e.g. "simple" the writer thread
> > > needs to be implemented in the same process context as the trace
> > > points that will be processed. Under libvirtd control, qemu gets
> > > first started in daemonized mode to privide its capabilities.
> > > Creating the writer thread in the initial process context then
> > > leads to a dead lock because the thread gets termined together with
> > > the initial parent. (-daemonize) This results in stale qemu
> > > processes. Fix this by deferring trace initialization.
> >
> > I don't think this works since trace events will fill up trace_buf[]
> > and eventually invoke flush_trace_file().
> >
> > At that point we use trace_available_cond and trace_empty_cond, which
> > may be NULL in Glib <2.31.0.
> >
> > Perhaps this can be made safe by checking trace_writeout_enabled. It
> > will be false before the backend has been initialized.
> >
> > Stefan
> >
>
> You mean something like this. I'll give it a try:
>
> ---
> trace/simple.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -53,7 +53,7 @@ static GCond *trace_empty_cond;
> #endif
>
> static bool trace_available;
> -static bool trace_writeout_enabled;
> +static bool trace_writeout_enabled = false;
static bool is automatically initialized to false.
> enum {
> TRACE_BUF_LEN = 4096 * 64,
> @@ -427,5 +427,6 @@ bool trace_backend_init(const char *even
> atexit(st_flush_trace_buffer);
> trace_backend_init_events(events);
> st_set_trace_file(file);
> + trace_writeout_enabled = false;
I was thinking along the lines of trace_record_finish() not calling
flush_trace_file() if trace_writeout_enabled is false.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process
2013-07-17 2:08 ` Stefan Hajnoczi
@ 2013-09-06 14:06 ` Michael Mueller
2013-09-23 13:09 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Michael Mueller @ 2013-09-06 14:06 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Christian Borntraeger, qemu-devel, Lluís
On Wed, 17 Jul 2013 10:08:15 +0800
Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 16, 2013 at 02:17:28PM +0200, Michael Mueller wrote:
> > On Tue, 16 Jul 2013 11:05:11 +0800
> > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >
> > > On Mon, Jul 15, 2013 at 09:41:19PM +0200, Christian Borntraeger wrote:
> > > > When running with trace backend e.g. "simple" the writer thread
> > > > needs to be implemented in the same process context as the trace
> > > > points that will be processed. Under libvirtd control, qemu gets
> > > > first started in daemonized mode to privide its capabilities.
> > > > Creating the writer thread in the initial process context then
> > > > leads to a dead lock because the thread gets termined together with
> > > > the initial parent. (-daemonize) This results in stale qemu
> > > > processes. Fix this by deferring trace initialization.
> > >
> > > I don't think this works since trace events will fill up trace_buf[]
> > > and eventually invoke flush_trace_file().
> > >
> > > At that point we use trace_available_cond and trace_empty_cond, which
> > > may be NULL in Glib <2.31.0.
> > >
> > > Perhaps this can be made safe by checking trace_writeout_enabled. It
> > > will be false before the backend has been initialized.
> > >
> > > Stefan
> > >
> >
> > You mean something like this. I'll give it a try:
> >
> > ---
> > trace/simple.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > --- a/trace/simple.c
> > +++ b/trace/simple.c
> > @@ -53,7 +53,7 @@ static GCond *trace_empty_cond;
> > #endif
> >
> > static bool trace_available;
> > -static bool trace_writeout_enabled;
> > +static bool trace_writeout_enabled = false;
>
> static bool is automatically initialized to false.
>
> > enum {
> > TRACE_BUF_LEN = 4096 * 64,
> > @@ -427,5 +427,6 @@ bool trace_backend_init(const char *even
> > atexit(st_flush_trace_buffer);
> > trace_backend_init_events(events);
> > st_set_trace_file(file);
> > + trace_writeout_enabled = false;
>
> I was thinking along the lines of trace_record_finish() not calling
> flush_trace_file() if trace_writeout_enabled is false.
>
> Stefan
>
I just looked into it again and think that it is save the way I suggested, because as long
trace_backend_init() isn't called, also trace_backend_init_events() hasn't registered any
events. Thus no trace records will be written and can fill up the trace buffer.
Michael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process
2013-09-06 14:06 ` Michael Mueller
@ 2013-09-23 13:09 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-09-23 13:09 UTC (permalink / raw)
To: Michael Mueller; +Cc: Christian Borntraeger, qemu-devel, Lluís
On Fri, Sep 06, 2013 at 04:06:08PM +0200, Michael Mueller wrote:
> On Wed, 17 Jul 2013 10:08:15 +0800
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> > On Tue, Jul 16, 2013 at 02:17:28PM +0200, Michael Mueller wrote:
> > > On Tue, 16 Jul 2013 11:05:11 +0800
> > > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > >
> > > > On Mon, Jul 15, 2013 at 09:41:19PM +0200, Christian Borntraeger wrote:
> > > > > When running with trace backend e.g. "simple" the writer thread
> > > > > needs to be implemented in the same process context as the trace
> > > > > points that will be processed. Under libvirtd control, qemu gets
> > > > > first started in daemonized mode to privide its capabilities.
> > > > > Creating the writer thread in the initial process context then
> > > > > leads to a dead lock because the thread gets termined together with
> > > > > the initial parent. (-daemonize) This results in stale qemu
> > > > > processes. Fix this by deferring trace initialization.
> > > >
> > > > I don't think this works since trace events will fill up trace_buf[]
> > > > and eventually invoke flush_trace_file().
> > > >
> > > > At that point we use trace_available_cond and trace_empty_cond, which
> > > > may be NULL in Glib <2.31.0.
> > > >
> > > > Perhaps this can be made safe by checking trace_writeout_enabled. It
> > > > will be false before the backend has been initialized.
> > > >
> > > > Stefan
> > > >
> > >
> > > You mean something like this. I'll give it a try:
> > >
> > > ---
> > > trace/simple.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > --- a/trace/simple.c
> > > +++ b/trace/simple.c
> > > @@ -53,7 +53,7 @@ static GCond *trace_empty_cond;
> > > #endif
> > >
> > > static bool trace_available;
> > > -static bool trace_writeout_enabled;
> > > +static bool trace_writeout_enabled = false;
> >
> > static bool is automatically initialized to false.
> >
> > > enum {
> > > TRACE_BUF_LEN = 4096 * 64,
> > > @@ -427,5 +427,6 @@ bool trace_backend_init(const char *even
> > > atexit(st_flush_trace_buffer);
> > > trace_backend_init_events(events);
> > > st_set_trace_file(file);
> > > + trace_writeout_enabled = false;
> >
> > I was thinking along the lines of trace_record_finish() not calling
> > flush_trace_file() if trace_writeout_enabled is false.
> >
> > Stefan
> >
>
> I just looked into it again and think that it is save the way I suggested, because as long
> trace_backend_init() isn't called, also trace_backend_init_events() hasn't registered any
> events. Thus no trace records will be written and can fill up the trace buffer.
Good point, Michael.
Do you mind resending your latest code rebased onto qemu.git/master?
It's been a while since we discussed these patches and I'd like to make
sure we have a fresh email thread to review and complete the merge.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-23 13:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-15 19:41 [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process Christian Borntraeger
2013-07-16 3:05 ` Stefan Hajnoczi
2013-07-16 12:17 ` Michael Mueller
2013-07-17 2:08 ` Stefan Hajnoczi
2013-09-06 14:06 ` Michael Mueller
2013-09-23 13:09 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.