| Message ID | 20220321153037.3622127-14-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | various virtio docs, fixes and tweaks | expand |
Hi Alex, On 21/3/22 16:30, Alex Bennée wrote: > At a couple of hundred bytes per notifier allocating one for every > potential queue is very wasteful as most devices only have a few > queues. Instead of having this handled statically dynamically assign > them and track in a GPtrArray. > > [AJB: it's hard to trigger the vhost notifiers code, I assume as it > requires a KVM guest with appropriate backend] > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/virtio/vhost-user.h | 42 ++++++++++++++++- > hw/virtio/vhost-user.c | 83 +++++++++++++++++++++++++++------- > hw/virtio/trace-events | 1 + > 3 files changed, 108 insertions(+), 18 deletions(-) > void vhost_user_cleanup(VhostUserState *user) > { > - int i; > - VhostUserHostNotifier *n; > - > if (!user->chr) { > return; > } > memory_region_transaction_begin(); > - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - n = &user->notifier[i]; > - vhost_user_host_notifier_remove(user, NULL, i); > - object_unparent(OBJECT(&n->mr)); > - } > + user->notifiers = (GPtrArray *) g_ptr_array_free(user->notifiers, true); Since you replaced an iteration by a single function call, the MR transaction guards can be removed now, right? > memory_region_transaction_commit(); > user->chr = NULL; > }
On Mon, 21 Mar 2022 at 11:59, Alex Bennée <alex.bennee@linaro.org> wrote: > > At a couple of hundred bytes per notifier allocating one for every > potential queue is very wasteful as most devices only have a few > queues. Instead of having this handled statically dynamically assign > them and track in a GPtrArray. > > [AJB: it's hard to trigger the vhost notifiers code, I assume as it > requires a KVM guest with appropriate backend] I think vhost works with TCG. There is ioeventfd emulation in QEMU's memory dispatch code, see memory_region_dispatch_write_eventfds(). There is irqfd emulation code for VIRTIO devices in virtio_queue_set_guest_notifier_fd_handler(). Why do you say it's hard to trigger? Stefan > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/virtio/vhost-user.h | 42 ++++++++++++++++- > hw/virtio/vhost-user.c | 83 +++++++++++++++++++++++++++------- > hw/virtio/trace-events | 1 + > 3 files changed, 108 insertions(+), 18 deletions(-) > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h > index 6e0e8a71a3..c6e693cd3f 100644 > --- a/include/hw/virtio/vhost-user.h > +++ b/include/hw/virtio/vhost-user.h > @@ -11,21 +11,61 @@ > #include "chardev/char-fe.h" > #include "hw/virtio/virtio.h" > > +/** > + * VhostUserHostNotifier - notifier information for one queue > + * @rcu: rcu_head for cleanup > + * @mr: memory region of notifier > + * @addr: current mapped address > + * @unmap_addr: address to be un-mapped > + * @idx: virtioqueue index > + * > + * The VhostUserHostNotifier entries are re-used. When an old mapping > + * is to be released it is moved to @unmap_addr and @addr is replaced. > + * Once the RCU process has completed the unmap @unmap_addr is > + * cleared. > + */ > typedef struct VhostUserHostNotifier { > struct rcu_head rcu; > MemoryRegion mr; > void *addr; > void *unmap_addr; > + int idx; > } VhostUserHostNotifier; > > +/** > + * VhostUserState - shared state for all vhost-user devices > + * @chr: the character backend for the socket > + * @notifiers: GPtrArray of @VhostUserHostnotifier > + * @memory_slots: > + */ > typedef struct VhostUserState { > CharBackend *chr; > - VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX]; > + GPtrArray *notifiers; > int memory_slots; > bool supports_config; > } VhostUserState; > > +/** > + * vhost_user_init() - initialise shared vhost_user state > + * @user: allocated area for storing shared state > + * @chr: the chardev for the vhost socket > + * @errp: error handle > + * > + * User can either directly g_new() space for the state or embed > + * VhostUserState in their larger device structure and just point to > + * it. > + * > + * Return: true on success, false on error while setting errp. > + */ > bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp); > + > +/** > + * vhost_user_cleanup() - cleanup state > + * @user: ptr to use state > + * > + * Cleans up shared state and notifiers, callee is responsible for > + * freeing the @VhostUserState memory itself. > + */ > void vhost_user_cleanup(VhostUserState *user); > > #endif > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 6ce082861b..4c0423de55 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -1174,14 +1174,16 @@ static void vhost_user_host_notifier_free(VhostUserHostNotifier *n) > n->unmap_addr = NULL; > } > > -static void vhost_user_host_notifier_remove(VhostUserState *user, > - VirtIODevice *vdev, int queue_idx) > +/* > + * clean-up function for notifier, will finally free the structure > + * under rcu. > + */ > +static void vhost_user_host_notifier_remove(VhostUserHostNotifier *n, > + VirtIODevice *vdev) > { > - VhostUserHostNotifier *n = &user->notifier[queue_idx]; > - > if (n->addr) { > if (vdev) { > - virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false); > + virtio_queue_set_host_notifier_mr(vdev, n->idx, &n->mr, false); > } > assert(!n->unmap_addr); > n->unmap_addr = n->addr; > @@ -1225,6 +1227,15 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable) > return 0; > } > > +static VhostUserHostNotifier *fetch_notifier(VhostUserState *u, > + int idx) > +{ > + if (idx >= u->notifiers->len) { > + return NULL; > + } > + return g_ptr_array_index(u->notifiers, idx); > +} > + > static int vhost_user_get_vring_base(struct vhost_dev *dev, > struct vhost_vring_state *ring) > { > @@ -1237,7 +1248,10 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev, > }; > struct vhost_user *u = dev->opaque; > > - vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index); > + VhostUserHostNotifier *n = fetch_notifier(u->user, ring->index); > + if (n) { > + vhost_user_host_notifier_remove(n, dev->vdev); > + } > > ret = vhost_user_write(dev, &msg, NULL, 0); > if (ret < 0) { > @@ -1502,6 +1516,29 @@ static int vhost_user_slave_handle_config_change(struct vhost_dev *dev) > return dev->config_ops->vhost_dev_config_notifier(dev); > } > > +/* > + * Fetch or create the notifier for a given idx. Newly created > + * notifiers are added to the pointer array that tracks them. > + */ > +static VhostUserHostNotifier *fetch_or_create_notifier(VhostUserState *u, > + int idx) > +{ > + VhostUserHostNotifier *n = NULL; > + if (idx >= u->notifiers->len) { > + g_ptr_array_set_size(u->notifiers, idx); > + } > + > + n = g_ptr_array_index(u->notifiers, idx); > + if (!n) { > + n = g_new0(VhostUserHostNotifier, 1); > + n->idx = idx; > + g_ptr_array_insert(u->notifiers, idx, n); > + trace_vhost_user_create_notifier(idx, n); > + } > + > + return n; > +} > + > static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev, > VhostUserVringArea *area, > int fd) > @@ -1521,9 +1558,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev, > return -EINVAL; > } > > - n = &user->notifier[queue_idx]; > - > - vhost_user_host_notifier_remove(user, vdev, queue_idx); > + /* > + * Fetch notifier and invalidate any old data before setting up > + * new mapped address. > + */ > + n = fetch_or_create_notifier(user, queue_idx); > + vhost_user_host_notifier_remove(n, vdev); > > if (area->u64 & VHOST_USER_VRING_NOFD_MASK) { > return 0; > @@ -2526,6 +2566,20 @@ static int vhost_user_set_inflight_fd(struct vhost_dev *dev, > return vhost_user_write(dev, &msg, &inflight->fd, 1); > } > > +static void vhost_user_state_destroy(gpointer data) > +{ > + VhostUserHostNotifier *n = (VhostUserHostNotifier *) data; > + if (n) { > + vhost_user_host_notifier_remove(n, NULL); > + object_unparent(OBJECT(&n->mr)); > + /* > + * We can't free until vhost_user_host_notifier_remove has > + * done it's thing so schedule the free with RCU. > + */ > + g_free_rcu(n, rcu); > + } > +} > + > bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp) > { > if (user->chr) { > @@ -2534,23 +2588,18 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp) > } > user->chr = chr; > user->memory_slots = 0; > + user->notifiers = g_ptr_array_new_full(VIRTIO_QUEUE_MAX / 4, > + &vhost_user_state_destroy); > return true; > } > > void vhost_user_cleanup(VhostUserState *user) > { > - int i; > - VhostUserHostNotifier *n; > - > if (!user->chr) { > return; > } > memory_region_transaction_begin(); > - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > - n = &user->notifier[i]; > - vhost_user_host_notifier_remove(user, NULL, i); > - object_unparent(OBJECT(&n->mr)); > - } > + user->notifiers = (GPtrArray *) g_ptr_array_free(user->notifiers, true); > memory_region_transaction_commit(); > user->chr = NULL; > } > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > index fd213e2a27..b40392a593 100644 > --- a/hw/virtio/trace-events > +++ b/hw/virtio/trace-events > @@ -23,6 +23,7 @@ vhost_user_postcopy_waker_found(uint64_t client_addr) "0x%"PRIx64 > vhost_user_postcopy_waker_nomatch(const char *rb, uint64_t rb_offset) "%s + 0x%"PRIx64 > vhost_user_read(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32"" > vhost_user_write(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32"" > +vhost_user_create_notifier(int idx, void *n) "idx:%d n:%p" > > # vhost-vdpa.c > vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint64_t iova, uint64_t size, uint64_t uaddr, uint8_t perm, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" uaddr: 0x%"PRIx64" perm: 0x%"PRIx8" type: %"PRIu8 > -- > 2.30.2 > >
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h index 6e0e8a71a3..c6e693cd3f 100644 --- a/include/hw/virtio/vhost-user.h +++ b/include/hw/virtio/vhost-user.h @@ -11,21 +11,61 @@ #include "chardev/char-fe.h" #include "hw/virtio/virtio.h" +/** + * VhostUserHostNotifier - notifier information for one queue + * @rcu: rcu_head for cleanup + * @mr: memory region of notifier + * @addr: current mapped address + * @unmap_addr: address to be un-mapped + * @idx: virtioqueue index + * + * The VhostUserHostNotifier entries are re-used. When an old mapping + * is to be released it is moved to @unmap_addr and @addr is replaced. + * Once the RCU process has completed the unmap @unmap_addr is + * cleared. + */ typedef struct VhostUserHostNotifier { struct rcu_head rcu; MemoryRegion mr; void *addr; void *unmap_addr; + int idx; } VhostUserHostNotifier; +/** + * VhostUserState - shared state for all vhost-user devices + * @chr: the character backend for the socket + * @notifiers: GPtrArray of @VhostUserHostnotifier + * @memory_slots: + */ typedef struct VhostUserState { CharBackend *chr; - VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX]; + GPtrArray *notifiers; int memory_slots; bool supports_config; } VhostUserState; +/** + * vhost_user_init() - initialise shared vhost_user state + * @user: allocated area for storing shared state + * @chr: the chardev for the vhost socket + * @errp: error handle + * + * User can either directly g_new() space for the state or embed + * VhostUserState in their larger device structure and just point to + * it. + * + * Return: true on success, false on error while setting errp. + */ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp); + +/** + * vhost_user_cleanup() - cleanup state + * @user: ptr to use state + * + * Cleans up shared state and notifiers, callee is responsible for + * freeing the @VhostUserState memory itself. + */ void vhost_user_cleanup(VhostUserState *user); #endif diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 6ce082861b..4c0423de55 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1174,14 +1174,16 @@ static void vhost_user_host_notifier_free(VhostUserHostNotifier *n) n->unmap_addr = NULL; } -static void vhost_user_host_notifier_remove(VhostUserState *user, - VirtIODevice *vdev, int queue_idx) +/* + * clean-up function for notifier, will finally free the structure + * under rcu. + */ +static void vhost_user_host_notifier_remove(VhostUserHostNotifier *n, + VirtIODevice *vdev) { - VhostUserHostNotifier *n = &user->notifier[queue_idx]; - if (n->addr) { if (vdev) { - virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false); + virtio_queue_set_host_notifier_mr(vdev, n->idx, &n->mr, false); } assert(!n->unmap_addr); n->unmap_addr = n->addr; @@ -1225,6 +1227,15 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable) return 0; } +static VhostUserHostNotifier *fetch_notifier(VhostUserState *u, + int idx) +{ + if (idx >= u->notifiers->len) { + return NULL; + } + return g_ptr_array_index(u->notifiers, idx); +} + static int vhost_user_get_vring_base(struct vhost_dev *dev, struct vhost_vring_state *ring) { @@ -1237,7 +1248,10 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev, }; struct vhost_user *u = dev->opaque; - vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index); + VhostUserHostNotifier *n = fetch_notifier(u->user, ring->index); + if (n) { + vhost_user_host_notifier_remove(n, dev->vdev); + } ret = vhost_user_write(dev, &msg, NULL, 0); if (ret < 0) { @@ -1502,6 +1516,29 @@ static int vhost_user_slave_handle_config_change(struct vhost_dev *dev) return dev->config_ops->vhost_dev_config_notifier(dev); } +/* + * Fetch or create the notifier for a given idx. Newly created + * notifiers are added to the pointer array that tracks them. + */ +static VhostUserHostNotifier *fetch_or_create_notifier(VhostUserState *u, + int idx) +{ + VhostUserHostNotifier *n = NULL; + if (idx >= u->notifiers->len) { + g_ptr_array_set_size(u->notifiers, idx); + } + + n = g_ptr_array_index(u->notifiers, idx); + if (!n) { + n = g_new0(VhostUserHostNotifier, 1); + n->idx = idx; + g_ptr_array_insert(u->notifiers, idx, n); + trace_vhost_user_create_notifier(idx, n); + } + + return n; +} + static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev, VhostUserVringArea *area, int fd) @@ -1521,9 +1558,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev, return -EINVAL; } - n = &user->notifier[queue_idx]; - - vhost_user_host_notifier_remove(user, vdev, queue_idx); + /* + * Fetch notifier and invalidate any old data before setting up + * new mapped address. + */ + n = fetch_or_create_notifier(user, queue_idx); + vhost_user_host_notifier_remove(n, vdev); if (area->u64 & VHOST_USER_VRING_NOFD_MASK) { return 0; @@ -2526,6 +2566,20 @@ static int vhost_user_set_inflight_fd(struct vhost_dev *dev, return vhost_user_write(dev, &msg, &inflight->fd, 1); } +static void vhost_user_state_destroy(gpointer data) +{ + VhostUserHostNotifier *n = (VhostUserHostNotifier *) data; + if (n) { + vhost_user_host_notifier_remove(n, NULL); + object_unparent(OBJECT(&n->mr)); + /* + * We can't free until vhost_user_host_notifier_remove has + * done it's thing so schedule the free with RCU. + */ + g_free_rcu(n, rcu); + } +} + bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp) { if (user->chr) { @@ -2534,23 +2588,18 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp) } user->chr = chr; user->memory_slots = 0; + user->notifiers = g_ptr_array_new_full(VIRTIO_QUEUE_MAX / 4, + &vhost_user_state_destroy); return true; } void vhost_user_cleanup(VhostUserState *user) { - int i; - VhostUserHostNotifier *n; - if (!user->chr) { return; } memory_region_transaction_begin(); - for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { - n = &user->notifier[i]; - vhost_user_host_notifier_remove(user, NULL, i); - object_unparent(OBJECT(&n->mr)); - } + user->notifiers = (GPtrArray *) g_ptr_array_free(user->notifiers, true); memory_region_transaction_commit(); user->chr = NULL; } diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index fd213e2a27..b40392a593 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -23,6 +23,7 @@ vhost_user_postcopy_waker_found(uint64_t client_addr) "0x%"PRIx64 vhost_user_postcopy_waker_nomatch(const char *rb, uint64_t rb_offset) "%s + 0x%"PRIx64 vhost_user_read(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32"" vhost_user_write(uint32_t req, uint32_t flags) "req:%d flags:0x%"PRIx32"" +vhost_user_create_notifier(int idx, void *n) "idx:%d n:%p" # vhost-vdpa.c vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint64_t iova, uint64_t size, uint64_t uaddr, uint8_t perm, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" uaddr: 0x%"PRIx64" perm: 0x%"PRIx8" type: %"PRIu8
At a couple of hundred bytes per notifier allocating one for every potential queue is very wasteful as most devices only have a few queues. Instead of having this handled statically dynamically assign them and track in a GPtrArray. [AJB: it's hard to trigger the vhost notifiers code, I assume as it requires a KVM guest with appropriate backend] Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/hw/virtio/vhost-user.h | 42 ++++++++++++++++- hw/virtio/vhost-user.c | 83 +++++++++++++++++++++++++++------- hw/virtio/trace-events | 1 + 3 files changed, 108 insertions(+), 18 deletions(-)