[dpdk-dev,v3,3/7] net/virtio_user: move vhost user specific code
Checks
Commit Message
To support vhost kernel as the backend of net_virtio_user in coming
patches, we move vhost_user specific structs and macros into
vhost_user.c, and only keep common definitions in vhost.h.
Besides, remove VHOST_USER_MQ feature check.
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
drivers/net/virtio/virtio_user/vhost.h | 36 ------------------------
drivers/net/virtio/virtio_user/vhost_user.c | 32 +++++++++++++++++++++
drivers/net/virtio/virtio_user/virtio_user_dev.c | 9 ------
3 files changed, 32 insertions(+), 45 deletions(-)
Comments
On Wed, Jan 04, 2017 at 03:59:22AM +0000, Jianfeng Tan wrote:
> To support vhost kernel as the backend of net_virtio_user in coming
> patches, we move vhost_user specific structs and macros into
> vhost_user.c, and only keep common definitions in vhost.h.
>
> Besides, remove VHOST_USER_MQ feature check.
Again, I have to ask, why? You don't only remove the check, also, you
removed this feature setting, which seems to break the MQ support?
--yliu
Hi Yuanhan,
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, January 4, 2017 2:03 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> Subject: Re: [PATCH v3 3/7] net/virtio_user: move vhost user specific code
>
> On Wed, Jan 04, 2017 at 03:59:22AM +0000, Jianfeng Tan wrote:
> > To support vhost kernel as the backend of net_virtio_user in coming
> > patches, we move vhost_user specific structs and macros into
> > vhost_user.c, and only keep common definitions in vhost.h.
> >
> > Besides, remove VHOST_USER_MQ feature check.
>
> Again, I have to ask, why? You don't only remove the check, also, you
> removed this feature setting, which seems to break the MQ support?
I have answered it here:
http://dpdk.org/ml/archives/dev/2016-December/053520.html
To be more clear, VHOST_USER_MQ is a not-well-defined macro: #define VHOST_USER_MQ (1ULL << VHOST_USER_F_PROTOCOL_FEATURES),
which is a feature bit in vhost user protocol.
According to QEMU/ docs/specs/vhost-user.txt, "If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the ring is initialized in an enabled state. "
But our DPDK vhost library does not take care of this feature bit. Just make this as default: the ring is initialized in an disabled state. And our virtio_user with vhost-user does send VHOST_USER_SET_VRING_ENABLE to enable each queue pair.
So I think it's not necessary to add it back.
How do you think?
Thanks,
Jianfeng
>
> --yliu
On Wed, Jan 04, 2017 at 06:46:34AM +0000, Tan, Jianfeng wrote:
> Hi Yuanhan,
>
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Wednesday, January 4, 2017 2:03 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> > Subject: Re: [PATCH v3 3/7] net/virtio_user: move vhost user specific code
> >
> > On Wed, Jan 04, 2017 at 03:59:22AM +0000, Jianfeng Tan wrote:
> > > To support vhost kernel as the backend of net_virtio_user in coming
> > > patches, we move vhost_user specific structs and macros into
> > > vhost_user.c, and only keep common definitions in vhost.h.
> > >
> > > Besides, remove VHOST_USER_MQ feature check.
> >
> > Again, I have to ask, why? You don't only remove the check, also, you
> > removed this feature setting, which seems to break the MQ support?
>
> I have answered it here:
> http://dpdk.org/ml/archives/dev/2016-December/053520.html
I thought we have made some agreements :/
>
> To be more clear, VHOST_USER_MQ is a not-well-defined macro: #define VHOST_USER_MQ (1ULL << VHOST_USER_F_PROTOCOL_FEATURES),
> which is a feature bit in vhost user protocol.
Yes, it's again named wrongly.
> According to QEMU/ docs/specs/vhost-user.txt, "If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the ring is initialized in an enabled state. "
>
> But our DPDK vhost library does not take care of this feature bit.
> Just make this as default: the ring is initialized in an disabled state. And our virtio_user with vhost-user does send VHOST_USER_SET_VRING_ENABLE to enable each queue pair.
VHOST_USER_F_PROTOCOL_FEATURES is a fundamental feature for quite many
vhost-user extended features, including the MQ. If it's not set, the MQ
should not work.
It may still work in your case, becase you made an assumtion that the
vhost backend supports the MQ feature (which is true in nowadays, as
the feature has been there for a quite while). However, that's not an
assumtion you can take while adding the vhost-user MQ support at that
time. And such feature bit (including the PROTOCOL_F_MQ) makes sure
that we will not try to enable MQ with and older vhost backend that
doesn't have the support.
Put simply, this feature is needed, and as the feature name states,
it's needed only for vhost-user.
--yliu
>
> So I think it's not necessary to add it back.
>
> How do you think?
>
> Thanks,
> Jianfeng
>
> >
> > --yliu
@@ -42,8 +42,6 @@
#include "../virtio_logs.h"
#include "../virtqueue.h"
-#define VHOST_MEMORY_MAX_NREGIONS 8
-
struct vhost_vring_state {
unsigned int index;
unsigned int num;
@@ -105,40 +103,6 @@ struct vhost_memory_region {
uint64_t mmap_offset;
};
-struct vhost_memory {
- uint32_t nregions;
- uint32_t padding;
- struct vhost_memory_region regions[VHOST_MEMORY_MAX_NREGIONS];
-};
-
-struct vhost_user_msg {
- enum vhost_user_request request;
-
-#define VHOST_USER_VERSION_MASK 0x3
-#define VHOST_USER_REPLY_MASK (0x1 << 2)
- uint32_t flags;
- uint32_t size; /* the following payload size */
- union {
-#define VHOST_USER_VRING_IDX_MASK 0xff
-#define VHOST_USER_VRING_NOFD_MASK (0x1 << 8)
- uint64_t u64;
- struct vhost_vring_state state;
- struct vhost_vring_addr addr;
- struct vhost_memory memory;
- } payload;
- int fds[VHOST_MEMORY_MAX_NREGIONS];
-} __attribute((packed));
-
-#define VHOST_USER_HDR_SIZE offsetof(struct vhost_user_msg, payload.u64)
-#define VHOST_USER_PAYLOAD_SIZE \
- (sizeof(struct vhost_user_msg) - VHOST_USER_HDR_SIZE)
-
-/* The version of the protocol we support */
-#define VHOST_USER_VERSION 0x1
-
-#define VHOST_USER_F_PROTOCOL_FEATURES 30
-#define VHOST_USER_MQ (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)
-
int vhost_user_sock(int vhostfd, enum vhost_user_request req, void *arg);
int vhost_user_setup(const char *path);
int vhost_user_enable_queue_pair(int vhostfd, uint16_t pair_idx, int enable);
@@ -42,6 +42,38 @@
#include "vhost.h"
+/* The version of the protocol we support */
+#define VHOST_USER_VERSION 0x1
+
+#define VHOST_MEMORY_MAX_NREGIONS 8
+struct vhost_memory {
+ uint32_t nregions;
+ uint32_t padding;
+ struct vhost_memory_region regions[VHOST_MEMORY_MAX_NREGIONS];
+};
+
+struct vhost_user_msg {
+ enum vhost_user_request request;
+
+#define VHOST_USER_VERSION_MASK 0x3
+#define VHOST_USER_REPLY_MASK (0x1 << 2)
+ uint32_t flags;
+ uint32_t size; /* the following payload size */
+ union {
+#define VHOST_USER_VRING_IDX_MASK 0xff
+#define VHOST_USER_VRING_NOFD_MASK (0x1 << 8)
+ uint64_t u64;
+ struct vhost_vring_state state;
+ struct vhost_vring_addr addr;
+ struct vhost_memory memory;
+ } payload;
+ int fds[VHOST_MEMORY_MAX_NREGIONS];
+} __attribute((packed));
+
+#define VHOST_USER_HDR_SIZE offsetof(struct vhost_user_msg, payload.u64)
+#define VHOST_USER_PAYLOAD_SIZE \
+ (sizeof(struct vhost_user_msg) - VHOST_USER_HDR_SIZE)
+
static int
vhost_user_write(int fd, void *buf, int len, int *fds, int fd_num)
{
@@ -151,8 +151,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
* VIRTIO_NET_F_MAC and VIRTIO_NET_F_CTRL_VQ is stripped.
*/
features = dev->features;
- if (dev->max_queue_pairs > 1)
- features |= VHOST_USER_MQ;
features &= ~(1ull << VIRTIO_NET_F_MAC);
features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
ret = vhost_user_sock(dev->vhostfd, VHOST_USER_SET_FEATURES, &features);
@@ -268,13 +266,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_MAC_ADDR);
}
- if (dev->max_queue_pairs > 1) {
- if (!(dev->features & VHOST_USER_MQ)) {
- PMD_INIT_LOG(ERR, "MQ not supported by the backend");
- return -1;
- }
- }
-
return 0;
}