| Message ID | 20150923065642.GG2339@yliu-dev.sh.intel.com |
|---|---|
| State | New |
| Headers | show |
On 09/23/2015 02:56 PM, Yuanhan Liu wrote: > On Wed, Sep 23, 2015 at 12:20:00PM +0800, Yuanhan Liu wrote: >> From: Changchun Ouyang <changchun.ouyang@intel.com> >> > [...] >> static void net_vhost_user_event(void *opaque, int event) >> { >> - VhostUserState *s = opaque; >> + const char *name = opaque; >> + NetClientState *ncs[MAX_QUEUE_NUM]; >> + VhostUserState *s; >> + Error *err = NULL; >> + int queues; >> >> + queues = qemu_find_net_clients_except(name, ncs, >> + NET_CLIENT_OPTIONS_KIND_NIC, >> + MAX_QUEUE_NUM); >> + s = DO_UPCAST(VhostUserState, nc, ncs[0]); >> switch (event) { >> case CHR_EVENT_OPENED: >> - vhost_user_start(s); >> - net_vhost_link_down(s, false); >> + if (vhost_user_start(queues, ncs) < 0) { >> + exit(1); >> + } >> + qmp_set_link(name, true, &err); >> error_report("chardev \"%s\" went up", s->chr->label); >> break; >> case CHR_EVENT_CLOSED: >> - net_vhost_link_down(s, true); >> - vhost_user_stop(s); >> + qmp_set_link(name, true, &err); >> + vhost_user_stop(queues, ncs); > Sigh... A silly Copy & Paste typo. Here is the udpated patch: > > --yliu > > ---8<--- > From 3793772867024dfabb9eb8eb5c53ae6714f88618 Mon Sep 17 00:00:00 2001 > From: Changchun Ouyang <changchun.ouyang@intel.com> > Date: Tue, 15 Sep 2015 14:28:38 +0800 > Subject: [PATCH v11 6/7] vhost-user: add multiple queue support > > This patch is initially based a patch from Nikolay Nikolaev. > > This patch adds vhost-user multiple queue support, by creating a nc > and vhost_net pair for each queue. > > Qemu exits if find that the backend can't support the number of requested > queues (by providing queues=# option). The max number is queried by a > new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol > feature VHOST_USER_PROTOCOL_F_MQ is present first. > > The max queue check is done at vhost-user initiation stage. We initiate > one queue first, which, in the meantime, also gets the max_queues the > backend supports. > > In older version, it was reported that some messages are sent more times > than necessary. Here we came an agreement with Michael that we could > categorize vhost user messages to 2 types: non-vring specific messages, > which should be sent only once, and vring specific messages, which should > be sent per queue. > > Here I introduced a helper function vhost_user_one_time_request(), which > lists following messages as non-vring specific messages: > > VHOST_USER_SET_OWNER > VHOST_USER_RESET_DEVICE > VHOST_USER_SET_MEM_TABLE > VHOST_USER_GET_QUEUE_NUM > > For above messages, we simply ignore them when they are not sent the first > time. > > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > --- > v11: inovke qmp_set_link() directly -- Suggested by Jason Wang > > v10: don't treat VHOST_USER_SET/GET_[PROTOCOL]_FEATURES as one time > request, as the two feature bits need to be stored at per-device. > > v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers() > once only, and invoke qemu_find_net_clients_except() at the handler > to gather all related ncs, for initiating all queues. Which addresses > a hidden bug in older verion in a more QEMU-like way. > > v8: set net->dev.vq_index correctly inside vhost_net_init() based on the > value from net->nc->queue_index. > --- > docs/specs/vhost-user.txt | 15 +++++ > hw/net/vhost_net.c | 10 ++-- > hw/virtio/vhost-user.c | 22 ++++++++ > hw/virtio/vhost.c | 5 +- > net/vhost-user.c | 141 ++++++++++++++++++++++++++++++---------------- > qapi-schema.json | 6 +- > qemu-options.hx | 5 +- > 7 files changed, 147 insertions(+), 57 deletions(-) Some nitpicks and comments. If you plan to send another version, please consider to fix them. Reviewed-by: Jason Wang <jasowang@redhat.com> Btw, it's better to extend current vhost-user unit test to support multiqueue. Please consider this on top this series. Thanks > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > index 43db9b4..cfc9d41 100644 > --- a/docs/specs/vhost-user.txt > +++ b/docs/specs/vhost-user.txt > @@ -135,6 +135,21 @@ As older slaves don't support negotiating protocol features, > a feature bit was dedicated for this purpose: > #define VHOST_USER_F_PROTOCOL_FEATURES 30 > > +Multiple queue support > +---------------------- > + > +Multiple queue is treated as a protocol extension, hence the slave has to > +implement protocol features first. The multiple queues feature is supported > +only when the protocol feature VHOST_USER_PROTOCOL_F_MQ (bit 0) is set: > +#define VHOST_USER_PROTOCOL_F_MQ 0 > + > +The max number of queues the slave supports can be queried with message > +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of > +requested queues is bigger than that. > + > +As all queues share one connection, the master uses a unique index for each > +queue in the sent message to identify a specified queue. > + > Message types > ------------- > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index f663e5a..ad82b5c 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -148,8 +148,11 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) > fprintf(stderr, "vhost-net requires net backend to be setup\n"); > goto fail; > } > + net->nc = options->net_backend; > > net->dev.max_queues = 1; > + net->dev.nvqs = 2; > + net->dev.vqs = net->vqs; > > if (backend_kernel) { > r = vhost_net_get_fd(options->net_backend); > @@ -164,11 +167,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) > net->dev.backend_features = 0; > net->dev.protocol_features = 0; > net->backend = -1; > - } > - net->nc = options->net_backend; > > - net->dev.nvqs = 2; > - net->dev.vqs = net->vqs; > + /* vhost-user needs vq_index to initiate a specific queue pair */ > + net->dev.vq_index = net->nc->queue_index * net->dev.nvqs; Better use vhost_net_set_vq_index() here. > + } > > r = vhost_dev_init(&net->dev, options->opaque, > options->backend_type); > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 5018fd6..e42fde6 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -187,6 +187,19 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > 0 : -1; > } > > +static bool vhost_user_one_time_request(VhostUserRequest request) > +{ > + switch (request) { > + case VHOST_USER_SET_OWNER: > + case VHOST_USER_RESET_DEVICE: > + case VHOST_USER_SET_MEM_TABLE: > + case VHOST_USER_GET_QUEUE_NUM: > + return true; > + default: > + return false; > + } > +} > + > static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, > void *arg) > { > @@ -207,6 +220,15 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, > msg_request = request; > } > > + /* > + * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE, > + * we just need send it once in the first time. For later such > + * request, we just ignore it. > + */ > + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) { > + return 0; > + } Looks like vhost_user_dev_request() is a better name here. > + > msg.request = msg_request; > msg.flags = VHOST_USER_VERSION; > msg.size = 0; > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 7a7812d..c0ed5b2 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -874,8 +874,9 @@ static void vhost_eventfd_del(MemoryListener *listener, > static int vhost_virtqueue_init(struct vhost_dev *dev, > struct vhost_virtqueue *vq, int n) > { > + int vhost_vq_index = dev->vhost_ops->vhost_backend_get_vq_index(dev, n); > struct vhost_vring_file file = { > - .index = n, > + .index = vhost_vq_index, > }; > int r = event_notifier_init(&vq->masked_notifier, 0); > if (r < 0) { > @@ -926,7 +927,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > } > > for (i = 0; i < hdev->nvqs; ++i) { > - r = vhost_virtqueue_init(hdev, hdev->vqs + i, i); > + r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i); > if (r < 0) { > goto fail_vq; > } > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 93dcecd..1abec97 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -14,6 +14,7 @@ > #include "sysemu/char.h" > #include "qemu/config-file.h" > #include "qemu/error-report.h" > +#include "qmp-commands.h" > > typedef struct VhostUserState { > NetClientState nc; > @@ -39,37 +40,77 @@ static int vhost_user_running(VhostUserState *s) > return (s->vhost_net) ? 1 : 0; > } > > -static int vhost_user_start(VhostUserState *s) > +static void vhost_user_stop(int queues, NetClientState *ncs[]) > { > - VhostNetOptions options; > - > - if (vhost_user_running(s)) { > - return 0; > - } > + VhostUserState *s; > + int i; > > - options.backend_type = VHOST_BACKEND_TYPE_USER; > - options.net_backend = &s->nc; > - options.opaque = s->chr; > + for (i = 0; i < queues; i++) { > + assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER); > > - s->vhost_net = vhost_net_init(&options); > + s = DO_UPCAST(VhostUserState, nc, ncs[i]); > + if (!vhost_user_running(s)) { > + continue; > + } > > - return vhost_user_running(s) ? 0 : -1; > + if (s->vhost_net) { > + vhost_net_cleanup(s->vhost_net); > + s->vhost_net = NULL; > + } > + } > } > > -static void vhost_user_stop(VhostUserState *s) > +static int vhost_user_start(int queues, NetClientState *ncs[]) > { > - if (vhost_user_running(s)) { > - vhost_net_cleanup(s->vhost_net); > + VhostNetOptions options; > + VhostUserState *s; > + int max_queues; > + int i; > + > + options.backend_type = VHOST_BACKEND_TYPE_USER; > + > + for (i = 0; i < queues; i++) { > + assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER); > + > + s = DO_UPCAST(VhostUserState, nc, ncs[i]); > + if (vhost_user_running(s)) { > + continue; > + } > + > + options.net_backend = ncs[i]; > + options.opaque = s->chr; > + s->vhost_net = vhost_net_init(&options); > + if (!s->vhost_net) { > + error_report("failed to init vhost_net for queue %d\n", i); > + goto err; > + } > + > + if (i == 0) { > + max_queues = vhost_net_get_max_queues(s->vhost_net); > + if (queues > max_queues) { > + error_report("you are asking more queues than " > + "supported: %d\n", max_queues); > + goto err; > + } > + } > } > > - s->vhost_net = 0; > + return 0; > + > +err: > + vhost_user_stop(i + 1, ncs); > + return -1; > } > > static void vhost_user_cleanup(NetClientState *nc) > { > VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); > > - vhost_user_stop(s); > + if (s->vhost_net) { > + vhost_net_cleanup(s->vhost_net); > + s->vhost_net = NULL; > + } > + > qemu_purge_queued_packets(nc); > } > > @@ -95,59 +136,61 @@ static NetClientInfo net_vhost_user_info = { > .has_ufo = vhost_user_has_ufo, > }; > > -static void net_vhost_link_down(VhostUserState *s, bool link_down) > -{ > - s->nc.link_down = link_down; > - > - if (s->nc.peer) { > - s->nc.peer->link_down = link_down; > - } > - > - if (s->nc.info->link_status_changed) { > - s->nc.info->link_status_changed(&s->nc); > - } > - > - if (s->nc.peer && s->nc.peer->info->link_status_changed) { > - s->nc.peer->info->link_status_changed(s->nc.peer); > - } > -} > - > static void net_vhost_user_event(void *opaque, int event) > { > - VhostUserState *s = opaque; > + const char *name = opaque; > + NetClientState *ncs[MAX_QUEUE_NUM]; > + VhostUserState *s; > + Error *err = NULL; > + int queues; > > + queues = qemu_find_net_clients_except(name, ncs, > + NET_CLIENT_OPTIONS_KIND_NIC, > + MAX_QUEUE_NUM); > + s = DO_UPCAST(VhostUserState, nc, ncs[0]); > switch (event) { > case CHR_EVENT_OPENED: > - vhost_user_start(s); > - net_vhost_link_down(s, false); > + if (vhost_user_start(queues, ncs) < 0) { > + exit(1); How about report a specific error instead of just abort here? > + } > + qmp_set_link(name, true, &err); > error_report("chardev \"%s\" went up", s->chr->label); > break; > case CHR_EVENT_CLOSED: > - net_vhost_link_down(s, true); > - vhost_user_stop(s); > + qmp_set_link(name, false, &err); > + vhost_user_stop(queues, ncs); > error_report("chardev \"%s\" went down", s->chr->label); > break; > } > + > + if (err) { > + error_report_err(err); > + } > } > > static int net_vhost_user_init(NetClientState *peer, const char *device, > - const char *name, CharDriverState *chr) > + const char *name, CharDriverState *chr, > + int 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); > > - s = DO_UPCAST(VhostUserState, nc, nc); > + /* We don't provide a receive callback */ > + nc->receive_disabled = 1; > + nc->queue_index = i; > > - /* We don't provide a receive callback */ > - s->nc.receive_disabled = 1; > - s->chr = chr; > + s = DO_UPCAST(VhostUserState, nc, nc); > + s->chr = chr; > + } > > - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); > + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, (void*)name); > > return 0; > } > @@ -226,6 +269,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp) > int net_init_vhost_user(const NetClientOptions *opts, const char *name, > NetClientState *peer, Error **errp) > { > + int queues; > const NetdevVhostUserOptions *vhost_user_opts; > CharDriverState *chr; > > @@ -243,6 +287,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name, > return -1; > } > > + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1; > > - return net_vhost_user_init(peer, "vhost_user", name, chr); > + return net_vhost_user_init(peer, "vhost_user", name, chr, queues); > } > diff --git a/qapi-schema.json b/qapi-schema.json > index 527690d..582a817 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2481,12 +2481,16 @@ > # > # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false). > # > +# @queues: #optional number of queues to be created for multiqueue vhost-user > +# (default: 1) (Since 2.5) > +# > # Since 2.1 > ## > { 'struct': 'NetdevVhostUserOptions', > 'data': { > 'chardev': 'str', > - '*vhostforce': 'bool' } } > + '*vhostforce': 'bool', > + '*queues': 'int' } } > > ## > # @NetClientOptions > diff --git a/qemu-options.hx b/qemu-options.hx > index 7e147b8..328404c 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1990,13 +1990,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single > netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the > required hub automatically. > > -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off] > +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n] > > Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should > be a unix domain socket backed one. The vhost-user uses a specifically defined > protocol to pass vhost ioctl replacement messages to an application on the other > end of the socket. On non-MSIX guests, the feature can be forced with > -@var{vhostforce}. > +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to > +be created for multiqueue vhost-user. > > Example: > @example
On Thu, Sep 24, 2015 at 01:34:31PM +0800, Jason Wang wrote: > > > Some nitpicks and comments. If you plan to send another version, please > consider to fix them. I will, but I'd like to hold a while before getting more comments from Michael; I don't want to repost a whole new version too often for minor changes. BTW, Michael, is this patchset being ready for merge? If not, please kindly tell me what is wrong so that I can fix them. TBH, I'd really like to see it be merged soon, as I'm gonna have holidays soon for two weeks start from this Saturday. I could still reply emails, even make patches during that, but I guess I only have limited time for that. > > Reviewed-by: Jason Wang <jasowang@redhat.com> Thank you! > > Btw, it's better to extend current vhost-user unit test to support > multiqueue. Please consider this on top this series. Yeah, I will. But, may I do that later? (And if possible, after the merge?) > > > > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt > > index 43db9b4..cfc9d41 100644 > > --- a/docs/specs/vhost-user.txt > > +++ b/docs/specs/vhost-user.txt > > @@ -135,6 +135,21 @@ As older slaves don't support negotiating protocol features, > > a feature bit was dedicated for this purpose: > > #define VHOST_USER_F_PROTOCOL_FEATURES 30 > > > > +Multiple queue support > > +---------------------- > > + > > +Multiple queue is treated as a protocol extension, hence the slave has to > > +implement protocol features first. The multiple queues feature is supported > > +only when the protocol feature VHOST_USER_PROTOCOL_F_MQ (bit 0) is set: > > +#define VHOST_USER_PROTOCOL_F_MQ 0 > > + > > +The max number of queues the slave supports can be queried with message > > +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of > > +requested queues is bigger than that. > > + > > +As all queues share one connection, the master uses a unique index for each > > +queue in the sent message to identify a specified queue. > > + > > Message types > > ------------- > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > index f663e5a..ad82b5c 100644 > > --- a/hw/net/vhost_net.c > > +++ b/hw/net/vhost_net.c > > @@ -148,8 +148,11 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) > > fprintf(stderr, "vhost-net requires net backend to be setup\n"); > > goto fail; > > } > > + net->nc = options->net_backend; > > > > net->dev.max_queues = 1; > > + net->dev.nvqs = 2; > > + net->dev.vqs = net->vqs; > > > > if (backend_kernel) { > > r = vhost_net_get_fd(options->net_backend); > > @@ -164,11 +167,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) > > net->dev.backend_features = 0; > > net->dev.protocol_features = 0; > > net->backend = -1; > > - } > > - net->nc = options->net_backend; > > > > - net->dev.nvqs = 2; > > - net->dev.vqs = net->vqs; > > + /* vhost-user needs vq_index to initiate a specific queue pair */ > > + net->dev.vq_index = net->nc->queue_index * net->dev.nvqs; > > Better use vhost_net_set_vq_index() here. I could do that. > > > + } > > > > r = vhost_dev_init(&net->dev, options->opaque, > > options->backend_type); > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index 5018fd6..e42fde6 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -187,6 +187,19 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, > > 0 : -1; > > } > > > > +static bool vhost_user_one_time_request(VhostUserRequest request) > > +{ > > + switch (request) { > > + case VHOST_USER_SET_OWNER: > > + case VHOST_USER_RESET_DEVICE: > > + case VHOST_USER_SET_MEM_TABLE: > > + case VHOST_USER_GET_QUEUE_NUM: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, > > void *arg) > > { > > @@ -207,6 +220,15 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, > > msg_request = request; > > } > > > > + /* > > + * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE, > > + * we just need send it once in the first time. For later such > > + * request, we just ignore it. > > + */ > > + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) { > > + return 0; > > + } > > Looks like vhost_user_dev_request() is a better name here. Yeah, indeed. Thanks. > > > + > > msg.request = msg_request; > > msg.flags = VHOST_USER_VERSION; > > msg.size = 0; > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 7a7812d..c0ed5b2 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -874,8 +874,9 @@ static void vhost_eventfd_del(MemoryListener *listener, > > static int vhost_virtqueue_init(struct vhost_dev *dev, > > struct vhost_virtqueue *vq, int n) > > { > > + int vhost_vq_index = dev->vhost_ops->vhost_backend_get_vq_index(dev, n); > > struct vhost_vring_file file = { > > - .index = n, > > + .index = vhost_vq_index, > > }; > > int r = event_notifier_init(&vq->masked_notifier, 0); > > if (r < 0) { > > @@ -926,7 +927,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > > } > > > > for (i = 0; i < hdev->nvqs; ++i) { > > - r = vhost_virtqueue_init(hdev, hdev->vqs + i, i); > > + r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i); > > if (r < 0) { > > goto fail_vq; > > } > > diff --git a/net/vhost-user.c b/net/vhost-user.c > > index 93dcecd..1abec97 100644 > > --- a/net/vhost-user.c > > +++ b/net/vhost-user.c > > @@ -14,6 +14,7 @@ > > #include "sysemu/char.h" > > #include "qemu/config-file.h" > > #include "qemu/error-report.h" > > +#include "qmp-commands.h" > > > > typedef struct VhostUserState { > > NetClientState nc; > > @@ -39,37 +40,77 @@ static int vhost_user_running(VhostUserState *s) > > return (s->vhost_net) ? 1 : 0; > > } > > > > -static int vhost_user_start(VhostUserState *s) > > +static void vhost_user_stop(int queues, NetClientState *ncs[]) > > { > > - VhostNetOptions options; > > - > > - if (vhost_user_running(s)) { > > - return 0; > > - } > > + VhostUserState *s; > > + int i; > > > > - options.backend_type = VHOST_BACKEND_TYPE_USER; > > - options.net_backend = &s->nc; > > - options.opaque = s->chr; > > + for (i = 0; i < queues; i++) { > > + assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER); > > > > - s->vhost_net = vhost_net_init(&options); > > + s = DO_UPCAST(VhostUserState, nc, ncs[i]); > > + if (!vhost_user_running(s)) { > > + continue; > > + } > > > > - return vhost_user_running(s) ? 0 : -1; > > + if (s->vhost_net) { > > + vhost_net_cleanup(s->vhost_net); > > + s->vhost_net = NULL; > > + } > > + } > > } > > > > -static void vhost_user_stop(VhostUserState *s) > > +static int vhost_user_start(int queues, NetClientState *ncs[]) > > { > > - if (vhost_user_running(s)) { > > - vhost_net_cleanup(s->vhost_net); > > + VhostNetOptions options; > > + VhostUserState *s; > > + int max_queues; > > + int i; > > + > > + options.backend_type = VHOST_BACKEND_TYPE_USER; > > + > > + for (i = 0; i < queues; i++) { > > + assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER); > > + > > + s = DO_UPCAST(VhostUserState, nc, ncs[i]); > > + if (vhost_user_running(s)) { > > + continue; > > + } > > + > > + options.net_backend = ncs[i]; > > + options.opaque = s->chr; > > + s->vhost_net = vhost_net_init(&options); > > + if (!s->vhost_net) { > > + error_report("failed to init vhost_net for queue %d\n", i); > > + goto err; > > + } > > + > > + if (i == 0) { > > + max_queues = vhost_net_get_max_queues(s->vhost_net); > > + if (queues > max_queues) { > > + error_report("you are asking more queues than " > > + "supported: %d\n", max_queues); > > + goto err; > > + } > > + } > > } > > > > - s->vhost_net = 0; > > + return 0; > > + > > +err: > > + vhost_user_stop(i + 1, ncs); > > + return -1; > > } > > > > static void vhost_user_cleanup(NetClientState *nc) > > { > > VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); > > > > - vhost_user_stop(s); > > + if (s->vhost_net) { > > + vhost_net_cleanup(s->vhost_net); > > + s->vhost_net = NULL; > > + } > > + > > qemu_purge_queued_packets(nc); > > } > > > > @@ -95,59 +136,61 @@ static NetClientInfo net_vhost_user_info = { > > .has_ufo = vhost_user_has_ufo, > > }; > > > > -static void net_vhost_link_down(VhostUserState *s, bool link_down) > > -{ > > - s->nc.link_down = link_down; > > - > > - if (s->nc.peer) { > > - s->nc.peer->link_down = link_down; > > - } > > - > > - if (s->nc.info->link_status_changed) { > > - s->nc.info->link_status_changed(&s->nc); > > - } > > - > > - if (s->nc.peer && s->nc.peer->info->link_status_changed) { > > - s->nc.peer->info->link_status_changed(s->nc.peer); > > - } > > -} > > - > > static void net_vhost_user_event(void *opaque, int event) > > { > > - VhostUserState *s = opaque; > > + const char *name = opaque; > > + NetClientState *ncs[MAX_QUEUE_NUM]; > > + VhostUserState *s; > > + Error *err = NULL; > > + int queues; > > > > + queues = qemu_find_net_clients_except(name, ncs, > > + NET_CLIENT_OPTIONS_KIND_NIC, > > + MAX_QUEUE_NUM); > > + s = DO_UPCAST(VhostUserState, nc, ncs[0]); > > switch (event) { > > case CHR_EVENT_OPENED: > > - vhost_user_start(s); > > - net_vhost_link_down(s, false); > > + if (vhost_user_start(queues, ncs) < 0) { > > + exit(1); > > How about report a specific error instead of just abort here? There is an error report at vhost_user_start(): error_report("you are asking more queues than " "supported: %d\n", max_queues); Or, do you mean something else? --yliu > > > + } > > + qmp_set_link(name, true, &err); > > error_report("chardev \"%s\" went up", s->chr->label); > > break; > > case CHR_EVENT_CLOSED: > > - net_vhost_link_down(s, true); > > - vhost_user_stop(s); > > + qmp_set_link(name, false, &err); > > + vhost_user_stop(queues, ncs); > > error_report("chardev \"%s\" went down", s->chr->label); > > break; > > } > > + > > + if (err) { > > + error_report_err(err); > > + } > > } > > > > static int net_vhost_user_init(NetClientState *peer, const char *device, > > - const char *name, CharDriverState *chr) > > + const char *name, CharDriverState *chr, > > + int 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); > > > > - s = DO_UPCAST(VhostUserState, nc, nc); > > + /* We don't provide a receive callback */ > > + nc->receive_disabled = 1; > > + nc->queue_index = i; > > > > - /* We don't provide a receive callback */ > > - s->nc.receive_disabled = 1; > > - s->chr = chr; > > + s = DO_UPCAST(VhostUserState, nc, nc); > > + s->chr = chr; > > + } > > > > - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); > > + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, (void*)name); > > > > return 0; > > } > > @@ -226,6 +269,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp) > > int net_init_vhost_user(const NetClientOptions *opts, const char *name, > > NetClientState *peer, Error **errp) > > { > > + int queues; > > const NetdevVhostUserOptions *vhost_user_opts; > > CharDriverState *chr; > > > > @@ -243,6 +287,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name, > > return -1; > > } > > > > + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1; > > > > - return net_vhost_user_init(peer, "vhost_user", name, chr); > > + return net_vhost_user_init(peer, "vhost_user", name, chr, queues); > > } > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 527690d..582a817 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -2481,12 +2481,16 @@ > > # > > # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false). > > # > > +# @queues: #optional number of queues to be created for multiqueue vhost-user > > +# (default: 1) (Since 2.5) > > +# > > # Since 2.1 > > ## > > { 'struct': 'NetdevVhostUserOptions', > > 'data': { > > 'chardev': 'str', > > - '*vhostforce': 'bool' } } > > + '*vhostforce': 'bool', > > + '*queues': 'int' } } > > > > ## > > # @NetClientOptions > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 7e147b8..328404c 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -1990,13 +1990,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single > > netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the > > required hub automatically. > > > > -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off] > > +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n] > > > > Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should > > be a unix domain socket backed one. The vhost-user uses a specifically defined > > protocol to pass vhost ioctl replacement messages to an application on the other > > end of the socket. On non-MSIX guests, the feature can be forced with > > -@var{vhostforce}. > > +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to > > +be created for multiqueue vhost-user. > > > > Example: > > @example
On 09/24/2015 01:57 PM, Yuanhan Liu wrote: > On Thu, Sep 24, 2015 at 01:34:31PM +0800, Jason Wang wrote: >> >> Some nitpicks and comments. If you plan to send another version, please >> consider to fix them. > I will, but I'd like to hold a while before getting more comments from > Michael; I don't want to repost a whole new version too often for minor > changes. > > BTW, Michael, is this patchset being ready for merge? If not, please > kindly tell me what is wrong so that I can fix them. TBH, I'd really > like to see it be merged soon, as I'm gonna have holidays soon for > two weeks start from this Saturday. I could still reply emails, even > make patches during that, but I guess I only have limited time for that. > >> Reviewed-by: Jason Wang <jasowang@redhat.com> > Thank you! > >> Btw, it's better to extend current vhost-user unit test to support >> multiqueue. Please consider this on top this series. > Yeah, I will. But, may I do that later? (And if possible, after the > merge?) > Yes, of course :) >>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt >>> index 43db9b4..cfc9d41 100644 >>> --- a/docs/specs/vhost-user.txt >>> +++ b/docs/specs/vhost-user.txt >>> @@ -135,6 +135,21 @@ As older slaves don't support negotiating protocol features, >>> a feature bit was dedicated for this purpose: >>> #define VHOST_USER_F_PROTOCOL_FEATURES 30 >>> [...] >>> >>> static void vhost_user_cleanup(NetClientState *nc) >>> { >>> VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); >>> >>> - vhost_user_stop(s); >>> + if (s->vhost_net) { >>> + vhost_net_cleanup(s->vhost_net); >>> + s->vhost_net = NULL; >>> + } >>> + >>> qemu_purge_queued_packets(nc); >>> } >>> >>> @@ -95,59 +136,61 @@ static NetClientInfo net_vhost_user_info = { >>> .has_ufo = vhost_user_has_ufo, >>> }; >>> >>> -static void net_vhost_link_down(VhostUserState *s, bool link_down) >>> -{ >>> - s->nc.link_down = link_down; >>> - >>> - if (s->nc.peer) { >>> - s->nc.peer->link_down = link_down; >>> - } >>> - >>> - if (s->nc.info->link_status_changed) { >>> - s->nc.info->link_status_changed(&s->nc); >>> - } >>> - >>> - if (s->nc.peer && s->nc.peer->info->link_status_changed) { >>> - s->nc.peer->info->link_status_changed(s->nc.peer); >>> - } >>> -} >>> - >>> static void net_vhost_user_event(void *opaque, int event) >>> { >>> - VhostUserState *s = opaque; >>> + const char *name = opaque; >>> + NetClientState *ncs[MAX_QUEUE_NUM]; >>> + VhostUserState *s; >>> + Error *err = NULL; >>> + int queues; >>> >>> + queues = qemu_find_net_clients_except(name, ncs, >>> + NET_CLIENT_OPTIONS_KIND_NIC, >>> + MAX_QUEUE_NUM); >>> + s = DO_UPCAST(VhostUserState, nc, ncs[0]); >>> switch (event) { >>> case CHR_EVENT_OPENED: >>> - vhost_user_start(s); >>> - net_vhost_link_down(s, false); >>> + if (vhost_user_start(queues, ncs) < 0) { >>> + exit(1); >> How about report a specific error instead of just abort here? > There is an error report at vhost_user_start(): > > error_report("you are asking more queues than " > "supported: %d\n", max_queues); > > Or, do you mean something else? > > > --yliu Not sure, but we have error_abort and error_fatal. Markus may know more about this. [...]
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt index 43db9b4..cfc9d41 100644 --- a/docs/specs/vhost-user.txt +++ b/docs/specs/vhost-user.txt @@ -135,6 +135,21 @@ As older slaves don't support negotiating protocol features, a feature bit was dedicated for this purpose: #define VHOST_USER_F_PROTOCOL_FEATURES 30 +Multiple queue support +---------------------- + +Multiple queue is treated as a protocol extension, hence the slave has to +implement protocol features first. The multiple queues feature is supported +only when the protocol feature VHOST_USER_PROTOCOL_F_MQ (bit 0) is set: +#define VHOST_USER_PROTOCOL_F_MQ 0 + +The max number of queues the slave supports can be queried with message +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of +requested queues is bigger than that. + +As all queues share one connection, the master uses a unique index for each +queue in the sent message to identify a specified queue. + Message types ------------- diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index f663e5a..ad82b5c 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -148,8 +148,11 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) fprintf(stderr, "vhost-net requires net backend to be setup\n"); goto fail; } + net->nc = options->net_backend; net->dev.max_queues = 1; + net->dev.nvqs = 2; + net->dev.vqs = net->vqs; if (backend_kernel) { r = vhost_net_get_fd(options->net_backend); @@ -164,11 +167,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) net->dev.backend_features = 0; net->dev.protocol_features = 0; net->backend = -1; - } - net->nc = options->net_backend; - net->dev.nvqs = 2; - net->dev.vqs = net->vqs; + /* vhost-user needs vq_index to initiate a specific queue pair */ + net->dev.vq_index = net->nc->queue_index * net->dev.nvqs; + } r = vhost_dev_init(&net->dev, options->opaque, options->backend_type); diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 5018fd6..e42fde6 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -187,6 +187,19 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg, 0 : -1; } +static bool vhost_user_one_time_request(VhostUserRequest request) +{ + switch (request) { + case VHOST_USER_SET_OWNER: + case VHOST_USER_RESET_DEVICE: + case VHOST_USER_SET_MEM_TABLE: + case VHOST_USER_GET_QUEUE_NUM: + return true; + default: + return false; + } +} + static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, void *arg) { @@ -207,6 +220,15 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request, msg_request = request; } + /* + * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE, + * we just need send it once in the first time. For later such + * request, we just ignore it. + */ + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) { + return 0; + } + msg.request = msg_request; msg.flags = VHOST_USER_VERSION; msg.size = 0; diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 7a7812d..c0ed5b2 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -874,8 +874,9 @@ static void vhost_eventfd_del(MemoryListener *listener, static int vhost_virtqueue_init(struct vhost_dev *dev, struct vhost_virtqueue *vq, int n) { + int vhost_vq_index = dev->vhost_ops->vhost_backend_get_vq_index(dev, n); struct vhost_vring_file file = { - .index = n, + .index = vhost_vq_index, }; int r = event_notifier_init(&vq->masked_notifier, 0); if (r < 0) { @@ -926,7 +927,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, } for (i = 0; i < hdev->nvqs; ++i) { - r = vhost_virtqueue_init(hdev, hdev->vqs + i, i); + r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i); if (r < 0) { goto fail_vq; } diff --git a/net/vhost-user.c b/net/vhost-user.c index 93dcecd..1abec97 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -14,6 +14,7 @@ #include "sysemu/char.h" #include "qemu/config-file.h" #include "qemu/error-report.h" +#include "qmp-commands.h" typedef struct VhostUserState { NetClientState nc; @@ -39,37 +40,77 @@ static int vhost_user_running(VhostUserState *s) return (s->vhost_net) ? 1 : 0; } -static int vhost_user_start(VhostUserState *s) +static void vhost_user_stop(int queues, NetClientState *ncs[]) { - VhostNetOptions options; - - if (vhost_user_running(s)) { - return 0; - } + VhostUserState *s; + int i; - options.backend_type = VHOST_BACKEND_TYPE_USER; - options.net_backend = &s->nc; - options.opaque = s->chr; + for (i = 0; i < queues; i++) { + assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER); - s->vhost_net = vhost_net_init(&options); + s = DO_UPCAST(VhostUserState, nc, ncs[i]); + if (!vhost_user_running(s)) { + continue; + } - return vhost_user_running(s) ? 0 : -1; + if (s->vhost_net) { + vhost_net_cleanup(s->vhost_net); + s->vhost_net = NULL; + } + } } -static void vhost_user_stop(VhostUserState *s) +static int vhost_user_start(int queues, NetClientState *ncs[]) { - if (vhost_user_running(s)) { - vhost_net_cleanup(s->vhost_net); + VhostNetOptions options; + VhostUserState *s; + int max_queues; + int i; + + options.backend_type = VHOST_BACKEND_TYPE_USER; + + for (i = 0; i < queues; i++) { + assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER); + + s = DO_UPCAST(VhostUserState, nc, ncs[i]); + if (vhost_user_running(s)) { + continue; + } + + options.net_backend = ncs[i]; + options.opaque = s->chr; + s->vhost_net = vhost_net_init(&options); + if (!s->vhost_net) { + error_report("failed to init vhost_net for queue %d\n", i); + goto err; + } + + if (i == 0) { + max_queues = vhost_net_get_max_queues(s->vhost_net); + if (queues > max_queues) { + error_report("you are asking more queues than " + "supported: %d\n", max_queues); + goto err; + } + } } - s->vhost_net = 0; + return 0; + +err: + vhost_user_stop(i + 1, ncs); + return -1; } static void vhost_user_cleanup(NetClientState *nc) { VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); - vhost_user_stop(s); + if (s->vhost_net) { + vhost_net_cleanup(s->vhost_net); + s->vhost_net = NULL; + } + qemu_purge_queued_packets(nc); } @@ -95,59 +136,61 @@ static NetClientInfo net_vhost_user_info = { .has_ufo = vhost_user_has_ufo, }; -static void net_vhost_link_down(VhostUserState *s, bool link_down) -{ - s->nc.link_down = link_down; - - if (s->nc.peer) { - s->nc.peer->link_down = link_down; - } - - if (s->nc.info->link_status_changed) { - s->nc.info->link_status_changed(&s->nc); - } - - if (s->nc.peer && s->nc.peer->info->link_status_changed) { - s->nc.peer->info->link_status_changed(s->nc.peer); - } -} - static void net_vhost_user_event(void *opaque, int event) { - VhostUserState *s = opaque; + const char *name = opaque; + NetClientState *ncs[MAX_QUEUE_NUM]; + VhostUserState *s; + Error *err = NULL; + int queues; + queues = qemu_find_net_clients_except(name, ncs, + NET_CLIENT_OPTIONS_KIND_NIC, + MAX_QUEUE_NUM); + s = DO_UPCAST(VhostUserState, nc, ncs[0]); switch (event) { case CHR_EVENT_OPENED: - vhost_user_start(s); - net_vhost_link_down(s, false); + if (vhost_user_start(queues, ncs) < 0) { + exit(1); + } + qmp_set_link(name, true, &err); error_report("chardev \"%s\" went up", s->chr->label); break; case CHR_EVENT_CLOSED: - net_vhost_link_down(s, true); - vhost_user_stop(s); + qmp_set_link(name, false, &err); + vhost_user_stop(queues, ncs); error_report("chardev \"%s\" went down", s->chr->label); break; } + + if (err) { + error_report_err(err); + } } static int net_vhost_user_init(NetClientState *peer, const char *device, - const char *name, CharDriverState *chr) + const char *name, CharDriverState *chr, + int 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); - s = DO_UPCAST(VhostUserState, nc, nc); + /* We don't provide a receive callback */ + nc->receive_disabled = 1; + nc->queue_index = i; - /* We don't provide a receive callback */ - s->nc.receive_disabled = 1; - s->chr = chr; + s = DO_UPCAST(VhostUserState, nc, nc); + s->chr = chr; + } - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, (void*)name); return 0; } @@ -226,6 +269,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp) int net_init_vhost_user(const NetClientOptions *opts, const char *name, NetClientState *peer, Error **errp) { + int queues; const NetdevVhostUserOptions *vhost_user_opts; CharDriverState *chr; @@ -243,6 +287,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name, return -1; } + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1; - return net_vhost_user_init(peer, "vhost_user", name, chr); + return net_vhost_user_init(peer, "vhost_user", name, chr, queues); } diff --git a/qapi-schema.json b/qapi-schema.json index 527690d..582a817 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2481,12 +2481,16 @@ # # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false). # +# @queues: #optional number of queues to be created for multiqueue vhost-user +# (default: 1) (Since 2.5) +# # Since 2.1 ## { 'struct': 'NetdevVhostUserOptions', 'data': { 'chardev': 'str', - '*vhostforce': 'bool' } } + '*vhostforce': 'bool', + '*queues': 'int' } } ## # @NetClientOptions diff --git a/qemu-options.hx b/qemu-options.hx index 7e147b8..328404c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1990,13 +1990,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the required hub automatically. -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off] +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n] Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should be a unix domain socket backed one. The vhost-user uses a specifically defined protocol to pass vhost ioctl replacement messages to an application on the other end of the socket. On non-MSIX guests, the feature can be forced with -@var{vhostforce}. +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to +be created for multiqueue vhost-user. Example: @example