* [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
@ 2018-01-09 16:40 Jay Zhou
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check Jay Zhou
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jay Zhou @ 2018-01-09 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: mst, imammedo, weidong.huang, wangxinxin.wang, arei.gonglei,
liuzhe13, jianjay.zhou
Making the followed up device_add to fall back to userspace
virtio when netdev_add fails if vhost force flag does not set.
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
net/tap.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 979e622..03f226f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
const char *model, const char *name,
const char *ifname, const char *script,
const char *downscript, const char *vhostfdname,
- int vnet_hdr, int fd, Error **errp)
+ int vnet_hdr, int fd, Error **errp,
+ bool *vhost_init_failed)
{
Error *err = NULL;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
@@ -687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
if (vhostfd == -1) {
error_propagate(errp, err);
+ *vhost_init_failed = true;
return;
}
} else {
@@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
if (vhostfd < 0) {
error_setg_errno(errp, errno,
"tap: open vhost char device failed");
+ *vhost_init_failed = true;
return;
}
fcntl(vhostfd, F_SETFL, O_NONBLOCK);
@@ -704,6 +707,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
if (!s->vhost_net) {
error_setg(errp,
"vhost-net requested but could not be initialized");
+ *vhost_init_failed = true;
return;
}
} else if (vhostfdname) {
@@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
Error *err = NULL;
const char *vhostfdname;
char ifname[128];
+ bool vhost_init_failed = false;
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
tap = &netdev->u.tap;
@@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
net_init_tap_one(tap, peer, "tap", name, NULL,
script, downscript,
- vhostfdname, vnet_hdr, fd, &err);
+ vhostfdname, vnet_hdr, fd, &err, &vhost_init_failed);
if (err) {
error_propagate(errp, err);
return -1;
@@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
net_init_tap_one(tap, peer, "tap", name, ifname,
script, downscript,
tap->has_vhostfds ? vhost_fds[i] : NULL,
- vnet_hdr, fd, &err);
+ vnet_hdr, fd, &err, &vhost_init_failed);
if (err) {
error_propagate(errp, err);
goto free_fail;
@@ -874,10 +879,12 @@ free_fail:
net_init_tap_one(tap, peer, "bridge", name, ifname,
script, downscript, vhostfdname,
- vnet_hdr, fd, &err);
+ vnet_hdr, fd, &err, &vhost_init_failed);
if (err) {
error_propagate(errp, err);
- close(fd);
+ if (tap->has_vhostforce && tap->vhostforce && vhost_init_failed) {
+ close(fd);
+ }
return -1;
}
} else {
@@ -913,10 +920,14 @@ free_fail:
net_init_tap_one(tap, peer, "tap", name, ifname,
i >= 1 ? "no" : script,
i >= 1 ? "no" : downscript,
- vhostfdname, vnet_hdr, fd, &err);
+ vhostfdname, vnet_hdr, fd, &err,
+ &vhost_init_failed);
if (err) {
error_propagate(errp, err);
- close(fd);
+ if (tap->has_vhostforce && tap->vhostforce &&
+ vhost_init_failed) {
+ close(fd);
+ }
return -1;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check
2018-01-09 16:40 [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Jay Zhou
@ 2018-01-09 16:40 ` Jay Zhou
2018-01-10 13:16 ` Igor Mammedov
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 4/4] vhost: used_memslots refactoring Jay Zhou
2018-01-10 4:18 ` [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Zhoujian (jay)
2 siblings, 1 reply; 11+ messages in thread
From: Jay Zhou @ 2018-01-09 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: mst, imammedo, weidong.huang, wangxinxin.wang, arei.gonglei,
liuzhe13, jianjay.zhou
Since used_memslots will be updated to the actual value after
registering memory listener for the first time, move the
memslots limit checking to the right place.
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
hw/virtio/vhost.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce..69b3599 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1251,13 +1251,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
goto fail;
}
- if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
- error_report("vhost backend memory slots limit is less"
- " than current number of present memory slots");
- r = -1;
- goto fail;
- }
-
r = hdev->vhost_ops->vhost_set_owner(hdev);
if (r < 0) {
VHOST_OPS_DEBUG("vhost_set_owner failed");
@@ -1339,6 +1332,18 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
hdev->memory_changed = false;
memory_listener_register(&hdev->memory_listener, &address_space_memory);
QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
+
+ if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+ error_report("vhost backend memory slots limit is less"
+ " than current number of present memory slots");
+ r = -1;
+ if (busyloop_timeout) {
+ goto fail_busyloop;
+ } else {
+ goto fail;
+ }
+ }
+
return 0;
fail_busyloop:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v5 4/4] vhost: used_memslots refactoring
2018-01-09 16:40 [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Jay Zhou
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check Jay Zhou
@ 2018-01-09 16:40 ` Jay Zhou
2018-01-10 4:18 ` [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Zhoujian (jay)
2 siblings, 0 replies; 11+ messages in thread
From: Jay Zhou @ 2018-01-09 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: mst, imammedo, weidong.huang, wangxinxin.wang, arei.gonglei,
liuzhe13, jianjay.zhou
Used_memslots is shared by vhost kernel and user, it is equal to
dev->mem->nregions, which is correct for vhost kernel, but not for
vhost user, the latter one uses memory regions that have file
descriptor. E.g. a VM has a vhost-user NIC and 8(vhost user memslot
upper limit) memory slots, it will be failed to hotplug a new DIMM
device since vhost_has_free_slot() finds no free slot left. It
should be successful if only part of memory slots have file
descriptor, so setting used memslots for vhost-user and
vhost-kernel respectively.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
Signed-off-by: Liuzhe <liuzhe13@huawei.com>
---
hw/virtio/vhost-backend.c | 15 +++++++-
hw/virtio/vhost-user.c | 77 ++++++++++++++++++++++++++-------------
hw/virtio/vhost.c | 13 +++----
include/hw/virtio/vhost-backend.h | 6 ++-
4 files changed, 75 insertions(+), 36 deletions(-)
diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 7f09efa..59def69 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -15,6 +15,8 @@
#include "hw/virtio/vhost-backend.h"
#include "qemu/error-report.h"
+static unsigned int vhost_kernel_used_memslots;
+
static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
void *arg)
{
@@ -62,6 +64,11 @@ static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
return limit;
}
+static bool vhost_kernel_has_free_memslots(struct vhost_dev *dev)
+{
+ return vhost_kernel_used_memslots < vhost_kernel_memslots_limit(dev);
+}
+
static int vhost_kernel_net_set_backend(struct vhost_dev *dev,
struct vhost_vring_file *file)
{
@@ -233,11 +240,16 @@ static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL, NULL);
}
+static void vhost_kernel_set_used_memslots(struct vhost_dev *dev)
+{
+ vhost_kernel_used_memslots = dev->mem->nregions;
+}
+
static const VhostOps kernel_ops = {
.backend_type = VHOST_BACKEND_TYPE_KERNEL,
.vhost_backend_init = vhost_kernel_init,
.vhost_backend_cleanup = vhost_kernel_cleanup,
- .vhost_backend_memslots_limit = vhost_kernel_memslots_limit,
+ .vhost_backend_has_free_memslots = vhost_kernel_has_free_memslots,
.vhost_net_set_backend = vhost_kernel_net_set_backend,
.vhost_scsi_set_endpoint = vhost_kernel_scsi_set_endpoint,
.vhost_scsi_clear_endpoint = vhost_kernel_scsi_clear_endpoint,
@@ -264,6 +276,7 @@ static const VhostOps kernel_ops = {
#endif /* CONFIG_VHOST_VSOCK */
.vhost_set_iotlb_callback = vhost_kernel_set_iotlb_callback,
.vhost_send_device_iotlb_msg = vhost_kernel_send_device_iotlb_msg,
+ .vhost_set_used_memslots = vhost_kernel_set_used_memslots,
};
int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8500562..11c7d52 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -122,6 +122,8 @@ static VhostUserMsg m __attribute__ ((unused));
/* The version of the protocol we support */
#define VHOST_USER_VERSION (0x1)
+static bool vhost_user_free_memslots = true;
+
struct vhost_user {
CharBackend *chr;
int slave_fd;
@@ -289,12 +291,43 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
return 0;
}
+static int vhost_user_prepare_msg(struct vhost_dev *dev, VhostUserMemory *mem,
+ int *fds)
+{
+ int i, fd;
+
+ vhost_user_free_memslots = true;
+ for (i = 0, mem->nregions = 0; i < dev->mem->nregions; ++i) {
+ struct vhost_memory_region *reg = dev->mem->regions + i;
+ ram_addr_t offset;
+ MemoryRegion *mr;
+
+ assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
+ mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
+ &offset);
+ fd = memory_region_get_fd(mr);
+ if (fd > 0) {
+ if (mem->nregions == VHOST_MEMORY_MAX_NREGIONS) {
+ vhost_user_free_memslots = false;
+ return -1;
+ }
+
+ mem->regions[mem->nregions].userspace_addr = reg->userspace_addr;
+ mem->regions[mem->nregions].memory_size = reg->memory_size;
+ mem->regions[mem->nregions].guest_phys_addr = reg->guest_phys_addr;
+ mem->regions[mem->nregions].mmap_offset = offset;
+ fds[mem->nregions++] = fd;
+ }
+ }
+
+ return 0;
+}
+
static int vhost_user_set_mem_table(struct vhost_dev *dev,
struct vhost_memory *mem)
{
int fds[VHOST_MEMORY_MAX_NREGIONS];
- int i, fd;
- size_t fd_num = 0;
+ size_t fd_num;
bool reply_supported = virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_REPLY_ACK);
@@ -307,29 +340,12 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
msg.flags |= VHOST_USER_NEED_REPLY_MASK;
}
- for (i = 0; i < dev->mem->nregions; ++i) {
- struct vhost_memory_region *reg = dev->mem->regions + i;
- ram_addr_t offset;
- MemoryRegion *mr;
-
- assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
- mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
- &offset);
- fd = memory_region_get_fd(mr);
- if (fd > 0) {
- if (fd_num == VHOST_MEMORY_MAX_NREGIONS) {
- error_report("Failed preparing vhost-user memory table msg");
- return -1;
- }
- msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
- msg.payload.memory.regions[fd_num].memory_size = reg->memory_size;
- msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
- msg.payload.memory.regions[fd_num].mmap_offset = offset;
- fds[fd_num++] = fd;
- }
+ if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+ error_report("Failed preparing vhost-user memory table msg");
+ return -1;
}
- msg.payload.memory.nregions = fd_num;
+ fd_num = msg.payload.memory.nregions;
if (!fd_num) {
error_report("Failed initializing vhost-user memory map, "
@@ -818,9 +834,9 @@ static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
return idx;
}
-static int vhost_user_memslots_limit(struct vhost_dev *dev)
+static bool vhost_user_has_free_memslots(struct vhost_dev *dev)
{
- return VHOST_MEMORY_MAX_NREGIONS;
+ return vhost_user_free_memslots;
}
static bool vhost_user_requires_shm_log(struct vhost_dev *dev)
@@ -925,11 +941,19 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
/* No-op as the receive channel is not dedicated to IOTLB messages. */
}
+static void vhost_user_set_used_memslots(struct vhost_dev *dev)
+{
+ int fds[VHOST_MEMORY_MAX_NREGIONS];
+ VhostUserMsg msg;
+
+ vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+}
+
const VhostOps user_ops = {
.backend_type = VHOST_BACKEND_TYPE_USER,
.vhost_backend_init = vhost_user_init,
.vhost_backend_cleanup = vhost_user_cleanup,
- .vhost_backend_memslots_limit = vhost_user_memslots_limit,
+ .vhost_backend_has_free_memslots = vhost_user_has_free_memslots,
.vhost_set_log_base = vhost_user_set_log_base,
.vhost_set_mem_table = vhost_user_set_mem_table,
.vhost_set_vring_addr = vhost_user_set_vring_addr,
@@ -951,4 +975,5 @@ const VhostOps user_ops = {
.vhost_net_set_mtu = vhost_user_net_set_mtu,
.vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
.vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
+ .vhost_set_used_memslots = vhost_user_set_used_memslots,
};
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 69b3599..f970ab7 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -43,20 +43,19 @@
static struct vhost_log *vhost_log;
static struct vhost_log *vhost_log_shm;
-static unsigned int used_memslots;
static QLIST_HEAD(, vhost_dev) vhost_devices =
QLIST_HEAD_INITIALIZER(vhost_devices);
bool vhost_has_free_slot(void)
{
- unsigned int slots_limit = ~0U;
struct vhost_dev *hdev;
QLIST_FOREACH(hdev, &vhost_devices, entry) {
- unsigned int r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
- slots_limit = MIN(slots_limit, r);
+ if (!hdev->vhost_ops->vhost_backend_has_free_memslots(hdev)) {
+ return false;
+ }
}
- return slots_limit > used_memslots;
+ return true;
}
static void vhost_dev_sync_region(struct vhost_dev *dev,
@@ -606,7 +605,7 @@ static void vhost_set_memory(MemoryListener *listener,
dev->mem_changed_start_addr = MIN(dev->mem_changed_start_addr, start_addr);
dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr + size - 1);
dev->memory_changed = true;
- used_memslots = dev->mem->nregions;
+ dev->vhost_ops->vhost_set_used_memslots(dev);
}
static bool vhost_section(MemoryRegionSection *section)
@@ -1333,7 +1332,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
memory_listener_register(&hdev->memory_listener, &address_space_memory);
QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
- if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+ if (!hdev->vhost_ops->vhost_backend_has_free_memslots(hdev)) {
error_report("vhost backend memory slots limit is less"
" than current number of present memory slots");
r = -1;
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index a7a5f22..ea01d5f 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -31,7 +31,7 @@ struct vhost_iotlb_msg;
typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
-typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
+typedef bool (*vhost_backend_has_free_memslots)(struct vhost_dev *dev);
typedef int (*vhost_net_set_backend_op)(struct vhost_dev *dev,
struct vhost_vring_file *file);
@@ -84,12 +84,13 @@ typedef void (*vhost_set_iotlb_callback_op)(struct vhost_dev *dev,
int enabled);
typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev,
struct vhost_iotlb_msg *imsg);
+typedef void (*vhost_set_used_memslots_op)(struct vhost_dev *dev);
typedef struct VhostOps {
VhostBackendType backend_type;
vhost_backend_init vhost_backend_init;
vhost_backend_cleanup vhost_backend_cleanup;
- vhost_backend_memslots_limit vhost_backend_memslots_limit;
+ vhost_backend_has_free_memslots vhost_backend_has_free_memslots;
vhost_net_set_backend_op vhost_net_set_backend;
vhost_net_set_mtu_op vhost_net_set_mtu;
vhost_scsi_set_endpoint_op vhost_scsi_set_endpoint;
@@ -118,6 +119,7 @@ typedef struct VhostOps {
vhost_vsock_set_running_op vhost_vsock_set_running;
vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
vhost_send_device_iotlb_msg_op vhost_send_device_iotlb_msg;
+ vhost_set_used_memslots_op vhost_set_used_memslots;
} VhostOps;
extern const VhostOps user_ops;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-09 16:40 [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Jay Zhou
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check Jay Zhou
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 4/4] vhost: used_memslots refactoring Jay Zhou
@ 2018-01-10 4:18 ` Zhoujian (jay)
2018-01-10 6:01 ` Jason Wang
2 siblings, 1 reply; 11+ messages in thread
From: Zhoujian (jay) @ 2018-01-10 4:18 UTC (permalink / raw)
To: qemu-devel@nongnu.org
Cc: mst@redhat.com, imammedo@redhat.com, Huangweidong (C),
wangxin (U), Gonglei (Arei), Liuzhe (Ahriy, Euler), Jason Wang
Sorry about missing to cc Jason.
Close the fd of the tap unconditionally when netdev_add tap,id=net0,vhost=on failed
in net_init_tap_one() will make the followed up device_add virtio-net-pci,netdev=net0
failed too, which prints:
TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
ioctl() failed: Bad file descriptor
This patch makes the followed up device_add be able to fall back to userspace virtio
when netdev_add failed with vhost turning on but vhost force flag does not set.
Here is the original discussion:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html
This is a RFC version, if I'm wrong, please let me know, thanks!
PS: there're two places updated, see below.
> -----Original Message-----
> From: Zhoujian (jay)
> Sent: Wednesday, January 10, 2018 12:40 AM
> To: qemu-devel@nongnu.org
> Cc: mst@redhat.com; imammedo@redhat.com; Huangweidong (C)
> <weidong.huang@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei
> (Arei) <arei.gonglei@huawei.com>; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>;
> Zhoujian (jay) <jianjay.zhou@huawei.com>
> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to
> initialize
>
> Making the followed up device_add to fall back to userspace virtio when
> netdev_add fails if vhost force flag does not set.
>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> ---
> net/tap.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..03f226f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
> const char *model, const char *name,
> const char *ifname, const char *script,
> const char *downscript, const char *vhostfdname,
> - int vnet_hdr, int fd, Error **errp)
> + int vnet_hdr, int fd, Error **errp,
> + bool *vhost_init_failed)
> {
> Error *err = NULL;
> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); @@ -
> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
> vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> if (vhostfd == -1) {
> error_propagate(errp, err);
> + *vhost_init_failed = true;
> return;
> }
> } else {
> @@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
> if (vhostfd < 0) {
> error_setg_errno(errp, errno,
> "tap: open vhost char device failed");
> + *vhost_init_failed = true;
> return;
> }
> fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7 @@ static
> void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> if (!s->vhost_net) {
> error_setg(errp,
> "vhost-net requested but could not be initialized");
> + *vhost_init_failed = true;
> return;
> }
> } else if (vhostfdname) {
> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> Error *err = NULL;
> const char *vhostfdname;
> char ifname[128];
> + bool vhost_init_failed = false;
>
> assert(netdev->type == NET_CLIENT_DRIVER_TAP);
> tap = &netdev->u.tap;
> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>
> net_init_tap_one(tap, peer, "tap", name, NULL,
> script, downscript,
> - vhostfdname, vnet_hdr, fd, &err);
> + vhostfdname, vnet_hdr, fd, &err,
> + &vhost_init_failed);
> if (err) {
> error_propagate(errp, err);
> return -1;
> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> net_init_tap_one(tap, peer, "tap", name, ifname,
> script, downscript,
> tap->has_vhostfds ? vhost_fds[i] : NULL,
> - vnet_hdr, fd, &err);
> + vnet_hdr, fd, &err, &vhost_init_failed);
> if (err) {
> error_propagate(errp, err);
> goto free_fail;
> @@ -874,10 +879,12 @@ free_fail:
>
> net_init_tap_one(tap, peer, "bridge", name, ifname,
> script, downscript, vhostfdname,
> - vnet_hdr, fd, &err);
> + vnet_hdr, fd, &err, &vhost_init_failed);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> + if (tap->has_vhostforce && tap->vhostforce && vhost_init_failed)
> {
> + close(fd);
> + }
This should be:
if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
close(fd);
}
> return -1;
> }
> } else {
> @@ -913,10 +920,14 @@ free_fail:
> net_init_tap_one(tap, peer, "tap", name, ifname,
> i >= 1 ? "no" : script,
> i >= 1 ? "no" : downscript,
> - vhostfdname, vnet_hdr, fd, &err);
> + vhostfdname, vnet_hdr, fd, &err,
> + &vhost_init_failed);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> + if (tap->has_vhostforce && tap->vhostforce &&
> + vhost_init_failed) {
> + close(fd);
> + }
Same here, this should be:
if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
close(fd);
}
> return -1;
> }
> }
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-10 4:18 ` [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Zhoujian (jay)
@ 2018-01-10 6:01 ` Jason Wang
2018-01-10 7:39 ` Zhoujian (jay)
0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2018-01-10 6:01 UTC (permalink / raw)
To: Zhoujian (jay), qemu-devel@nongnu.org
Cc: Huangweidong (C), mst@redhat.com, wangxin (U), Gonglei (Arei),
imammedo@redhat.com, Liuzhe (Ahriy, Euler)
On 2018年01月10日 12:18, Zhoujian (jay) wrote:
> Sorry about missing to cc Jason.
>
>
> Close the fd of the tap unconditionally when netdev_add tap,id=net0,vhost=on failed
> in net_init_tap_one() will make the followed up device_add virtio-net-pci,netdev=net0
> failed too, which prints:
>
> TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
> ioctl() failed: Bad file descriptor
>
> This patch makes the followed up device_add be able to fall back to userspace virtio
> when netdev_add failed with vhost turning on but vhost force flag does not set.
>
> Here is the original discussion:
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html
>
>
> This is a RFC version, if I'm wrong, please let me know, thanks!
>
> PS: there're two places updated, see below.
>
>
>> -----Original Message-----
>> From: Zhoujian (jay)
>> Sent: Wednesday, January 10, 2018 12:40 AM
>> To: qemu-devel@nongnu.org
>> Cc: mst@redhat.com; imammedo@redhat.com; Huangweidong (C)
>> <weidong.huang@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei
>> (Arei) <arei.gonglei@huawei.com>; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>;
>> Zhoujian (jay) <jianjay.zhou@huawei.com>
>> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to
>> initialize
>>
>> Making the followed up device_add to fall back to userspace virtio when
>> netdev_add fails if vhost force flag does not set.
>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
>> ---
>> net/tap.c | 25 ++++++++++++++++++-------
>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 979e622..03f226f 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
>> NetClientState *peer,
>> const char *model, const char *name,
>> const char *ifname, const char *script,
>> const char *downscript, const char *vhostfdname,
>> - int vnet_hdr, int fd, Error **errp)
>> + int vnet_hdr, int fd, Error **errp,
>> + bool *vhost_init_failed)
>> {
>> Error *err = NULL;
>> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); @@ -
>> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
>> NetClientState *peer,
>> vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>> if (vhostfd == -1) {
>> error_propagate(errp, err);
>> + *vhost_init_failed = true;
>> return;
>> }
>> } else {
>> @@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
>> NetClientState *peer,
>> if (vhostfd < 0) {
>> error_setg_errno(errp, errno,
>> "tap: open vhost char device failed");
>> + *vhost_init_failed = true;
>> return;
>> }
>> fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7 @@ static
>> void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>> if (!s->vhost_net) {
>> error_setg(errp,
>> "vhost-net requested but could not be initialized");
>> + *vhost_init_failed = true;
Why not simply check s->vhost_net after call net_init_tap_one()?
Thanks
>> return;
>> }
>> } else if (vhostfdname) {
>> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> Error *err = NULL;
>> const char *vhostfdname;
>> char ifname[128];
>> + bool vhost_init_failed = false;
>>
>> assert(netdev->type == NET_CLIENT_DRIVER_TAP);
>> tap = &netdev->u.tap;
>> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>
>> net_init_tap_one(tap, peer, "tap", name, NULL,
>> script, downscript,
>> - vhostfdname, vnet_hdr, fd, &err);
>> + vhostfdname, vnet_hdr, fd, &err,
>> + &vhost_init_failed);
>> if (err) {
>> error_propagate(errp, err);
>> return -1;
>> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> net_init_tap_one(tap, peer, "tap", name, ifname,
>> script, downscript,
>> tap->has_vhostfds ? vhost_fds[i] : NULL,
>> - vnet_hdr, fd, &err);
>> + vnet_hdr, fd, &err, &vhost_init_failed);
>> if (err) {
>> error_propagate(errp, err);
>> goto free_fail;
>> @@ -874,10 +879,12 @@ free_fail:
>>
>> net_init_tap_one(tap, peer, "bridge", name, ifname,
>> script, downscript, vhostfdname,
>> - vnet_hdr, fd, &err);
>> + vnet_hdr, fd, &err, &vhost_init_failed);
>> if (err) {
>> error_propagate(errp, err);
>> - close(fd);
>> + if (tap->has_vhostforce && tap->vhostforce && vhost_init_failed)
>> {
>> + close(fd);
>> + }
> This should be:
>
> if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
> close(fd);
> }
>
>> return -1;
>> }
>> } else {
>> @@ -913,10 +920,14 @@ free_fail:
>> net_init_tap_one(tap, peer, "tap", name, ifname,
>> i >= 1 ? "no" : script,
>> i >= 1 ? "no" : downscript,
>> - vhostfdname, vnet_hdr, fd, &err);
>> + vhostfdname, vnet_hdr, fd, &err,
>> + &vhost_init_failed);
>> if (err) {
>> error_propagate(errp, err);
>> - close(fd);
>> + if (tap->has_vhostforce && tap->vhostforce &&
>> + vhost_init_failed) {
>> + close(fd);
>> + }
> Same here, this should be:
>
> if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
> close(fd);
> }
>
>> return -1;
>> }
>> }
>> --
>> 1.8.3.1
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-10 6:01 ` Jason Wang
@ 2018-01-10 7:39 ` Zhoujian (jay)
2018-01-11 3:34 ` Jason Wang
0 siblings, 1 reply; 11+ messages in thread
From: Zhoujian (jay) @ 2018-01-10 7:39 UTC (permalink / raw)
To: Jason Wang, qemu-devel@nongnu.org
Cc: Huangweidong (C), mst@redhat.com, wangxin (U), Gonglei (Arei),
imammedo@redhat.com, Liuzhe (Ahriy, Euler)
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Wednesday, January 10, 2018 2:02 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
>
>
>
> On 2018年01月10日 12:18, Zhoujian (jay) wrote:
> > Sorry about missing to cc Jason.
> >
> >
> > Close the fd of the tap unconditionally when netdev_add
> > tap,id=net0,vhost=on failed in net_init_tap_one() will make the
> > followed up device_add virtio-net-pci,netdev=net0 failed too, which prints:
> >
> > TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
> > ioctl() failed: Bad file descriptor
> >
> > This patch makes the followed up device_add be able to fall back to
> > userspace virtio when netdev_add failed with vhost turning on but vhost
> force flag does not set.
> >
> > Here is the original discussion:
> > https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html
> >
> >
> > This is a RFC version, if I'm wrong, please let me know, thanks!
> >
> > PS: there're two places updated, see below.
> >
> >
> >> -----Original Message-----
> >> From: Zhoujian (jay)
> >> Sent: Wednesday, January 10, 2018 12:40 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: mst@redhat.com; imammedo@redhat.com; Huangweidong (C)
> >> <weidong.huang@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> >> Gonglei
> >> (Arei) <arei.gonglei@huawei.com>; Liuzhe (Ahriy, Euler)
> >> <liuzhe13@huawei.com>; Zhoujian (jay) <jianjay.zhou@huawei.com>
> >> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost
> >> failed to initialize
> >>
> >> Making the followed up device_add to fall back to userspace virtio
> >> when netdev_add fails if vhost force flag does not set.
> >>
> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> >> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> >> ---
> >> net/tap.c | 25 ++++++++++++++++++-------
> >> 1 file changed, 18 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/tap.c b/net/tap.c
> >> index 979e622..03f226f 100644
> >> --- a/net/tap.c
> >> +++ b/net/tap.c
> >> @@ -642,7 +642,8 @@ static void net_init_tap_one(const
> >> NetdevTapOptions *tap, NetClientState *peer,
> >> const char *model, const char *name,
> >> const char *ifname, const char *script,
> >> const char *downscript, const char
> *vhostfdname,
> >> - int vnet_hdr, int fd, Error **errp)
> >> + int vnet_hdr, int fd, Error **errp,
> >> + bool *vhost_init_failed)
> >> {
> >> Error *err = NULL;
> >> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> >> @@ -
> >> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions
> >> *tap, NetClientState *peer,
> >> vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> >> if (vhostfd == -1) {
> >> error_propagate(errp, err);
> >> + *vhost_init_failed = true;
> >> return;
> >> }
> >> } else {
> >> @@ -694,6 +696,7 @@ static void net_init_tap_one(const
> >> NetdevTapOptions *tap, NetClientState *peer,
> >> if (vhostfd < 0) {
> >> error_setg_errno(errp, errno,
> >> "tap: open vhost char device
> >> failed");
> >> + *vhost_init_failed = true;
> >> return;
> >> }
> >> fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7
> >> @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
> >> if (!s->vhost_net) {
> >> error_setg(errp,
> >> "vhost-net requested but could not be
> >> initialized");
> >> + *vhost_init_failed = true;
>
> Why not simply check s->vhost_net after call net_init_tap_one()?
s->vhost_net is always NULL if net_init_tap_one() failed, it can't distinguish
failure reasons.
fd should be closed in these two cases:
(1) tap_set_sndbuf() failed, regardless of whether having vhost or vhostforce flag
(2) with vhostforce flag
fd should not be closed if tap_set_sndbuf() succeeded and vhost init failed without
vhostforce flag.
Regards,
Jay
>
> Thanks
>
> >> return;
> >> }
> >> } else if (vhostfdname) {
> >> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char
> *name,
> >> Error *err = NULL;
> >> const char *vhostfdname;
> >> char ifname[128];
> >> + bool vhost_init_failed = false;
> >>
> >> assert(netdev->type == NET_CLIENT_DRIVER_TAP);
> >> tap = &netdev->u.tap;
> >> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char
> >> *name,
> >>
> >> net_init_tap_one(tap, peer, "tap", name, NULL,
> >> script, downscript,
> >> - vhostfdname, vnet_hdr, fd, &err);
> >> + vhostfdname, vnet_hdr, fd, &err,
> >> + &vhost_init_failed);
> >> if (err) {
> >> error_propagate(errp, err);
> >> return -1;
> >> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char
> *name,
> >> net_init_tap_one(tap, peer, "tap", name, ifname,
> >> script, downscript,
> >> tap->has_vhostfds ? vhost_fds[i] : NULL,
> >> - vnet_hdr, fd, &err);
> >> + vnet_hdr, fd, &err,
> >> + &vhost_init_failed);
> >> if (err) {
> >> error_propagate(errp, err);
> >> goto free_fail;
> >> @@ -874,10 +879,12 @@ free_fail:
> >>
> >> net_init_tap_one(tap, peer, "bridge", name, ifname,
> >> script, downscript, vhostfdname,
> >> - vnet_hdr, fd, &err);
> >> + vnet_hdr, fd, &err, &vhost_init_failed);
> >> if (err) {
> >> error_propagate(errp, err);
> >> - close(fd);
> >> + if (tap->has_vhostforce && tap->vhostforce &&
> >> + vhost_init_failed)
> >> {
> >> + close(fd);
> >> + }
> > This should be:
> >
> > if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce))
> {
> > close(fd);
> > }
> >
> >> return -1;
> >> }
> >> } else {
> >> @@ -913,10 +920,14 @@ free_fail:
> >> net_init_tap_one(tap, peer, "tap", name, ifname,
> >> i >= 1 ? "no" : script,
> >> i >= 1 ? "no" : downscript,
> >> - vhostfdname, vnet_hdr, fd, &err);
> >> + vhostfdname, vnet_hdr, fd, &err,
> >> + &vhost_init_failed);
> >> if (err) {
> >> error_propagate(errp, err);
> >> - close(fd);
> >> + if (tap->has_vhostforce && tap->vhostforce &&
> >> + vhost_init_failed) {
> >> + close(fd);
> >> + }
> > Same here, this should be:
> >
> > if (!vhost_init_failed || (tap->has_vhostforce && tap-
> >vhostforce)) {
> > close(fd);
> > }
> >
> >> return -1;
> >> }
> >> }
> >> --
> >> 1.8.3.1
> >>
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check Jay Zhou
@ 2018-01-10 13:16 ` Igor Mammedov
0 siblings, 0 replies; 11+ messages in thread
From: Igor Mammedov @ 2018-01-10 13:16 UTC (permalink / raw)
To: Jay Zhou
Cc: qemu-devel, weidong.huang, mst, wangxinxin.wang, arei.gonglei,
liuzhe13
On Wed, 10 Jan 2018 00:40:12 +0800
Jay Zhou <jianjay.zhou@huawei.com> wrote:
> Since used_memslots will be updated to the actual value after
> registering memory listener for the first time, move the
> memslots limit checking to the right place.
>
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/virtio/vhost.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce..69b3599 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1251,13 +1251,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> goto fail;
> }
>
> - if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> - error_report("vhost backend memory slots limit is less"
> - " than current number of present memory slots");
> - r = -1;
> - goto fail;
> - }
> -
> r = hdev->vhost_ops->vhost_set_owner(hdev);
> if (r < 0) {
> VHOST_OPS_DEBUG("vhost_set_owner failed");
> @@ -1339,6 +1332,18 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> hdev->memory_changed = false;
> memory_listener_register(&hdev->memory_listener, &address_space_memory);
> QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
> +
> + if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> + error_report("vhost backend memory slots limit is less"
> + " than current number of present memory slots");
> + r = -1;
> + if (busyloop_timeout) {
> + goto fail_busyloop;
> + } else {
> + goto fail;
> + }
> + }
> +
> return 0;
>
> fail_busyloop:
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-10 7:39 ` Zhoujian (jay)
@ 2018-01-11 3:34 ` Jason Wang
2018-01-11 3:54 ` Zhoujian (jay)
0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2018-01-11 3:34 UTC (permalink / raw)
To: Zhoujian (jay), qemu-devel@nongnu.org
Cc: Huangweidong (C), mst@redhat.com, wangxin (U), Gonglei (Arei),
imammedo@redhat.com, Liuzhe (Ahriy, Euler)
On 2018年01月10日 15:39, Zhoujian (jay) wrote:
>>>> + *vhost_init_failed = true;
>> Why not simply check s->vhost_net after call net_init_tap_one()?
> s->vhost_net is always NULL if net_init_tap_one() failed, it can't distinguish
> failure reasons.
On which condition net_init_tap_one() fail but s->vhost_net is set?
Thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-11 3:34 ` Jason Wang
@ 2018-01-11 3:54 ` Zhoujian (jay)
2018-01-11 10:30 ` Jason Wang
0 siblings, 1 reply; 11+ messages in thread
From: Zhoujian (jay) @ 2018-01-11 3:54 UTC (permalink / raw)
To: Jason Wang, qemu-devel@nongnu.org
Cc: Huangweidong (C), mst@redhat.com, wangxin (U), Gonglei (Arei),
imammedo@redhat.com, Liuzhe (Ahriy, Euler)
Hi Jason,
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Thursday, January 11, 2018 11:35 AM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
>
>
>
> On 2018年01月10日 15:39, Zhoujian (jay) wrote:
> >>>> + *vhost_init_failed = true;
> >> Why not simply check s->vhost_net after call net_init_tap_one()?
> > s->vhost_net is always NULL if net_init_tap_one() failed, it can't
> distinguish
> > failure reasons.
>
> On which condition net_init_tap_one() fail but s->vhost_net is set?
No, it does not exist, so we could not use s->vhost_net to decide
whether close the fd of tap when error occurred.
Maybe the patch below will be much better to understand, please have a look.
diff --git a/net/tap.c b/net/tap.c
index 979e622..a5c5111 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
const char *model, const char *name,
const char *ifname, const char *script,
const char *downscript, const char *vhostfdname,
- int vnet_hdr, int fd, Error **errp)
+ int vnet_hdr, int fd, Error **errp,
+ bool *error_is_set_sndbuf)
{
Error *err = NULL;
TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
@@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
tap_set_sndbuf(s->fd, tap, &err);
if (err) {
error_propagate(errp, err);
+ if (error_is_set_sndbuf) {
+ *error_is_set_sndbuf = true;
+ }
return;
}
@@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
Error *err = NULL;
const char *vhostfdname;
char ifname[128];
+ bool error_is_set_sndbuf = false;
assert(netdev->type == NET_CLIENT_DRIVER_TAP);
tap = &netdev->u.tap;
@@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
net_init_tap_one(tap, peer, "tap", name, NULL,
script, downscript,
- vhostfdname, vnet_hdr, fd, &err);
+ vhostfdname, vnet_hdr, fd, &err, NULL);
if (err) {
error_propagate(errp, err);
return -1;
@@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
net_init_tap_one(tap, peer, "tap", name, ifname,
script, downscript,
tap->has_vhostfds ? vhost_fds[i] : NULL,
- vnet_hdr, fd, &err);
+ vnet_hdr, fd, &err, NULL);
if (err) {
error_propagate(errp, err);
goto free_fail;
@@ -874,10 +879,13 @@ free_fail:
net_init_tap_one(tap, peer, "bridge", name, ifname,
script, downscript, vhostfdname,
- vnet_hdr, fd, &err);
+ vnet_hdr, fd, &err, &error_is_set_sndbuf);
if (err) {
error_propagate(errp, err);
- close(fd);
+ if (error_is_set_sndbuf || (tap->has_vhostforce &&
+ tap->vhostforce)) {
+ close(fd);
+ }
return -1;
}
} else {
@@ -913,10 +921,14 @@ free_fail:
net_init_tap_one(tap, peer, "tap", name, ifname,
i >= 1 ? "no" : script,
i >= 1 ? "no" : downscript,
- vhostfdname, vnet_hdr, fd, &err);
+ vhostfdname, vnet_hdr, fd, &err,
+ &error_is_set_sndbuf);
if (err) {
error_propagate(errp, err);
- close(fd);
+ if (error_is_set_sndbuf || (tap->has_vhostforce &&
+ tap->vhostforce)) {
+ close(fd);
+ }
return -1;
}
}
PS: I think I do not express the meaning clearly... I can express it in
Chinese in private if necessary
Regards,
Jay
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-11 3:54 ` Zhoujian (jay)
@ 2018-01-11 10:30 ` Jason Wang
2018-01-11 12:31 ` Zhoujian (jay)
0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2018-01-11 10:30 UTC (permalink / raw)
To: Zhoujian (jay), qemu-devel@nongnu.org
Cc: Huangweidong (C), mst@redhat.com, wangxin (U), Gonglei (Arei),
imammedo@redhat.com, Liuzhe (Ahriy, Euler)
On 2018年01月11日 11:54, Zhoujian (jay) wrote:
> Hi Jason,
>
>> -----Original Message-----
>> From: Jason Wang [mailto:jasowang@redhat.com]
>> Sent: Thursday, January 11, 2018 11:35 AM
>> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
>> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
>> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
>> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
>> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
>> vhost failed to initialize
>>
>>
>>
>> On 2018年01月10日 15:39, Zhoujian (jay) wrote:
>>>>>> + *vhost_init_failed = true;
>>>> Why not simply check s->vhost_net after call net_init_tap_one()?
>>> s->vhost_net is always NULL if net_init_tap_one() failed, it can't
>> distinguish
>>> failure reasons.
>> On which condition net_init_tap_one() fail but s->vhost_net is set?
> No, it does not exist, so we could not use s->vhost_net to decide
> whether close the fd of tap when error occurred.
>
>
> Maybe the patch below will be much better to understand, please have a look.
>
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..a5c5111 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> const char *model, const char *name,
> const char *ifname, const char *script,
> const char *downscript, const char *vhostfdname,
> - int vnet_hdr, int fd, Error **errp)
> + int vnet_hdr, int fd, Error **errp,
> + bool *error_is_set_sndbuf)
> {
> Error *err = NULL;
> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> @@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> tap_set_sndbuf(s->fd, tap, &err);
> if (err) {
> error_propagate(errp, err);
> + if (error_is_set_sndbuf) {
> + *error_is_set_sndbuf = true;
> + }
> return;
> }
>
> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> Error *err = NULL;
> const char *vhostfdname;
> char ifname[128];
> + bool error_is_set_sndbuf = false;
>
> assert(netdev->type == NET_CLIENT_DRIVER_TAP);
> tap = &netdev->u.tap;
> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>
> net_init_tap_one(tap, peer, "tap", name, NULL,
> script, downscript,
> - vhostfdname, vnet_hdr, fd, &err);
> + vhostfdname, vnet_hdr, fd, &err, NULL);
> if (err) {
> error_propagate(errp, err);
> return -1;
> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> net_init_tap_one(tap, peer, "tap", name, ifname,
> script, downscript,
> tap->has_vhostfds ? vhost_fds[i] : NULL,
> - vnet_hdr, fd, &err);
> + vnet_hdr, fd, &err, NULL);
> if (err) {
> error_propagate(errp, err);
> goto free_fail;
> @@ -874,10 +879,13 @@ free_fail:
>
> net_init_tap_one(tap, peer, "bridge", name, ifname,
> script, downscript, vhostfdname,
> - vnet_hdr, fd, &err);
> + vnet_hdr, fd, &err, &error_is_set_sndbuf);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> + if (error_is_set_sndbuf || (tap->has_vhostforce &&
> + tap->vhostforce)) {
> + close(fd);
> + }
> return -1;
> }
> } else {
> @@ -913,10 +921,14 @@ free_fail:
> net_init_tap_one(tap, peer, "tap", name, ifname,
> i >= 1 ? "no" : script,
> i >= 1 ? "no" : downscript,
> - vhostfdname, vnet_hdr, fd, &err);
> + vhostfdname, vnet_hdr, fd, &err,
> + &error_is_set_sndbuf);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> + if (error_is_set_sndbuf || (tap->has_vhostforce &&
> + tap->vhostforce)) {
> + close(fd);
> + }
> return -1;
> }
> }
>
>
>
> PS: I think I do not express the meaning clearly... I can express it in
> Chinese in private if necessary
>
> Regards,
> Jay
That's kind of ugly.
I would suggest do all the correct thing in net_tap_init_one().
Thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize
2018-01-11 10:30 ` Jason Wang
@ 2018-01-11 12:31 ` Zhoujian (jay)
0 siblings, 0 replies; 11+ messages in thread
From: Zhoujian (jay) @ 2018-01-11 12:31 UTC (permalink / raw)
To: Jason Wang, qemu-devel@nongnu.org
Cc: Huangweidong (C), mst@redhat.com, wangxin (U), Gonglei (Arei),
imammedo@redhat.com, Liuzhe (Ahriy, Euler)
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Thursday, January 11, 2018 6:30 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
>
>
>
> On 2018年01月11日 11:54, Zhoujian (jay) wrote:
> > Hi Jason,
> >
> >> -----Original Message-----
> >> From: Jason Wang [mailto:jasowang@redhat.com]
> >> Sent: Thursday, January 11, 2018 11:35 AM
> >> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> >> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com;
> >> wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> >> <arei.gonglei@huawei.com>; imammedo@redhat.com; Liuzhe (Ahriy, Euler)
> >> <liuzhe13@huawei.com>
> >> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if
> >> only vhost failed to initialize
> >>
> >>
> >>
> >> On 2018年01月10日 15:39, Zhoujian (jay) wrote:
> >>>>>> + *vhost_init_failed = true;
> >>>> Why not simply check s->vhost_net after call net_init_tap_one()?
> >>> s->vhost_net is always NULL if net_init_tap_one() failed, it can't
> >> distinguish
> >>> failure reasons.
> >> On which condition net_init_tap_one() fail but s->vhost_net is set?
> > No, it does not exist, so we could not use s->vhost_net to decide
> > whether close the fd of tap when error occurred.
> >
> >
> > Maybe the patch below will be much better to understand, please have a look.
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index 979e622..a5c5111 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> > const char *model, const char *name,
> > const char *ifname, const char *script,
> > const char *downscript, const char
> *vhostfdname,
> > - int vnet_hdr, int fd, Error **errp)
> > + int vnet_hdr, int fd, Error **errp,
> > + bool *error_is_set_sndbuf)
> > {
> > Error *err = NULL;
> > TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> > @@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> > tap_set_sndbuf(s->fd, tap, &err);
> > if (err) {
> > error_propagate(errp, err);
> > + if (error_is_set_sndbuf) {
> > + *error_is_set_sndbuf = true;
> > + }
> > return;
> > }
> >
> > @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> > Error *err = NULL;
> > const char *vhostfdname;
> > char ifname[128];
> > + bool error_is_set_sndbuf = false;
> >
> > assert(netdev->type == NET_CLIENT_DRIVER_TAP);
> > tap = &netdev->u.tap;
> > @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char
> > *name,
> >
> > net_init_tap_one(tap, peer, "tap", name, NULL,
> > script, downscript,
> > - vhostfdname, vnet_hdr, fd, &err);
> > + vhostfdname, vnet_hdr, fd, &err, NULL);
> > if (err) {
> > error_propagate(errp, err);
> > return -1;
> > @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> > net_init_tap_one(tap, peer, "tap", name, ifname,
> > script, downscript,
> > tap->has_vhostfds ? vhost_fds[i] : NULL,
> > - vnet_hdr, fd, &err);
> > + vnet_hdr, fd, &err, NULL);
> > if (err) {
> > error_propagate(errp, err);
> > goto free_fail;
> > @@ -874,10 +879,13 @@ free_fail:
> >
> > net_init_tap_one(tap, peer, "bridge", name, ifname,
> > script, downscript, vhostfdname,
> > - vnet_hdr, fd, &err);
> > + vnet_hdr, fd, &err, &error_is_set_sndbuf);
> > if (err) {
> > error_propagate(errp, err);
> > - close(fd);
> > + if (error_is_set_sndbuf || (tap->has_vhostforce &&
> > + tap->vhostforce)) {
> > + close(fd);
> > + }
> > return -1;
> > }
> > } else {
> > @@ -913,10 +921,14 @@ free_fail:
> > net_init_tap_one(tap, peer, "tap", name, ifname,
> > i >= 1 ? "no" : script,
> > i >= 1 ? "no" : downscript,
> > - vhostfdname, vnet_hdr, fd, &err);
> > + vhostfdname, vnet_hdr, fd, &err,
> > + &error_is_set_sndbuf);
> > if (err) {
> > error_propagate(errp, err);
> > - close(fd);
> > + if (error_is_set_sndbuf || (tap->has_vhostforce &&
> > + tap->vhostforce)) {
> > + close(fd);
> > + }
> > return -1;
> > }
> > }
> >
> >
> >
> > PS: I think I do not express the meaning clearly... I can express it
> > in Chinese in private if necessary
> >
> > Regards,
> > Jay
>
> That's kind of ugly.
Oops...
>
> I would suggest do all the correct thing in net_tap_init_one().
Yes, you're right, we could do it better, :)
How about this one(I have done some tests, it works):
---
diff --git a/net/tap.c b/net/tap.c
index 979e622..3ed72eb 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -651,6 +651,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
tap_set_sndbuf(s->fd, tap, &err);
if (err) {
error_propagate(errp, err);
+ if (!tap->has_fd && !tap->has_fds) {
+ close(fd);
+ }
return;
}
@@ -687,14 +690,14 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
if (vhostfd == -1) {
error_propagate(errp, err);
- return;
+ goto cleanup;
}
} else {
vhostfd = open("/dev/vhost-net", O_RDWR);
if (vhostfd < 0) {
error_setg_errno(errp, errno,
"tap: open vhost char device failed");
- return;
+ goto cleanup;
}
fcntl(vhostfd, F_SETFL, O_NONBLOCK);
}
@@ -704,11 +707,18 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
if (!s->vhost_net) {
error_setg(errp,
"vhost-net requested but could not be initialized");
- return;
+ goto cleanup;
}
} else if (vhostfdname) {
error_setg(errp, "vhostfd(s)= is not valid without vhost");
}
+
+cleanup:
+ if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce &&
+ tap->vhostforce) {
+ close(fd);
+ }
+ return;
}
static int get_fds(char *str, char *fds[], int max)
@@ -877,7 +887,6 @@ free_fail:
vnet_hdr, fd, &err);
if (err) {
error_propagate(errp, err);
- close(fd);
return -1;
}
} else {
@@ -916,7 +925,6 @@ free_fail:
vhostfdname, vnet_hdr, fd, &err);
if (err) {
error_propagate(errp, err);
- close(fd);
return -1;
}
}
---
>
> Thanks
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-01-11 12:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-09 16:40 [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Jay Zhou
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check Jay Zhou
2018-01-10 13:16 ` Igor Mammedov
2018-01-09 16:40 ` [Qemu-devel] [PATCH v5 4/4] vhost: used_memslots refactoring Jay Zhou
2018-01-10 4:18 ` [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize Zhoujian (jay)
2018-01-10 6:01 ` Jason Wang
2018-01-10 7:39 ` Zhoujian (jay)
2018-01-11 3:34 ` Jason Wang
2018-01-11 3:54 ` Zhoujian (jay)
2018-01-11 10:30 ` Jason Wang
2018-01-11 12:31 ` Zhoujian (jay)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).