Re: [Qemu-devel] [PATCH] vhost-user: multiqueue support

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 10:04:37AM +0100, Olivier MATZ wrote:
> Hi Nikolay,
> 
> On 12/06/2014 05:52 PM, Nikolay Nikolaev wrote:
> > Vhost-user will implement the multiqueueu support in a similar way to what
> > vhost already has - a separate thread for each queue.
> > 
> > To enable multiquue funcionality - a new command line parameter
> > "queues" is introduced for the vhost-user netdev.
> > 
> > Signed-off-by: Nikolay Nikolaev <address@hidden>
> > ---
> >  docs/specs/vhost-user.txt |    7 +++++++
> >  hw/virtio/vhost-user.c    |    6 +++++-
> >  net/vhost-user.c          |   35 +++++++++++++++++++++++------------
> >  qapi-schema.json          |    5 ++++-
> >  qemu-options.hx           |    5 +++--
> >  5 files changed, 42 insertions(+), 16 deletions(-)
> > [...]
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -134,25 +134,27 @@ static void net_vhost_user_event(void *opaque, int 
> > event)
> >  
> >  static int net_vhost_user_init(NetClientState *peer, const char *device,
> >                                 const char *name, CharDriverState *chr,
> > -                               bool vhostforce)
> > +                               bool vhostforce, uint32_t queues)
> >  {
> >      NetClientState *nc;
> >      VhostUserState *s;
> > +    int i;
> >  
> > -    nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> > +    for (i = 0; i < queues; i++) {
> > +        nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> >  
> > -    snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> > -             chr->label);
> > +        snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> > +                 i, chr->label);
> >  
> 
> Now that there several vhost-user are pointing to the same unix socket,
> it could make sense to display "nc->info_str" instead of "s->chr->label"
> in net_vhost_user_event(). Something like that:
> 
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -122,36 +122,39 @@ static void net_vhost_user_event(void *opaque, int
> event)
>       case CHR_EVENT_OPENED:
>           vhost_user_start(s);
>           net_vhost_link_down(s, false);
> -        error_report("chardev \"%s\" went up\n", s->chr->label);
> +        error_report("chardev \"%s\" went up\n", s->nc.info_str);
>           break;
>       case CHR_EVENT_CLOSED:
>           net_vhost_link_down(s, true);
>           vhost_user_stop(s);
> -        error_report("chardev \"%s\" went down\n", s->chr->label);
> +        error_report("chardev \"%s\" went down\n", s->nc.info_str);
>           break;
>       }
>   }
> 
> 
> Also, another comment: if I understand well, the messages like
> VHOST_USER_SET_OWNER, VHOST_USER_SET_FEATURES, VHOST_SET_MEM_TABLE,
> (...) will be send once per queue pair and not once per device.

One wonders why that's necessary.

> I don't think it's a problem, but maybe it deserves a small comment
> in the protocol documentation.
> 
> 
> Apart from these 2 small comments, the approach looks correct, so
> Acked-by: Olivier Matz <address@hidden>
> 
> 
> Regards,
> Olivier