[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thr
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 02/12] trace+libvirt: start trace processing thread in final child process |
Date: |
Mon, 23 Sep 2013 15:09:13 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Sep 06, 2013 at 04:06:08PM +0200, Michael Mueller wrote:
> On Wed, 17 Jul 2013 10:08:15 +0800
> Stefan Hajnoczi <address@hidden> 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 <address@hidden> 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