* [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support
@ 2015-09-08 7:38 Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 1/7] vhost-user: use VHOST_USER_XXX macro for switch statement Yuanhan Liu
` (6 more replies)
0 siblings, 7 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
Hi,
Here is the updated patch set for enabling vhost-user multiple queue.
This patch set introduces 2 more vhost user messages: VHOST_USER_GET_QUEUE_NUM,
for querying how many queues the backend supports, and VHOST_USER_SET_VRING_FLAG,
for enabling/disabling a specific virt queue.
Both of th two new messages are treated as vhost protocol extension,
and that's why Michaels's patch "vhost-user: add protocol feature
negotiation" is also included here.
Patch 1-5 are all prepare work for actually enabling multiple queue.
Patch 6 is the key patch for enabling multiple queue, which address
two major concerns from Michael: no feedback from backend if it can't
support # of requested queues, and all messages are sent N time. It
also fixes an hidden bug.
Patch 7 introduces the VHOST_USER_SET_VRING_FLAG message.
Note that I haven't done any formal test yet, it just passes build
test and basic functional test, such as it does exit when backend
doesn't support # of requested queues. Here I sent it out just for
more comments, and for avoiding spending too much effort on the wrong
track.
Thanks.
--yliu
---
Changchun Ouyang (1):
vhost-user: add a new message to disable/enable a specific virt queue.
Michael S. Tsirkin (1):
vhost-user: add protocol feature negotiation
Ouyang Changchun (1):
vhost-user: add multiple queue support
Yuanhan Liu (4):
vhost-user: use VHOST_USER_XXX macro for switch statement
vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE
vhost-user: add VHOST_USER_GET_QUEUE_NUM message
vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
docs/specs/vhost-user.txt | 75 +++++++++++++++++++-
hw/net/vhost_net.c | 45 +++++++++---
hw/net/virtio-net.c | 2 +
hw/virtio/vhost-user.c | 140 ++++++++++++++++++++++++++++++++------
include/hw/virtio/vhost-backend.h | 2 +
include/hw/virtio/vhost.h | 2 +
include/net/net.h | 1 +
include/net/vhost_net.h | 2 +
linux-headers/linux/vhost.h | 2 +-
net/vhost-user.c | 135 +++++++++++++++++++++++++-----------
qapi-schema.json | 6 +-
qemu-options.hx | 5 +-
tests/vhost-user-test.c | 2 +-
13 files changed, 340 insertions(+), 79 deletions(-)
--
1.9.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 1/7] vhost-user: use VHOST_USER_XXX macro for switch statement
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 2/7] vhost-user: add protocol feature negotiation Yuanhan Liu
` (5 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
So that we could let vhost_user_call to handle extented requests,
such as VHOST_USER_GET/SET_PROTOCOL_FEATURES, instead of invoking
vhost_user_read/write and constructing the msg again by ourself.
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
hw/virtio/vhost-user.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e7ab829..33d1b00 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -193,27 +193,32 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
- msg_request = vhost_user_request_translate(request);
+ /* only translate vhost ioctl requests */
+ if (request > VHOST_USER_MAX)
+ msg_request = vhost_user_request_translate(request);
+ else
+ msg_request = request;
+
msg.request = msg_request;
msg.flags = VHOST_USER_VERSION;
msg.size = 0;
- switch (request) {
- case VHOST_GET_FEATURES:
+ switch (msg_request) {
+ case VHOST_USER_GET_FEATURES:
need_reply = 1;
break;
- case VHOST_SET_FEATURES:
- case VHOST_SET_LOG_BASE:
+ case VHOST_USER_SET_FEATURES:
+ case VHOST_USER_SET_LOG_BASE:
msg.u64 = *((__u64 *) arg);
msg.size = sizeof(m.u64);
break;
- case VHOST_SET_OWNER:
- case VHOST_RESET_OWNER:
+ case VHOST_USER_SET_OWNER:
+ case VHOST_USER_RESET_OWNER:
break;
- case VHOST_SET_MEM_TABLE:
+ case VHOST_USER_SET_MEM_TABLE:
for (i = 0; i < dev->mem->nregions; ++i) {
struct vhost_memory_region *reg = dev->mem->regions + i;
ram_addr_t ram_addr;
@@ -246,30 +251,30 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
break;
- case VHOST_SET_LOG_FD:
+ case VHOST_USER_SET_LOG_FD:
fds[fd_num++] = *((int *) arg);
break;
- case VHOST_SET_VRING_NUM:
- case VHOST_SET_VRING_BASE:
+ case VHOST_USER_SET_VRING_NUM:
+ case VHOST_USER_SET_VRING_BASE:
memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
msg.size = sizeof(m.state);
break;
- case VHOST_GET_VRING_BASE:
+ case VHOST_USER_GET_VRING_BASE:
memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
msg.size = sizeof(m.state);
need_reply = 1;
break;
- case VHOST_SET_VRING_ADDR:
+ case VHOST_USER_SET_VRING_ADDR:
memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
msg.size = sizeof(m.addr);
break;
- case VHOST_SET_VRING_KICK:
- case VHOST_SET_VRING_CALL:
- case VHOST_SET_VRING_ERR:
+ case VHOST_USER_SET_VRING_KICK:
+ case VHOST_USER_SET_VRING_CALL:
+ case VHOST_USER_SET_VRING_ERR:
file = arg;
msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
msg.size = sizeof(m.u64);
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 2/7] vhost-user: add protocol feature negotiation
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 1/7] vhost-user: use VHOST_USER_XXX macro for switch statement Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 3/7] vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE Yuanhan Liu
` (4 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
From: "Michael S. Tsirkin" <mst@redhat.com>
Support a separate bitmask for vhost-user protocol features,
and messages to get/set protocol features.
Invoke them at init.
No features are defined yet.
v2: leverage vhost_user_call for request handling -- Yuanhan Liu
Signed-off-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 37 +++++++++++++++++++++++++++++++++++++
hw/net/vhost_net.c | 2 ++
hw/virtio/vhost-user.c | 28 ++++++++++++++++++++++++++++
include/hw/virtio/vhost.h | 1 +
4 files changed, 68 insertions(+)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 650bb18..70da3b1 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -113,6 +113,7 @@ message replies. Most of the requests don't require replies. Here is a list of
the ones that do:
* VHOST_GET_FEATURES
+ * VHOST_GET_PROTOCOL_FEATURES
* VHOST_GET_VRING_BASE
There are several messages that the master sends with file descriptors passed
@@ -127,6 +128,13 @@ in the ancillary data:
If Master is unable to send the full message or receives a wrong reply it will
close the connection. An optional reconnection mechanism can be implemented.
+Any protocol extensions are gated by protocol feature bits,
+which allows full backwards compatibility on both master
+and slave.
+As older slaves don't support negotiating protocol features,
+a feature bit was dedicated for this purpose:
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+
Message types
-------------
@@ -138,6 +146,8 @@ Message types
Slave payload: u64
Get from the underlying vhost implementation the features bitmask.
+ Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals slave support for
+ VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.
* VHOST_USER_SET_FEATURES
@@ -146,6 +156,33 @@ Message types
Master payload: u64
Enable features in the underlying vhost implementation using a bitmask.
+ Feature bit VHOST_USER_F_PROTOCOL_FEATURES signals slave support for
+ VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES.
+
+ * VHOST_USER_GET_PROTOCOL_FEATURES
+
+ Id: 15
+ Equivalent ioctl: VHOST_GET_FEATURES
+ Master payload: N/A
+ Slave payload: u64
+
+ Get the protocol feature bitmask from the underlying vhost implementation.
+ Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present in
+ VHOST_USER_GET_FEATURES.
+ Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
+ this message even before VHOST_USER_SET_FEATURES was called.
+
+ * VHOST_USER_SET_PROTOCOL_FEATURES
+
+ Id: 16
+ Ioctl: VHOST_SET_FEATURES
+ Master payload: u64
+
+ Enable protocol features in the underlying vhost implementation.
+ Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is present in
+ VHOST_USER_GET_FEATURES.
+ Note: slave that reported VHOST_USER_F_PROTOCOL_FEATURES must support
+ this message even before VHOST_USER_SET_FEATURES was called.
* VHOST_USER_SET_OWNER
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 5c1d11f..c864237 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -152,8 +152,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
net->dev.backend_features = qemu_has_vnet_hdr(options->net_backend)
? 0 : (1ULL << VHOST_NET_F_VIRTIO_NET_HDR);
net->backend = r;
+ net->dev.protocol_features = 0;
} else {
net->dev.backend_features = 0;
+ net->dev.protocol_features = 0;
net->backend = -1;
}
net->nc = options->net_backend;
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 33d1b00..b4d8952 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -24,6 +24,8 @@
#include <linux/vhost.h>
#define VHOST_MEMORY_MAX_NREGIONS 8
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
typedef enum VhostUserRequest {
VHOST_USER_NONE = 0,
@@ -41,6 +43,8 @@ typedef enum VhostUserRequest {
VHOST_USER_SET_VRING_KICK = 12,
VHOST_USER_SET_VRING_CALL = 13,
VHOST_USER_SET_VRING_ERR = 14,
+ VHOST_USER_GET_PROTOCOL_FEATURES = 15,
+ VHOST_USER_SET_PROTOCOL_FEATURES = 16,
VHOST_USER_MAX
} VhostUserRequest;
@@ -205,11 +209,13 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
switch (msg_request) {
case VHOST_USER_GET_FEATURES:
+ case VHOST_USER_GET_PROTOCOL_FEATURES:
need_reply = 1;
break;
case VHOST_USER_SET_FEATURES:
case VHOST_USER_SET_LOG_BASE:
+ case VHOST_USER_SET_PROTOCOL_FEATURES:
msg.u64 = *((__u64 *) arg);
msg.size = sizeof(m.u64);
break;
@@ -307,6 +313,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
switch (msg_request) {
case VHOST_USER_GET_FEATURES:
+ case VHOST_USER_GET_PROTOCOL_FEATURES:
if (msg.size != sizeof(m.u64)) {
error_report("Received bad msg size.");
return -1;
@@ -332,10 +339,31 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
static int vhost_user_init(struct vhost_dev *dev, void *opaque)
{
+ unsigned long long features;
+ int err;
+
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
dev->opaque = opaque;
+ err = vhost_user_call(dev, VHOST_USER_GET_FEATURES, &features);
+ if (err < 0)
+ return err;
+
+ if (__virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
+ dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+
+ err = vhost_user_call(dev, VHOST_USER_GET_PROTOCOL_FEATURES, &features);
+ if (err < 0)
+ return err;
+
+ dev->protocol_features = features & VHOST_USER_PROTOCOL_FEATURE_MASK;
+ err = vhost_user_call(dev, VHOST_USER_SET_PROTOCOL_FEATURES,
+ &dev->protocol_features);
+ if (err < 0)
+ return err;
+ }
+
return 0;
}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index dd51050..6467c73 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -47,6 +47,7 @@ struct vhost_dev {
unsigned long long features;
unsigned long long acked_features;
unsigned long long backend_features;
+ unsigned long long protocol_features;
bool started;
bool log_enabled;
unsigned long long log_size;
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 3/7] vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 1/7] vhost-user: use VHOST_USER_XXX macro for switch statement Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 2/7] vhost-user: add protocol feature negotiation Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 4/7] vhost-user: add VHOST_USER_GET_QUEUE_NUM message Yuanhan Liu
` (3 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
Quote from Michael:
We really should rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE.
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 4 ++--
hw/net/vhost_net.c | 2 +-
hw/virtio/vhost-user.c | 6 +++---
| 2 +-
tests/vhost-user-test.c | 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 70da3b1..ccbbcbb 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -194,10 +194,10 @@ Message types
as an owner of the session. This can be used on the Slave as a
"session start" flag.
- * VHOST_USER_RESET_OWNER
+ * VHOST_USER_RESET_DEVICE
Id: 4
- Equivalent ioctl: VHOST_RESET_OWNER
+ Equivalent ioctl: VHOST_RESET_DEVICE
Master payload: N/A
Issued when a new connection is about to be closed. The Master will no
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index c864237..33b0e97 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -287,7 +287,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
} else if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
const VhostOps *vhost_ops = net->dev.vhost_ops;
- int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
+ int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_DEVICE,
NULL);
assert(r >= 0);
}
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b4d8952..192ea4e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -32,7 +32,7 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_FEATURES = 1,
VHOST_USER_SET_FEATURES = 2,
VHOST_USER_SET_OWNER = 3,
- VHOST_USER_RESET_OWNER = 4,
+ VHOST_USER_RESET_DEVICE = 4,
VHOST_USER_SET_MEM_TABLE = 5,
VHOST_USER_SET_LOG_BASE = 6,
VHOST_USER_SET_LOG_FD = 7,
@@ -98,7 +98,7 @@ static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
- VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
+ VHOST_RESET_DEVICE, /* VHOST_USER_RESET_DEVICE */
VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
@@ -221,7 +221,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
break;
case VHOST_USER_SET_OWNER:
- case VHOST_USER_RESET_OWNER:
+ case VHOST_USER_RESET_DEVICE:
break;
case VHOST_USER_SET_MEM_TABLE:
--git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
index ead86db..14a0160 100644
--- a/linux-headers/linux/vhost.h
+++ b/linux-headers/linux/vhost.h
@@ -78,7 +78,7 @@ struct vhost_memory {
#define VHOST_SET_OWNER _IO(VHOST_VIRTIO, 0x01)
/* Give up ownership, and reset the device to default values.
* Allows subsequent call to VHOST_OWNER_SET to succeed. */
-#define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
+#define VHOST_RESET_DEVICE _IO(VHOST_VIRTIO, 0x02)
/* Set up/modify memory layout */
#define VHOST_SET_MEM_TABLE _IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 75fedf0..e301db7 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -58,7 +58,7 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_FEATURES = 1,
VHOST_USER_SET_FEATURES = 2,
VHOST_USER_SET_OWNER = 3,
- VHOST_USER_RESET_OWNER = 4,
+ VHOST_USER_RESET_DEVICE = 4,
VHOST_USER_SET_MEM_TABLE = 5,
VHOST_USER_SET_LOG_BASE = 6,
VHOST_USER_SET_LOG_FD = 7,
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 4/7] vhost-user: add VHOST_USER_GET_QUEUE_NUM message
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
` (2 preceding siblings ...)
2015-09-08 7:38 ` [Qemu-devel] [PATCH 3/7] vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init Yuanhan Liu
` (2 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
This is for querying how many queues the backend supports if it has mq
support(when VHOST_USER_PROTOCOL_F_MQ flag is set from the quried
protocol features).
vhost_net_get_max_queues() is the interface to export that value, and
to tell if the backend supports # of queues user requested, which is
done in the following patch.
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 11 +++++++++++
hw/net/vhost_net.c | 7 +++++++
hw/virtio/vhost-user.c | 14 +++++++++++++-
include/hw/virtio/vhost.h | 1 +
include/net/vhost_net.h | 1 +
5 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index ccbbcbb..43db9b4 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -301,3 +301,14 @@ Message types
Bits (0-7) of the payload contain the vring index. Bit 8 is the
invalid FD flag. This flag is set when there is no file descriptor
in the ancillary data.
+
+ * VHOST_USER_GET_QUEUE_NUM
+
+ Id: 17
+ Equivalent ioctl: N/A
+ Master payload: N/A
+ Slave payload: u64
+
+ Query how many queues the backend supports. This request should be
+ sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
+ features by VHOST_USER_GET_PROTOCOL_FEATURES.
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 33b0e97..f9441e9 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -122,6 +122,11 @@ void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features);
}
+uint64_t vhost_net_get_max_queues(VHostNetState *net)
+{
+ return net->dev.max_queues;
+}
+
static int vhost_net_get_fd(NetClientState *backend)
{
switch (backend->info->type) {
@@ -144,6 +149,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
goto fail;
}
+ net->dev.max_queues = 1;
+
if (backend_kernel) {
r = vhost_net_get_fd(options->net_backend);
if (r < 0) {
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 192ea4e..8046bc0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -25,7 +25,9 @@
#define VHOST_MEMORY_MAX_NREGIONS 8
#define VHOST_USER_F_PROTOCOL_FEATURES 30
-#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
+#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
+
+#define VHOST_USER_PROTOCOL_F_MQ 0
typedef enum VhostUserRequest {
VHOST_USER_NONE = 0,
@@ -45,6 +47,7 @@ typedef enum VhostUserRequest {
VHOST_USER_SET_VRING_ERR = 14,
VHOST_USER_GET_PROTOCOL_FEATURES = 15,
VHOST_USER_SET_PROTOCOL_FEATURES = 16,
+ VHOST_USER_GET_QUEUE_NUM = 17,
VHOST_USER_MAX
} VhostUserRequest;
@@ -210,6 +213,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
switch (msg_request) {
case VHOST_USER_GET_FEATURES:
case VHOST_USER_GET_PROTOCOL_FEATURES:
+ case VHOST_USER_GET_QUEUE_NUM:
need_reply = 1;
break;
@@ -314,6 +318,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
switch (msg_request) {
case VHOST_USER_GET_FEATURES:
case VHOST_USER_GET_PROTOCOL_FEATURES:
+ case VHOST_USER_GET_QUEUE_NUM:
if (msg.size != sizeof(m.u64)) {
error_report("Received bad msg size.");
return -1;
@@ -362,6 +367,13 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
&dev->protocol_features);
if (err < 0)
return err;
+
+ /* query the max queues we support if backend supports Multiple Queue */
+ if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ)) {
+ err = vhost_user_call(dev, VHOST_USER_GET_QUEUE_NUM, &dev->max_queues);
+ if (err < 0)
+ return err;
+ }
}
return 0;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 6467c73..c3758f3 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -48,6 +48,7 @@ struct vhost_dev {
unsigned long long acked_features;
unsigned long long backend_features;
unsigned long long protocol_features;
+ unsigned long long max_queues;
bool started;
bool log_enabled;
unsigned long long log_size;
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 840d4b1..8db723e 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -13,6 +13,7 @@ typedef struct VhostNetOptions {
void *opaque;
} VhostNetOptions;
+uint64_t vhost_net_get_max_queues(VHostNetState *net);
struct vhost_net *vhost_net_init(VhostNetOptions *options);
int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, int total_queues);
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
` (3 preceding siblings ...)
2015-09-08 7:38 ` [Qemu-devel] [PATCH 4/7] vhost-user: add VHOST_USER_GET_QUEUE_NUM message Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-10 3:14 ` Jason Wang
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue Yuanhan Liu
6 siblings, 1 reply; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
So that we could use the `vq_index' as well in the vhost_net_init
stage, which is required when adding vhost-user multiple-queue support,
where we need the vq_index to indicate which queue pair we are gonna
initiate.
vhost-user has no multiple queue support yet, hence no queue_index set
before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
will be set properly soon in the next patch.
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
hw/net/vhost_net.c | 16 +++++++---------
net/vhost-user.c | 1 +
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f9441e9..141b557 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
}
}
+static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
+{
+ net->dev.vq_index = vq_index;
+}
+
struct vhost_net *vhost_net_init(VhostNetOptions *options)
{
int r;
@@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
}
net->nc = options->net_backend;
+ vhost_net_set_vq_index(net, net->nc->queue_index * 2);
+
net->dev.nvqs = 2;
net->dev.vqs = net->vqs;
@@ -196,11 +203,6 @@ fail:
return NULL;
}
-static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
-{
- net->dev.vq_index = vq_index;
-}
-
static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer,
bool set)
{
@@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
goto err;
}
- for (i = 0; i < total_queues; i++) {
- vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
- }
-
r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
if (r < 0) {
error_report("Error binding guest notifier: %d", -r);
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 93dcecd..2d6bbe5 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
/* We don't provide a receive callback */
s->nc.receive_disabled = 1;
s->chr = chr;
+ nc->queue_index = 0;
qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
` (4 preceding siblings ...)
2015-09-08 7:38 ` [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-08 21:22 ` Eric Blake
` (4 more replies)
2015-09-08 7:38 ` [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue Yuanhan Liu
6 siblings, 5 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
From: Ouyang Changchun <changchun.ouyang@intel.com>
This patch is initially based a patch from Nikolay Nikolaev.
Here is the latest version for adding vhost-user multiple queue support,
by creating a nc and vhost_net pair for each queue.
What differs from last version is that this patch addresses two major
concerns from Michael, and fixes one hidden bug.
- Concern #1: no feedback when the backend can't support # of
requested queues(by providing queues=# option).
Here we address this issue by querying VHOST_USER_PROTOCOL_F_MQ
protocol features first, if not set, it means the backend don't
support mq feature, and let max_queues be 1. Otherwise, we send
another message, VHOST_USER_GET_QUEUE_NUM, for getting the max_queues
the backend supports.
At vhost-user initiation stage(net_vhost_user_start), we then initiate
one queue first, which, in the meantime, also gets the max_queues.
We then do a simple compare: if requested_queues > max_queues, we
exit(I guess it's safe to exit here, as the vm is not running yet).
- Concern #2: some messages are sent more times than necessary.
We came an agreement with Michael that we could categorize vhost
user messages to 2 types: none-vring specific messages, which should
be sent only once, and vring specific messages, which should be sent
per queue.
Here I introduced a helper function vhost_user_one_time_request(),
which lists following messages as none-vring specific messages:
VHOST_USER_GET_FEATURES
VHOST_USER_SET_FEATURES
VHOST_USER_GET_PROTOCOL_FEATURES
VHOST_USER_SET_PROTOCOL_FEATURES
VHOST_USER_SET_OWNER
VHOST_USER_RESET_DEVICE
VHOST_USER_SET_MEM_TABLE
VHOST_USER_GET_QUEUE_NUM
For above messages, we simply ignore them when they are not sent the first
time.
I also observed a hidden bug from last version. We register the char dev
event handler N times, which is not necessary, as well as buggy: A later
register overwrites the former one, as qemu_chr_add_handlers() will not
chain those handlers, but instead overwrites the old one. So, in theory,
invoking qemu_chr_add_handlers N times will not end up with calling the
handler N times.
However, the reason the handler is invoked N times is because we start
the backend(the connection server) first, and hence when net_vhost_user_init()
is executed, the connection is already established, and qemu_chr_add_handlers()
then invoke the handler immediately, which just looks like we invoke the
handler(net_vhost_user_event) directly from net_vhost_user_init().
The solution I came up with is to make VhostUserState as an upper level
structure, making it includes N nc and vhost_net pairs:
struct VhostUserNetPeer {
NetClientState *nc;
VHostNetState *vhost_net;
};
typedef struct VhostUserState {
CharDriverState *chr;
bool running;
int queues;
struct VhostUserNetPeer peers[];
} VhostUserState;
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 13 +++++
hw/virtio/vhost-user.c | 31 ++++++++++-
include/net/net.h | 1 +
net/vhost-user.c | 136 ++++++++++++++++++++++++++++++++--------------
qapi-schema.json | 6 +-
qemu-options.hx | 5 +-
6 files changed, 146 insertions(+), 46 deletions(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 43db9b4..99d79be 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
a feature bit was dedicated for this purpose:
#define VHOST_USER_F_PROTOCOL_FEATURES 30
+Multiple queue support
+-------------------
+Multiple queue is treated as a protocal extension, hence the slave has to
+implement protocol features first. Multiple queues is supported only when
+the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
+
+The max # of queues the slave support can be queried with message
+VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
+queues is bigger than that.
+
+As all queues share one connection, the master use a unique index for each
+queue in the sent message to identify one specified queue.
+
Message types
-------------
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8046bc0..11e46b5 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,6 +187,23 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
0 : -1;
}
+static bool vhost_user_one_time_request(VhostUserRequest request)
+{
+ switch (request) {
+ case VHOST_USER_GET_FEATURES:
+ case VHOST_USER_SET_FEATURES:
+ case VHOST_USER_GET_PROTOCOL_FEATURES:
+ case VHOST_USER_SET_PROTOCOL_FEATURES:
+ case VHOST_USER_SET_OWNER:
+ case VHOST_USER_RESET_DEVICE:
+ case VHOST_USER_SET_MEM_TABLE:
+ case VHOST_USER_GET_QUEUE_NUM:
+ return true;
+ default:
+ return false;
+ }
+}
+
static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
void *arg)
{
@@ -206,6 +223,14 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
else
msg_request = request;
+ /*
+ * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
+ * we just need send it once in the first time. For later such
+ * request, we just ignore it.
+ */
+ if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0)
+ return 0;
+
msg.request = msg_request;
msg.flags = VHOST_USER_VERSION;
msg.size = 0;
@@ -268,17 +293,20 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
case VHOST_USER_SET_VRING_NUM:
case VHOST_USER_SET_VRING_BASE:
memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
+ msg.addr.index += dev->vq_index;
msg.size = sizeof(m.state);
break;
case VHOST_USER_GET_VRING_BASE:
memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
+ msg.addr.index += dev->vq_index;
msg.size = sizeof(m.state);
need_reply = 1;
break;
case VHOST_USER_SET_VRING_ADDR:
memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
+ msg.addr.index += dev->vq_index;
msg.size = sizeof(m.addr);
break;
@@ -286,7 +314,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
case VHOST_USER_SET_VRING_CALL:
case VHOST_USER_SET_VRING_ERR:
file = arg;
- msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
+ msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
msg.size = sizeof(m.u64);
if (ioeventfd_enabled() && file->fd > 0) {
fds[fd_num++] = file->fd;
@@ -330,6 +358,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
error_report("Received bad msg size.");
return -1;
}
+ msg.state.index -= dev->vq_index;
memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
break;
default:
diff --git a/include/net/net.h b/include/net/net.h
index 6a6cbef..6f20656 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -92,6 +92,7 @@ struct NetClientState {
NetClientDestructor *destructor;
unsigned int queue_index;
unsigned rxfilter_notify_enabled:1;
+ void *opaque;
};
typedef struct NICState {
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 2d6bbe5..7d4ac69 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -15,10 +15,16 @@
#include "qemu/config-file.h"
#include "qemu/error-report.h"
+struct VhostUserNetPeer {
+ NetClientState *nc;
+ VHostNetState *vhost_net;
+};
+
typedef struct VhostUserState {
- NetClientState nc;
CharDriverState *chr;
- VHostNetState *vhost_net;
+ bool running;
+ int queues;
+ struct VhostUserNetPeer peers[];
} VhostUserState;
typedef struct VhostUserChardevProps {
@@ -29,48 +35,75 @@ typedef struct VhostUserChardevProps {
VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
{
- VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
+ VhostUserState *s = nc->opaque;
assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
- return s->vhost_net;
-}
-
-static int vhost_user_running(VhostUserState *s)
-{
- return (s->vhost_net) ? 1 : 0;
+ return s->peers[nc->queue_index].vhost_net;
}
static int vhost_user_start(VhostUserState *s)
{
VhostNetOptions options;
+ VHostNetState *vhost_net;
+ int max_queues;
+ int i = 0;
- if (vhost_user_running(s)) {
+ if (s->running)
return 0;
- }
options.backend_type = VHOST_BACKEND_TYPE_USER;
- options.net_backend = &s->nc;
options.opaque = s->chr;
- s->vhost_net = vhost_net_init(&options);
+ options.net_backend = s->peers[i].nc;
+ vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
+
+ max_queues = vhost_net_get_max_queues(vhost_net);
+ if (s->queues >= max_queues) {
+ error_report("you are asking more queues than supported: %d\n",
+ max_queues);
+ return -1;
+ }
+
+ for (; i < s->queues; i++) {
+ options.net_backend = s->peers[i].nc;
+
+ s->peers[i].vhost_net = vhost_net_init(&options);
+ if (!s->peers[i].vhost_net)
+ return -1;
+ }
+ s->running = true;
- return vhost_user_running(s) ? 0 : -1;
+ return 0;
}
static void vhost_user_stop(VhostUserState *s)
{
- if (vhost_user_running(s)) {
- vhost_net_cleanup(s->vhost_net);
+ int i;
+ VHostNetState *vhost_net;
+
+ if (!s->running)
+ return;
+
+ for (i = 0; i < s->queues; i++) {
+ vhost_net = s->peers[i].vhost_net;
+ if (vhost_net)
+ vhost_net_cleanup(vhost_net);
}
- s->vhost_net = 0;
+ s->running = false;
}
static void vhost_user_cleanup(NetClientState *nc)
{
- VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
+ VhostUserState *s = nc->opaque;
+ VHostNetState *vhost_net = s->peers[nc->queue_index].vhost_net;
+
+ if (vhost_net)
+ vhost_net_cleanup(vhost_net);
- vhost_user_stop(s);
qemu_purge_queued_packets(nc);
+
+ if (nc->queue_index == s->queues - 1)
+ free(s);
}
static bool vhost_user_has_vnet_hdr(NetClientState *nc)
@@ -89,7 +122,7 @@ static bool vhost_user_has_ufo(NetClientState *nc)
static NetClientInfo net_vhost_user_info = {
.type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
- .size = sizeof(VhostUserState),
+ .size = sizeof(NetClientState),
.cleanup = vhost_user_cleanup,
.has_vnet_hdr = vhost_user_has_vnet_hdr,
.has_ufo = vhost_user_has_ufo,
@@ -97,18 +130,25 @@ static NetClientInfo net_vhost_user_info = {
static void net_vhost_link_down(VhostUserState *s, bool link_down)
{
- s->nc.link_down = link_down;
+ NetClientState *nc;
+ int i;
- if (s->nc.peer) {
- s->nc.peer->link_down = link_down;
- }
+ for (i = 0; i < s->queues; i++) {
+ nc = s->peers[i].nc;
- if (s->nc.info->link_status_changed) {
- s->nc.info->link_status_changed(&s->nc);
- }
+ nc->link_down = link_down;
+
+ if (nc->peer) {
+ nc->peer->link_down = link_down;
+ }
- if (s->nc.peer && s->nc.peer->info->link_status_changed) {
- s->nc.peer->info->link_status_changed(s->nc.peer);
+ if (nc->info->link_status_changed) {
+ nc->info->link_status_changed(nc);
+ }
+
+ if (nc->peer && nc->peer->info->link_status_changed) {
+ nc->peer->info->link_status_changed(nc->peer);
+ }
}
}
@@ -118,7 +158,8 @@ static void net_vhost_user_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_OPENED:
- vhost_user_start(s);
+ if (vhost_user_start(s) < 0)
+ exit(1);
net_vhost_link_down(s, false);
error_report("chardev \"%s\" went up", s->chr->label);
break;
@@ -131,24 +172,28 @@ static void net_vhost_user_event(void *opaque, int event)
}
static int net_vhost_user_init(NetClientState *peer, const char *device,
- const char *name, CharDriverState *chr)
+ const char *name, VhostUserState *s)
{
NetClientState *nc;
- VhostUserState *s;
+ CharDriverState *chr = s->chr;
+ int i;
- nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
+ for (i = 0; i < s->queues; i++) {
+ nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
- snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
- chr->label);
+ snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
+ i, chr->label);
- s = DO_UPCAST(VhostUserState, nc, nc);
+ /* We don't provide a receive callback */
+ nc->receive_disabled = 1;
- /* We don't provide a receive callback */
- s->nc.receive_disabled = 1;
- s->chr = chr;
- nc->queue_index = 0;
+ nc->queue_index = i;
+ nc->opaque = s;
- qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
+ s->peers[i].nc = nc;
+ }
+
+ qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, s);
return 0;
}
@@ -227,8 +272,10 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
int net_init_vhost_user(const NetClientOptions *opts, const char *name,
NetClientState *peer, Error **errp)
{
+ int queues;
const NetdevVhostUserOptions *vhost_user_opts;
CharDriverState *chr;
+ VhostUserState *s;
assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
vhost_user_opts = opts->vhost_user;
@@ -244,6 +291,11 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
return -1;
}
+ queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
+ s = g_malloc0(sizeof(VhostUserState) +
+ queues * sizeof(struct VhostUserNetPeer));
+ s->queues = queues;
+ s->chr = chr;
- return net_vhost_user_init(peer, "vhost_user", name, chr);
+ return net_vhost_user_init(peer, "vhost_user", name, s);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 67fef37..55c33db 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2480,12 +2480,16 @@
#
# @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
#
+# @queues: #optional number of queues to be created for multiqueue vhost-user
+# (default: 1) (Since 2.5)
+#
# Since 2.1
##
{ 'struct': 'NetdevVhostUserOptions',
'data': {
'chardev': 'str',
- '*vhostforce': 'bool' } }
+ '*vhostforce': 'bool',
+ '*queues': 'int' } }
##
# @NetClientOptions
diff --git a/qemu-options.hx b/qemu-options.hx
index 77f5853..5bfa7a3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1963,13 +1963,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the
required hub automatically.
-@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
+@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
be a unix domain socket backed one. The vhost-user uses a specifically defined
protocol to pass vhost ioctl replacement messages to an application on the other
end of the socket. On non-MSIX guests, the feature can be forced with
-@var{vhostforce}.
+@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
+be created for multiqueue vhost-user.
Example:
@example
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
` (5 preceding siblings ...)
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
@ 2015-09-08 7:38 ` Yuanhan Liu
2015-09-09 10:45 ` Michael S. Tsirkin
6 siblings, 1 reply; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-08 7:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, changchun.ouyang, mst
From: Changchun Ouyang <changchun.ouyang@intel.com>
Add a new message, VHOST_USER_SET_VRING_FLAG, to enable and disable
a specific virt queue, which is similar to attach/detach queue for
tap device.
virtio driver on guest doesn't have to use max virt queue pair, it
could enable any number of virt queue ranging from 1 to max virt
queue pair.
Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 10 ++++++++++
hw/net/vhost_net.c | 18 ++++++++++++++++++
hw/net/virtio-net.c | 2 ++
hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++----
include/hw/virtio/vhost-backend.h | 2 ++
include/net/vhost_net.h | 1 +
6 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 99d79be..c2c38f3 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -325,3 +325,13 @@ Message types
Query how many queues the backend supports. This request should be
sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
features by VHOST_USER_GET_PROTOCOL_FEATURES.
+
+ * VHOST_USER_SET_VRING_FLAG
+
+ Id: 18
+ Equivalent ioctl: N/A
+ Master payload: vring state description
+
+ Set the flag(1 for enable and 0 for disable) to signal slave to enable
+ or disable corresponding virt queue. This request should be sent only
+ when the protocol feature bit VHOST_USER_PROTOCOL_F_VRING_FLAG is set.
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 141b557..7d9cc8d 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -418,6 +418,19 @@ VHostNetState *get_vhost_net(NetClientState *nc)
return vhost_net;
}
+
+int vhost_set_vring_flag(NetClientState *nc, int enable)
+{
+ if (nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
+ VHostNetState *net = get_vhost_net(nc);
+ const VhostOps *vhost_ops = net->dev.vhost_ops;
+ if (vhost_ops->vhost_backend_mq_set_vring_flag)
+ return vhost_ops->vhost_backend_mq_set_vring_flag(&net->dev, enable);
+ }
+
+ return 0;
+}
+
#else
struct vhost_net *vhost_net_init(VhostNetOptions *options)
{
@@ -463,4 +476,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
{
return 0;
}
+
+int vhost_set_vring_flag(NetClientState *nc, int enable)
+{
+ return 0;
+}
#endif
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 8d28e45..53f93b1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -407,6 +407,7 @@ static int peer_attach(VirtIONet *n, int index)
}
if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
+ vhost_set_vring_flag(nc->peer, 1);
return 0;
}
@@ -422,6 +423,7 @@ static int peer_detach(VirtIONet *n, int index)
}
if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
+ vhost_set_vring_flag(nc->peer, 0);
return 0;
}
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 11e46b5..ca6f7fa 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -25,9 +25,10 @@
#define VHOST_MEMORY_MAX_NREGIONS 8
#define VHOST_USER_F_PROTOCOL_FEATURES 30
-#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
+#define VHOST_USER_PROTOCOL_FEATURE_MASK 0xfULL
-#define VHOST_USER_PROTOCOL_F_MQ 0
+#define VHOST_USER_PROTOCOL_F_MQ 0
+#define VHOST_USER_PROTOCOL_F_VRING_FLAG 1
typedef enum VhostUserRequest {
VHOST_USER_NONE = 0,
@@ -48,6 +49,7 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_PROTOCOL_FEATURES = 15,
VHOST_USER_SET_PROTOCOL_FEATURES = 16,
VHOST_USER_GET_QUEUE_NUM = 17,
+ VHOST_USER_SET_VRING_FLAG = 18,
VHOST_USER_MAX
} VhostUserRequest;
@@ -296,6 +298,11 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
msg.addr.index += dev->vq_index;
msg.size = sizeof(m.state);
break;
+ case VHOST_USER_SET_VRING_FLAG:
+ msg.state.index = dev->vq_index;
+ msg.state.num = *(int*)arg;
+ msg.size = sizeof(msg.state);
+ break;
case VHOST_USER_GET_VRING_BASE:
memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
@@ -408,6 +415,22 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
return 0;
}
+static int vhost_user_set_vring_flag(struct vhost_dev *dev, int enable)
+{
+ int err;
+
+ assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
+
+ if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_VRING_FLAG)))
+ return -1;
+
+ err = vhost_user_call(dev, VHOST_USER_SET_VRING_FLAG, &enable);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int vhost_user_cleanup(struct vhost_dev *dev)
{
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
@@ -421,5 +444,6 @@ const VhostOps user_ops = {
.backend_type = VHOST_BACKEND_TYPE_USER,
.vhost_call = vhost_user_call,
.vhost_backend_init = vhost_user_init,
- .vhost_backend_cleanup = vhost_user_cleanup
- };
+ .vhost_backend_cleanup = vhost_user_cleanup,
+ .vhost_backend_mq_set_vring_flag = vhost_user_set_vring_flag,
+};
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index e472f29..6fc76b7 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
void *arg);
typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
+typedef int (*vhost_backend_mq_set_vring_flag)(struct vhost_dev *dev, int enable);
typedef struct VhostOps {
VhostBackendType backend_type;
vhost_call vhost_call;
vhost_backend_init vhost_backend_init;
vhost_backend_cleanup vhost_backend_cleanup;
+ vhost_backend_mq_set_vring_flag vhost_backend_mq_set_vring_flag;
} VhostOps;
extern const VhostOps user_ops;
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 8db723e..3ec33ff 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -28,4 +28,5 @@ bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
int idx, bool mask);
VHostNetState *get_vhost_net(NetClientState *nc);
+int vhost_set_vring_flag(NetClientState * nc, int enable);
#endif
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
@ 2015-09-08 21:22 ` Eric Blake
2015-09-09 1:47 ` Yuanhan Liu
2015-09-09 8:05 ` Ouyang, Changchun
` (3 subsequent siblings)
4 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-08 21:22 UTC (permalink / raw)
To: Yuanhan Liu, qemu-devel; +Cc: mst, changchun.ouyang
[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]
On 09/08/2015 01:38 AM, Yuanhan Liu wrote:
> From: Ouyang Changchun <changchun.ouyang@intel.com>
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> Here is the latest version for adding vhost-user multiple queue support,
> by creating a nc and vhost_net pair for each queue.
>
Reviewing grammar and interface only:
> +++ b/docs/specs/vhost-user.txt
> @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> a feature bit was dedicated for this purpose:
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +Multiple queue support
> +-------------------
> +Multiple queue is treated as a protocal extension, hence the slave has to
s/protocal/protocol/
> +implement protocol features first. Multiple queues is supported only when
> +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
> +
> +The max # of queues the slave support can be queried with message
s/#/number/
s/support/supports/
> +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
s/#/number/
> +queues is bigger than that.
> +
> +As all queues share one connection, the master use a unique index for each
s/use/uses/
> +queue in the sent message to identify one specified queue.
> +
> +++ b/qapi-schema.json
> @@ -2480,12 +2480,16 @@
> #
> # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> #
> +# @queues: #optional number of queues to be created for multiqueue vhost-user
> +# (default: 1) (Since 2.5)
> +#
> # Since 2.1
> ##
> { 'struct': 'NetdevVhostUserOptions',
> 'data': {
> 'chardev': 'str',
> - '*vhostforce': 'bool' } }
> + '*vhostforce': 'bool',
> + '*queues': 'int' } }
Looks okay.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 21:22 ` Eric Blake
@ 2015-09-09 1:47 ` Yuanhan Liu
0 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-09 1:47 UTC (permalink / raw)
To: Eric Blake; +Cc: changchun.ouyang, qemu-devel, mst
On Tue, Sep 08, 2015 at 03:22:30PM -0600, Eric Blake wrote:
> On 09/08/2015 01:38 AM, Yuanhan Liu wrote:
> > From: Ouyang Changchun <changchun.ouyang@intel.com>
> >
> > This patch is initially based a patch from Nikolay Nikolaev.
> >
> > Here is the latest version for adding vhost-user multiple queue support,
> > by creating a nc and vhost_net pair for each queue.
> >
>
> Reviewing grammar and interface only:
Thanks, and will fix them in next patch.
--yliu
>
> > +++ b/docs/specs/vhost-user.txt
> > @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> > a feature bit was dedicated for this purpose:
> > #define VHOST_USER_F_PROTOCOL_FEATURES 30
> >
> > +Multiple queue support
> > +-------------------
> > +Multiple queue is treated as a protocal extension, hence the slave has to
>
> s/protocal/protocol/
>
> > +implement protocol features first. Multiple queues is supported only when
> > +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
> > +
> > +The max # of queues the slave support can be queried with message
>
> s/#/number/
> s/support/supports/
>
> > +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
>
> s/#/number/
>
> > +queues is bigger than that.
> > +
> > +As all queues share one connection, the master use a unique index for each
>
> s/use/uses/
>
> > +queue in the sent message to identify one specified queue.
> > +
>
> > +++ b/qapi-schema.json
> > @@ -2480,12 +2480,16 @@
> > #
> > # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> > #
> > +# @queues: #optional number of queues to be created for multiqueue vhost-user
> > +# (default: 1) (Since 2.5)
> > +#
> > # Since 2.1
> > ##
> > { 'struct': 'NetdevVhostUserOptions',
> > 'data': {
> > 'chardev': 'str',
> > - '*vhostforce': 'bool' } }
> > + '*vhostforce': 'bool',
> > + '*queues': 'int' } }
>
> Looks okay.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
2015-09-08 21:22 ` Eric Blake
@ 2015-09-09 8:05 ` Ouyang, Changchun
2015-09-09 8:11 ` Yuanhan Liu
2015-09-09 12:18 ` Michael S. Tsirkin
` (2 subsequent siblings)
4 siblings, 1 reply; 39+ messages in thread
From: Ouyang, Changchun @ 2015-09-09 8:05 UTC (permalink / raw)
To: Yuanhan Liu, qemu-devel; +Cc: Ouyang, Changchun, mst
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Tuesday, September 8, 2015 3:39 PM
> To: qemu-devel@nongnu.org
> Cc: mst@redhat.com; Ouyang, Changchun; Yuanhan Liu
> Subject: [PATCH 6/7] vhost-user: add multiple queue support
>
> From: Ouyang Changchun <changchun.ouyang@intel.com>
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> Here is the latest version for adding vhost-user multiple queue support, by
> creating a nc and vhost_net pair for each queue.
>
>
> static int vhost_user_start(VhostUserState *s) {
> VhostNetOptions options;
> + VHostNetState *vhost_net;
> + int max_queues;
> + int i = 0;
>
> - if (vhost_user_running(s)) {
> + if (s->running)
> return 0;
> - }
>
> options.backend_type = VHOST_BACKEND_TYPE_USER;
> - options.net_backend = &s->nc;
> options.opaque = s->chr;
>
> - s->vhost_net = vhost_net_init(&options);
> + options.net_backend = s->peers[i].nc;
> + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> +
> + max_queues = vhost_net_get_max_queues(vhost_net);
> + if (s->queues >= max_queues) {
use '>' rather than '>=' here?
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-09 8:05 ` Ouyang, Changchun
@ 2015-09-09 8:11 ` Yuanhan Liu
0 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-09 8:11 UTC (permalink / raw)
To: Ouyang, Changchun; +Cc: qemu-devel, mst
On Wed, Sep 09, 2015 at 08:05:11AM +0000, Ouyang, Changchun wrote:
>
>
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Tuesday, September 8, 2015 3:39 PM
> > To: qemu-devel@nongnu.org
> > Cc: mst@redhat.com; Ouyang, Changchun; Yuanhan Liu
> > Subject: [PATCH 6/7] vhost-user: add multiple queue support
> >
> > From: Ouyang Changchun <changchun.ouyang@intel.com>
> >
> > This patch is initially based a patch from Nikolay Nikolaev.
> >
> > Here is the latest version for adding vhost-user multiple queue support, by
> > creating a nc and vhost_net pair for each queue.
> >
> >
> > static int vhost_user_start(VhostUserState *s) {
> > VhostNetOptions options;
> > + VHostNetState *vhost_net;
> > + int max_queues;
> > + int i = 0;
> >
> > - if (vhost_user_running(s)) {
> > + if (s->running)
> > return 0;
> > - }
> >
> > options.backend_type = VHOST_BACKEND_TYPE_USER;
> > - options.net_backend = &s->nc;
> > options.opaque = s->chr;
> >
> > - s->vhost_net = vhost_net_init(&options);
> > + options.net_backend = s->peers[i].nc;
> > + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> > +
> > + max_queues = vhost_net_get_max_queues(vhost_net);
> > + if (s->queues >= max_queues) {
>
> use '>' rather than '>=' here?
Right, and thanks!
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.
2015-09-08 7:38 ` [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue Yuanhan Liu
@ 2015-09-09 10:45 ` Michael S. Tsirkin
2015-09-09 13:38 ` Yuanhan Liu
2015-09-14 7:36 ` Yuanhan Liu
0 siblings, 2 replies; 39+ messages in thread
From: Michael S. Tsirkin @ 2015-09-09 10:45 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel, changchun.ouyang
On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
> From: Changchun Ouyang <changchun.ouyang@intel.com>
>
> Add a new message, VHOST_USER_SET_VRING_FLAG, to enable and disable
> a specific virt queue, which is similar to attach/detach queue for
> tap device.
>
> virtio driver on guest doesn't have to use max virt queue pair, it
> could enable any number of virt queue ranging from 1 to max virt
> queue pair.
>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Fine, but I would like to make this dependent on the MQ flag.
> ---
> docs/specs/vhost-user.txt | 10 ++++++++++
> hw/net/vhost_net.c | 18 ++++++++++++++++++
> hw/net/virtio-net.c | 2 ++
> hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++----
> include/hw/virtio/vhost-backend.h | 2 ++
> include/net/vhost_net.h | 1 +
> 6 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 99d79be..c2c38f3 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -325,3 +325,13 @@ Message types
> Query how many queues the backend supports. This request should be
> sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
> features by VHOST_USER_GET_PROTOCOL_FEATURES.
> +
> + * VHOST_USER_SET_VRING_FLAG
SET_VRIHG_ENABLE would be a better name.
> +
> + Id: 18
> + Equivalent ioctl: N/A
> + Master payload: vring state description
> +
> + Set the flag(1 for enable and 0 for disable) to signal slave to enable
Space before ( please.
> + or disable corresponding virt queue. This request should be sent only
> + when the protocol feature bit VHOST_USER_PROTOCOL_F_VRING_FLAG is set.
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 141b557..7d9cc8d 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -418,6 +418,19 @@ VHostNetState *get_vhost_net(NetClientState *nc)
>
> return vhost_net;
> }
> +
> +int vhost_set_vring_flag(NetClientState *nc, int enable)
> +{
> + if (nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> + VHostNetState *net = get_vhost_net(nc);
> + const VhostOps *vhost_ops = net->dev.vhost_ops;
add an empty line after declaration pls.
> + if (vhost_ops->vhost_backend_mq_set_vring_flag)
> + return vhost_ops->vhost_backend_mq_set_vring_flag(&net->dev, enable);
Coding style violation:
line too long, and should use {}.
> + }
> +
> + return 0;
> +}
> +
> #else
> struct vhost_net *vhost_net_init(VhostNetOptions *options)
> {
> @@ -463,4 +476,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> {
> return 0;
> }
> +
> +int vhost_set_vring_flag(NetClientState *nc, int enable)
> +{
> + return 0;
> +}
> #endif
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 8d28e45..53f93b1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -407,6 +407,7 @@ static int peer_attach(VirtIONet *n, int index)
> }
>
> if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> + vhost_set_vring_flag(nc->peer, 1);
> return 0;
> }
>
> @@ -422,6 +423,7 @@ static int peer_detach(VirtIONet *n, int index)
> }
>
> if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> + vhost_set_vring_flag(nc->peer, 0);
> return 0;
> }
>
makes no sense to call it for !== tap only.
Either call this unconditionally, or just for vhost user.
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 11e46b5..ca6f7fa 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -25,9 +25,10 @@
>
> #define VHOST_MEMORY_MAX_NREGIONS 8
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
> -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0xfULL
>
> -#define VHOST_USER_PROTOCOL_F_MQ 0
> +#define VHOST_USER_PROTOCOL_F_MQ 0
> +#define VHOST_USER_PROTOCOL_F_VRING_FLAG 1
>
> typedef enum VhostUserRequest {
> VHOST_USER_NONE = 0,
> @@ -48,6 +49,7 @@ typedef enum VhostUserRequest {
> VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> VHOST_USER_GET_QUEUE_NUM = 17,
> + VHOST_USER_SET_VRING_FLAG = 18,
> VHOST_USER_MAX
> } VhostUserRequest;
>
> @@ -296,6 +298,11 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.state);
> break;
> + case VHOST_USER_SET_VRING_FLAG:
> + msg.state.index = dev->vq_index;
> + msg.state.num = *(int*)arg;
> + msg.size = sizeof(msg.state);
> + break;
>
> case VHOST_USER_GET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> @@ -408,6 +415,22 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> return 0;
> }
>
> +static int vhost_user_set_vring_flag(struct vhost_dev *dev, int enable)
> +{
> + int err;
> +
> + assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> +
> + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_VRING_FLAG)))
> + return -1;
> +
> + err = vhost_user_call(dev, VHOST_USER_SET_VRING_FLAG, &enable);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> static int vhost_user_cleanup(struct vhost_dev *dev)
> {
> assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
In fact, it's a per VQ pair call the way it's coded.
So pls fix it, call it for all VQs.
> @@ -421,5 +444,6 @@ const VhostOps user_ops = {
> .backend_type = VHOST_BACKEND_TYPE_USER,
> .vhost_call = vhost_user_call,
> .vhost_backend_init = vhost_user_init,
> - .vhost_backend_cleanup = vhost_user_cleanup
> - };
> + .vhost_backend_cleanup = vhost_user_cleanup,
> + .vhost_backend_mq_set_vring_flag = vhost_user_set_vring_flag,
> +};
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index e472f29..6fc76b7 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> void *arg);
> typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> +typedef int (*vhost_backend_mq_set_vring_flag)(struct vhost_dev *dev, int enable);
>
> typedef struct VhostOps {
> VhostBackendType backend_type;
> vhost_call vhost_call;
> vhost_backend_init vhost_backend_init;
> vhost_backend_cleanup vhost_backend_cleanup;
> + vhost_backend_mq_set_vring_flag vhost_backend_mq_set_vring_flag;
That's quite ugly: mq is a vhost net thing.
Why not enable each VQ?
> } VhostOps;
>
> extern const VhostOps user_ops;
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 8db723e..3ec33ff 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -28,4 +28,5 @@ bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
> void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
> int idx, bool mask);
> VHostNetState *get_vhost_net(NetClientState *nc);
> +int vhost_set_vring_flag(NetClientState * nc, int enable);
> #endif
> --
> 1.9.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
2015-09-08 21:22 ` Eric Blake
2015-09-09 8:05 ` Ouyang, Changchun
@ 2015-09-09 12:18 ` Michael S. Tsirkin
2015-09-09 13:19 ` Yuanhan Liu
2015-09-09 20:55 ` Michael S. Tsirkin
2015-09-14 10:00 ` Jason Wang
4 siblings, 1 reply; 39+ messages in thread
From: Michael S. Tsirkin @ 2015-09-09 12:18 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel, changchun.ouyang
On Tue, Sep 08, 2015 at 03:38:46PM +0800, Yuanhan Liu wrote:
> From: Ouyang Changchun <changchun.ouyang@intel.com>
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> Here is the latest version for adding vhost-user multiple queue support,
> by creating a nc and vhost_net pair for each queue.
>
> What differs from last version is that this patch addresses two major
> concerns from Michael, and fixes one hidden bug.
>
> - Concern #1: no feedback when the backend can't support # of
> requested queues(by providing queues=# option).
>
> Here we address this issue by querying VHOST_USER_PROTOCOL_F_MQ
> protocol features first, if not set, it means the backend don't
> support mq feature, and let max_queues be 1. Otherwise, we send
> another message, VHOST_USER_GET_QUEUE_NUM, for getting the max_queues
> the backend supports.
>
> At vhost-user initiation stage(net_vhost_user_start), we then initiate
> one queue first, which, in the meantime, also gets the max_queues.
> We then do a simple compare: if requested_queues > max_queues, we
> exit(I guess it's safe to exit here, as the vm is not running yet).
>
> - Concern #2: some messages are sent more times than necessary.
>
> We came an agreement with Michael that we could categorize vhost
> user messages to 2 types: none-vring specific messages, which should
> be sent only once, and vring specific messages, which should be sent
> per queue.
>
> Here I introduced a helper function vhost_user_one_time_request(),
> which lists following messages as none-vring specific messages:
>
> VHOST_USER_GET_FEATURES
> VHOST_USER_SET_FEATURES
> VHOST_USER_GET_PROTOCOL_FEATURES
> VHOST_USER_SET_PROTOCOL_FEATURES
> VHOST_USER_SET_OWNER
> VHOST_USER_RESET_DEVICE
> VHOST_USER_SET_MEM_TABLE
> VHOST_USER_GET_QUEUE_NUM
>
> For above messages, we simply ignore them when they are not sent the first
> time.
>
> I also observed a hidden bug from last version. We register the char dev
> event handler N times, which is not necessary, as well as buggy: A later
> register overwrites the former one, as qemu_chr_add_handlers() will not
> chain those handlers, but instead overwrites the old one. So, in theory,
> invoking qemu_chr_add_handlers N times will not end up with calling the
> handler N times.
>
> However, the reason the handler is invoked N times is because we start
> the backend(the connection server) first, and hence when net_vhost_user_init()
> is executed, the connection is already established, and qemu_chr_add_handlers()
> then invoke the handler immediately, which just looks like we invoke the
> handler(net_vhost_user_event) directly from net_vhost_user_init().
>
> The solution I came up with is to make VhostUserState as an upper level
> structure, making it includes N nc and vhost_net pairs:
>
> struct VhostUserNetPeer {
> NetClientState *nc;
> VHostNetState *vhost_net;
> };
>
> typedef struct VhostUserState {
> CharDriverState *chr;
> bool running;
> int queues;
> struct VhostUserNetPeer peers[];
> } VhostUserState;
>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> docs/specs/vhost-user.txt | 13 +++++
> hw/virtio/vhost-user.c | 31 ++++++++++-
> include/net/net.h | 1 +
> net/vhost-user.c | 136 ++++++++++++++++++++++++++++++++--------------
> qapi-schema.json | 6 +-
> qemu-options.hx | 5 +-
> 6 files changed, 146 insertions(+), 46 deletions(-)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 43db9b4..99d79be 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> a feature bit was dedicated for this purpose:
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +Multiple queue support
> +-------------------
> +Multiple queue is treated as a protocal extension, hence the slave has to
> +implement protocol features first. Multiple queues is supported only when
> +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
> +
> +The max # of queues the slave support can be queried with message
> +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
> +queues is bigger than that.
> +
> +As all queues share one connection, the master use a unique index for each
> +queue in the sent message to identify one specified queue.
> +
Please also document that only 2 queues are enabled initially.
More queues are enabled dynamically.
To enable more queues, the new message needs to be sent
(added by patch 7).
> Message types
> -------------
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8046bc0..11e46b5 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -187,6 +187,23 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
> 0 : -1;
> }
>
> +static bool vhost_user_one_time_request(VhostUserRequest request)
> +{
> + switch (request) {
> + case VHOST_USER_GET_FEATURES:
> + case VHOST_USER_SET_FEATURES:
> + case VHOST_USER_GET_PROTOCOL_FEATURES:
> + case VHOST_USER_SET_PROTOCOL_FEATURES:
> + case VHOST_USER_SET_OWNER:
> + case VHOST_USER_RESET_DEVICE:
> + case VHOST_USER_SET_MEM_TABLE:
> + case VHOST_USER_GET_QUEUE_NUM:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> void *arg)
> {
> @@ -206,6 +223,14 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> else
> msg_request = request;
>
> + /*
> + * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
> + * we just need send it once in the first time. For later such
> + * request, we just ignore it.
> + */
> + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0)
> + return 0;
> +
> msg.request = msg_request;
> msg.flags = VHOST_USER_VERSION;
> msg.size = 0;
> @@ -268,17 +293,20 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> case VHOST_USER_SET_VRING_NUM:
> case VHOST_USER_SET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.state);
> break;
>
> case VHOST_USER_GET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.state);
> need_reply = 1;
> break;
>
> case VHOST_USER_SET_VRING_ADDR:
> memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.addr);
> break;
>
> @@ -286,7 +314,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> case VHOST_USER_SET_VRING_CALL:
> case VHOST_USER_SET_VRING_ERR:
> file = arg;
> - msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> + msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
> msg.size = sizeof(m.u64);
> if (ioeventfd_enabled() && file->fd > 0) {
> fds[fd_num++] = file->fd;
> @@ -330,6 +358,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> error_report("Received bad msg size.");
> return -1;
> }
> + msg.state.index -= dev->vq_index;
> memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> break;
> default:
> diff --git a/include/net/net.h b/include/net/net.h
> index 6a6cbef..6f20656 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -92,6 +92,7 @@ struct NetClientState {
> NetClientDestructor *destructor;
> unsigned int queue_index;
> unsigned rxfilter_notify_enabled:1;
> + void *opaque;
> };
>
> typedef struct NICState {
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 2d6bbe5..7d4ac69 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -15,10 +15,16 @@
> #include "qemu/config-file.h"
> #include "qemu/error-report.h"
>
> +struct VhostUserNetPeer {
> + NetClientState *nc;
> + VHostNetState *vhost_net;
> +};
> +
> typedef struct VhostUserState {
> - NetClientState nc;
> CharDriverState *chr;
> - VHostNetState *vhost_net;
> + bool running;
> + int queues;
> + struct VhostUserNetPeer peers[];
> } VhostUserState;
>
> typedef struct VhostUserChardevProps {
> @@ -29,48 +35,75 @@ typedef struct VhostUserChardevProps {
>
> VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> {
> - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> + VhostUserState *s = nc->opaque;
> assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> - return s->vhost_net;
> -}
> -
> -static int vhost_user_running(VhostUserState *s)
> -{
> - return (s->vhost_net) ? 1 : 0;
> + return s->peers[nc->queue_index].vhost_net;
> }
>
> static int vhost_user_start(VhostUserState *s)
> {
> VhostNetOptions options;
> + VHostNetState *vhost_net;
> + int max_queues;
> + int i = 0;
>
> - if (vhost_user_running(s)) {
> + if (s->running)
> return 0;
> - }
>
> options.backend_type = VHOST_BACKEND_TYPE_USER;
> - options.net_backend = &s->nc;
> options.opaque = s->chr;
>
> - s->vhost_net = vhost_net_init(&options);
> + options.net_backend = s->peers[i].nc;
> + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> +
> + max_queues = vhost_net_get_max_queues(vhost_net);
> + if (s->queues >= max_queues) {
> + error_report("you are asking more queues than supported: %d\n",
> + max_queues);
> + return -1;
> + }
> +
> + for (; i < s->queues; i++) {
> + options.net_backend = s->peers[i].nc;
> +
> + s->peers[i].vhost_net = vhost_net_init(&options);
> + if (!s->peers[i].vhost_net)
> + return -1;
> + }
> + s->running = true;
>
> - return vhost_user_running(s) ? 0 : -1;
> + return 0;
> }
>
> static void vhost_user_stop(VhostUserState *s)
> {
> - if (vhost_user_running(s)) {
> - vhost_net_cleanup(s->vhost_net);
> + int i;
> + VHostNetState *vhost_net;
> +
> + if (!s->running)
> + return;
> +
> + for (i = 0; i < s->queues; i++) {
> + vhost_net = s->peers[i].vhost_net;
> + if (vhost_net)
> + vhost_net_cleanup(vhost_net);
> }
>
> - s->vhost_net = 0;
> + s->running = false;
> }
>
> static void vhost_user_cleanup(NetClientState *nc)
> {
> - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> + VhostUserState *s = nc->opaque;
> + VHostNetState *vhost_net = s->peers[nc->queue_index].vhost_net;
> +
> + if (vhost_net)
> + vhost_net_cleanup(vhost_net);
>
> - vhost_user_stop(s);
> qemu_purge_queued_packets(nc);
> +
> + if (nc->queue_index == s->queues - 1)
> + free(s);
> }
>
> static bool vhost_user_has_vnet_hdr(NetClientState *nc)
> @@ -89,7 +122,7 @@ static bool vhost_user_has_ufo(NetClientState *nc)
>
> static NetClientInfo net_vhost_user_info = {
> .type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
> - .size = sizeof(VhostUserState),
> + .size = sizeof(NetClientState),
> .cleanup = vhost_user_cleanup,
> .has_vnet_hdr = vhost_user_has_vnet_hdr,
> .has_ufo = vhost_user_has_ufo,
> @@ -97,18 +130,25 @@ static NetClientInfo net_vhost_user_info = {
>
> static void net_vhost_link_down(VhostUserState *s, bool link_down)
> {
> - s->nc.link_down = link_down;
> + NetClientState *nc;
> + int i;
>
> - if (s->nc.peer) {
> - s->nc.peer->link_down = link_down;
> - }
> + for (i = 0; i < s->queues; i++) {
> + nc = s->peers[i].nc;
>
> - if (s->nc.info->link_status_changed) {
> - s->nc.info->link_status_changed(&s->nc);
> - }
> + nc->link_down = link_down;
> +
> + if (nc->peer) {
> + nc->peer->link_down = link_down;
> + }
>
> - if (s->nc.peer && s->nc.peer->info->link_status_changed) {
> - s->nc.peer->info->link_status_changed(s->nc.peer);
> + if (nc->info->link_status_changed) {
> + nc->info->link_status_changed(nc);
> + }
> +
> + if (nc->peer && nc->peer->info->link_status_changed) {
> + nc->peer->info->link_status_changed(nc->peer);
> + }
> }
> }
>
> @@ -118,7 +158,8 @@ static void net_vhost_user_event(void *opaque, int event)
>
> switch (event) {
> case CHR_EVENT_OPENED:
> - vhost_user_start(s);
> + if (vhost_user_start(s) < 0)
> + exit(1);
> net_vhost_link_down(s, false);
> error_report("chardev \"%s\" went up", s->chr->label);
> break;
> @@ -131,24 +172,28 @@ static void net_vhost_user_event(void *opaque, int event)
> }
>
> static int net_vhost_user_init(NetClientState *peer, const char *device,
> - const char *name, CharDriverState *chr)
> + const char *name, VhostUserState *s)
> {
> NetClientState *nc;
> - VhostUserState *s;
> + CharDriverState *chr = s->chr;
> + int i;
>
> - nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> + for (i = 0; i < s->queues; i++) {
> + nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
>
> - snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> - chr->label);
> + snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> + i, chr->label);
>
> - s = DO_UPCAST(VhostUserState, nc, nc);
> + /* We don't provide a receive callback */
> + nc->receive_disabled = 1;
>
> - /* We don't provide a receive callback */
> - s->nc.receive_disabled = 1;
> - s->chr = chr;
> - nc->queue_index = 0;
> + nc->queue_index = i;
> + nc->opaque = s;
>
> - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> + s->peers[i].nc = nc;
> + }
> +
> + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, s);
>
> return 0;
> }
> @@ -227,8 +272,10 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
> int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> NetClientState *peer, Error **errp)
> {
> + int queues;
> const NetdevVhostUserOptions *vhost_user_opts;
> CharDriverState *chr;
> + VhostUserState *s;
>
> assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> vhost_user_opts = opts->vhost_user;
> @@ -244,6 +291,11 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> return -1;
> }
>
> + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
> + s = g_malloc0(sizeof(VhostUserState) +
> + queues * sizeof(struct VhostUserNetPeer));
> + s->queues = queues;
> + s->chr = chr;
>
> - return net_vhost_user_init(peer, "vhost_user", name, chr);
> + return net_vhost_user_init(peer, "vhost_user", name, s);
> }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 67fef37..55c33db 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2480,12 +2480,16 @@
> #
> # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> #
> +# @queues: #optional number of queues to be created for multiqueue vhost-user
> +# (default: 1) (Since 2.5)
> +#
> # Since 2.1
> ##
> { 'struct': 'NetdevVhostUserOptions',
> 'data': {
> 'chardev': 'str',
> - '*vhostforce': 'bool' } }
> + '*vhostforce': 'bool',
> + '*queues': 'int' } }
>
> ##
> # @NetClientOptions
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 77f5853..5bfa7a3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1963,13 +1963,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
> netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the
> required hub automatically.
>
> -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
> +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
>
> Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
> be a unix domain socket backed one. The vhost-user uses a specifically defined
> protocol to pass vhost ioctl replacement messages to an application on the other
> end of the socket. On non-MSIX guests, the feature can be forced with
> -@var{vhostforce}.
> +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
> +be created for multiqueue vhost-user.
>
> Example:
> @example
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-09 12:18 ` Michael S. Tsirkin
@ 2015-09-09 13:19 ` Yuanhan Liu
0 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-09 13:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, changchun.ouyang
On Wed, Sep 09, 2015 at 03:18:53PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 08, 2015 at 03:38:46PM +0800, Yuanhan Liu wrote:
> > From: Ouyang Changchun <changchun.ouyang@intel.com>
> >
> > This patch is initially based a patch from Nikolay Nikolaev.
> >
> > Here is the latest version for adding vhost-user multiple queue support,
> > by creating a nc and vhost_net pair for each queue.
> >
> > What differs from last version is that this patch addresses two major
> > concerns from Michael, and fixes one hidden bug.
> >
> > - Concern #1: no feedback when the backend can't support # of
> > requested queues(by providing queues=# option).
> >
> > Here we address this issue by querying VHOST_USER_PROTOCOL_F_MQ
> > protocol features first, if not set, it means the backend don't
> > support mq feature, and let max_queues be 1. Otherwise, we send
> > another message, VHOST_USER_GET_QUEUE_NUM, for getting the max_queues
> > the backend supports.
> >
> > At vhost-user initiation stage(net_vhost_user_start), we then initiate
> > one queue first, which, in the meantime, also gets the max_queues.
> > We then do a simple compare: if requested_queues > max_queues, we
> > exit(I guess it's safe to exit here, as the vm is not running yet).
> >
> > - Concern #2: some messages are sent more times than necessary.
> >
> > We came an agreement with Michael that we could categorize vhost
> > user messages to 2 types: none-vring specific messages, which should
> > be sent only once, and vring specific messages, which should be sent
> > per queue.
> >
> > Here I introduced a helper function vhost_user_one_time_request(),
> > which lists following messages as none-vring specific messages:
> >
> > VHOST_USER_GET_FEATURES
> > VHOST_USER_SET_FEATURES
> > VHOST_USER_GET_PROTOCOL_FEATURES
> > VHOST_USER_SET_PROTOCOL_FEATURES
> > VHOST_USER_SET_OWNER
> > VHOST_USER_RESET_DEVICE
> > VHOST_USER_SET_MEM_TABLE
> > VHOST_USER_GET_QUEUE_NUM
> >
> > For above messages, we simply ignore them when they are not sent the first
> > time.
> >
> > I also observed a hidden bug from last version. We register the char dev
> > event handler N times, which is not necessary, as well as buggy: A later
> > register overwrites the former one, as qemu_chr_add_handlers() will not
> > chain those handlers, but instead overwrites the old one. So, in theory,
> > invoking qemu_chr_add_handlers N times will not end up with calling the
> > handler N times.
> >
> > However, the reason the handler is invoked N times is because we start
> > the backend(the connection server) first, and hence when net_vhost_user_init()
> > is executed, the connection is already established, and qemu_chr_add_handlers()
> > then invoke the handler immediately, which just looks like we invoke the
> > handler(net_vhost_user_event) directly from net_vhost_user_init().
> >
> > The solution I came up with is to make VhostUserState as an upper level
> > structure, making it includes N nc and vhost_net pairs:
> >
> > struct VhostUserNetPeer {
> > NetClientState *nc;
> > VHostNetState *vhost_net;
> > };
> >
> > typedef struct VhostUserState {
> > CharDriverState *chr;
> > bool running;
> > int queues;
> > struct VhostUserNetPeer peers[];
> > } VhostUserState;
> >
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> > docs/specs/vhost-user.txt | 13 +++++
> > hw/virtio/vhost-user.c | 31 ++++++++++-
> > include/net/net.h | 1 +
> > net/vhost-user.c | 136 ++++++++++++++++++++++++++++++++--------------
> > qapi-schema.json | 6 +-
> > qemu-options.hx | 5 +-
> > 6 files changed, 146 insertions(+), 46 deletions(-)
> >
> > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > index 43db9b4..99d79be 100644
> > --- a/docs/specs/vhost-user.txt
> > +++ b/docs/specs/vhost-user.txt
> > @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> > a feature bit was dedicated for this purpose:
> > #define VHOST_USER_F_PROTOCOL_FEATURES 30
> >
> > +Multiple queue support
> > +-------------------
> > +Multiple queue is treated as a protocal extension, hence the slave has to
> > +implement protocol features first. Multiple queues is supported only when
> > +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
> > +
> > +The max # of queues the slave support can be queried with message
> > +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
> > +queues is bigger than that.
> > +
> > +As all queues share one connection, the master use a unique index for each
> > +queue in the sent message to identify one specified queue.
> > +
>
> Please also document that only 2 queues are enabled initially.
> More queues are enabled dynamically.
> To enable more queues, the new message needs to be sent
> (added by patch 7).
Hi Michael,
Will do that in next version. Besides that, do you have more comments?
Say, does this patch address your two concerns?
--yliu
>
> > Message types
> > -------------
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 8046bc0..11e46b5 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -187,6 +187,23 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
> > 0 : -1;
> > }
> >
> > +static bool vhost_user_one_time_request(VhostUserRequest request)
> > +{
> > + switch (request) {
> > + case VHOST_USER_GET_FEATURES:
> > + case VHOST_USER_SET_FEATURES:
> > + case VHOST_USER_GET_PROTOCOL_FEATURES:
> > + case VHOST_USER_SET_PROTOCOL_FEATURES:
> > + case VHOST_USER_SET_OWNER:
> > + case VHOST_USER_RESET_DEVICE:
> > + case VHOST_USER_SET_MEM_TABLE:
> > + case VHOST_USER_GET_QUEUE_NUM:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > void *arg)
> > {
> > @@ -206,6 +223,14 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > else
> > msg_request = request;
> >
> > + /*
> > + * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
> > + * we just need send it once in the first time. For later such
> > + * request, we just ignore it.
> > + */
> > + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0)
> > + return 0;
> > +
> > msg.request = msg_request;
> > msg.flags = VHOST_USER_VERSION;
> > msg.size = 0;
> > @@ -268,17 +293,20 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > case VHOST_USER_SET_VRING_NUM:
> > case VHOST_USER_SET_VRING_BASE:
> > memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > + msg.addr.index += dev->vq_index;
> > msg.size = sizeof(m.state);
> > break;
> >
> > case VHOST_USER_GET_VRING_BASE:
> > memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > + msg.addr.index += dev->vq_index;
> > msg.size = sizeof(m.state);
> > need_reply = 1;
> > break;
> >
> > case VHOST_USER_SET_VRING_ADDR:
> > memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> > + msg.addr.index += dev->vq_index;
> > msg.size = sizeof(m.addr);
> > break;
> >
> > @@ -286,7 +314,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > case VHOST_USER_SET_VRING_CALL:
> > case VHOST_USER_SET_VRING_ERR:
> > file = arg;
> > - msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> > + msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
> > msg.size = sizeof(m.u64);
> > if (ioeventfd_enabled() && file->fd > 0) {
> > fds[fd_num++] = file->fd;
> > @@ -330,6 +358,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > error_report("Received bad msg size.");
> > return -1;
> > }
> > + msg.state.index -= dev->vq_index;
> > memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> > break;
> > default:
> > diff --git a/include/net/net.h b/include/net/net.h
> > index 6a6cbef..6f20656 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -92,6 +92,7 @@ struct NetClientState {
> > NetClientDestructor *destructor;
> > unsigned int queue_index;
> > unsigned rxfilter_notify_enabled:1;
> > + void *opaque;
> > };
> >
> > typedef struct NICState {
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 2d6bbe5..7d4ac69 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -15,10 +15,16 @@
> > #include "qemu/config-file.h"
> > #include "qemu/error-report.h"
> >
> > +struct VhostUserNetPeer {
> > + NetClientState *nc;
> > + VHostNetState *vhost_net;
> > +};
> > +
> > typedef struct VhostUserState {
> > - NetClientState nc;
> > CharDriverState *chr;
> > - VHostNetState *vhost_net;
> > + bool running;
> > + int queues;
> > + struct VhostUserNetPeer peers[];
> > } VhostUserState;
> >
> > typedef struct VhostUserChardevProps {
> > @@ -29,48 +35,75 @@ typedef struct VhostUserChardevProps {
> >
> > VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> > {
> > - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> > + VhostUserState *s = nc->opaque;
> > assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> > - return s->vhost_net;
> > -}
> > -
> > -static int vhost_user_running(VhostUserState *s)
> > -{
> > - return (s->vhost_net) ? 1 : 0;
> > + return s->peers[nc->queue_index].vhost_net;
> > }
> >
> > static int vhost_user_start(VhostUserState *s)
> > {
> > VhostNetOptions options;
> > + VHostNetState *vhost_net;
> > + int max_queues;
> > + int i = 0;
> >
> > - if (vhost_user_running(s)) {
> > + if (s->running)
> > return 0;
> > - }
> >
> > options.backend_type = VHOST_BACKEND_TYPE_USER;
> > - options.net_backend = &s->nc;
> > options.opaque = s->chr;
> >
> > - s->vhost_net = vhost_net_init(&options);
> > + options.net_backend = s->peers[i].nc;
> > + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> > +
> > + max_queues = vhost_net_get_max_queues(vhost_net);
> > + if (s->queues >= max_queues) {
> > + error_report("you are asking more queues than supported: %d\n",
> > + max_queues);
> > + return -1;
> > + }
> > +
> > + for (; i < s->queues; i++) {
> > + options.net_backend = s->peers[i].nc;
> > +
> > + s->peers[i].vhost_net = vhost_net_init(&options);
> > + if (!s->peers[i].vhost_net)
> > + return -1;
> > + }
> > + s->running = true;
> >
> > - return vhost_user_running(s) ? 0 : -1;
> > + return 0;
> > }
> >
> > static void vhost_user_stop(VhostUserState *s)
> > {
> > - if (vhost_user_running(s)) {
> > - vhost_net_cleanup(s->vhost_net);
> > + int i;
> > + VHostNetState *vhost_net;
> > +
> > + if (!s->running)
> > + return;
> > +
> > + for (i = 0; i < s->queues; i++) {
> > + vhost_net = s->peers[i].vhost_net;
> > + if (vhost_net)
> > + vhost_net_cleanup(vhost_net);
> > }
> >
> > - s->vhost_net = 0;
> > + s->running = false;
> > }
> >
> > static void vhost_user_cleanup(NetClientState *nc)
> > {
> > - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> > + VhostUserState *s = nc->opaque;
> > + VHostNetState *vhost_net = s->peers[nc->queue_index].vhost_net;
> > +
> > + if (vhost_net)
> > + vhost_net_cleanup(vhost_net);
> >
> > - vhost_user_stop(s);
> > qemu_purge_queued_packets(nc);
> > +
> > + if (nc->queue_index == s->queues - 1)
> > + free(s);
> > }
> >
> > static bool vhost_user_has_vnet_hdr(NetClientState *nc)
> > @@ -89,7 +122,7 @@ static bool vhost_user_has_ufo(NetClientState *nc)
> >
> > static NetClientInfo net_vhost_user_info = {
> > .type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
> > - .size = sizeof(VhostUserState),
> > + .size = sizeof(NetClientState),
> > .cleanup = vhost_user_cleanup,
> > .has_vnet_hdr = vhost_user_has_vnet_hdr,
> > .has_ufo = vhost_user_has_ufo,
> > @@ -97,18 +130,25 @@ static NetClientInfo net_vhost_user_info = {
> >
> > static void net_vhost_link_down(VhostUserState *s, bool link_down)
> > {
> > - s->nc.link_down = link_down;
> > + NetClientState *nc;
> > + int i;
> >
> > - if (s->nc.peer) {
> > - s->nc.peer->link_down = link_down;
> > - }
> > + for (i = 0; i < s->queues; i++) {
> > + nc = s->peers[i].nc;
> >
> > - if (s->nc.info->link_status_changed) {
> > - s->nc.info->link_status_changed(&s->nc);
> > - }
> > + nc->link_down = link_down;
> > +
> > + if (nc->peer) {
> > + nc->peer->link_down = link_down;
> > + }
> >
> > - if (s->nc.peer && s->nc.peer->info->link_status_changed) {
> > - s->nc.peer->info->link_status_changed(s->nc.peer);
> > + if (nc->info->link_status_changed) {
> > + nc->info->link_status_changed(nc);
> > + }
> > +
> > + if (nc->peer && nc->peer->info->link_status_changed) {
> > + nc->peer->info->link_status_changed(nc->peer);
> > + }
> > }
> > }
> >
> > @@ -118,7 +158,8 @@ static void net_vhost_user_event(void *opaque, int event)
> >
> > switch (event) {
> > case CHR_EVENT_OPENED:
> > - vhost_user_start(s);
> > + if (vhost_user_start(s) < 0)
> > + exit(1);
> > net_vhost_link_down(s, false);
> > error_report("chardev \"%s\" went up", s->chr->label);
> > break;
> > @@ -131,24 +172,28 @@ static void net_vhost_user_event(void *opaque, int event)
> > }
> >
> > static int net_vhost_user_init(NetClientState *peer, const char *device,
> > - const char *name, CharDriverState *chr)
> > + const char *name, VhostUserState *s)
> > {
> > NetClientState *nc;
> > - VhostUserState *s;
> > + CharDriverState *chr = s->chr;
> > + int i;
> >
> > - nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> > + for (i = 0; i < s->queues; i++) {
> > + nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> >
> > - snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> > - chr->label);
> > + snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> > + i, chr->label);
> >
> > - s = DO_UPCAST(VhostUserState, nc, nc);
> > + /* We don't provide a receive callback */
> > + nc->receive_disabled = 1;
> >
> > - /* We don't provide a receive callback */
> > - s->nc.receive_disabled = 1;
> > - s->chr = chr;
> > - nc->queue_index = 0;
> > + nc->queue_index = i;
> > + nc->opaque = s;
> >
> > - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> > + s->peers[i].nc = nc;
> > + }
> > +
> > + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, s);
> >
> > return 0;
> > }
> > @@ -227,8 +272,10 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
> > int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> > NetClientState *peer, Error **errp)
> > {
> > + int queues;
> > const NetdevVhostUserOptions *vhost_user_opts;
> > CharDriverState *chr;
> > + VhostUserState *s;
> >
> > assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> > vhost_user_opts = opts->vhost_user;
> > @@ -244,6 +291,11 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> > return -1;
> > }
> >
> > + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
> > + s = g_malloc0(sizeof(VhostUserState) +
> > + queues * sizeof(struct VhostUserNetPeer));
> > + s->queues = queues;
> > + s->chr = chr;
> >
> > - return net_vhost_user_init(peer, "vhost_user", name, chr);
> > + return net_vhost_user_init(peer, "vhost_user", name, s);
> > }
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 67fef37..55c33db 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2480,12 +2480,16 @@
> > #
> > # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> > #
> > +# @queues: #optional number of queues to be created for multiqueue vhost-user
> > +# (default: 1) (Since 2.5)
> > +#
> > # Since 2.1
> > ##
> > { 'struct': 'NetdevVhostUserOptions',
> > 'data': {
> > 'chardev': 'str',
> > - '*vhostforce': 'bool' } }
> > + '*vhostforce': 'bool',
> > + '*queues': 'int' } }
> >
> > ##
> > # @NetClientOptions
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 77f5853..5bfa7a3 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1963,13 +1963,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
> > netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the
> > required hub automatically.
> >
> > -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
> > +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
> >
> > Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
> > be a unix domain socket backed one. The vhost-user uses a specifically defined
> > protocol to pass vhost ioctl replacement messages to an application on the other
> > end of the socket. On non-MSIX guests, the feature can be forced with
> > -@var{vhostforce}.
> > +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
> > +be created for multiqueue vhost-user.
> >
> > Example:
> > @example
> > --
> > 1.9.0
> >
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.
2015-09-09 10:45 ` Michael S. Tsirkin
@ 2015-09-09 13:38 ` Yuanhan Liu
2015-09-09 14:18 ` Michael S. Tsirkin
2015-09-09 20:55 ` Michael S. Tsirkin
2015-09-14 7:36 ` Yuanhan Liu
1 sibling, 2 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-09 13:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, changchun.ouyang
On Wed, Sep 09, 2015 at 01:45:50PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
> > From: Changchun Ouyang <changchun.ouyang@intel.com>
> >
> > Add a new message, VHOST_USER_SET_VRING_FLAG, to enable and disable
> > a specific virt queue, which is similar to attach/detach queue for
> > tap device.
> >
> > virtio driver on guest doesn't have to use max virt queue pair, it
> > could enable any number of virt queue ranging from 1 to max virt
> > queue pair.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>
> Fine, but I would like to make this dependent on the MQ flag.
Reasonable.
>
>
> > ---
> > docs/specs/vhost-user.txt | 10 ++++++++++
> > hw/net/vhost_net.c | 18 ++++++++++++++++++
> > hw/net/virtio-net.c | 2 ++
> > hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++----
> > include/hw/virtio/vhost-backend.h | 2 ++
> > include/net/vhost_net.h | 1 +
> > 6 files changed, 61 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > index 99d79be..c2c38f3 100644
> > --- a/docs/specs/vhost-user.txt
> > +++ b/docs/specs/vhost-user.txt
> > @@ -325,3 +325,13 @@ Message types
> > Query how many queues the backend supports. This request should be
> > sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
> > features by VHOST_USER_GET_PROTOCOL_FEATURES.
> > +
> > + * VHOST_USER_SET_VRING_FLAG
>
>
> SET_VRIHG_ENABLE would be a better name.
Okay.
>
> > +
> > + Id: 18
> > + Equivalent ioctl: N/A
> > + Master payload: vring state description
> > +
> > + Set the flag(1 for enable and 0 for disable) to signal slave to enable
>
> Space before ( please.
Okay.
>
> > + or disable corresponding virt queue. This request should be sent only
> > + when the protocol feature bit VHOST_USER_PROTOCOL_F_VRING_FLAG is set.
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 141b557..7d9cc8d 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -418,6 +418,19 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> >
> > return vhost_net;
> > }
> > +
> > +int vhost_set_vring_flag(NetClientState *nc, int enable)
> > +{
> > + if (nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> > + VHostNetState *net = get_vhost_net(nc);
> > + const VhostOps *vhost_ops = net->dev.vhost_ops;
>
> add an empty line after declaration pls.
Got it, thanks.
>
> > + if (vhost_ops->vhost_backend_mq_set_vring_flag)
> > + return vhost_ops->vhost_backend_mq_set_vring_flag(&net->dev, enable);
>
> Coding style violation:
> line too long,
Yeah, 81 chars. I thougth that's kind of acceptable. But, anyway, since
you pointed it out, I will fix it next time.
> and should use {}.
TBH, I seldom use {} for one statement, and you could see that a lot from
this patch set. Is it a must in qemu community? If so, I could fix them all.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > #else
> > struct vhost_net *vhost_net_init(VhostNetOptions *options)
> > {
> > @@ -463,4 +476,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > {
> > return 0;
> > }
> > +
> > +int vhost_set_vring_flag(NetClientState *nc, int enable)
> > +{
> > + return 0;
> > +}
> > #endif
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 8d28e45..53f93b1 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -407,6 +407,7 @@ static int peer_attach(VirtIONet *n, int index)
> > }
> >
> > if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> > + vhost_set_vring_flag(nc->peer, 1);
> > return 0;
> > }
> >
> > @@ -422,6 +423,7 @@ static int peer_detach(VirtIONet *n, int index)
> > }
> >
> > if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> > + vhost_set_vring_flag(nc->peer, 0);
> > return 0;
> > }
> >
>
> makes no sense to call it for !== tap only.
> Either call this unconditionally, or just for vhost user.
Agreed. I will make it vhost-usre specific then.
>
>
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 11e46b5..ca6f7fa 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -25,9 +25,10 @@
> >
> > #define VHOST_MEMORY_MAX_NREGIONS 8
> > #define VHOST_USER_F_PROTOCOL_FEATURES 30
> > -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
> > +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0xfULL
> >
> > -#define VHOST_USER_PROTOCOL_F_MQ 0
> > +#define VHOST_USER_PROTOCOL_F_MQ 0
> > +#define VHOST_USER_PROTOCOL_F_VRING_FLAG 1
> >
> > typedef enum VhostUserRequest {
> > VHOST_USER_NONE = 0,
> > @@ -48,6 +49,7 @@ typedef enum VhostUserRequest {
> > VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> > VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> > VHOST_USER_GET_QUEUE_NUM = 17,
> > + VHOST_USER_SET_VRING_FLAG = 18,
> > VHOST_USER_MAX
> > } VhostUserRequest;
> >
> > @@ -296,6 +298,11 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > msg.addr.index += dev->vq_index;
> > msg.size = sizeof(m.state);
> > break;
> > + case VHOST_USER_SET_VRING_FLAG:
> > + msg.state.index = dev->vq_index;
> > + msg.state.num = *(int*)arg;
> > + msg.size = sizeof(msg.state);
> > + break;
> >
> > case VHOST_USER_GET_VRING_BASE:
> > memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > @@ -408,6 +415,22 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> > return 0;
> > }
> >
> > +static int vhost_user_set_vring_flag(struct vhost_dev *dev, int enable)
> > +{
> > + int err;
> > +
> > + assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> > +
> > + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_VRING_FLAG)))
> > + return -1;
> > +
> > + err = vhost_user_call(dev, VHOST_USER_SET_VRING_FLAG, &enable);
> > + if (err < 0)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > static int vhost_user_cleanup(struct vhost_dev *dev)
> > {
> > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>
> In fact, it's a per VQ pair call the way it's coded.
> So pls fix it, call it for all VQs.
Let me try.
>
>
> > @@ -421,5 +444,6 @@ const VhostOps user_ops = {
> > .backend_type = VHOST_BACKEND_TYPE_USER,
> > .vhost_call = vhost_user_call,
> > .vhost_backend_init = vhost_user_init,
> > - .vhost_backend_cleanup = vhost_user_cleanup
> > - };
> > + .vhost_backend_cleanup = vhost_user_cleanup,
> > + .vhost_backend_mq_set_vring_flag = vhost_user_set_vring_flag,
> > +};
> > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > index e472f29..6fc76b7 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> > void *arg);
> > typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > +typedef int (*vhost_backend_mq_set_vring_flag)(struct vhost_dev *dev, int enable);
> >
> > typedef struct VhostOps {
> > VhostBackendType backend_type;
> > vhost_call vhost_call;
> > vhost_backend_init vhost_backend_init;
> > vhost_backend_cleanup vhost_backend_cleanup;
> > + vhost_backend_mq_set_vring_flag vhost_backend_mq_set_vring_flag;
>
> That's quite ugly: mq is a vhost net thing.
> Why not enable each VQ?
To define it as `vhost_backend_enable_vring'?
Thanks.
--yliu
>
> > } VhostOps;
> >
> > extern const VhostOps user_ops;
> > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > index 8db723e..3ec33ff 100644
> > --- a/include/net/vhost_net.h
> > +++ b/include/net/vhost_net.h
> > @@ -28,4 +28,5 @@ bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
> > void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
> > int idx, bool mask);
> > VHostNetState *get_vhost_net(NetClientState *nc);
> > +int vhost_set_vring_flag(NetClientState * nc, int enable);
> > #endif
> > --
> > 1.9.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.
2015-09-09 13:38 ` Yuanhan Liu
@ 2015-09-09 14:18 ` Michael S. Tsirkin
2015-09-09 20:55 ` Michael S. Tsirkin
1 sibling, 0 replies; 39+ messages in thread
From: Michael S. Tsirkin @ 2015-09-09 14:18 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel, changchun.ouyang
On Wed, Sep 09, 2015 at 09:38:12PM +0800, Yuanhan Liu wrote:
> On Wed, Sep 09, 2015 at 01:45:50PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
> > > From: Changchun Ouyang <changchun.ouyang@intel.com>
> > >
> > > Add a new message, VHOST_USER_SET_VRING_FLAG, to enable and disable
> > > a specific virt queue, which is similar to attach/detach queue for
> > > tap device.
> > >
> > > virtio driver on guest doesn't have to use max virt queue pair, it
> > > could enable any number of virt queue ranging from 1 to max virt
> > > queue pair.
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >
> > Fine, but I would like to make this dependent on the MQ flag.
>
> Reasonable.
>
> >
> >
> > > ---
> > > docs/specs/vhost-user.txt | 10 ++++++++++
> > > hw/net/vhost_net.c | 18 ++++++++++++++++++
> > > hw/net/virtio-net.c | 2 ++
> > > hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++----
> > > include/hw/virtio/vhost-backend.h | 2 ++
> > > include/net/vhost_net.h | 1 +
> > > 6 files changed, 61 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > > index 99d79be..c2c38f3 100644
> > > --- a/docs/specs/vhost-user.txt
> > > +++ b/docs/specs/vhost-user.txt
> > > @@ -325,3 +325,13 @@ Message types
> > > Query how many queues the backend supports. This request should be
> > > sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
> > > features by VHOST_USER_GET_PROTOCOL_FEATURES.
> > > +
> > > + * VHOST_USER_SET_VRING_FLAG
> >
> >
> > SET_VRIHG_ENABLE would be a better name.
>
> Okay.
>
> >
> > > +
> > > + Id: 18
> > > + Equivalent ioctl: N/A
> > > + Master payload: vring state description
> > > +
> > > + Set the flag(1 for enable and 0 for disable) to signal slave to enable
> >
> > Space before ( please.
>
> Okay.
>
> >
> > > + or disable corresponding virt queue. This request should be sent only
> > > + when the protocol feature bit VHOST_USER_PROTOCOL_F_VRING_FLAG is set.
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 141b557..7d9cc8d 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -418,6 +418,19 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > >
> > > return vhost_net;
> > > }
> > > +
> > > +int vhost_set_vring_flag(NetClientState *nc, int enable)
> > > +{
> > > + if (nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> > > + VHostNetState *net = get_vhost_net(nc);
> > > + const VhostOps *vhost_ops = net->dev.vhost_ops;
> >
> > add an empty line after declaration pls.
>
> Got it, thanks.
>
> >
> > > + if (vhost_ops->vhost_backend_mq_set_vring_flag)
> > > + return vhost_ops->vhost_backend_mq_set_vring_flag(&net->dev, enable);
> >
> > Coding style violation:
> > line too long,
>
> Yeah, 81 chars. I thougth that's kind of acceptable. But, anyway, since
> you pointed it out, I will fix it next time.
>
> > and should use {}.
>
> TBH, I seldom use {} for one statement, and you could see that a lot from
> this patch set. Is it a must in qemu community?
Yes it is. It's different from the kernel.
> If so, I could fix them all.
>
> >
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > #else
> > > struct vhost_net *vhost_net_init(VhostNetOptions *options)
> > > {
> > > @@ -463,4 +476,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > > {
> > > return 0;
> > > }
> > > +
> > > +int vhost_set_vring_flag(NetClientState *nc, int enable)
> > > +{
> > > + return 0;
> > > +}
> > > #endif
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 8d28e45..53f93b1 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -407,6 +407,7 @@ static int peer_attach(VirtIONet *n, int index)
> > > }
> > >
> > > if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> > > + vhost_set_vring_flag(nc->peer, 1);
> > > return 0;
> > > }
> > >
> > > @@ -422,6 +423,7 @@ static int peer_detach(VirtIONet *n, int index)
> > > }
> > >
> > > if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> > > + vhost_set_vring_flag(nc->peer, 0);
> > > return 0;
> > > }
> > >
> >
> > makes no sense to call it for !== tap only.
> > Either call this unconditionally, or just for vhost user.
>
> Agreed. I will make it vhost-usre specific then.
>
> >
> >
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index 11e46b5..ca6f7fa 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -25,9 +25,10 @@
> > >
> > > #define VHOST_MEMORY_MAX_NREGIONS 8
> > > #define VHOST_USER_F_PROTOCOL_FEATURES 30
> > > -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
> > > +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0xfULL
> > >
> > > -#define VHOST_USER_PROTOCOL_F_MQ 0
> > > +#define VHOST_USER_PROTOCOL_F_MQ 0
> > > +#define VHOST_USER_PROTOCOL_F_VRING_FLAG 1
> > >
> > > typedef enum VhostUserRequest {
> > > VHOST_USER_NONE = 0,
> > > @@ -48,6 +49,7 @@ typedef enum VhostUserRequest {
> > > VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> > > VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> > > VHOST_USER_GET_QUEUE_NUM = 17,
> > > + VHOST_USER_SET_VRING_FLAG = 18,
> > > VHOST_USER_MAX
> > > } VhostUserRequest;
> > >
> > > @@ -296,6 +298,11 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > > msg.addr.index += dev->vq_index;
> > > msg.size = sizeof(m.state);
> > > break;
> > > + case VHOST_USER_SET_VRING_FLAG:
> > > + msg.state.index = dev->vq_index;
> > > + msg.state.num = *(int*)arg;
> > > + msg.size = sizeof(msg.state);
> > > + break;
> > >
> > > case VHOST_USER_GET_VRING_BASE:
> > > memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > > @@ -408,6 +415,22 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> > > return 0;
> > > }
> > >
> > > +static int vhost_user_set_vring_flag(struct vhost_dev *dev, int enable)
> > > +{
> > > + int err;
> > > +
> > > + assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> > > +
> > > + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_VRING_FLAG)))
> > > + return -1;
> > > +
> > > + err = vhost_user_call(dev, VHOST_USER_SET_VRING_FLAG, &enable);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int vhost_user_cleanup(struct vhost_dev *dev)
> > > {
> > > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >
> > In fact, it's a per VQ pair call the way it's coded.
> > So pls fix it, call it for all VQs.
>
> Let me try.
>
> >
> >
> > > @@ -421,5 +444,6 @@ const VhostOps user_ops = {
> > > .backend_type = VHOST_BACKEND_TYPE_USER,
> > > .vhost_call = vhost_user_call,
> > > .vhost_backend_init = vhost_user_init,
> > > - .vhost_backend_cleanup = vhost_user_cleanup
> > > - };
> > > + .vhost_backend_cleanup = vhost_user_cleanup,
> > > + .vhost_backend_mq_set_vring_flag = vhost_user_set_vring_flag,
> > > +};
> > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > > index e472f29..6fc76b7 100644
> > > --- a/include/hw/virtio/vhost-backend.h
> > > +++ b/include/hw/virtio/vhost-backend.h
> > > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> > > void *arg);
> > > typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > > typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > > +typedef int (*vhost_backend_mq_set_vring_flag)(struct vhost_dev *dev, int enable);
> > >
> > > typedef struct VhostOps {
> > > VhostBackendType backend_type;
> > > vhost_call vhost_call;
> > > vhost_backend_init vhost_backend_init;
> > > vhost_backend_cleanup vhost_backend_cleanup;
> > > + vhost_backend_mq_set_vring_flag vhost_backend_mq_set_vring_flag;
> >
> > That's quite ugly: mq is a vhost net thing.
> > Why not enable each VQ?
>
> To define it as `vhost_backend_enable_vring'?
>
> Thanks.
>
> --yliu
Sure.
> >
> > > } VhostOps;
> > >
> > > extern const VhostOps user_ops;
> > > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > > index 8db723e..3ec33ff 100644
> > > --- a/include/net/vhost_net.h
> > > +++ b/include/net/vhost_net.h
> > > @@ -28,4 +28,5 @@ bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
> > > void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
> > > int idx, bool mask);
> > > VHostNetState *get_vhost_net(NetClientState *nc);
> > > +int vhost_set_vring_flag(NetClientState * nc, int enable);
> > > #endif
> > > --
> > > 1.9.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.
2015-09-09 13:38 ` Yuanhan Liu
2015-09-09 14:18 ` Michael S. Tsirkin
@ 2015-09-09 20:55 ` Michael S. Tsirkin
1 sibling, 0 replies; 39+ messages in thread
From: Michael S. Tsirkin @ 2015-09-09 20:55 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel, changchun.ouyang
On Wed, Sep 09, 2015 at 09:38:12PM +0800, Yuanhan Liu wrote:
> On Wed, Sep 09, 2015 at 01:45:50PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
> > > From: Changchun Ouyang <changchun.ouyang@intel.com>
> > >
> > > Add a new message, VHOST_USER_SET_VRING_FLAG, to enable and disable
> > > a specific virt queue, which is similar to attach/detach queue for
> > > tap device.
> > >
> > > virtio driver on guest doesn't have to use max virt queue pair, it
> > > could enable any number of virt queue ranging from 1 to max virt
> > > queue pair.
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >
> > Fine, but I would like to make this dependent on the MQ flag.
>
> Reasonable.
>
> >
> >
> > > ---
> > > docs/specs/vhost-user.txt | 10 ++++++++++
> > > hw/net/vhost_net.c | 18 ++++++++++++++++++
> > > hw/net/virtio-net.c | 2 ++
> > > hw/virtio/vhost-user.c | 32 ++++++++++++++++++++++++++++----
> > > include/hw/virtio/vhost-backend.h | 2 ++
> > > include/net/vhost_net.h | 1 +
> > > 6 files changed, 61 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > > index 99d79be..c2c38f3 100644
> > > --- a/docs/specs/vhost-user.txt
> > > +++ b/docs/specs/vhost-user.txt
> > > @@ -325,3 +325,13 @@ Message types
> > > Query how many queues the backend supports. This request should be
> > > sent only when VHOST_USER_PROTOCOL_F_MQ is set in quried protocol
> > > features by VHOST_USER_GET_PROTOCOL_FEATURES.
> > > +
> > > + * VHOST_USER_SET_VRING_FLAG
> >
> >
> > SET_VRIHG_ENABLE would be a better name.
>
> Okay.
>
> >
> > > +
> > > + Id: 18
> > > + Equivalent ioctl: N/A
> > > + Master payload: vring state description
> > > +
> > > + Set the flag(1 for enable and 0 for disable) to signal slave to enable
> >
> > Space before ( please.
>
> Okay.
>
> >
> > > + or disable corresponding virt queue. This request should be sent only
> > > + when the protocol feature bit VHOST_USER_PROTOCOL_F_VRING_FLAG is set.
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 141b557..7d9cc8d 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -418,6 +418,19 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > >
> > > return vhost_net;
> > > }
> > > +
> > > +int vhost_set_vring_flag(NetClientState *nc, int enable)
> > > +{
> > > + if (nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> > > + VHostNetState *net = get_vhost_net(nc);
> > > + const VhostOps *vhost_ops = net->dev.vhost_ops;
> >
> > add an empty line after declaration pls.
>
> Got it, thanks.
>
> >
> > > + if (vhost_ops->vhost_backend_mq_set_vring_flag)
> > > + return vhost_ops->vhost_backend_mq_set_vring_flag(&net->dev, enable);
> >
> > Coding style violation:
> > line too long,
>
> Yeah, 81 chars. I thougth that's kind of acceptable. But, anyway, since
> you pointed it out, I will fix it next time.
>
> > and should use {}.
>
> TBH, I seldom use {} for one statement, and you could see that a lot from
> this patch set. Is it a must in qemu community?
Yes it is. It's different from the kernel.
> If so, I could fix them all.
>
> >
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > #else
> > > struct vhost_net *vhost_net_init(VhostNetOptions *options)
> > > {
> > > @@ -463,4 +476,9 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > > {
> > > return 0;
> > > }
> > > +
> > > +int vhost_set_vring_flag(NetClientState *nc, int enable)
> > > +{
> > > + return 0;
> > > +}
> > > #endif
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 8d28e45..53f93b1 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -407,6 +407,7 @@ static int peer_attach(VirtIONet *n, int index)
> > > }
> > >
> > > if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> > > + vhost_set_vring_flag(nc->peer, 1);
> > > return 0;
> > > }
> > >
> > > @@ -422,6 +423,7 @@ static int peer_detach(VirtIONet *n, int index)
> > > }
> > >
> > > if (nc->peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP) {
> > > + vhost_set_vring_flag(nc->peer, 0);
> > > return 0;
> > > }
> > >
> >
> > makes no sense to call it for !== tap only.
> > Either call this unconditionally, or just for vhost user.
>
> Agreed. I will make it vhost-usre specific then.
>
> >
> >
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index 11e46b5..ca6f7fa 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -25,9 +25,10 @@
> > >
> > > #define VHOST_MEMORY_MAX_NREGIONS 8
> > > #define VHOST_USER_F_PROTOCOL_FEATURES 30
> > > -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
> > > +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0xfULL
> > >
> > > -#define VHOST_USER_PROTOCOL_F_MQ 0
> > > +#define VHOST_USER_PROTOCOL_F_MQ 0
> > > +#define VHOST_USER_PROTOCOL_F_VRING_FLAG 1
> > >
> > > typedef enum VhostUserRequest {
> > > VHOST_USER_NONE = 0,
> > > @@ -48,6 +49,7 @@ typedef enum VhostUserRequest {
> > > VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> > > VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> > > VHOST_USER_GET_QUEUE_NUM = 17,
> > > + VHOST_USER_SET_VRING_FLAG = 18,
> > > VHOST_USER_MAX
> > > } VhostUserRequest;
> > >
> > > @@ -296,6 +298,11 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > > msg.addr.index += dev->vq_index;
> > > msg.size = sizeof(m.state);
> > > break;
> > > + case VHOST_USER_SET_VRING_FLAG:
> > > + msg.state.index = dev->vq_index;
> > > + msg.state.num = *(int*)arg;
> > > + msg.size = sizeof(msg.state);
> > > + break;
> > >
> > > case VHOST_USER_GET_VRING_BASE:
> > > memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > > @@ -408,6 +415,22 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque)
> > > return 0;
> > > }
> > >
> > > +static int vhost_user_set_vring_flag(struct vhost_dev *dev, int enable)
> > > +{
> > > + int err;
> > > +
> > > + assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> > > +
> > > + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_VRING_FLAG)))
> > > + return -1;
> > > +
> > > + err = vhost_user_call(dev, VHOST_USER_SET_VRING_FLAG, &enable);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int vhost_user_cleanup(struct vhost_dev *dev)
> > > {
> > > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >
> > In fact, it's a per VQ pair call the way it's coded.
> > So pls fix it, call it for all VQs.
>
> Let me try.
>
> >
> >
> > > @@ -421,5 +444,6 @@ const VhostOps user_ops = {
> > > .backend_type = VHOST_BACKEND_TYPE_USER,
> > > .vhost_call = vhost_user_call,
> > > .vhost_backend_init = vhost_user_init,
> > > - .vhost_backend_cleanup = vhost_user_cleanup
> > > - };
> > > + .vhost_backend_cleanup = vhost_user_cleanup,
> > > + .vhost_backend_mq_set_vring_flag = vhost_user_set_vring_flag,
> > > +};
> > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> > > index e472f29..6fc76b7 100644
> > > --- a/include/hw/virtio/vhost-backend.h
> > > +++ b/include/hw/virtio/vhost-backend.h
> > > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> > > void *arg);
> > > typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > > typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > > +typedef int (*vhost_backend_mq_set_vring_flag)(struct vhost_dev *dev, int enable);
> > >
> > > typedef struct VhostOps {
> > > VhostBackendType backend_type;
> > > vhost_call vhost_call;
> > > vhost_backend_init vhost_backend_init;
> > > vhost_backend_cleanup vhost_backend_cleanup;
> > > + vhost_backend_mq_set_vring_flag vhost_backend_mq_set_vring_flag;
> >
> > That's quite ugly: mq is a vhost net thing.
> > Why not enable each VQ?
>
> To define it as `vhost_backend_enable_vring'?
>
> Thanks.
>
> --yliu
Sure.
> >
> > > } VhostOps;
> > >
> > > extern const VhostOps user_ops;
> > > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> > > index 8db723e..3ec33ff 100644
> > > --- a/include/net/vhost_net.h
> > > +++ b/include/net/vhost_net.h
> > > @@ -28,4 +28,5 @@ bool vhost_net_virtqueue_pending(VHostNetState *net, int n);
> > > void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
> > > int idx, bool mask);
> > > VHostNetState *get_vhost_net(NetClientState *nc);
> > > +int vhost_set_vring_flag(NetClientState * nc, int enable);
> > > #endif
> > > --
> > > 1.9.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
` (2 preceding siblings ...)
2015-09-09 12:18 ` Michael S. Tsirkin
@ 2015-09-09 20:55 ` Michael S. Tsirkin
2015-09-14 10:00 ` Jason Wang
4 siblings, 0 replies; 39+ messages in thread
From: Michael S. Tsirkin @ 2015-09-09 20:55 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel, changchun.ouyang
On Tue, Sep 08, 2015 at 03:38:46PM +0800, Yuanhan Liu wrote:
> From: Ouyang Changchun <changchun.ouyang@intel.com>
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> Here is the latest version for adding vhost-user multiple queue support,
> by creating a nc and vhost_net pair for each queue.
>
> What differs from last version is that this patch addresses two major
> concerns from Michael, and fixes one hidden bug.
>
> - Concern #1: no feedback when the backend can't support # of
> requested queues(by providing queues=# option).
>
> Here we address this issue by querying VHOST_USER_PROTOCOL_F_MQ
> protocol features first, if not set, it means the backend don't
> support mq feature, and let max_queues be 1. Otherwise, we send
> another message, VHOST_USER_GET_QUEUE_NUM, for getting the max_queues
> the backend supports.
>
> At vhost-user initiation stage(net_vhost_user_start), we then initiate
> one queue first, which, in the meantime, also gets the max_queues.
> We then do a simple compare: if requested_queues > max_queues, we
> exit(I guess it's safe to exit here, as the vm is not running yet).
>
> - Concern #2: some messages are sent more times than necessary.
>
> We came an agreement with Michael that we could categorize vhost
> user messages to 2 types: none-vring specific messages, which should
> be sent only once, and vring specific messages, which should be sent
> per queue.
>
> Here I introduced a helper function vhost_user_one_time_request(),
> which lists following messages as none-vring specific messages:
>
> VHOST_USER_GET_FEATURES
> VHOST_USER_SET_FEATURES
> VHOST_USER_GET_PROTOCOL_FEATURES
> VHOST_USER_SET_PROTOCOL_FEATURES
> VHOST_USER_SET_OWNER
> VHOST_USER_RESET_DEVICE
> VHOST_USER_SET_MEM_TABLE
> VHOST_USER_GET_QUEUE_NUM
>
> For above messages, we simply ignore them when they are not sent the first
> time.
>
> I also observed a hidden bug from last version. We register the char dev
> event handler N times, which is not necessary, as well as buggy: A later
> register overwrites the former one, as qemu_chr_add_handlers() will not
> chain those handlers, but instead overwrites the old one. So, in theory,
> invoking qemu_chr_add_handlers N times will not end up with calling the
> handler N times.
>
> However, the reason the handler is invoked N times is because we start
> the backend(the connection server) first, and hence when net_vhost_user_init()
> is executed, the connection is already established, and qemu_chr_add_handlers()
> then invoke the handler immediately, which just looks like we invoke the
> handler(net_vhost_user_event) directly from net_vhost_user_init().
>
> The solution I came up with is to make VhostUserState as an upper level
> structure, making it includes N nc and vhost_net pairs:
>
> struct VhostUserNetPeer {
> NetClientState *nc;
> VHostNetState *vhost_net;
> };
>
> typedef struct VhostUserState {
> CharDriverState *chr;
> bool running;
> int queues;
> struct VhostUserNetPeer peers[];
> } VhostUserState;
>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> docs/specs/vhost-user.txt | 13 +++++
> hw/virtio/vhost-user.c | 31 ++++++++++-
> include/net/net.h | 1 +
> net/vhost-user.c | 136 ++++++++++++++++++++++++++++++++--------------
> qapi-schema.json | 6 +-
> qemu-options.hx | 5 +-
> 6 files changed, 146 insertions(+), 46 deletions(-)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 43db9b4..99d79be 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> a feature bit was dedicated for this purpose:
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +Multiple queue support
> +-------------------
> +Multiple queue is treated as a protocal extension, hence the slave has to
> +implement protocol features first. Multiple queues is supported only when
> +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
> +
> +The max # of queues the slave support can be queried with message
> +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
> +queues is bigger than that.
> +
> +As all queues share one connection, the master use a unique index for each
> +queue in the sent message to identify one specified queue.
> +
Please also document that only 2 queues are enabled initially.
More queues are enabled dynamically.
To enable more queues, the new message needs to be sent
(added by patch 7).
> Message types
> -------------
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8046bc0..11e46b5 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -187,6 +187,23 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
> 0 : -1;
> }
>
> +static bool vhost_user_one_time_request(VhostUserRequest request)
> +{
> + switch (request) {
> + case VHOST_USER_GET_FEATURES:
> + case VHOST_USER_SET_FEATURES:
> + case VHOST_USER_GET_PROTOCOL_FEATURES:
> + case VHOST_USER_SET_PROTOCOL_FEATURES:
> + case VHOST_USER_SET_OWNER:
> + case VHOST_USER_RESET_DEVICE:
> + case VHOST_USER_SET_MEM_TABLE:
> + case VHOST_USER_GET_QUEUE_NUM:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> void *arg)
> {
> @@ -206,6 +223,14 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> else
> msg_request = request;
>
> + /*
> + * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
> + * we just need send it once in the first time. For later such
> + * request, we just ignore it.
> + */
> + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0)
> + return 0;
> +
> msg.request = msg_request;
> msg.flags = VHOST_USER_VERSION;
> msg.size = 0;
> @@ -268,17 +293,20 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> case VHOST_USER_SET_VRING_NUM:
> case VHOST_USER_SET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.state);
> break;
>
> case VHOST_USER_GET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.state);
> need_reply = 1;
> break;
>
> case VHOST_USER_SET_VRING_ADDR:
> memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.addr);
> break;
>
> @@ -286,7 +314,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> case VHOST_USER_SET_VRING_CALL:
> case VHOST_USER_SET_VRING_ERR:
> file = arg;
> - msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> + msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
> msg.size = sizeof(m.u64);
> if (ioeventfd_enabled() && file->fd > 0) {
> fds[fd_num++] = file->fd;
> @@ -330,6 +358,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> error_report("Received bad msg size.");
> return -1;
> }
> + msg.state.index -= dev->vq_index;
> memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> break;
> default:
> diff --git a/include/net/net.h b/include/net/net.h
> index 6a6cbef..6f20656 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -92,6 +92,7 @@ struct NetClientState {
> NetClientDestructor *destructor;
> unsigned int queue_index;
> unsigned rxfilter_notify_enabled:1;
> + void *opaque;
> };
>
> typedef struct NICState {
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 2d6bbe5..7d4ac69 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -15,10 +15,16 @@
> #include "qemu/config-file.h"
> #include "qemu/error-report.h"
>
> +struct VhostUserNetPeer {
> + NetClientState *nc;
> + VHostNetState *vhost_net;
> +};
> +
> typedef struct VhostUserState {
> - NetClientState nc;
> CharDriverState *chr;
> - VHostNetState *vhost_net;
> + bool running;
> + int queues;
> + struct VhostUserNetPeer peers[];
> } VhostUserState;
>
> typedef struct VhostUserChardevProps {
> @@ -29,48 +35,75 @@ typedef struct VhostUserChardevProps {
>
> VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> {
> - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> + VhostUserState *s = nc->opaque;
> assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> - return s->vhost_net;
> -}
> -
> -static int vhost_user_running(VhostUserState *s)
> -{
> - return (s->vhost_net) ? 1 : 0;
> + return s->peers[nc->queue_index].vhost_net;
> }
>
> static int vhost_user_start(VhostUserState *s)
> {
> VhostNetOptions options;
> + VHostNetState *vhost_net;
> + int max_queues;
> + int i = 0;
>
> - if (vhost_user_running(s)) {
> + if (s->running)
> return 0;
> - }
>
> options.backend_type = VHOST_BACKEND_TYPE_USER;
> - options.net_backend = &s->nc;
> options.opaque = s->chr;
>
> - s->vhost_net = vhost_net_init(&options);
> + options.net_backend = s->peers[i].nc;
> + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> +
> + max_queues = vhost_net_get_max_queues(vhost_net);
> + if (s->queues >= max_queues) {
> + error_report("you are asking more queues than supported: %d\n",
> + max_queues);
> + return -1;
> + }
> +
> + for (; i < s->queues; i++) {
> + options.net_backend = s->peers[i].nc;
> +
> + s->peers[i].vhost_net = vhost_net_init(&options);
> + if (!s->peers[i].vhost_net)
> + return -1;
> + }
> + s->running = true;
>
> - return vhost_user_running(s) ? 0 : -1;
> + return 0;
> }
>
> static void vhost_user_stop(VhostUserState *s)
> {
> - if (vhost_user_running(s)) {
> - vhost_net_cleanup(s->vhost_net);
> + int i;
> + VHostNetState *vhost_net;
> +
> + if (!s->running)
> + return;
> +
> + for (i = 0; i < s->queues; i++) {
> + vhost_net = s->peers[i].vhost_net;
> + if (vhost_net)
> + vhost_net_cleanup(vhost_net);
> }
>
> - s->vhost_net = 0;
> + s->running = false;
> }
>
> static void vhost_user_cleanup(NetClientState *nc)
> {
> - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> + VhostUserState *s = nc->opaque;
> + VHostNetState *vhost_net = s->peers[nc->queue_index].vhost_net;
> +
> + if (vhost_net)
> + vhost_net_cleanup(vhost_net);
>
> - vhost_user_stop(s);
> qemu_purge_queued_packets(nc);
> +
> + if (nc->queue_index == s->queues - 1)
> + free(s);
> }
>
> static bool vhost_user_has_vnet_hdr(NetClientState *nc)
> @@ -89,7 +122,7 @@ static bool vhost_user_has_ufo(NetClientState *nc)
>
> static NetClientInfo net_vhost_user_info = {
> .type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
> - .size = sizeof(VhostUserState),
> + .size = sizeof(NetClientState),
> .cleanup = vhost_user_cleanup,
> .has_vnet_hdr = vhost_user_has_vnet_hdr,
> .has_ufo = vhost_user_has_ufo,
> @@ -97,18 +130,25 @@ static NetClientInfo net_vhost_user_info = {
>
> static void net_vhost_link_down(VhostUserState *s, bool link_down)
> {
> - s->nc.link_down = link_down;
> + NetClientState *nc;
> + int i;
>
> - if (s->nc.peer) {
> - s->nc.peer->link_down = link_down;
> - }
> + for (i = 0; i < s->queues; i++) {
> + nc = s->peers[i].nc;
>
> - if (s->nc.info->link_status_changed) {
> - s->nc.info->link_status_changed(&s->nc);
> - }
> + nc->link_down = link_down;
> +
> + if (nc->peer) {
> + nc->peer->link_down = link_down;
> + }
>
> - if (s->nc.peer && s->nc.peer->info->link_status_changed) {
> - s->nc.peer->info->link_status_changed(s->nc.peer);
> + if (nc->info->link_status_changed) {
> + nc->info->link_status_changed(nc);
> + }
> +
> + if (nc->peer && nc->peer->info->link_status_changed) {
> + nc->peer->info->link_status_changed(nc->peer);
> + }
> }
> }
>
> @@ -118,7 +158,8 @@ static void net_vhost_user_event(void *opaque, int event)
>
> switch (event) {
> case CHR_EVENT_OPENED:
> - vhost_user_start(s);
> + if (vhost_user_start(s) < 0)
> + exit(1);
> net_vhost_link_down(s, false);
> error_report("chardev \"%s\" went up", s->chr->label);
> break;
> @@ -131,24 +172,28 @@ static void net_vhost_user_event(void *opaque, int event)
> }
>
> static int net_vhost_user_init(NetClientState *peer, const char *device,
> - const char *name, CharDriverState *chr)
> + const char *name, VhostUserState *s)
> {
> NetClientState *nc;
> - VhostUserState *s;
> + CharDriverState *chr = s->chr;
> + int i;
>
> - nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> + for (i = 0; i < s->queues; i++) {
> + nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
>
> - snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> - chr->label);
> + snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> + i, chr->label);
>
> - s = DO_UPCAST(VhostUserState, nc, nc);
> + /* We don't provide a receive callback */
> + nc->receive_disabled = 1;
>
> - /* We don't provide a receive callback */
> - s->nc.receive_disabled = 1;
> - s->chr = chr;
> - nc->queue_index = 0;
> + nc->queue_index = i;
> + nc->opaque = s;
>
> - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> + s->peers[i].nc = nc;
> + }
> +
> + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, s);
>
> return 0;
> }
> @@ -227,8 +272,10 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
> int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> NetClientState *peer, Error **errp)
> {
> + int queues;
> const NetdevVhostUserOptions *vhost_user_opts;
> CharDriverState *chr;
> + VhostUserState *s;
>
> assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> vhost_user_opts = opts->vhost_user;
> @@ -244,6 +291,11 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> return -1;
> }
>
> + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
> + s = g_malloc0(sizeof(VhostUserState) +
> + queues * sizeof(struct VhostUserNetPeer));
> + s->queues = queues;
> + s->chr = chr;
>
> - return net_vhost_user_init(peer, "vhost_user", name, chr);
> + return net_vhost_user_init(peer, "vhost_user", name, s);
> }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 67fef37..55c33db 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2480,12 +2480,16 @@
> #
> # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> #
> +# @queues: #optional number of queues to be created for multiqueue vhost-user
> +# (default: 1) (Since 2.5)
> +#
> # Since 2.1
> ##
> { 'struct': 'NetdevVhostUserOptions',
> 'data': {
> 'chardev': 'str',
> - '*vhostforce': 'bool' } }
> + '*vhostforce': 'bool',
> + '*queues': 'int' } }
>
> ##
> # @NetClientOptions
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 77f5853..5bfa7a3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1963,13 +1963,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
> netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the
> required hub automatically.
>
> -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
> +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
>
> Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
> be a unix domain socket backed one. The vhost-user uses a specifically defined
> protocol to pass vhost ioctl replacement messages to an application on the other
> end of the socket. On non-MSIX guests, the feature can be forced with
> -@var{vhostforce}.
> +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
> +be created for multiqueue vhost-user.
>
> Example:
> @example
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-08 7:38 ` [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init Yuanhan Liu
@ 2015-09-10 3:14 ` Jason Wang
2015-09-10 3:57 ` Yuanhan Liu
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2015-09-10 3:14 UTC (permalink / raw)
To: Yuanhan Liu, qemu-devel; +Cc: mst, changchun.ouyang
On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> So that we could use the `vq_index' as well in the vhost_net_init
> stage, which is required when adding vhost-user multiple-queue support,
> where we need the vq_index to indicate which queue pair we are gonna
> initiate.
>
> vhost-user has no multiple queue support yet, hence no queue_index set
> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
> will be set properly soon in the next patch.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> hw/net/vhost_net.c | 16 +++++++---------
> net/vhost-user.c | 1 +
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index f9441e9..141b557 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
> }
> }
>
> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> +{
> + net->dev.vq_index = vq_index;
> +}
> +
> struct vhost_net *vhost_net_init(VhostNetOptions *options)
> {
> int r;
> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> }
> net->nc = options->net_backend;
>
> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
> +
This breaks vhost kernel multiqueue since queue_index was not
initialized at this time. We do this in set_netdev() instead of setting
it in each kind of netdev.
> net->dev.nvqs = 2;
> net->dev.vqs = net->vqs;
>
> @@ -196,11 +203,6 @@ fail:
> return NULL;
> }
>
> -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> -{
> - net->dev.vq_index = vq_index;
> -}
> -
> static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer,
> bool set)
> {
> @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> goto err;
> }
>
> - for (i = 0; i < total_queues; i++) {
> - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
> - }
> -
> r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
> if (r < 0) {
> error_report("Error binding guest notifier: %d", -r);
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 93dcecd..2d6bbe5 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
> /* We don't provide a receive callback */
> s->nc.receive_disabled = 1;
> s->chr = chr;
> + nc->queue_index = 0;
>
Fail to understand why this is needed. It will be set to correct value
in set_netdev().
> qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 3:14 ` Jason Wang
@ 2015-09-10 3:57 ` Yuanhan Liu
2015-09-10 4:46 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-10 3:57 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel, changchun.ouyang
On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
>
>
> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> > So that we could use the `vq_index' as well in the vhost_net_init
> > stage, which is required when adding vhost-user multiple-queue support,
> > where we need the vq_index to indicate which queue pair we are gonna
> > initiate.
> >
> > vhost-user has no multiple queue support yet, hence no queue_index set
> > before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
> > will be set properly soon in the next patch.
> >
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> > hw/net/vhost_net.c | 16 +++++++---------
> > net/vhost-user.c | 1 +
> > 2 files changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index f9441e9..141b557 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
> > }
> > }
> >
> > +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> > +{
> > + net->dev.vq_index = vq_index;
> > +}
> > +
> > struct vhost_net *vhost_net_init(VhostNetOptions *options)
> > {
> > int r;
> > @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> > }
> > net->nc = options->net_backend;
> >
> > + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
> > +
>
> This breaks vhost kernel multiqueue since queue_index was not
> initialized at this time.
Right, thanks for pointing it out.
> We do this in set_netdev() instead of setting
> it in each kind of netdev.
Can we move it to net_init_tap() for setting the right queue_index
for each nc?
Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
vhost-user mq support), another one at vhost_net_start(for vhost kernel
mq support)?
Or, do you have better ideas?
>
> > net->dev.nvqs = 2;
> > net->dev.vqs = net->vqs;
> >
> > @@ -196,11 +203,6 @@ fail:
> > return NULL;
> > }
> >
> > -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> > -{
> > - net->dev.vq_index = vq_index;
> > -}
> > -
> > static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer,
> > bool set)
> > {
> > @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> > goto err;
> > }
> >
> > - for (i = 0; i < total_queues; i++) {
> > - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
> > - }
> > -
> > r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
> > if (r < 0) {
> > error_report("Error binding guest notifier: %d", -r);
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 93dcecd..2d6bbe5 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
> > /* We don't provide a receive callback */
> > s->nc.receive_disabled = 1;
> > s->chr = chr;
> > + nc->queue_index = 0;
> >
>
> Fail to understand why this is needed. It will be set to correct value
> in set_netdev().
Right, it's unnecessary.
BTW, May I ask you a quick question: why set_netdev is invoked after
tap init, while it is invoked before vhost-user init?
--yliu
>
> > qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> >
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 3:57 ` Yuanhan Liu
@ 2015-09-10 4:46 ` Jason Wang
2015-09-10 5:17 ` Yuanhan Liu
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2015-09-10 4:46 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: mst, qemu-devel, changchun.ouyang
On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
> On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
>>
>> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
>>> So that we could use the `vq_index' as well in the vhost_net_init
>>> stage, which is required when adding vhost-user multiple-queue support,
>>> where we need the vq_index to indicate which queue pair we are gonna
>>> initiate.
>>>
>>> vhost-user has no multiple queue support yet, hence no queue_index set
>>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
>>> will be set properly soon in the next patch.
>>>
>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>> ---
>>> hw/net/vhost_net.c | 16 +++++++---------
>>> net/vhost-user.c | 1 +
>>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>> index f9441e9..141b557 100644
>>> --- a/hw/net/vhost_net.c
>>> +++ b/hw/net/vhost_net.c
>>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
>>> }
>>> }
>>>
>>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
>>> +{
>>> + net->dev.vq_index = vq_index;
>>> +}
>>> +
>>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>> {
>>> int r;
>>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>> }
>>> net->nc = options->net_backend;
>>>
>>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
>>> +
>> This breaks vhost kernel multiqueue since queue_index was not
>> initialized at this time.
> Right, thanks for pointing it out.
>
>> We do this in set_netdev() instead of setting
>> it in each kind of netdev.
> Can we move it to net_init_tap() for setting the right queue_index
> for each nc?
>
> Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
> vhost-user mq support), another one at vhost_net_start(for vhost kernel
> mq support)?
>
> Or, do you have better ideas?
I think setting queue_index in net_init_tap() looks ok. But a question
is that why need we do this at so early stage? ( Even before its peers
is connected.)
>
>>> net->dev.nvqs = 2;
>>> net->dev.vqs = net->vqs;
>>>
>>> @@ -196,11 +203,6 @@ fail:
>>> return NULL;
>>> }
>>>
>>> -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
>>> -{
>>> - net->dev.vq_index = vq_index;
>>> -}
>>> -
>>> static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer,
>>> bool set)
>>> {
>>> @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>> goto err;
>>> }
>>>
>>> - for (i = 0; i < total_queues; i++) {
>>> - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
>>> - }
>>> -
>>> r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
>>> if (r < 0) {
>>> error_report("Error binding guest notifier: %d", -r);
>>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>>> index 93dcecd..2d6bbe5 100644
>>> --- a/net/vhost-user.c
>>> +++ b/net/vhost-user.c
>>> @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
>>> /* We don't provide a receive callback */
>>> s->nc.receive_disabled = 1;
>>> s->chr = chr;
>>> + nc->queue_index = 0;
>>>
>> Fail to understand why this is needed. It will be set to correct value
>> in set_netdev().
> Right, it's unnecessary.
>
> BTW, May I ask you a quick question: why set_netdev is invoked after
> tap init, while it is invoked before vhost-user init?
>
> --yliu
I think the reason is that you must initialize all netdevs before a
device tries to connect them.
>>> qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
>>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 4:46 ` Jason Wang
@ 2015-09-10 5:17 ` Yuanhan Liu
2015-09-10 5:52 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-10 5:17 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel, changchun.ouyang
On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
>
>
> On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
> > On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
> >>
> >> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> >>> So that we could use the `vq_index' as well in the vhost_net_init
> >>> stage, which is required when adding vhost-user multiple-queue support,
> >>> where we need the vq_index to indicate which queue pair we are gonna
> >>> initiate.
> >>>
> >>> vhost-user has no multiple queue support yet, hence no queue_index set
> >>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
> >>> will be set properly soon in the next patch.
> >>>
> >>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>> ---
> >>> hw/net/vhost_net.c | 16 +++++++---------
> >>> net/vhost-user.c | 1 +
> >>> 2 files changed, 8 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >>> index f9441e9..141b557 100644
> >>> --- a/hw/net/vhost_net.c
> >>> +++ b/hw/net/vhost_net.c
> >>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
> >>> }
> >>> }
> >>>
> >>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> >>> +{
> >>> + net->dev.vq_index = vq_index;
> >>> +}
> >>> +
> >>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>> {
> >>> int r;
> >>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>> }
> >>> net->nc = options->net_backend;
> >>>
> >>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
> >>> +
> >> This breaks vhost kernel multiqueue since queue_index was not
> >> initialized at this time.
> > Right, thanks for pointing it out.
> >
> >> We do this in set_netdev() instead of setting
> >> it in each kind of netdev.
> > Can we move it to net_init_tap() for setting the right queue_index
> > for each nc?
> >
> > Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
> > vhost-user mq support), another one at vhost_net_start(for vhost kernel
> > mq support)?
> >
> > Or, do you have better ideas?
>
> I think setting queue_index in net_init_tap() looks ok.
Good to know.
> But a question
> is that why need we do this at so early stage? ( Even before its peers
> is connected.)
For vhost-user multiple queues support, we will invoke vhost_net_init()
N times for each queue pair, and hence we need to distinguish which
queue it is while sending messages like VHOST_SET_VRING_CALL for
initializing corresponding queue pair.
Does that make sense to you?
>
> >
> >>> net->dev.nvqs = 2;
> >>> net->dev.vqs = net->vqs;
> >>>
> >>> @@ -196,11 +203,6 @@ fail:
> >>> return NULL;
> >>> }
> >>>
> >>> -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> >>> -{
> >>> - net->dev.vq_index = vq_index;
> >>> -}
> >>> -
> >>> static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer,
> >>> bool set)
> >>> {
> >>> @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >>> goto err;
> >>> }
> >>>
> >>> - for (i = 0; i < total_queues; i++) {
> >>> - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
> >>> - }
> >>> -
> >>> r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
> >>> if (r < 0) {
> >>> error_report("Error binding guest notifier: %d", -r);
> >>> diff --git a/net/vhost-user.c b/net/vhost-user.c
> >>> index 93dcecd..2d6bbe5 100644
> >>> --- a/net/vhost-user.c
> >>> +++ b/net/vhost-user.c
> >>> @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
> >>> /* We don't provide a receive callback */
> >>> s->nc.receive_disabled = 1;
> >>> s->chr = chr;
> >>> + nc->queue_index = 0;
> >>>
> >> Fail to understand why this is needed. It will be set to correct value
> >> in set_netdev().
> > Right, it's unnecessary.
> >
> > BTW, May I ask you a quick question: why set_netdev is invoked after
> > tap init, while it is invoked before vhost-user init?
> >
> > --yliu
>
> I think the reason is that you must initialize all netdevs before a
> device tries to connect them.
Thanks!
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 5:17 ` Yuanhan Liu
@ 2015-09-10 5:52 ` Jason Wang
2015-09-10 6:18 ` Yuanhan Liu
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2015-09-10 5:52 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: mst, qemu-devel, changchun.ouyang
On 09/10/2015 01:17 PM, Yuanhan Liu wrote:
> On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
>> >
>> >
>> > On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
>>> > > On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
>>>> > >>
>>>> > >> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
>>>>> > >>> So that we could use the `vq_index' as well in the vhost_net_init
>>>>> > >>> stage, which is required when adding vhost-user multiple-queue support,
>>>>> > >>> where we need the vq_index to indicate which queue pair we are gonna
>>>>> > >>> initiate.
>>>>> > >>>
>>>>> > >>> vhost-user has no multiple queue support yet, hence no queue_index set
>>>>> > >>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
>>>>> > >>> will be set properly soon in the next patch.
>>>>> > >>>
>>>>> > >>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>> > >>> ---
>>>>> > >>> hw/net/vhost_net.c | 16 +++++++---------
>>>>> > >>> net/vhost-user.c | 1 +
>>>>> > >>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>>>> > >>>
>>>>> > >>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>> > >>> index f9441e9..141b557 100644
>>>>> > >>> --- a/hw/net/vhost_net.c
>>>>> > >>> +++ b/hw/net/vhost_net.c
>>>>> > >>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
>>>>> > >>> }
>>>>> > >>> }
>>>>> > >>>
>>>>> > >>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
>>>>> > >>> +{
>>>>> > >>> + net->dev.vq_index = vq_index;
>>>>> > >>> +}
>>>>> > >>> +
>>>>> > >>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>>>> > >>> {
>>>>> > >>> int r;
>>>>> > >>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>>>> > >>> }
>>>>> > >>> net->nc = options->net_backend;
>>>>> > >>>
>>>>> > >>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
>>>>> > >>> +
>>>> > >> This breaks vhost kernel multiqueue since queue_index was not
>>>> > >> initialized at this time.
>>> > > Right, thanks for pointing it out.
>>> > >
>>>> > >> We do this in set_netdev() instead of setting
>>>> > >> it in each kind of netdev.
>>> > > Can we move it to net_init_tap() for setting the right queue_index
>>> > > for each nc?
>>> > >
>>> > > Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
>>> > > vhost-user mq support), another one at vhost_net_start(for vhost kernel
>>> > > mq support)?
>>> > >
>>> > > Or, do you have better ideas?
>> >
>> > I think setting queue_index in net_init_tap() looks ok.
> Good to know.
>
>> > But a question
>> > is that why need we do this at so early stage? ( Even before its peers
>> > is connected.)
> For vhost-user multiple queues support, we will invoke vhost_net_init()
> N times for each queue pair, and hence we need to distinguish which
> queue it is while sending messages like VHOST_SET_VRING_CALL for
> initializing corresponding queue pair.
>
> Does that make sense to you?
>
Not sure. Since current codes works for vhost-kernel. (vhost_net_init()
was also called N times). We don't want to break existed vhost-kernel
API when developing multiqueue. For each virtqueue TX/RX pair, we have
one vhost net device and it has no knowledge for the others (which was
hide by qemu). So VHOST_SET_VRING_CALL works without any change here.
For the case here, since you still have multiple instances of vhost_net
structure. Maybe the vhost-user backend can distinguish form this?
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 5:52 ` Jason Wang
@ 2015-09-10 6:18 ` Yuanhan Liu
2015-09-10 6:54 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-10 6:18 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel, changchun.ouyang
On Thu, Sep 10, 2015 at 01:52:30PM +0800, Jason Wang wrote:
>
>
> On 09/10/2015 01:17 PM, Yuanhan Liu wrote:
> > On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
> >> >
> >> >
> >> > On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
> >>> > > On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
> >>>> > >>
> >>>> > >> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> >>>>> > >>> So that we could use the `vq_index' as well in the vhost_net_init
> >>>>> > >>> stage, which is required when adding vhost-user multiple-queue support,
> >>>>> > >>> where we need the vq_index to indicate which queue pair we are gonna
> >>>>> > >>> initiate.
> >>>>> > >>>
> >>>>> > >>> vhost-user has no multiple queue support yet, hence no queue_index set
> >>>>> > >>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
> >>>>> > >>> will be set properly soon in the next patch.
> >>>>> > >>>
> >>>>> > >>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>> > >>> ---
> >>>>> > >>> hw/net/vhost_net.c | 16 +++++++---------
> >>>>> > >>> net/vhost-user.c | 1 +
> >>>>> > >>> 2 files changed, 8 insertions(+), 9 deletions(-)
> >>>>> > >>>
> >>>>> > >>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >>>>> > >>> index f9441e9..141b557 100644
> >>>>> > >>> --- a/hw/net/vhost_net.c
> >>>>> > >>> +++ b/hw/net/vhost_net.c
> >>>>> > >>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
> >>>>> > >>> }
> >>>>> > >>> }
> >>>>> > >>>
> >>>>> > >>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> >>>>> > >>> +{
> >>>>> > >>> + net->dev.vq_index = vq_index;
> >>>>> > >>> +}
> >>>>> > >>> +
> >>>>> > >>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>>>> > >>> {
> >>>>> > >>> int r;
> >>>>> > >>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>>>> > >>> }
> >>>>> > >>> net->nc = options->net_backend;
> >>>>> > >>>
> >>>>> > >>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
> >>>>> > >>> +
> >>>> > >> This breaks vhost kernel multiqueue since queue_index was not
> >>>> > >> initialized at this time.
> >>> > > Right, thanks for pointing it out.
> >>> > >
> >>>> > >> We do this in set_netdev() instead of setting
> >>>> > >> it in each kind of netdev.
> >>> > > Can we move it to net_init_tap() for setting the right queue_index
> >>> > > for each nc?
> >>> > >
> >>> > > Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
> >>> > > vhost-user mq support), another one at vhost_net_start(for vhost kernel
> >>> > > mq support)?
> >>> > >
> >>> > > Or, do you have better ideas?
> >> >
> >> > I think setting queue_index in net_init_tap() looks ok.
> > Good to know.
> >
> >> > But a question
> >> > is that why need we do this at so early stage? ( Even before its peers
> >> > is connected.)
> > For vhost-user multiple queues support, we will invoke vhost_net_init()
> > N times for each queue pair, and hence we need to distinguish which
> > queue it is while sending messages like VHOST_SET_VRING_CALL for
> > initializing corresponding queue pair.
> >
> > Does that make sense to you?
> >
>
> Not sure. Since current codes works for vhost-kernel. (vhost_net_init()
> was also called N times). We don't want to break existed vhost-kernel
> API when developing multiqueue. For each virtqueue TX/RX pair, we have
> one vhost net device and it has no knowledge for the others (which was
> hide by qemu). So VHOST_SET_VRING_CALL works without any change here.
>
> For the case here, since you still have multiple instances of vhost_net
> structure. Maybe the vhost-user backend can distinguish form this?
Yeah, I guess that's the difference between vhost-user and vhost-kernel.
Vhost-kernel opens a char device(/dev/vhost-net) for each vhost_dev,
hence it's distinguishable. But for vhost-user, all vhost_dev share one
char device(a socket) for communication, hence, it's not distinguishable.
I was thinking maybe we could export vhost_net_set_vq_index() and invoke
it at net/vhost-user.c, so that we break nothing, and in the meantime,
it keeps the logic inside vhost-user.
What do you think?
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 6:18 ` Yuanhan Liu
@ 2015-09-10 6:54 ` Jason Wang
2015-09-10 7:06 ` Yuanhan Liu
2015-09-14 5:49 ` Yuanhan Liu
0 siblings, 2 replies; 39+ messages in thread
From: Jason Wang @ 2015-09-10 6:54 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: changchun.ouyang, qemu-devel, mst
On 09/10/2015 02:18 PM, Yuanhan Liu wrote:
> On Thu, Sep 10, 2015 at 01:52:30PM +0800, Jason Wang wrote:
>>
>> On 09/10/2015 01:17 PM, Yuanhan Liu wrote:
>>> On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
>>>>>
>>>>> On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
>>>>>>> On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
>>>>>>>>> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
>>>>>>>>>>> So that we could use the `vq_index' as well in the vhost_net_init
>>>>>>>>>>> stage, which is required when adding vhost-user multiple-queue support,
>>>>>>>>>>> where we need the vq_index to indicate which queue pair we are gonna
>>>>>>>>>>> initiate.
>>>>>>>>>>>
>>>>>>>>>>> vhost-user has no multiple queue support yet, hence no queue_index set
>>>>>>>>>>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
>>>>>>>>>>> will be set properly soon in the next patch.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>> hw/net/vhost_net.c | 16 +++++++---------
>>>>>>>>>>> net/vhost-user.c | 1 +
>>>>>>>>>>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>>>>>>>> index f9441e9..141b557 100644
>>>>>>>>>>> --- a/hw/net/vhost_net.c
>>>>>>>>>>> +++ b/hw/net/vhost_net.c
>>>>>>>>>>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
>>>>>>>>>>> +{
>>>>>>>>>>> + net->dev.vq_index = vq_index;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>>>>>>>>>> {
>>>>>>>>>>> int r;
>>>>>>>>>>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>>>>>>>>>>> }
>>>>>>>>>>> net->nc = options->net_backend;
>>>>>>>>>>>
>>>>>>>>>>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
>>>>>>>>>>> +
>>>>>>>>> This breaks vhost kernel multiqueue since queue_index was not
>>>>>>>>> initialized at this time.
>>>>>>> Right, thanks for pointing it out.
>>>>>>>
>>>>>>>>> We do this in set_netdev() instead of setting
>>>>>>>>> it in each kind of netdev.
>>>>>>> Can we move it to net_init_tap() for setting the right queue_index
>>>>>>> for each nc?
>>>>>>>
>>>>>>> Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
>>>>>>> vhost-user mq support), another one at vhost_net_start(for vhost kernel
>>>>>>> mq support)?
>>>>>>>
>>>>>>> Or, do you have better ideas?
>>>>> I think setting queue_index in net_init_tap() looks ok.
>>> Good to know.
>>>
>>>>> But a question
>>>>> is that why need we do this at so early stage? ( Even before its peers
>>>>> is connected.)
>>> For vhost-user multiple queues support, we will invoke vhost_net_init()
>>> N times for each queue pair, and hence we need to distinguish which
>>> queue it is while sending messages like VHOST_SET_VRING_CALL for
>>> initializing corresponding queue pair.
>>>
>>> Does that make sense to you?
>>>
>> Not sure. Since current codes works for vhost-kernel. (vhost_net_init()
>> was also called N times). We don't want to break existed vhost-kernel
>> API when developing multiqueue. For each virtqueue TX/RX pair, we have
>> one vhost net device and it has no knowledge for the others (which was
>> hide by qemu). So VHOST_SET_VRING_CALL works without any change here.
>>
>> For the case here, since you still have multiple instances of vhost_net
>> structure. Maybe the vhost-user backend can distinguish form this?
> Yeah, I guess that's the difference between vhost-user and vhost-kernel.
> Vhost-kernel opens a char device(/dev/vhost-net) for each vhost_dev,
> hence it's distinguishable. But for vhost-user, all vhost_dev share one
> char device(a socket) for communication, hence, it's not distinguishable.
How about using individual socket in this case? This seems can also
minimize the changes of backend.
>
> I was thinking maybe we could export vhost_net_set_vq_index() and invoke
> it at net/vhost-user.c, so that we break nothing, and in the meantime,
> it keeps the logic inside vhost-user.
>
> What do you think?
>
> --yliu
>
Sounds work. Then I believe you will need to set queue_index in
vhost_user initialization code?
Thanks
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 6:54 ` Jason Wang
@ 2015-09-10 7:06 ` Yuanhan Liu
2015-09-14 5:49 ` Yuanhan Liu
1 sibling, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-10 7:06 UTC (permalink / raw)
To: Jason Wang; +Cc: changchun.ouyang, qemu-devel, mst
On Thu, Sep 10, 2015 at 02:54:02PM +0800, Jason Wang wrote:
>
>
> On 09/10/2015 02:18 PM, Yuanhan Liu wrote:
> > On Thu, Sep 10, 2015 at 01:52:30PM +0800, Jason Wang wrote:
> >>
> >> On 09/10/2015 01:17 PM, Yuanhan Liu wrote:
> >>> On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
> >>>>>
> >>>>> On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
> >>>>>>> On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
> >>>>>>>>> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> >>>>>>>>>>> So that we could use the `vq_index' as well in the vhost_net_init
> >>>>>>>>>>> stage, which is required when adding vhost-user multiple-queue support,
> >>>>>>>>>>> where we need the vq_index to indicate which queue pair we are gonna
> >>>>>>>>>>> initiate.
> >>>>>>>>>>>
> >>>>>>>>>>> vhost-user has no multiple queue support yet, hence no queue_index set
> >>>>>>>>>>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
> >>>>>>>>>>> will be set properly soon in the next patch.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>> hw/net/vhost_net.c | 16 +++++++---------
> >>>>>>>>>>> net/vhost-user.c | 1 +
> >>>>>>>>>>> 2 files changed, 8 insertions(+), 9 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >>>>>>>>>>> index f9441e9..141b557 100644
> >>>>>>>>>>> --- a/hw/net/vhost_net.c
> >>>>>>>>>>> +++ b/hw/net/vhost_net.c
> >>>>>>>>>>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
> >>>>>>>>>>> }
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> >>>>>>>>>>> +{
> >>>>>>>>>>> + net->dev.vq_index = vq_index;
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>>>>>>>>>> {
> >>>>>>>>>>> int r;
> >>>>>>>>>>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>>>>>>>>>> }
> >>>>>>>>>>> net->nc = options->net_backend;
> >>>>>>>>>>>
> >>>>>>>>>>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
> >>>>>>>>>>> +
> >>>>>>>>> This breaks vhost kernel multiqueue since queue_index was not
> >>>>>>>>> initialized at this time.
> >>>>>>> Right, thanks for pointing it out.
> >>>>>>>
> >>>>>>>>> We do this in set_netdev() instead of setting
> >>>>>>>>> it in each kind of netdev.
> >>>>>>> Can we move it to net_init_tap() for setting the right queue_index
> >>>>>>> for each nc?
> >>>>>>>
> >>>>>>> Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
> >>>>>>> vhost-user mq support), another one at vhost_net_start(for vhost kernel
> >>>>>>> mq support)?
> >>>>>>>
> >>>>>>> Or, do you have better ideas?
> >>>>> I think setting queue_index in net_init_tap() looks ok.
> >>> Good to know.
> >>>
> >>>>> But a question
> >>>>> is that why need we do this at so early stage? ( Even before its peers
> >>>>> is connected.)
> >>> For vhost-user multiple queues support, we will invoke vhost_net_init()
> >>> N times for each queue pair, and hence we need to distinguish which
> >>> queue it is while sending messages like VHOST_SET_VRING_CALL for
> >>> initializing corresponding queue pair.
> >>>
> >>> Does that make sense to you?
> >>>
> >> Not sure. Since current codes works for vhost-kernel. (vhost_net_init()
> >> was also called N times). We don't want to break existed vhost-kernel
> >> API when developing multiqueue. For each virtqueue TX/RX pair, we have
> >> one vhost net device and it has no knowledge for the others (which was
> >> hide by qemu). So VHOST_SET_VRING_CALL works without any change here.
> >>
> >> For the case here, since you still have multiple instances of vhost_net
> >> structure. Maybe the vhost-user backend can distinguish form this?
> > Yeah, I guess that's the difference between vhost-user and vhost-kernel.
> > Vhost-kernel opens a char device(/dev/vhost-net) for each vhost_dev,
> > hence it's distinguishable. But for vhost-user, all vhost_dev share one
> > char device(a socket) for communication, hence, it's not distinguishable.
>
> How about using individual socket in this case? This seems can also
> minimize the changes of backend.
I doubt that will work, for the socket is given at cmd line by "-chardev"
option. You can image that it's not flexible when you want to have 10
queues, or more.
>
> >
> > I was thinking maybe we could export vhost_net_set_vq_index() and invoke
> > it at net/vhost-user.c, so that we break nothing, and in the meantime,
> > it keeps the logic inside vhost-user.
> >
> > What do you think?
> >
> > --yliu
> >
>
> Sounds work.
Good.
> Then I believe you will need to set queue_index in
> vhost_user initialization code?
Yes. I then will go this way if no objection.
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init
2015-09-10 6:54 ` Jason Wang
2015-09-10 7:06 ` Yuanhan Liu
@ 2015-09-14 5:49 ` Yuanhan Liu
1 sibling, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-14 5:49 UTC (permalink / raw)
To: Jason Wang; +Cc: changchun.ouyang, qemu-devel, mst
On Thu, Sep 10, 2015 at 02:54:02PM +0800, Jason Wang wrote:
>
>
> On 09/10/2015 02:18 PM, Yuanhan Liu wrote:
> > On Thu, Sep 10, 2015 at 01:52:30PM +0800, Jason Wang wrote:
> >>
> >> On 09/10/2015 01:17 PM, Yuanhan Liu wrote:
> >>> On Thu, Sep 10, 2015 at 12:46:00PM +0800, Jason Wang wrote:
> >>>>>
> >>>>> On 09/10/2015 11:57 AM, Yuanhan Liu wrote:
> >>>>>>> On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote:
> >>>>>>>>> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> >>>>>>>>>>> So that we could use the `vq_index' as well in the vhost_net_init
> >>>>>>>>>>> stage, which is required when adding vhost-user multiple-queue support,
> >>>>>>>>>>> where we need the vq_index to indicate which queue pair we are gonna
> >>>>>>>>>>> initiate.
> >>>>>>>>>>>
> >>>>>>>>>>> vhost-user has no multiple queue support yet, hence no queue_index set
> >>>>>>>>>>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it
> >>>>>>>>>>> will be set properly soon in the next patch.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>> hw/net/vhost_net.c | 16 +++++++---------
> >>>>>>>>>>> net/vhost-user.c | 1 +
> >>>>>>>>>>> 2 files changed, 8 insertions(+), 9 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >>>>>>>>>>> index f9441e9..141b557 100644
> >>>>>>>>>>> --- a/hw/net/vhost_net.c
> >>>>>>>>>>> +++ b/hw/net/vhost_net.c
> >>>>>>>>>>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend)
> >>>>>>>>>>> }
> >>>>>>>>>>> }
> >>>>>>>>>>>
> >>>>>>>>>>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
> >>>>>>>>>>> +{
> >>>>>>>>>>> + net->dev.vq_index = vq_index;
> >>>>>>>>>>> +}
> >>>>>>>>>>> +
> >>>>>>>>>>> struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>>>>>>>>>> {
> >>>>>>>>>>> int r;
> >>>>>>>>>>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> >>>>>>>>>>> }
> >>>>>>>>>>> net->nc = options->net_backend;
> >>>>>>>>>>>
> >>>>>>>>>>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2);
> >>>>>>>>>>> +
> >>>>>>>>> This breaks vhost kernel multiqueue since queue_index was not
> >>>>>>>>> initialized at this time.
> >>>>>>> Right, thanks for pointing it out.
> >>>>>>>
> >>>>>>>>> We do this in set_netdev() instead of setting
> >>>>>>>>> it in each kind of netdev.
> >>>>>>> Can we move it to net_init_tap() for setting the right queue_index
> >>>>>>> for each nc?
> >>>>>>>
> >>>>>>> Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for
> >>>>>>> vhost-user mq support), another one at vhost_net_start(for vhost kernel
> >>>>>>> mq support)?
> >>>>>>>
> >>>>>>> Or, do you have better ideas?
> >>>>> I think setting queue_index in net_init_tap() looks ok.
> >>> Good to know.
> >>>
> >>>>> But a question
> >>>>> is that why need we do this at so early stage? ( Even before its peers
> >>>>> is connected.)
> >>> For vhost-user multiple queues support, we will invoke vhost_net_init()
> >>> N times for each queue pair, and hence we need to distinguish which
> >>> queue it is while sending messages like VHOST_SET_VRING_CALL for
> >>> initializing corresponding queue pair.
> >>>
> >>> Does that make sense to you?
> >>>
> >> Not sure. Since current codes works for vhost-kernel. (vhost_net_init()
> >> was also called N times). We don't want to break existed vhost-kernel
> >> API when developing multiqueue. For each virtqueue TX/RX pair, we have
> >> one vhost net device and it has no knowledge for the others (which was
> >> hide by qemu). So VHOST_SET_VRING_CALL works without any change here.
> >>
> >> For the case here, since you still have multiple instances of vhost_net
> >> structure. Maybe the vhost-user backend can distinguish form this?
> > Yeah, I guess that's the difference between vhost-user and vhost-kernel.
> > Vhost-kernel opens a char device(/dev/vhost-net) for each vhost_dev,
> > hence it's distinguishable. But for vhost-user, all vhost_dev share one
> > char device(a socket) for communication, hence, it's not distinguishable.
>
> How about using individual socket in this case? This seems can also
> minimize the changes of backend.
>
> >
> > I was thinking maybe we could export vhost_net_set_vq_index() and invoke
> > it at net/vhost-user.c, so that we break nothing, and in the meantime,
> > it keeps the logic inside vhost-user.
> >
> > What do you think?
> >
> > --yliu
> >
>
> Sounds work. Then I believe you will need to set queue_index in
> vhost_user initialization code?
Nah, it will not work, as vhost_net_set_vq_index() needs a vhost_net as
it's parameter: you can't do that before vhost_net_init(), but it's
useless to do that after vhost_net_init(). My bad for not being aware it
in the first time.
How about following then?
Thanks.
--yliu
---
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e2985bc..7197c3f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -148,6 +148,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
fprintf(stderr, "vhost-net requires net backend to be setup\n");
goto fail;
}
+ net->nc = options->net_backend;
net->dev.max_queues = 1;
@@ -164,8 +165,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
net->dev.backend_features = 0;
net->dev.protocol_features = 0;
net->backend = -1;
+
+ /* vhost-user needs vq_index to initiate a specific queue pair */
+ net->dev.vq_index = net->nc->queue_index * 2;
}
- net->nc = options->net_backend;
net->dev.nvqs = 2;
net->dev.vqs = net->vqs;
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue.
2015-09-09 10:45 ` Michael S. Tsirkin
2015-09-09 13:38 ` Yuanhan Liu
@ 2015-09-14 7:36 ` Yuanhan Liu
1 sibling, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-14 7:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, changchun.ouyang
On Wed, Sep 09, 2015 at 01:45:50PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 08, 2015 at 03:38:47PM +0800, Yuanhan Liu wrote:
[snip ....]
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 11e46b5..ca6f7fa 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -25,9 +25,10 @@
[snip ....]
> > static int vhost_user_cleanup(struct vhost_dev *dev)
> > {
> > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>
> In fact, it's a per VQ pair call the way it's coded.
> So pls fix it, call it for all VQs.
I'm trying to address this comment, and I'm a bit confused now: this
call is based on one 'vhost_net' structure, which is related to one
'vhost_dev' structure. One 'vhost_dev', on the other hand, represents
two VQs.
So, you are suggesting to call vhost_user_cleanup() twice at it's upper
level?
--yliu
---
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2712c6f..6944543 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -988,7 +988,10 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
}
g_free(hdev->mem);
g_free(hdev->mem_sections);
- hdev->vhost_ops->vhost_backend_cleanup(hdev);
+
+ for (i = 0; i < hdev->nvqs; i++) {
+ hdev->vhost_ops->vhost_backend_cleanup(hdev, i);
+ }
}
/* Stop processing guest IO notifications in qemu.
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
` (3 preceding siblings ...)
2015-09-09 20:55 ` Michael S. Tsirkin
@ 2015-09-14 10:00 ` Jason Wang
2015-09-15 2:15 ` Yuanhan Liu
4 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2015-09-14 10:00 UTC (permalink / raw)
To: Yuanhan Liu, qemu-devel; +Cc: mst, changchun.ouyang
On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> From: Ouyang Changchun <changchun.ouyang@intel.com>
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> Here is the latest version for adding vhost-user multiple queue support,
> by creating a nc and vhost_net pair for each queue.
>
> What differs from last version is that this patch addresses two major
> concerns from Michael, and fixes one hidden bug.
>
> - Concern #1: no feedback when the backend can't support # of
> requested queues(by providing queues=# option).
>
> Here we address this issue by querying VHOST_USER_PROTOCOL_F_MQ
> protocol features first, if not set, it means the backend don't
> support mq feature, and let max_queues be 1. Otherwise, we send
> another message, VHOST_USER_GET_QUEUE_NUM, for getting the max_queues
> the backend supports.
>
> At vhost-user initiation stage(net_vhost_user_start), we then initiate
> one queue first, which, in the meantime, also gets the max_queues.
> We then do a simple compare: if requested_queues > max_queues, we
> exit(I guess it's safe to exit here, as the vm is not running yet).
>
> - Concern #2: some messages are sent more times than necessary.
>
> We came an agreement with Michael that we could categorize vhost
> user messages to 2 types: none-vring specific messages, which should
> be sent only once, and vring specific messages, which should be sent
> per queue.
>
> Here I introduced a helper function vhost_user_one_time_request(),
> which lists following messages as none-vring specific messages:
>
> VHOST_USER_GET_FEATURES
> VHOST_USER_SET_FEATURES
> VHOST_USER_GET_PROTOCOL_FEATURES
> VHOST_USER_SET_PROTOCOL_FEATURES
> VHOST_USER_SET_OWNER
> VHOST_USER_RESET_DEVICE
> VHOST_USER_SET_MEM_TABLE
> VHOST_USER_GET_QUEUE_NUM
>
> For above messages, we simply ignore them when they are not sent the first
> time.
>
> I also observed a hidden bug from last version. We register the char dev
> event handler N times, which is not necessary, as well as buggy: A later
> register overwrites the former one, as qemu_chr_add_handlers() will not
> chain those handlers, but instead overwrites the old one. So, in theory,
> invoking qemu_chr_add_handlers N times will not end up with calling the
> handler N times.
>
> However, the reason the handler is invoked N times is because we start
> the backend(the connection server) first, and hence when net_vhost_user_init()
> is executed, the connection is already established, and qemu_chr_add_handlers()
> then invoke the handler immediately, which just looks like we invoke the
> handler(net_vhost_user_event) directly from net_vhost_user_init().
>
> The solution I came up with is to make VhostUserState as an upper level
> structure, making it includes N nc and vhost_net pairs:
>
> struct VhostUserNetPeer {
> NetClientState *nc;
> VHostNetState *vhost_net;
> };
>
> typedef struct VhostUserState {
> CharDriverState *chr;
> bool running;
> int queues;
> struct VhostUserNetPeer peers[];
> } VhostUserState;
Haven't looked at previous versions but alternatively, can you do this by:
- keep current VhostUserState
- allocate multiple VhostUserState
- register the char handler only on VhostUserState[0]
- enumerate all others in handler by qemu_find_net_clients_except() ?
>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> docs/specs/vhost-user.txt | 13 +++++
> hw/virtio/vhost-user.c | 31 ++++++++++-
> include/net/net.h | 1 +
> net/vhost-user.c | 136 ++++++++++++++++++++++++++++++++--------------
> qapi-schema.json | 6 +-
> qemu-options.hx | 5 +-
> 6 files changed, 146 insertions(+), 46 deletions(-)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 43db9b4..99d79be 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> a feature bit was dedicated for this purpose:
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +Multiple queue support
> +-------------------
> +Multiple queue is treated as a protocal extension, hence the slave has to
> +implement protocol features first. Multiple queues is supported only when
> +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
> +
> +The max # of queues the slave support can be queried with message
> +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the # of requested
> +queues is bigger than that.
> +
> +As all queues share one connection, the master use a unique index for each
> +queue in the sent message to identify one specified queue.
> +
> Message types
> -------------
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8046bc0..11e46b5 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -187,6 +187,23 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
> 0 : -1;
> }
>
> +static bool vhost_user_one_time_request(VhostUserRequest request)
> +{
> + switch (request) {
> + case VHOST_USER_GET_FEATURES:
> + case VHOST_USER_SET_FEATURES:
> + case VHOST_USER_GET_PROTOCOL_FEATURES:
> + case VHOST_USER_SET_PROTOCOL_FEATURES:
> + case VHOST_USER_SET_OWNER:
> + case VHOST_USER_RESET_DEVICE:
> + case VHOST_USER_SET_MEM_TABLE:
> + case VHOST_USER_GET_QUEUE_NUM:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> void *arg)
> {
> @@ -206,6 +223,14 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> else
> msg_request = request;
>
> + /*
> + * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
> + * we just need send it once in the first time. For later such
> + * request, we just ignore it.
> + */
> + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0)
> + return 0;
> +
> msg.request = msg_request;
> msg.flags = VHOST_USER_VERSION;
> msg.size = 0;
> @@ -268,17 +293,20 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> case VHOST_USER_SET_VRING_NUM:
> case VHOST_USER_SET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.state);
> break;
>
> case VHOST_USER_GET_VRING_BASE:
> memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> + msg.addr.index += dev->vq_index;
Let's don't do this silently here. Since we've done a minus in
vhost_virtqueue_start(), I think we could introduce a new callback and
do correct calculation there.
> msg.size = sizeof(m.state);
> need_reply = 1;
> break;
>
> case VHOST_USER_SET_VRING_ADDR:
> memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> + msg.addr.index += dev->vq_index;
> msg.size = sizeof(m.addr);
> break;
>
> @@ -286,7 +314,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> case VHOST_USER_SET_VRING_CALL:
> case VHOST_USER_SET_VRING_ERR:
> file = arg;
> - msg.u64 = file->index & VHOST_USER_VRING_IDX_MASK;
> + msg.u64 = (file->index + dev->vq_index) & VHOST_USER_VRING_IDX_MASK;
> msg.size = sizeof(m.u64);
> if (ioeventfd_enabled() && file->fd > 0) {
> fds[fd_num++] = file->fd;
> @@ -330,6 +358,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> error_report("Received bad msg size.");
> return -1;
> }
> + msg.state.index -= dev->vq_index;
> memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> break;
> default:
> diff --git a/include/net/net.h b/include/net/net.h
> index 6a6cbef..6f20656 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -92,6 +92,7 @@ struct NetClientState {
> NetClientDestructor *destructor;
> unsigned int queue_index;
> unsigned rxfilter_notify_enabled:1;
> + void *opaque;
> };
>
> typedef struct NICState {
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 2d6bbe5..7d4ac69 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -15,10 +15,16 @@
> #include "qemu/config-file.h"
> #include "qemu/error-report.h"
>
> +struct VhostUserNetPeer {
> + NetClientState *nc;
> + VHostNetState *vhost_net;
> +};
> +
> typedef struct VhostUserState {
> - NetClientState nc;
> CharDriverState *chr;
> - VHostNetState *vhost_net;
> + bool running;
> + int queues;
> + struct VhostUserNetPeer peers[];
> } VhostUserState;
>
> typedef struct VhostUserChardevProps {
> @@ -29,48 +35,75 @@ typedef struct VhostUserChardevProps {
>
> VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> {
> - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> + VhostUserState *s = nc->opaque;
> assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> - return s->vhost_net;
> -}
> -
> -static int vhost_user_running(VhostUserState *s)
> -{
> - return (s->vhost_net) ? 1 : 0;
> + return s->peers[nc->queue_index].vhost_net;
> }
>
> static int vhost_user_start(VhostUserState *s)
> {
> VhostNetOptions options;
> + VHostNetState *vhost_net;
> + int max_queues;
> + int i = 0;
>
> - if (vhost_user_running(s)) {
> + if (s->running)
> return 0;
> - }
>
> options.backend_type = VHOST_BACKEND_TYPE_USER;
> - options.net_backend = &s->nc;
> options.opaque = s->chr;
>
> - s->vhost_net = vhost_net_init(&options);
> + options.net_backend = s->peers[i].nc;
> + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> +
> + max_queues = vhost_net_get_max_queues(vhost_net);
> + if (s->queues >= max_queues) {
> + error_report("you are asking more queues than supported: %d\n",
> + max_queues);
> + return -1;
Memory leak here?
> + }
> +
> + for (; i < s->queues; i++) {
> + options.net_backend = s->peers[i].nc;
> +
> + s->peers[i].vhost_net = vhost_net_init(&options);
> + if (!s->peers[i].vhost_net)
> + return -1;
> + }
Maybe you can initialize all vhost_net in one loop, and check for the
max_queues afterwards to simplify the code.
> + s->running = true;
>
> - return vhost_user_running(s) ? 0 : -1;
> + return 0;
> }
>
> static void vhost_user_stop(VhostUserState *s)
> {
> - if (vhost_user_running(s)) {
> - vhost_net_cleanup(s->vhost_net);
> + int i;
> + VHostNetState *vhost_net;
> +
> + if (!s->running)
> + return;
> +
> + for (i = 0; i < s->queues; i++) {
> + vhost_net = s->peers[i].vhost_net;
> + if (vhost_net)
> + vhost_net_cleanup(vhost_net);
> }
>
> - s->vhost_net = 0;
> + s->running = false;
> }
>
> static void vhost_user_cleanup(NetClientState *nc)
> {
> - VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> + VhostUserState *s = nc->opaque;
> + VHostNetState *vhost_net = s->peers[nc->queue_index].vhost_net;
> +
> + if (vhost_net)
> + vhost_net_cleanup(vhost_net);
>
> - vhost_user_stop(s);
> qemu_purge_queued_packets(nc);
> +
> + if (nc->queue_index == s->queues - 1)
> + free(s);
> }
>
> static bool vhost_user_has_vnet_hdr(NetClientState *nc)
> @@ -89,7 +122,7 @@ static bool vhost_user_has_ufo(NetClientState *nc)
>
> static NetClientInfo net_vhost_user_info = {
> .type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
> - .size = sizeof(VhostUserState),
> + .size = sizeof(NetClientState),
> .cleanup = vhost_user_cleanup,
> .has_vnet_hdr = vhost_user_has_vnet_hdr,
> .has_ufo = vhost_user_has_ufo,
> @@ -97,18 +130,25 @@ static NetClientInfo net_vhost_user_info = {
>
> static void net_vhost_link_down(VhostUserState *s, bool link_down)
> {
> - s->nc.link_down = link_down;
> + NetClientState *nc;
> + int i;
>
> - if (s->nc.peer) {
> - s->nc.peer->link_down = link_down;
> - }
> + for (i = 0; i < s->queues; i++) {
> + nc = s->peers[i].nc;
>
> - if (s->nc.info->link_status_changed) {
> - s->nc.info->link_status_changed(&s->nc);
> - }
> + nc->link_down = link_down;
> +
> + if (nc->peer) {
> + nc->peer->link_down = link_down;
> + }
>
> - if (s->nc.peer && s->nc.peer->info->link_status_changed) {
> - s->nc.peer->info->link_status_changed(s->nc.peer);
> + if (nc->info->link_status_changed) {
> + nc->info->link_status_changed(nc);
> + }
> +
> + if (nc->peer && nc->peer->info->link_status_changed) {
> + nc->peer->info->link_status_changed(nc->peer);
> + }
> }
> }
>
> @@ -118,7 +158,8 @@ static void net_vhost_user_event(void *opaque, int event)
>
> switch (event) {
> case CHR_EVENT_OPENED:
> - vhost_user_start(s);
> + if (vhost_user_start(s) < 0)
> + exit(1);
> net_vhost_link_down(s, false);
> error_report("chardev \"%s\" went up", s->chr->label);
> break;
> @@ -131,24 +172,28 @@ static void net_vhost_user_event(void *opaque, int event)
> }
>
> static int net_vhost_user_init(NetClientState *peer, const char *device,
> - const char *name, CharDriverState *chr)
> + const char *name, VhostUserState *s)
> {
> NetClientState *nc;
> - VhostUserState *s;
> + CharDriverState *chr = s->chr;
> + int i;
>
> - nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> + for (i = 0; i < s->queues; i++) {
> + nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
>
> - snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> - chr->label);
> + snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> + i, chr->label);
>
> - s = DO_UPCAST(VhostUserState, nc, nc);
> + /* We don't provide a receive callback */
> + nc->receive_disabled = 1;
>
> - /* We don't provide a receive callback */
> - s->nc.receive_disabled = 1;
> - s->chr = chr;
> - nc->queue_index = 0;
> + nc->queue_index = i;
> + nc->opaque = s;
>
> - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> + s->peers[i].nc = nc;
So actually "peers" is confusing. Better rename it to "ncs" or something
else.
> + }
> +
> + qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, s);
>
> return 0;
> }
> @@ -227,8 +272,10 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
> int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> NetClientState *peer, Error **errp)
> {
> + int queues;
> const NetdevVhostUserOptions *vhost_user_opts;
> CharDriverState *chr;
> + VhostUserState *s;
>
> assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> vhost_user_opts = opts->vhost_user;
> @@ -244,6 +291,11 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
> return -1;
> }
>
> + queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
> + s = g_malloc0(sizeof(VhostUserState) +
> + queues * sizeof(struct VhostUserNetPeer));
> + s->queues = queues;
> + s->chr = chr;
>
> - return net_vhost_user_init(peer, "vhost_user", name, chr);
> + return net_vhost_user_init(peer, "vhost_user", name, s);
> }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 67fef37..55c33db 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2480,12 +2480,16 @@
> #
> # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> #
> +# @queues: #optional number of queues to be created for multiqueue vhost-user
> +# (default: 1) (Since 2.5)
> +#
> # Since 2.1
> ##
> { 'struct': 'NetdevVhostUserOptions',
> 'data': {
> 'chardev': 'str',
> - '*vhostforce': 'bool' } }
> + '*vhostforce': 'bool',
> + '*queues': 'int' } }
>
> ##
> # @NetClientOptions
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 77f5853..5bfa7a3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1963,13 +1963,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
> netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the
> required hub automatically.
>
> -@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
> +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
>
> Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
> be a unix domain socket backed one. The vhost-user uses a specifically defined
> protocol to pass vhost ioctl replacement messages to an application on the other
> end of the socket. On non-MSIX guests, the feature can be forced with
> -@var{vhostforce}.
> +@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
> +be created for multiqueue vhost-user.
>
> Example:
> @example
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-14 10:00 ` Jason Wang
@ 2015-09-15 2:15 ` Yuanhan Liu
0 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-15 2:15 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel, changchun.ouyang
On Mon, Sep 14, 2015 at 06:00:41PM +0800, Jason Wang wrote:
>
>
> On 09/08/2015 03:38 PM, Yuanhan Liu wrote:
> > From: Ouyang Changchun <changchun.ouyang@intel.com>
> >
> > This patch is initially based a patch from Nikolay Nikolaev.
> >
> > Here is the latest version for adding vhost-user multiple queue support,
> > by creating a nc and vhost_net pair for each queue.
> >
> > What differs from last version is that this patch addresses two major
> > concerns from Michael, and fixes one hidden bug.
> >
> > - Concern #1: no feedback when the backend can't support # of
> > requested queues(by providing queues=# option).
> >
> > Here we address this issue by querying VHOST_USER_PROTOCOL_F_MQ
> > protocol features first, if not set, it means the backend don't
> > support mq feature, and let max_queues be 1. Otherwise, we send
> > another message, VHOST_USER_GET_QUEUE_NUM, for getting the max_queues
> > the backend supports.
> >
> > At vhost-user initiation stage(net_vhost_user_start), we then initiate
> > one queue first, which, in the meantime, also gets the max_queues.
> > We then do a simple compare: if requested_queues > max_queues, we
> > exit(I guess it's safe to exit here, as the vm is not running yet).
> >
> > - Concern #2: some messages are sent more times than necessary.
> >
> > We came an agreement with Michael that we could categorize vhost
> > user messages to 2 types: none-vring specific messages, which should
> > be sent only once, and vring specific messages, which should be sent
> > per queue.
> >
> > Here I introduced a helper function vhost_user_one_time_request(),
> > which lists following messages as none-vring specific messages:
> >
> > VHOST_USER_GET_FEATURES
> > VHOST_USER_SET_FEATURES
> > VHOST_USER_GET_PROTOCOL_FEATURES
> > VHOST_USER_SET_PROTOCOL_FEATURES
> > VHOST_USER_SET_OWNER
> > VHOST_USER_RESET_DEVICE
> > VHOST_USER_SET_MEM_TABLE
> > VHOST_USER_GET_QUEUE_NUM
> >
> > For above messages, we simply ignore them when they are not sent the first
> > time.
> >
> > I also observed a hidden bug from last version. We register the char dev
> > event handler N times, which is not necessary, as well as buggy: A later
> > register overwrites the former one, as qemu_chr_add_handlers() will not
> > chain those handlers, but instead overwrites the old one. So, in theory,
> > invoking qemu_chr_add_handlers N times will not end up with calling the
> > handler N times.
> >
> > However, the reason the handler is invoked N times is because we start
> > the backend(the connection server) first, and hence when net_vhost_user_init()
> > is executed, the connection is already established, and qemu_chr_add_handlers()
> > then invoke the handler immediately, which just looks like we invoke the
> > handler(net_vhost_user_event) directly from net_vhost_user_init().
> >
> > The solution I came up with is to make VhostUserState as an upper level
> > structure, making it includes N nc and vhost_net pairs:
> >
> > struct VhostUserNetPeer {
> > NetClientState *nc;
> > VHostNetState *vhost_net;
> > };
> >
> > typedef struct VhostUserState {
> > CharDriverState *chr;
> > bool running;
> > int queues;
> > struct VhostUserNetPeer peers[];
> > } VhostUserState;
>
> Haven't looked at previous versions but alternatively, can you do this by:
>
> - keep current VhostUserState
> - allocate multiple VhostUserState
> - register the char handler only on VhostUserState[0]
> - enumerate all others in handler by qemu_find_net_clients_except() ?
That does sound work and much better! I will give it a try soon. Thanks!
>
> >
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
...
> > case VHOST_USER_GET_VRING_BASE:
> > memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > + msg.addr.index += dev->vq_index;
>
> Let's don't do this silently here. Since we've done a minus in
> vhost_virtqueue_start(), I think we could introduce a new callback and
> do correct calculation there.
Good point, I will try it as well.
> > - s->vhost_net = vhost_net_init(&options);
> > + options.net_backend = s->peers[i].nc;
> > + vhost_net = s->peers[i++].vhost_net = vhost_net_init(&options);
> > +
> > + max_queues = vhost_net_get_max_queues(vhost_net);
> > + if (s->queues >= max_queues) {
> > + error_report("you are asking more queues than supported: %d\n",
> > + max_queues);
> > + return -1;
>
> Memory leak here?
Yeah, it is. But Does it matter? Since we will call exit() soon, which will
reclaim everything in the end. Or, even though, it's still needed?
Either way, I can do that.
>
> > + }
> > +
> > + for (; i < s->queues; i++) {
> > + options.net_backend = s->peers[i].nc;
> > +
> > + s->peers[i].vhost_net = vhost_net_init(&options);
> > + if (!s->peers[i].vhost_net)
> > + return -1;
> > + }
>
> Maybe you can initialize all vhost_net in one loop, and check for the
> max_queues afterwards to simplify the code.
Okay.
> >
...
> > static int net_vhost_user_init(NetClientState *peer, const char *device,
> > - const char *name, CharDriverState *chr)
> > + const char *name, VhostUserState *s)
> > {
> > NetClientState *nc;
> > - VhostUserState *s;
> > + CharDriverState *chr = s->chr;
> > + int i;
> >
> > - nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> > + for (i = 0; i < s->queues; i++) {
> > + nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> >
> > - snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> > - chr->label);
> > + snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
> > + i, chr->label);
> >
> > - s = DO_UPCAST(VhostUserState, nc, nc);
> > + /* We don't provide a receive callback */
> > + nc->receive_disabled = 1;
> >
> > - /* We don't provide a receive callback */
> > - s->nc.receive_disabled = 1;
> > - s->chr = chr;
> > - nc->queue_index = 0;
> > + nc->queue_index = i;
> > + nc->opaque = s;
> >
> > - qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
> > + s->peers[i].nc = nc;
>
> So actually "peers" is confusing. Better rename it to "ncs" or something
> else.
Yeah, that's why I renamed it to pairs in later version. But anyway, I guess
it's not needed anymore with qemu_find_net_clients_except(), right? :)
Thanks!
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-16 14:15 ` Eric Blake
@ 2015-09-16 14:53 ` Yuanhan Liu
0 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-16 14:53 UTC (permalink / raw)
To: Eric Blake; +Cc: jasowang, mst, qemu-devel, changchun.ouyang
On Wed, Sep 16, 2015 at 08:15:59AM -0600, Eric Blake wrote:
> On 09/15/2015 08:06 PM, Yuanhan Liu wrote:
>
> >> Up to here is mostly fine for the commit message. Meanwhile...
> >>
> >>> v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
> >>> once only, and invoke qemu_find_net_clients_except() at the handler
> >>> to gather all related ncs, for initiating all queues. Which addresses
> >>> a hidden bug in older verion in a more QEMU-like way.
> >>>
> >>> v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
> >>> value from net->nc->queue_index.
> >>
> >> ...this chunk here is useful only on the mailing list, and not in git,
> >> and therefore, should appear...
> >
> > Sorry that I may disagree here.
> >
> > First of all, it does no harm at all to include few old version
> > descriptions. Instead, it may give some hints to the reader how
> > the patch evolves. Meanwhile, you will find they are quite common
> > in some well known open source projects, such as linux kernel:
>
> There's nothing wrong with including patch changelog in your mail; in
> fact it is encouraged by:
> http://wiki.qemu.org/Contribute/SubmitAPatch
>
> However, we DO request that you put it after ---, because we do NOT want
> it in qemu.git log.
>
> I don't care if the kernel has a slightly different policy, and is more
> tolerant of changelogs leaked into the git log.
>
>
> > You can find them at qemu project (though not that common), too:
> >
> > [yliu@yliu-dev ~/qemu]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
> > 390
>
> They're not common for a reason - we've documented that we don't like
> them in git. And that's because we expect the --- separator between the
> git message and the changelog.
>
> >
> > So, if it's a qemu community specific culture, I could do what
> > you suggested.
>
> Yes, since it is documented at
> http://wiki.qemu.org/Contribute/SubmitAPatch, it is qemu culture.
Good to know, then.
Thanks.
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-16 2:06 ` Yuanhan Liu
2015-09-16 2:48 ` Yuanhan Liu
2015-09-16 8:10 ` Michael S. Tsirkin
@ 2015-09-16 14:15 ` Eric Blake
2015-09-16 14:53 ` Yuanhan Liu
2 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-16 14:15 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: jasowang, mst, qemu-devel, changchun.ouyang
[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]
On 09/15/2015 08:06 PM, Yuanhan Liu wrote:
>> Up to here is mostly fine for the commit message. Meanwhile...
>>
>>> v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
>>> once only, and invoke qemu_find_net_clients_except() at the handler
>>> to gather all related ncs, for initiating all queues. Which addresses
>>> a hidden bug in older verion in a more QEMU-like way.
>>>
>>> v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
>>> value from net->nc->queue_index.
>>
>> ...this chunk here is useful only on the mailing list, and not in git,
>> and therefore, should appear...
>
> Sorry that I may disagree here.
>
> First of all, it does no harm at all to include few old version
> descriptions. Instead, it may give some hints to the reader how
> the patch evolves. Meanwhile, you will find they are quite common
> in some well known open source projects, such as linux kernel:
There's nothing wrong with including patch changelog in your mail; in
fact it is encouraged by:
http://wiki.qemu.org/Contribute/SubmitAPatch
However, we DO request that you put it after ---, because we do NOT want
it in qemu.git log.
I don't care if the kernel has a slightly different policy, and is more
tolerant of changelogs leaked into the git log.
> You can find them at qemu project (though not that common), too:
>
> [yliu@yliu-dev ~/qemu]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
> 390
They're not common for a reason - we've documented that we don't like
them in git. And that's because we expect the --- separator between the
git message and the changelog.
>
> So, if it's a qemu community specific culture, I could do what
> you suggested.
Yes, since it is documented at
http://wiki.qemu.org/Contribute/SubmitAPatch, it is qemu culture.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-16 8:10 ` Michael S. Tsirkin
@ 2015-09-16 8:23 ` Yuanhan Liu
0 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-16 8:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, qemu-devel, changchun.ouyang
On Wed, Sep 16, 2015 at 11:10:35AM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 16, 2015 at 10:06:56AM +0800, Yuanhan Liu wrote:
> > If not, I'd like to put them into the commit log, as putting it
> > outside the commit log gives unnecessary extra burden to patch
> > author when he need update several version change information
> > in a patch set: he has to format the patch set first, and add
> > them one by one by editing those patches.
>
> I personally just describe the changes in the cover letter.
I did that as well. I just thought metioning them inside the patch
again will make it clear for you to review.
--yliu
>
> In fact, while not a lot of people do this, git-backport-diff
> (from https://github.com/codyprime/git-scripts.git)
> is a handy tool to show which patches changed since the
> previous version, and it only works if the change log
> is in the cover letter.
>
> Or you can stick --- in the commit log as you have found out -
> but that depends on the patch author being careful to log
> all edits.
> --
> MST
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-16 2:06 ` Yuanhan Liu
2015-09-16 2:48 ` Yuanhan Liu
@ 2015-09-16 8:10 ` Michael S. Tsirkin
2015-09-16 8:23 ` Yuanhan Liu
2015-09-16 14:15 ` Eric Blake
2 siblings, 1 reply; 39+ messages in thread
From: Michael S. Tsirkin @ 2015-09-16 8:10 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: jasowang, qemu-devel, changchun.ouyang
On Wed, Sep 16, 2015 at 10:06:56AM +0800, Yuanhan Liu wrote:
> If not, I'd like to put them into the commit log, as putting it
> outside the commit log gives unnecessary extra burden to patch
> author when he need update several version change information
> in a patch set: he has to format the patch set first, and add
> them one by one by editing those patches.
I personally just describe the changes in the cover letter.
In fact, while not a lot of people do this, git-backport-diff
(from https://github.com/codyprime/git-scripts.git)
is a handy tool to show which patches changed since the
previous version, and it only works if the change log
is in the cover letter.
Or you can stick --- in the commit log as you have found out -
but that depends on the patch author being careful to log
all edits.
--
MST
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-16 2:06 ` Yuanhan Liu
@ 2015-09-16 2:48 ` Yuanhan Liu
2015-09-16 8:10 ` Michael S. Tsirkin
2015-09-16 14:15 ` Eric Blake
2 siblings, 0 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-16 2:48 UTC (permalink / raw)
To: Eric Blake; +Cc: jasowang, mst, qemu-devel, changchun.ouyang
On Wed, Sep 16, 2015 at 10:06:56AM +0800, Yuanhan Liu wrote:
> On Tue, Sep 15, 2015 at 09:02:07AM -0600, Eric Blake wrote:
> > On 09/15/2015 01:10 AM, Yuanhan Liu wrote:
> > > From: Changchun Ouyang <changchun.ouyang@intel.com>
> > >
> > > This patch is initially based a patch from Nikolay Nikolaev.
> > >
> > > Here is the latest version for adding vhost-user multiple queue support,
> > > by creating a nc and vhost_net pair for each queue.
> >
> > The phrase "Here is the latest version" doesn't make much sense in the
> > long term in git (that is, a year from now, we won't care how many
> > preliminary versions there were, just about the version that got
> > committed; and if anything in git changes after that point, it is no
> > longer the latest version).
>
> Yeah, good point.
>
> >
> > >
> > > Qemu exits if find that the backend can't support number of requested
> > > queues(by providing queues=# option). The max number is queried by a
> >
> > space before ( in English.
>
> Michael reminded me behore, and sorry for making such mistake again.
>
> And thanks for other typo corrections.
>
> >
> > > new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol
> > > feature VHOST_USER_PROTOCOL_F_MQ is present first.
> > >
> > > The max queue check is done at vhost-user initiation stage. We initiate
> > > one queue first, which, in the meantime, also gets the max_queues the
> > > backend supports.
> > >
> > > In older version, it was reported that some messages are sent more times
> > > than necessary. Here we came an agreement with Michael that we could
> > > categorize vhost user messages to 2 types: none-vring specific messages,
> >
> > s/none/non/
> >
> > > which should be sent only once, and vring specific messages, which should
> > > be sent per queue.
> > >
> > > Here I introduced a helper function vhost_user_one_time_request(), which
> > > lists following messages as none-vring specific messages:
> >
> > s/none/non/
> >
> > >
> > > VHOST_USER_GET_FEATURES
> > > VHOST_USER_SET_FEATURES
> > > VHOST_USER_GET_PROTOCOL_FEATURES
> > > VHOST_USER_SET_PROTOCOL_FEATURES
> > > VHOST_USER_SET_OWNER
> > > VHOST_USER_RESET_DEVICE
> > > VHOST_USER_SET_MEM_TABLE
> > > VHOST_USER_GET_QUEUE_NUM
> > >
> > > For above messages, we simply ignore them when they are not sent the first
> > > time.
> > >
> >
> > Up to here is mostly fine for the commit message. Meanwhile...
> >
> > > v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
> > > once only, and invoke qemu_find_net_clients_except() at the handler
> > > to gather all related ncs, for initiating all queues. Which addresses
> > > a hidden bug in older verion in a more QEMU-like way.
> > >
> > > v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
> > > value from net->nc->queue_index.
> >
> > ...this chunk here is useful only on the mailing list, and not in git,
> > and therefore, should appear...
>
> Sorry that I may disagree here.
>
> First of all, it does no harm at all to include few old version
> descriptions. Instead, it may give some hints to the reader how
> the patch evolves. Meanwhile, you will find they are quite common
> in some well known open source projects, such as linux kernel:
>
> [yliu@yliu-dev ~/linux]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
> 10828
>
> You can find them at qemu project (though not that common), too:
>
> [yliu@yliu-dev ~/qemu]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
> 390
>
> So, if it's a qemu community specific culture, I could do what
> you suggested.
>
> If not, I'd like to put them into the commit log, as putting it
> outside the commit log gives unnecessary extra burden to patch
> author when he need update several version change information
> in a patch set: he has to format the patch set first, and add
> them one by one by editing those patches.
Oops, I just found we can do that while editing the commit log (IIRC,
I had a similar try long time ago, and it seems it didn't work).
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-15 15:02 ` Eric Blake
@ 2015-09-16 2:06 ` Yuanhan Liu
2015-09-16 2:48 ` Yuanhan Liu
` (2 more replies)
0 siblings, 3 replies; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-16 2:06 UTC (permalink / raw)
To: Eric Blake; +Cc: jasowang, mst, qemu-devel, changchun.ouyang
On Tue, Sep 15, 2015 at 09:02:07AM -0600, Eric Blake wrote:
> On 09/15/2015 01:10 AM, Yuanhan Liu wrote:
> > From: Changchun Ouyang <changchun.ouyang@intel.com>
> >
> > This patch is initially based a patch from Nikolay Nikolaev.
> >
> > Here is the latest version for adding vhost-user multiple queue support,
> > by creating a nc and vhost_net pair for each queue.
>
> The phrase "Here is the latest version" doesn't make much sense in the
> long term in git (that is, a year from now, we won't care how many
> preliminary versions there were, just about the version that got
> committed; and if anything in git changes after that point, it is no
> longer the latest version).
Yeah, good point.
>
> >
> > Qemu exits if find that the backend can't support number of requested
> > queues(by providing queues=# option). The max number is queried by a
>
> space before ( in English.
Michael reminded me behore, and sorry for making such mistake again.
And thanks for other typo corrections.
>
> > new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol
> > feature VHOST_USER_PROTOCOL_F_MQ is present first.
> >
> > The max queue check is done at vhost-user initiation stage. We initiate
> > one queue first, which, in the meantime, also gets the max_queues the
> > backend supports.
> >
> > In older version, it was reported that some messages are sent more times
> > than necessary. Here we came an agreement with Michael that we could
> > categorize vhost user messages to 2 types: none-vring specific messages,
>
> s/none/non/
>
> > which should be sent only once, and vring specific messages, which should
> > be sent per queue.
> >
> > Here I introduced a helper function vhost_user_one_time_request(), which
> > lists following messages as none-vring specific messages:
>
> s/none/non/
>
> >
> > VHOST_USER_GET_FEATURES
> > VHOST_USER_SET_FEATURES
> > VHOST_USER_GET_PROTOCOL_FEATURES
> > VHOST_USER_SET_PROTOCOL_FEATURES
> > VHOST_USER_SET_OWNER
> > VHOST_USER_RESET_DEVICE
> > VHOST_USER_SET_MEM_TABLE
> > VHOST_USER_GET_QUEUE_NUM
> >
> > For above messages, we simply ignore them when they are not sent the first
> > time.
> >
>
> Up to here is mostly fine for the commit message. Meanwhile...
>
> > v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
> > once only, and invoke qemu_find_net_clients_except() at the handler
> > to gather all related ncs, for initiating all queues. Which addresses
> > a hidden bug in older verion in a more QEMU-like way.
> >
> > v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
> > value from net->nc->queue_index.
>
> ...this chunk here is useful only on the mailing list, and not in git,
> and therefore, should appear...
Sorry that I may disagree here.
First of all, it does no harm at all to include few old version
descriptions. Instead, it may give some hints to the reader how
the patch evolves. Meanwhile, you will find they are quite common
in some well known open source projects, such as linux kernel:
[yliu@yliu-dev ~/linux]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
10828
You can find them at qemu project (though not that common), too:
[yliu@yliu-dev ~/qemu]$ git log | grep '\<[vV][1-9][0-9]*:' | wc -l
390
So, if it's a qemu community specific culture, I could do what
you suggested.
If not, I'd like to put them into the commit log, as putting it
outside the commit log gives unnecessary extra burden to patch
author when he need update several version change information
in a patch set: he has to format the patch set first, and add
them one by one by editing those patches.
Thanks.
--yliu
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-15 7:10 ` [Qemu-devel] [PATCH 6/7] vhost-user: add " Yuanhan Liu
@ 2015-09-15 15:02 ` Eric Blake
2015-09-16 2:06 ` Yuanhan Liu
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-15 15:02 UTC (permalink / raw)
To: Yuanhan Liu, qemu-devel; +Cc: jasowang, mst, changchun.ouyang
[-- Attachment #1: Type: text/plain, Size: 5598 bytes --]
On 09/15/2015 01:10 AM, Yuanhan Liu wrote:
> From: Changchun Ouyang <changchun.ouyang@intel.com>
>
> This patch is initially based a patch from Nikolay Nikolaev.
>
> Here is the latest version for adding vhost-user multiple queue support,
> by creating a nc and vhost_net pair for each queue.
The phrase "Here is the latest version" doesn't make much sense in the
long term in git (that is, a year from now, we won't care how many
preliminary versions there were, just about the version that got
committed; and if anything in git changes after that point, it is no
longer the latest version).
>
> Qemu exits if find that the backend can't support number of requested
> queues(by providing queues=# option). The max number is queried by a
space before ( in English.
> new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol
> feature VHOST_USER_PROTOCOL_F_MQ is present first.
>
> The max queue check is done at vhost-user initiation stage. We initiate
> one queue first, which, in the meantime, also gets the max_queues the
> backend supports.
>
> In older version, it was reported that some messages are sent more times
> than necessary. Here we came an agreement with Michael that we could
> categorize vhost user messages to 2 types: none-vring specific messages,
s/none/non/
> which should be sent only once, and vring specific messages, which should
> be sent per queue.
>
> Here I introduced a helper function vhost_user_one_time_request(), which
> lists following messages as none-vring specific messages:
s/none/non/
>
> VHOST_USER_GET_FEATURES
> VHOST_USER_SET_FEATURES
> VHOST_USER_GET_PROTOCOL_FEATURES
> VHOST_USER_SET_PROTOCOL_FEATURES
> VHOST_USER_SET_OWNER
> VHOST_USER_RESET_DEVICE
> VHOST_USER_SET_MEM_TABLE
> VHOST_USER_GET_QUEUE_NUM
>
> For above messages, we simply ignore them when they are not sent the first
> time.
>
Up to here is mostly fine for the commit message. Meanwhile...
> v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
> once only, and invoke qemu_find_net_clients_except() at the handler
> to gather all related ncs, for initiating all queues. Which addresses
> a hidden bug in older verion in a more QEMU-like way.
>
> v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
> value from net->nc->queue_index.
...this chunk here is useful only on the mailing list, and not in git,
and therefore, should appear...
>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
...here, after the --- separator.
> docs/specs/vhost-user.txt | 13 +++++
> hw/net/vhost_net.c | 10 ++--
> hw/virtio/vhost-user.c | 26 +++++++++
> hw/virtio/vhost.c | 5 +-
> net/vhost-user.c | 146 +++++++++++++++++++++++++++++++++-------------
> qapi-schema.json | 6 +-
> qemu-options.hx | 5 +-
> 7 files changed, 161 insertions(+), 50 deletions(-)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 43db9b4..acf5708 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
> a feature bit was dedicated for this purpose:
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +Multiple queue support
> +-------------------
Nit: the ---- line looks better when it is as long as the text it is
highlighting (3 bytes more)
> +Multiple queue is treated as a protocol extension, hence the slave has to
> +implement protocol features first. Multiple queues is supported only when
Sounds awkward. Either "Multiple queues are supported" or "The multiple
queues feature is supported"
> +the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
space before ( in English.
> +
> +The max number of queues the slave supports can be queried with message
> +VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of
> +requested queues is bigger than that.
> +
> +As all queues share one connection, the master uses a unique index for each
> +queue in the sent message to identify a specified queue.
> +
> Message types
> -------------
>
> @@ -207,6 +224,15 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> msg_request = request;
> }
>
> + /*
> + * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
s/none/non/
> + * we just need send it once in the first time. For later such
> + * request, we just ignore it.
> + */
> + if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) {
> + return 0;
> + }
> +++ b/qapi-schema.json
> @@ -2480,12 +2480,16 @@
> #
> # @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
> #
> +# @queues: #optional number of queues to be created for multiqueue vhost-user
> +# (default: 1) (Since 2.5)
> +#
> # Since 2.1
> ##
> { 'struct': 'NetdevVhostUserOptions',
> 'data': {
> 'chardev': 'str',
> - '*vhostforce': 'bool' } }
> + '*vhostforce': 'bool',
> + '*queues': 'int' } }
Interface looks okay.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support
2015-09-15 7:10 [Qemu-devel] [PATCH 0/7 v9] vhost-user multiple queue support Yuanhan Liu
@ 2015-09-15 7:10 ` Yuanhan Liu
2015-09-15 15:02 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Yuanhan Liu @ 2015-09-15 7:10 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, changchun.ouyang, Yuanhan Liu, mst
From: Changchun Ouyang <changchun.ouyang@intel.com>
This patch is initially based a patch from Nikolay Nikolaev.
Here is the latest version for adding vhost-user multiple queue support,
by creating a nc and vhost_net pair for each queue.
Qemu exits if find that the backend can't support number of requested
queues(by providing queues=# option). The max number is queried by a
new message, VHOST_USER_GET_QUEUE_NUM, and is sent only when protocol
feature VHOST_USER_PROTOCOL_F_MQ is present first.
The max queue check is done at vhost-user initiation stage. We initiate
one queue first, which, in the meantime, also gets the max_queues the
backend supports.
In older version, it was reported that some messages are sent more times
than necessary. Here we came an agreement with Michael that we could
categorize vhost user messages to 2 types: none-vring specific messages,
which should be sent only once, and vring specific messages, which should
be sent per queue.
Here I introduced a helper function vhost_user_one_time_request(), which
lists following messages as none-vring specific messages:
VHOST_USER_GET_FEATURES
VHOST_USER_SET_FEATURES
VHOST_USER_GET_PROTOCOL_FEATURES
VHOST_USER_SET_PROTOCOL_FEATURES
VHOST_USER_SET_OWNER
VHOST_USER_RESET_DEVICE
VHOST_USER_SET_MEM_TABLE
VHOST_USER_GET_QUEUE_NUM
For above messages, we simply ignore them when they are not sent the first
time.
v9: per suggested by Jason Wang, we could invoke qemu_chr_add_handlers()
once only, and invoke qemu_find_net_clients_except() at the handler
to gather all related ncs, for initiating all queues. Which addresses
a hidden bug in older verion in a more QEMU-like way.
v8: set net->dev.vq_index correctly inside vhost_net_init() based on the
value from net->nc->queue_index.
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 13 +++++
hw/net/vhost_net.c | 10 ++--
hw/virtio/vhost-user.c | 26 +++++++++
hw/virtio/vhost.c | 5 +-
net/vhost-user.c | 146 +++++++++++++++++++++++++++++++++-------------
qapi-schema.json | 6 +-
qemu-options.hx | 5 +-
7 files changed, 161 insertions(+), 50 deletions(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 43db9b4..acf5708 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -135,6 +135,19 @@ As older slaves don't support negotiating protocol features,
a feature bit was dedicated for this purpose:
#define VHOST_USER_F_PROTOCOL_FEATURES 30
+Multiple queue support
+-------------------
+Multiple queue is treated as a protocol extension, hence the slave has to
+implement protocol features first. Multiple queues is supported only when
+the protocol feature VHOST_USER_PROTOCOL_F_MQ(bit 0) is set.
+
+The max number of queues the slave supports can be queried with message
+VHOST_USER_GET_PROTOCOL_FEATURES. Master should stop when the number of
+requested queues is bigger than that.
+
+As all queues share one connection, the master uses a unique index for each
+queue in the sent message to identify a specified queue.
+
Message types
-------------
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f9441e9..c114304 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -148,8 +148,11 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
fprintf(stderr, "vhost-net requires net backend to be setup\n");
goto fail;
}
+ net->nc = options->net_backend;
net->dev.max_queues = 1;
+ net->dev.nvqs = 2;
+ net->dev.vqs = net->vqs;
if (backend_kernel) {
r = vhost_net_get_fd(options->net_backend);
@@ -164,11 +167,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
net->dev.backend_features = 0;
net->dev.protocol_features = 0;
net->backend = -1;
- }
- net->nc = options->net_backend;
- net->dev.nvqs = 2;
- net->dev.vqs = net->vqs;
+ /* vhost-user needs vq_index to initiate a specific queue pair */
+ net->dev.vq_index = net->nc->queue_index * net->dev.nvqs;
+ }
r = vhost_dev_init(&net->dev, options->opaque,
options->backend_type);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 69b085b..dfb5a61 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,6 +187,23 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
0 : -1;
}
+static bool vhost_user_one_time_request(VhostUserRequest request)
+{
+ switch (request) {
+ case VHOST_USER_GET_FEATURES:
+ case VHOST_USER_SET_FEATURES:
+ case VHOST_USER_GET_PROTOCOL_FEATURES:
+ case VHOST_USER_SET_PROTOCOL_FEATURES:
+ case VHOST_USER_SET_OWNER:
+ case VHOST_USER_RESET_DEVICE:
+ case VHOST_USER_SET_MEM_TABLE:
+ case VHOST_USER_GET_QUEUE_NUM:
+ return true;
+ default:
+ return false;
+ }
+}
+
static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
void *arg)
{
@@ -207,6 +224,15 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
msg_request = request;
}
+ /*
+ * For none-vring specific requests, like VHOST_USER_GET_FEATURES,
+ * we just need send it once in the first time. For later such
+ * request, we just ignore it.
+ */
+ if (vhost_user_one_time_request(msg_request) && dev->vq_index != 0) {
+ return 0;
+ }
+
msg.request = msg_request;
msg.flags = VHOST_USER_VERSION;
msg.size = 0;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 717a0ba..14518d0 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -874,8 +874,9 @@ static void vhost_eventfd_del(MemoryListener *listener,
static int vhost_virtqueue_init(struct vhost_dev *dev,
struct vhost_virtqueue *vq, int n)
{
+ int vhost_vq_index = dev->vhost_ops->vhost_backend_get_vq_index(dev, n);
struct vhost_vring_file file = {
- .index = n,
+ .index = vhost_vq_index,
};
int r = event_notifier_init(&vq->masked_notifier, 0);
if (r < 0) {
@@ -926,7 +927,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
for (i = 0; i < hdev->nvqs; ++i) {
- r = vhost_virtqueue_init(hdev, hdev->vqs + i, i);
+ r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
if (r < 0) {
goto fail_vq;
}
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 93dcecd..4fa3d64 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -39,37 +39,77 @@ static int vhost_user_running(VhostUserState *s)
return (s->vhost_net) ? 1 : 0;
}
-static int vhost_user_start(VhostUserState *s)
+static void vhost_user_stop(int queues, NetClientState *ncs[])
{
- VhostNetOptions options;
-
- if (vhost_user_running(s)) {
- return 0;
- }
+ VhostUserState *s;
+ int i;
- options.backend_type = VHOST_BACKEND_TYPE_USER;
- options.net_backend = &s->nc;
- options.opaque = s->chr;
+ for (i = 0; i < queues; i++) {
+ assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
- s->vhost_net = vhost_net_init(&options);
+ s = DO_UPCAST(VhostUserState, nc, ncs[i]);
+ if (!vhost_user_running(s)) {
+ continue;
+ }
- return vhost_user_running(s) ? 0 : -1;
+ if (s->vhost_net) {
+ vhost_net_cleanup(s->vhost_net);
+ s->vhost_net = NULL;
+ }
+ }
}
-static void vhost_user_stop(VhostUserState *s)
+static int vhost_user_start(int queues, NetClientState *ncs[])
{
- if (vhost_user_running(s)) {
- vhost_net_cleanup(s->vhost_net);
+ VhostNetOptions options;
+ VhostUserState *s;
+ int max_queues;
+ int i;
+
+ options.backend_type = VHOST_BACKEND_TYPE_USER;
+
+ for (i = 0; i < queues; i++) {
+ assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
+
+ s = DO_UPCAST(VhostUserState, nc, ncs[i]);
+ if (vhost_user_running(s)) {
+ continue;
+ }
+
+ options.net_backend = ncs[i];
+ options.opaque = s->chr;
+ s->vhost_net = vhost_net_init(&options);
+ if (!s->vhost_net) {
+ error_report("failed to init vhost_net for queue %d\n", i);
+ goto err;
+ }
+
+ if (i == 0) {
+ max_queues = vhost_net_get_max_queues(s->vhost_net);
+ if (queues > max_queues) {
+ error_report("you are asking more queues than "
+ "supported: %d\n", max_queues);
+ goto err;
+ }
+ }
}
- s->vhost_net = 0;
+ return 0;
+
+err:
+ vhost_user_stop(i + 1, ncs);
+ return -1;
}
static void vhost_user_cleanup(NetClientState *nc)
{
VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
- vhost_user_stop(s);
+ if (s->vhost_net) {
+ vhost_net_cleanup(s->vhost_net);
+ s->vhost_net = NULL;
+ }
+
qemu_purge_queued_packets(nc);
}
@@ -95,59 +135,81 @@ static NetClientInfo net_vhost_user_info = {
.has_ufo = vhost_user_has_ufo,
};
-static void net_vhost_link_down(VhostUserState *s, bool link_down)
+static void net_vhost_link_down(int queues, NetClientState *ncs[],
+ bool link_down)
{
- s->nc.link_down = link_down;
+ NetClientState *nc;
+ int i;
- if (s->nc.peer) {
- s->nc.peer->link_down = link_down;
- }
+ for (i = 0; i < queues; i++) {
+ nc = ncs[i];
- if (s->nc.info->link_status_changed) {
- s->nc.info->link_status_changed(&s->nc);
- }
+ nc->link_down = link_down;
+
+ if (nc->peer) {
+ nc->peer->link_down = link_down;
+ }
+
+ if (nc->info->link_status_changed) {
+ nc->info->link_status_changed(nc);
+ }
- if (s->nc.peer && s->nc.peer->info->link_status_changed) {
- s->nc.peer->info->link_status_changed(s->nc.peer);
+ if (nc->peer && nc->peer->info->link_status_changed) {
+ nc->peer->info->link_status_changed(nc->peer);
+ }
}
}
static void net_vhost_user_event(void *opaque, int event)
{
- VhostUserState *s = opaque;
+ const char *name = opaque;
+ NetClientState *ncs[MAX_QUEUE_NUM];
+ VhostUserState *s;
+ int queues;
+ queues = qemu_find_net_clients_except(name, ncs,
+ NET_CLIENT_OPTIONS_KIND_NIC,
+ MAX_QUEUE_NUM);
+ s = DO_UPCAST(VhostUserState, nc, ncs[0]);
switch (event) {
case CHR_EVENT_OPENED:
- vhost_user_start(s);
- net_vhost_link_down(s, false);
+ if (vhost_user_start(queues, ncs) < 0) {
+ exit(1);
+ }
+ net_vhost_link_down(queues, ncs, false);
error_report("chardev \"%s\" went up", s->chr->label);
break;
case CHR_EVENT_CLOSED:
- net_vhost_link_down(s, true);
- vhost_user_stop(s);
+ net_vhost_link_down(queues, ncs, true);
+ vhost_user_stop(queues, ncs);
error_report("chardev \"%s\" went down", s->chr->label);
break;
}
}
static int net_vhost_user_init(NetClientState *peer, const char *device,
- const char *name, CharDriverState *chr)
+ const char *name, CharDriverState *chr,
+ int queues)
{
NetClientState *nc;
VhostUserState *s;
+ int i;
- nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
+ for (i = 0; i < queues; i++) {
+ nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
- snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
- chr->label);
+ snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
+ i, chr->label);
- s = DO_UPCAST(VhostUserState, nc, nc);
+ /* We don't provide a receive callback */
+ nc->receive_disabled = 1;
+ nc->queue_index = i;
- /* We don't provide a receive callback */
- s->nc.receive_disabled = 1;
- s->chr = chr;
+ s = DO_UPCAST(VhostUserState, nc, nc);
+ s->chr = chr;
+ }
- qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s);
+ qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, (void*)name);
return 0;
}
@@ -226,6 +288,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
int net_init_vhost_user(const NetClientOptions *opts, const char *name,
NetClientState *peer, Error **errp)
{
+ int queues;
const NetdevVhostUserOptions *vhost_user_opts;
CharDriverState *chr;
@@ -243,6 +306,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
return -1;
}
+ queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
- return net_vhost_user_init(peer, "vhost_user", name, chr);
+ return net_vhost_user_init(peer, "vhost_user", name, chr, queues);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 67fef37..55c33db 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2480,12 +2480,16 @@
#
# @vhostforce: #optional vhost on for non-MSIX virtio guests (default: false).
#
+# @queues: #optional number of queues to be created for multiqueue vhost-user
+# (default: 1) (Since 2.5)
+#
# Since 2.1
##
{ 'struct': 'NetdevVhostUserOptions',
'data': {
'chardev': 'str',
- '*vhostforce': 'bool' } }
+ '*vhostforce': 'bool',
+ '*queues': 'int' } }
##
# @NetClientOptions
diff --git a/qemu-options.hx b/qemu-options.hx
index 77f5853..5bfa7a3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1963,13 +1963,14 @@ The hubport netdev lets you connect a NIC to a QEMU "vlan" instead of a single
netdev. @code{-net} and @code{-device} with parameter @option{vlan} create the
required hub automatically.
-@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
+@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off][,queues=n]
Establish a vhost-user netdev, backed by a chardev @var{id}. The chardev should
be a unix domain socket backed one. The vhost-user uses a specifically defined
protocol to pass vhost ioctl replacement messages to an application on the other
end of the socket. On non-MSIX guests, the feature can be forced with
-@var{vhostforce}.
+@var{vhostforce}. Use 'queues=@var{n}' to specify the number of queues to
+be created for multiqueue vhost-user.
Example:
@example
--
1.9.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
end of thread, other threads:[~2015-09-16 14:52 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-08 7:38 [Qemu-devel] [PATCH 0/7 v7] vhost-user multiple queue support Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 1/7] vhost-user: use VHOST_USER_XXX macro for switch statement Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 2/7] vhost-user: add protocol feature negotiation Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 3/7] vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 4/7] vhost-user: add VHOST_USER_GET_QUEUE_NUM message Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init Yuanhan Liu
2015-09-10 3:14 ` Jason Wang
2015-09-10 3:57 ` Yuanhan Liu
2015-09-10 4:46 ` Jason Wang
2015-09-10 5:17 ` Yuanhan Liu
2015-09-10 5:52 ` Jason Wang
2015-09-10 6:18 ` Yuanhan Liu
2015-09-10 6:54 ` Jason Wang
2015-09-10 7:06 ` Yuanhan Liu
2015-09-14 5:49 ` Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 6/7] vhost-user: add multiple queue support Yuanhan Liu
2015-09-08 21:22 ` Eric Blake
2015-09-09 1:47 ` Yuanhan Liu
2015-09-09 8:05 ` Ouyang, Changchun
2015-09-09 8:11 ` Yuanhan Liu
2015-09-09 12:18 ` Michael S. Tsirkin
2015-09-09 13:19 ` Yuanhan Liu
2015-09-09 20:55 ` Michael S. Tsirkin
2015-09-14 10:00 ` Jason Wang
2015-09-15 2:15 ` Yuanhan Liu
2015-09-08 7:38 ` [Qemu-devel] [PATCH 7/7] vhost-user: add a new message to disable/enable a specific virt queue Yuanhan Liu
2015-09-09 10:45 ` Michael S. Tsirkin
2015-09-09 13:38 ` Yuanhan Liu
2015-09-09 14:18 ` Michael S. Tsirkin
2015-09-09 20:55 ` Michael S. Tsirkin
2015-09-14 7:36 ` Yuanhan Liu
2015-09-15 7:10 [Qemu-devel] [PATCH 0/7 v9] vhost-user multiple queue support Yuanhan Liu
2015-09-15 7:10 ` [Qemu-devel] [PATCH 6/7] vhost-user: add " Yuanhan Liu
2015-09-15 15:02 ` Eric Blake
2015-09-16 2:06 ` Yuanhan Liu
2015-09-16 2:48 ` Yuanhan Liu
2015-09-16 8:10 ` Michael S. Tsirkin
2015-09-16 8:23 ` Yuanhan Liu
2015-09-16 14:15 ` Eric Blake
2015-09-16 14:53 ` Yuanhan Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.