| Message ID | 20200614183907.514282-2-coiby.xu@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user block device backend implementation | expand |
Am 14.06.2020 um 20:39 hat Coiby Xu geschrieben: > Allow vu_message_read to be replaced by one which will make use of the > QIOChannel functions. Thus reading vhost-user message won't stall the > guest. > > Signed-off-by: Coiby Xu <coiby.xu@gmail.com> _vu_queue_notify() still has a direct call of vu_message_read() instead of using the pointer. Is this intentional? Renaming the function would make sure that such semantic merge conflicts don't stay unnoticed. > @@ -1704,6 +1702,7 @@ vu_deinit(VuDev *dev) > } > > if (vq->kick_fd != -1) { > + dev->remove_watch(dev, vq->kick_fd); > close(vq->kick_fd); > vq->kick_fd = -1; > } This hunk looks unrelated. Kevin
On Thu, Jun 18, 2020 at 12:43:47PM +0200, Kevin Wolf wrote: >Am 14.06.2020 um 20:39 hat Coiby Xu geschrieben: >> Allow vu_message_read to be replaced by one which will make use of the >> QIOChannel functions. Thus reading vhost-user message won't stall the >> guest. >> >> Signed-off-by: Coiby Xu <coiby.xu@gmail.com> > >_vu_queue_notify() still has a direct call of vu_message_read() instead >of using the pointer. Is this intentional? This is a mistake. Thank you for reminding me! >Renaming the function would make sure that such semantic merge conflicts >don't stay unnoticed. Thank you for this tip! Do you suggest renaming the function only for the purpose of testing or should I adopt a name when submitting the patch? For the latter case, I will change it to vu_message_read_default then. >> @@ -1704,6 +1702,7 @@ vu_deinit(VuDev *dev) >> } >> >> if (vq->kick_fd != -1) { >> + dev->remove_watch(dev, vq->kick_fd); >> close(vq->kick_fd); >> vq->kick_fd = -1; >> } > >This hunk looks unrelated. In v4, I made the comment to explain why it's needed. But libvhost-user is supposed to be independent from QEMU, so Stefan suggested to remove it, > > @@ -1627,6 +1647,12 @@ vu_deinit(VuDev *dev) > > } > > > > if (vq->kick_fd != -1) { > > + /* remove watch for kick_fd > > + * When client process is running in gdb and > > + * quit command is run in gdb, QEMU will still dispatch the event > > + * which will cause segment fault in the callback function > > + */ > > Code and comments in libvhost-user should not refer to QEMU specifics. > Removing the watch is a good idea regardless of the application or event > loop implementation. No comment is needed here. > + /* remove watch for kick_fd > + * When client process is running in gdb and > + * quit command is run in gdb, QEMU will still dispatch the event > + * which will cause segment fault in the callback function > + */ > + dev->remove_watch(dev, vq->kick_fd); -- Best regards, Coiby
Am 24.06.2020 um 05:36 hat Coiby Xu geschrieben: > On Thu, Jun 18, 2020 at 12:43:47PM +0200, Kevin Wolf wrote: > > Am 14.06.2020 um 20:39 hat Coiby Xu geschrieben: > > > Allow vu_message_read to be replaced by one which will make use of the > > > QIOChannel functions. Thus reading vhost-user message won't stall the > > > guest. > > > > > > Signed-off-by: Coiby Xu <coiby.xu@gmail.com> > > > > _vu_queue_notify() still has a direct call of vu_message_read() instead > > of using the pointer. Is this intentional? > > This is a mistake. Thank you for reminding me! > > > Renaming the function would make sure that such semantic merge conflicts > > don't stay unnoticed. > > Thank you for this tip! Do you suggest renaming the function only for > the purpose of testing or should I adopt a name when submitting the > patch? For the latter case, I will change it to vu_message_read_default > then. I think I would rename it permanently and keep the new name for the actual submission. The reason is that if someone else is working on a series adding new references, the compiler would catch it there, too. vu_message_read_default() sounds good to me. > > > @@ -1704,6 +1702,7 @@ vu_deinit(VuDev *dev) > > > } > > > > > > if (vq->kick_fd != -1) { > > > + dev->remove_watch(dev, vq->kick_fd); > > > close(vq->kick_fd); > > > vq->kick_fd = -1; > > > } > > > > This hunk looks unrelated. > > In v4, I made the comment to explain why it's needed. But libvhost-user > is supposed to be independent from QEMU, so Stefan suggested to remove it, Yes, I saw the reason why you need it in later patches. If you can remove it completely, that is even better, but otherwise I would make the addition only later (either in the patch that actually needs it or in a new separate patch) because it's not really part of "allowing vu_message_read to be replaced", as the commit message says. Kevin
diff --git a/contrib/libvhost-user/libvhost-user-glib.c b/contrib/libvhost-user/libvhost-user-glib.c index 53f1ca4cdd..0df2ec9271 100644 --- a/contrib/libvhost-user/libvhost-user-glib.c +++ b/contrib/libvhost-user/libvhost-user-glib.c @@ -147,7 +147,7 @@ vug_init(VugDev *dev, uint16_t max_queues, int socket, g_assert(dev); g_assert(iface); - if (!vu_init(&dev->parent, max_queues, socket, panic, set_watch, + if (!vu_init(&dev->parent, max_queues, socket, panic, NULL, set_watch, remove_watch, iface)) { return false; } diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index 3bca996c62..0c7368baa2 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -67,8 +67,6 @@ /* The version of inflight buffer */ #define INFLIGHT_VERSION 1 -#define VHOST_USER_HDR_SIZE offsetof(VhostUserMsg, payload.u64) - /* The version of the protocol we support */ #define VHOST_USER_VERSION 1 #define LIBVHOST_USER_DEBUG 0 @@ -412,7 +410,7 @@ vu_process_message_reply(VuDev *dev, const VhostUserMsg *vmsg) goto out; } - if (!vu_message_read(dev, dev->slave_fd, &msg_reply)) { + if (!dev->read_msg(dev, dev->slave_fd, &msg_reply)) { goto out; } @@ -647,7 +645,7 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg) /* Wait for QEMU to confirm that it's registered the handler for the * faults. */ - if (!vu_message_read(dev, dev->sock, vmsg) || + if (!dev->read_msg(dev, dev->sock, vmsg) || vmsg->size != sizeof(vmsg->payload.u64) || vmsg->payload.u64 != 0) { vu_panic(dev, "failed to receive valid ack for postcopy set-mem-table"); @@ -1653,7 +1651,7 @@ vu_dispatch(VuDev *dev) int reply_requested; bool need_reply, success = false; - if (!vu_message_read(dev, dev->sock, &vmsg)) { + if (!dev->read_msg(dev, dev->sock, &vmsg)) { goto end; } @@ -1704,6 +1702,7 @@ vu_deinit(VuDev *dev) } if (vq->kick_fd != -1) { + dev->remove_watch(dev, vq->kick_fd); close(vq->kick_fd); vq->kick_fd = -1; } @@ -1751,6 +1750,7 @@ vu_init(VuDev *dev, uint16_t max_queues, int socket, vu_panic_cb panic, + vu_read_msg_cb read_msg, vu_set_watch_cb set_watch, vu_remove_watch_cb remove_watch, const VuDevIface *iface) @@ -1768,6 +1768,7 @@ vu_init(VuDev *dev, dev->sock = socket; dev->panic = panic; + dev->read_msg = read_msg ? read_msg : vu_message_read; dev->set_watch = set_watch; dev->remove_watch = remove_watch; dev->iface = iface; diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h index f30394fab6..d756da8548 100644 --- a/contrib/libvhost-user/libvhost-user.h +++ b/contrib/libvhost-user/libvhost-user.h @@ -30,6 +30,8 @@ #define VHOST_MEMORY_MAX_NREGIONS 8 +#define VHOST_USER_HDR_SIZE offsetof(VhostUserMsg, payload.u64) + typedef enum VhostSetConfigType { VHOST_SET_CONFIG_TYPE_MASTER = 0, VHOST_SET_CONFIG_TYPE_MIGRATION = 1, @@ -205,6 +207,7 @@ typedef uint64_t (*vu_get_features_cb) (VuDev *dev); typedef void (*vu_set_features_cb) (VuDev *dev, uint64_t features); typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg, int *do_reply); +typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg *vmsg); typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool started); typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int qidx); typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t len); @@ -373,6 +376,23 @@ struct VuDev { bool broken; uint16_t max_queues; + /* @read_msg: custom method to read vhost-user message + * + * Read data from vhost_user socket fd and fill up + * the passed VhostUserMsg *vmsg struct. + * + * If reading fails, it should close the received set of file + * descriptors as socket message's auxiliary data. + * + * For the details, please refer to vu_message_read in libvhost-user.c + * which will be used by default if not custom method is provided when + * calling vu_init + * + * Returns: true if vhost-user message successfully received, + * otherwise return false. + * + */ + vu_read_msg_cb read_msg; /* @set_watch: add or update the given fd to the watch set, * call cb when condition is met */ vu_set_watch_cb set_watch; @@ -416,6 +436,7 @@ bool vu_init(VuDev *dev, uint16_t max_queues, int socket, vu_panic_cb panic, + vu_read_msg_cb read_msg, vu_set_watch_cb set_watch, vu_remove_watch_cb remove_watch, const VuDevIface *iface); diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c index 6c3d490611..bd43607a4d 100644 --- a/tests/vhost-user-bridge.c +++ b/tests/vhost-user-bridge.c @@ -520,6 +520,7 @@ vubr_accept_cb(int sock, void *ctx) VHOST_USER_BRIDGE_MAX_QUEUES, conn_fd, vubr_panic, + NULL, vubr_set_watch, vubr_remove_watch, &vuiface)) { @@ -573,6 +574,7 @@ vubr_new(const char *path, bool client) VHOST_USER_BRIDGE_MAX_QUEUES, dev->sock, vubr_panic, + NULL, vubr_set_watch, vubr_remove_watch, &vuiface)) { diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index 3b6d16a041..666945c897 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -980,8 +980,8 @@ int virtio_session_mount(struct fuse_session *se) se->vu_socketfd = data_sock; se->virtio_dev->se = se; pthread_rwlock_init(&se->virtio_dev->vu_dispatch_rwlock, NULL); - vu_init(&se->virtio_dev->dev, 2, se->vu_socketfd, fv_panic, fv_set_watch, - fv_remove_watch, &fv_iface); + vu_init(&se->virtio_dev->dev, 2, se->vu_socketfd, fv_panic, NULL, + fv_set_watch, fv_remove_watch, &fv_iface); return 0; }
Allow vu_message_read to be replaced by one which will make use of the QIOChannel functions. Thus reading vhost-user message won't stall the guest. Signed-off-by: Coiby Xu <coiby.xu@gmail.com> --- contrib/libvhost-user/libvhost-user-glib.c | 2 +- contrib/libvhost-user/libvhost-user.c | 11 ++++++----- contrib/libvhost-user/libvhost-user.h | 21 +++++++++++++++++++++ tests/vhost-user-bridge.c | 2 ++ tools/virtiofsd/fuse_virtio.c | 4 ++-- 5 files changed, 32 insertions(+), 8 deletions(-)