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

[Stefan Hajnoczi]

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.

> @@ -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);

>      } else {

assert(!context);  /* HMP does not support IOThreads */

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

signature.asc
Description: PGP signature