Re: [Qemu-devel] [RFC v5 10/26] monitor: allow to use IO thread for parsing

[Peter Xu]

On Wed, Dec 13, 2017 at 04:35:00PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 05, 2017 at 01:51:44PM +0800, Peter Xu wrote:
> > @@ -583,6 +585,7 @@ static void monitor_data_init(Monitor *mon, bool 
> > skip_flush)
> >      /* Use *mon_cmds by default. */
> >      mon->cmd_table = mon_cmds;
> >      mon->skip_flush = skip_flush;
> > +    mon->use_io_thr = use_io_thr;
> 
> Why is mon->use_io_thr is a field instead of a monitor_init() argument.

It's used in other part of code when we want to know whether IOThread
is enabled for a monitor.

> 
> > @@ -4117,19 +4121,37 @@ void monitor_init(Chardev *chr, int flags)
> >          monitor_read_command(mon, 0);
> >      }
> >  
> > +    if (mon->use_io_thr) {
> > +        /*
> > +         * When use_io_thr is set, we use the global shared dedicated
> > +         * IO thread for this monitor to handle input/output.
> > +         */
> > +        context = monitor_io_context_get();
> > +        /* We should have inited globals before reaching here. */
> > +        assert(context);
> > +    } else {
> > +        /* The default main loop, which is the main thread */
> > +        context = NULL;
> > +    }
> > +
> > +    /*
> > +     * Add the monitor before running it (which is triggered by
> > +     * qemu_chr_fe_set_handlers), otherwise one monitor may find
> > +     * itself not on the mon_list when running.
> > +     */
> > +    qemu_mutex_lock(&monitor_lock);
> > +    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> > +    qemu_mutex_unlock(&monitor_lock);
> > +
> >      if (monitor_is_qmp(mon)) {
> >          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, 
> > monitor_qmp_read,
> > -                                 monitor_qmp_event, NULL, mon, NULL, true);
> > +                                 monitor_qmp_event, NULL, mon, context, 
> > true);
> >          qemu_chr_fe_set_echo(&mon->chr, true);
> >          json_message_parser_init(&mon->qmp.parser, handle_qmp_command, 
> > mon);
> 
> The comment above mentions the race condition between
> qemu_chr_fe_set_handlers() and mon_list.  I think that means the order
> must be:
> 
>   json_message_parser_init(&mon->qmp.parser, handle_qmp_command, mon);
>   qemu_chr_fe_set_echo(&mon->chr, true);
>   qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
>                            monitor_qmp_event, NULL, mon, context, true);

Yeh this looks safer!

> 
> >      } else {
> 
> assert(!context);  /* HMP does not support IOThreads */

I can add this.  Thanks,

> 
> >          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
> >                                   monitor_event, NULL, mon, NULL, true);
> >      }
> > -
> > -    qemu_mutex_lock(&monitor_lock);
> > -    QLIST_INSERT_HEAD(&mon_list, mon, entry);
> > -    qemu_mutex_unlock(&monitor_lock);
> >  }
> >  
> >  void monitor_cleanup(void)
> > -- 
> > 2.14.3
> > 



-- 
Peter Xu