[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] libvhost-user: support many virtqueues
|
From: |
Marc-André Lureau |
|
Subject: |
Re: [Qemu-devel] [PATCH 2/4] libvhost-user: support many virtqueues |
|
Date: |
Fri, 21 Jun 2019 15:48:36 +0200 |
Hi
On Fri, Jun 21, 2019 at 11:40 AM Stefan Hajnoczi <address@hidden> wrote:
>
> Currently libvhost-user is hardcoded to at most 8 virtqueues. The
> device backend should decide the number of virtqueues, not
> libvhost-user. This is important for multiqueue device backends where
> the guest driver needs an accurate number of virtqueues.
>
> This change breaks libvhost-user and libvhost-user-glib API stability.
> There is no stability guarantee yet, so make this change now and update
> all in-tree library users.
>
> This patch touches up vhost-user-blk, vhost-user-gpu, vhost-user-input,
> vhost-user-scsi, and vhost-user-bridge. If the device has a fixed
> number of queues that exact number is used. Otherwise the previous
> default of 8 virtqueues is used.
>
> vu_init() and vug_init() can now fail if malloc() returns NULL. I
> considered aborting with an error in libvhost-user but it should be safe
> to instantiate new vhost-user instances at runtime without risk of
> terminating the process. Therefore callers need to handle the vu_init()
> failure now.
>
> vhost-user-blk and vhost-user-scsi duplicate virtqueue index checks that
> are already performed by libvhost-user. This code would need to be
> modified to use max_queues but remove it completely instead since it's
> redundant.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> contrib/libvhost-user/libvhost-user-glib.h | 2 +-
> contrib/libvhost-user/libvhost-user.h | 10 ++++--
> contrib/libvhost-user/libvhost-user-glib.c | 12 +++++--
> contrib/libvhost-user/libvhost-user.c | 32 ++++++++++++-----
> contrib/vhost-user-blk/vhost-user-blk.c | 16 +++++----
> contrib/vhost-user-gpu/main.c | 9 ++++-
> contrib/vhost-user-input/main.c | 11 +++++-
> contrib/vhost-user-scsi/vhost-user-scsi.c | 21 +++++------
> tests/vhost-user-bridge.c | 42 ++++++++++++++--------
> 9 files changed, 104 insertions(+), 51 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user-glib.h
> b/contrib/libvhost-user/libvhost-user-glib.h
> index d3200f3afc..64d539d93a 100644
> --- a/contrib/libvhost-user/libvhost-user-glib.h
> +++ b/contrib/libvhost-user/libvhost-user-glib.h
> @@ -25,7 +25,7 @@ typedef struct VugDev {
> GSource *src;
> } VugDev;
>
> -void vug_init(VugDev *dev, int socket,
> +bool vug_init(VugDev *dev, uint16_t max_queues, int socket,
> vu_panic_cb panic, const VuDevIface *iface);
> void vug_deinit(VugDev *dev);
>
> diff --git a/contrib/libvhost-user/libvhost-user.h
> b/contrib/libvhost-user/libvhost-user.h
> index 3b888ff0a5..46b600799b 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -25,7 +25,6 @@
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
> #define VHOST_LOG_PAGE 4096
>
> -#define VHOST_MAX_NR_VIRTQUEUE 8
> #define VIRTQUEUE_MAX_SIZE 1024
>
> #define VHOST_MEMORY_MAX_NREGIONS 8
> @@ -353,7 +352,7 @@ struct VuDev {
> int sock;
> uint32_t nregions;
> VuDevRegion regions[VHOST_MEMORY_MAX_NREGIONS];
> - VuVirtq vq[VHOST_MAX_NR_VIRTQUEUE];
> + VuVirtq *vq;
> VuDevInflightInfo inflight_info;
> int log_call_fd;
> int slave_fd;
> @@ -362,6 +361,7 @@ struct VuDev {
> uint64_t features;
> uint64_t protocol_features;
> bool broken;
> + uint16_t max_queues;
>
> /* @set_watch: add or update the given fd to the watch set,
> * call cb when condition is met */
> @@ -391,6 +391,7 @@ typedef struct VuVirtqElement {
> /**
> * vu_init:
> * @dev: a VuDev context
> + * @max_queues: maximum number of virtqueues
> * @socket: the socket connected to vhost-user master
> * @panic: a panic callback
> * @set_watch: a set_watch callback
> @@ -398,8 +399,11 @@ typedef struct VuVirtqElement {
> * @iface: a VuDevIface structure with vhost-user device callbacks
> *
> * Intializes a VuDev vhost-user context.
> + *
> + * Returns: true on success, false on failure.
> **/
> -void vu_init(VuDev *dev,
> +bool vu_init(VuDev *dev,
> + uint16_t max_queues,
> int socket,
> vu_panic_cb panic,
> vu_set_watch_cb set_watch,
> diff --git a/contrib/libvhost-user/libvhost-user-glib.c
> b/contrib/libvhost-user/libvhost-user-glib.c
> index 42660a1b36..99edd2f3de 100644
> --- a/contrib/libvhost-user/libvhost-user-glib.c
> +++ b/contrib/libvhost-user/libvhost-user-glib.c
> @@ -131,18 +131,24 @@ static void vug_watch(VuDev *dev, int condition, void
> *data)
> }
> }
>
> -void
> -vug_init(VugDev *dev, int socket,
> +bool
> +vug_init(VugDev *dev, uint16_t max_queues, int socket,
> vu_panic_cb panic, const VuDevIface *iface)
> {
> g_assert(dev);
> g_assert(iface);
>
> - vu_init(&dev->parent, socket, panic, set_watch, remove_watch, iface);
> + if (!vu_init(&dev->parent, max_queues, socket, panic, set_watch,
> + remove_watch, iface)) {
> + return false;
> + }
> +
> dev->fdmap = g_hash_table_new_full(NULL, NULL, NULL,
> (GDestroyNotify) g_source_destroy);
>
> dev->src = vug_source_new(dev, socket, G_IO_IN, vug_watch, NULL);
> +
> + return true;
> }
>
> void
> diff --git a/contrib/libvhost-user/libvhost-user.c
> b/contrib/libvhost-user/libvhost-user.c
> index a8657c7af2..0c88431e8f 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -493,9 +493,9 @@ vu_get_features_exec(VuDev *dev, VhostUserMsg *vmsg)
> static void
> vu_set_enable_all_rings(VuDev *dev, bool enabled)
> {
> - int i;
> + uint16_t i;
>
> - for (i = 0; i < VHOST_MAX_NR_VIRTQUEUE; i++) {
> + for (i = 0; i < dev->max_queues; i++) {
> dev->vq[i].enable = enabled;
> }
> }
> @@ -916,7 +916,7 @@ vu_check_queue_msg_file(VuDev *dev, VhostUserMsg *vmsg)
> {
> int index = vmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
>
> - if (index >= VHOST_MAX_NR_VIRTQUEUE) {
> + if (index >= dev->max_queues) {
> vmsg_close_fds(vmsg);
> vu_panic(dev, "Invalid queue index: %u", index);
> return false;
> @@ -1213,7 +1213,7 @@ vu_set_vring_enable_exec(VuDev *dev, VhostUserMsg *vmsg)
> DPRINT("State.index: %d\n", index);
> DPRINT("State.enable: %d\n", enable);
>
> - if (index >= VHOST_MAX_NR_VIRTQUEUE) {
> + if (index >= dev->max_queues) {
> vu_panic(dev, "Invalid vring_enable index: %u", index);
> return false;
> }
> @@ -1582,7 +1582,7 @@ vu_deinit(VuDev *dev)
> }
> dev->nregions = 0;
>
> - for (i = 0; i < VHOST_MAX_NR_VIRTQUEUE; i++) {
> + for (i = 0; i < dev->max_queues; i++) {
> VuVirtq *vq = &dev->vq[i];
>
> if (vq->call_fd != -1) {
> @@ -1627,18 +1627,23 @@ vu_deinit(VuDev *dev)
> if (dev->sock != -1) {
> close(dev->sock);
> }
> +
> + free(dev->vq);
> + dev->vq = NULL;
> }
>
> -void
> +bool
> vu_init(VuDev *dev,
> + uint16_t max_queues,
> int socket,
> vu_panic_cb panic,
> vu_set_watch_cb set_watch,
> vu_remove_watch_cb remove_watch,
> const VuDevIface *iface)
> {
> - int i;
> + uint16_t i;
>
> + assert(max_queues > 0);
> assert(socket >= 0);
> assert(set_watch);
> assert(remove_watch);
> @@ -1654,18 +1659,27 @@ vu_init(VuDev *dev,
> dev->iface = iface;
> dev->log_call_fd = -1;
> dev->slave_fd = -1;
> - for (i = 0; i < VHOST_MAX_NR_VIRTQUEUE; i++) {
> +
> + dev->vq = malloc(max_queues * sizeof(dev->vq[0]));
> + if (!dev->vq) {
> + DPRINT("%s: failed to malloc virtqueues\n", __func__);
> + return false;
> + }
> +
> + for (i = 0; i < max_queues; i++) {
> dev->vq[i] = (VuVirtq) {
> .call_fd = -1, .kick_fd = -1, .err_fd = -1,
> .notification = true,
> };
> }
> +
> + return true;
> }
>
> VuVirtq *
> vu_get_queue(VuDev *dev, int qidx)
> {
> - assert(qidx < VHOST_MAX_NR_VIRTQUEUE);
> + assert(qidx < dev->max_queues);
> return &dev->vq[qidx];
> }
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c
> b/contrib/vhost-user-blk/vhost-user-blk.c
> index 86a3987744..ae61034656 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -25,6 +25,10 @@
> #include <sys/ioctl.h>
> #endif
>
> +enum {
> + VHOST_USER_BLK_MAX_QUEUES = 8,
> +};
why do you use enum,(and not const int) ? (similarly for other devices)
other than than
Reviewed-by: Marc-André Lureau <address@hidden>
> +
> struct virtio_blk_inhdr {
> unsigned char status;
> };
> @@ -334,12 +338,6 @@ static void vub_process_vq(VuDev *vu_dev, int idx)
> VuVirtq *vq;
> int ret;
>
> - if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
> - fprintf(stderr, "VQ Index out of range: %d\n", idx);
> - vub_panic_cb(vu_dev, NULL);
> - return;
> - }
> -
> gdev = container_of(vu_dev, VugDev, parent);
> vdev_blk = container_of(gdev, VubDev, parent);
> assert(vdev_blk);
> @@ -631,7 +629,11 @@ int main(int argc, char **argv)
> vdev_blk->enable_ro = true;
> }
>
> - vug_init(&vdev_blk->parent, csock, vub_panic_cb, &vub_iface);
> + if (!vug_init(&vdev_blk->parent, VHOST_USER_BLK_MAX_QUEUES, csock,
> + vub_panic_cb, &vub_iface)) {
> + fprintf(stderr, "Failed to initialized libvhost-user-glib\n");
> + goto err;
> + }
>
> g_main_loop_run(vdev_blk->loop);
>
> diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
> index 04b753046f..b45d2019b4 100644
> --- a/contrib/vhost-user-gpu/main.c
> +++ b/contrib/vhost-user-gpu/main.c
> @@ -25,6 +25,10 @@
> #include "virgl.h"
> #include "vugbm.h"
>
> +enum {
> + VHOST_USER_GPU_MAX_QUEUES = 2,
> +};
> +
> struct virtio_gpu_simple_resource {
> uint32_t resource_id;
> uint32_t width;
> @@ -1169,7 +1173,10 @@ main(int argc, char *argv[])
> exit(EXIT_FAILURE);
> }
>
> - vug_init(&g.dev, fd, vg_panic, &vuiface);
> + if (!vug_init(&g.dev, VHOST_USER_GPU_MAX_QUEUES, fd, vg_panic,
> &vuiface)) {
> + g_printerr("Failed to initialize libvhost-user-glib.\n");
> + exit(EXIT_FAILURE);
> + }
>
> loop = g_main_loop_new(NULL, FALSE);
> g_main_loop_run(loop);
> diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
> index 8b4e7d2536..449fd2171a 100644
> --- a/contrib/vhost-user-input/main.c
> +++ b/contrib/vhost-user-input/main.c
> @@ -17,6 +17,10 @@
> #include "standard-headers/linux/virtio_input.h"
> #include "qapi/error.h"
>
> +enum {
> + VHOST_USER_INPUT_MAX_QUEUES = 2,
> +};
> +
> typedef struct virtio_input_event virtio_input_event;
> typedef struct virtio_input_config virtio_input_config;
>
> @@ -384,7 +388,12 @@ main(int argc, char *argv[])
> g_printerr("Invalid vhost-user socket.\n");
> exit(EXIT_FAILURE);
> }
> - vug_init(&vi.dev, fd, vi_panic, &vuiface);
> +
> + if (!vug_init(&vi.dev, VHOST_USER_INPUT_MAX_QUEUES, fd, vi_panic,
> + &vuiface)) {
> + g_printerr("Failed to initialize libvhost-user-glib.\n");
> + exit(EXIT_FAILURE);
> + }
>
> loop = g_main_loop_new(NULL, FALSE);
> g_main_loop_run(loop);
> diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c
> b/contrib/vhost-user-scsi/vhost-user-scsi.c
> index 496dd6e693..0fc14d7899 100644
> --- a/contrib/vhost-user-scsi/vhost-user-scsi.c
> +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
> @@ -19,6 +19,10 @@
>
> #define VUS_ISCSI_INITIATOR "iqn.2016-11.com.nutanix:vhost-user-scsi"
>
> +enum {
> + VHOST_USER_SCSI_MAX_QUEUES = 8,
> +};
> +
> typedef struct VusIscsiLun {
> struct iscsi_context *iscsi_ctx;
> int iscsi_lun;
> @@ -231,11 +235,6 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>
> gdev = container_of(vu_dev, VugDev, parent);
> vdev_scsi = container_of(gdev, VusDev, parent);
> - if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
> - g_warning("VQ Index out of range: %d", idx);
> - vus_panic_cb(vu_dev, NULL);
> - return;
> - }
>
> vq = vu_get_queue(vu_dev, idx);
> if (!vq) {
> @@ -295,12 +294,6 @@ static void vus_queue_set_started(VuDev *vu_dev, int
> idx, bool started)
>
> assert(vu_dev);
>
> - if (idx < 0 || idx >= VHOST_MAX_NR_VIRTQUEUE) {
> - g_warning("VQ Index out of range: %d", idx);
> - vus_panic_cb(vu_dev, NULL);
> - return;
> - }
> -
> vq = vu_get_queue(vu_dev, idx);
>
> if (idx == 0 || idx == 1) {
> @@ -398,7 +391,11 @@ int main(int argc, char **argv)
> goto err;
> }
>
> - vug_init(&vdev_scsi->parent, csock, vus_panic_cb, &vus_iface);
> + if (!vug_init(&vdev_scsi->parent, VHOST_USER_SCSI_MAX_QUEUES, csock,
> + vus_panic_cb, &vus_iface)) {
> + g_printerr("Failed to initialize libvhost-user-glib\n");
> + goto err;
> + }
>
> g_main_loop_run(vdev_scsi->loop);
>
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 0bb03af0e5..c4e350e1f5 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -45,6 +45,10 @@
> } \
> } while (0)
>
> +enum {
> + VHOST_USER_BRIDGE_MAX_QUEUES = 8,
> +};
> +
> typedef void (*CallbackFunc)(int sock, void *ctx);
>
> typedef struct Event {
> @@ -512,12 +516,16 @@ vubr_accept_cb(int sock, void *ctx)
> }
> DPRINT("Got connection from remote peer on sock %d\n", conn_fd);
>
> - vu_init(&dev->vudev,
> - conn_fd,
> - vubr_panic,
> - vubr_set_watch,
> - vubr_remove_watch,
> - &vuiface);
> + if (!vu_init(&dev->vudev,
> + VHOST_USER_BRIDGE_MAX_QUEUES,
> + conn_fd,
> + vubr_panic,
> + vubr_set_watch,
> + vubr_remove_watch,
> + &vuiface)) {
> + fprintf(stderr, "Failed to initialize libvhost-user\n");
> + exit(1);
> + }
>
> dispatcher_add(&dev->dispatcher, conn_fd, ctx, vubr_receive_cb);
> dispatcher_remove(&dev->dispatcher, sock);
> @@ -560,12 +568,18 @@ vubr_new(const char *path, bool client)
> if (connect(dev->sock, (struct sockaddr *)&un, len) == -1) {
> vubr_die("connect");
> }
> - vu_init(&dev->vudev,
> - dev->sock,
> - vubr_panic,
> - vubr_set_watch,
> - vubr_remove_watch,
> - &vuiface);
> +
> + if (!vu_init(&dev->vudev,
> + VHOST_USER_BRIDGE_MAX_QUEUES,
> + dev->sock,
> + vubr_panic,
> + vubr_set_watch,
> + vubr_remove_watch,
> + &vuiface)) {
> + fprintf(stderr, "Failed to initialize libvhost-user\n");
> + exit(1);
> + }
> +
> cb = vubr_receive_cb;
> }
>
> @@ -584,7 +598,7 @@ static void *notifier_thread(void *arg)
> int qidx;
>
> while (true) {
> - for (qidx = 0; qidx < VHOST_MAX_NR_VIRTQUEUE; qidx++) {
> + for (qidx = 0; qidx < VHOST_USER_BRIDGE_MAX_QUEUES; qidx++) {
> uint16_t *n = vubr->notifier.addr + pagesize * qidx;
>
> if (*n == qidx) {
> @@ -616,7 +630,7 @@ vubr_host_notifier_setup(VubrDev *dev)
> void *addr;
> int fd;
>
> - length = getpagesize() * VHOST_MAX_NR_VIRTQUEUE;
> + length = getpagesize() * VHOST_USER_BRIDGE_MAX_QUEUES;
>
> fd = mkstemp(template);
> if (fd < 0) {
> --
> 2.21.0
>