On Wed, Aug 19, 2015 at 02:28:51PM +0800, Yuanhan Liu wrote:

On Wed, Aug 19, 2015 at 05:54:09AM +0000, Ouyang, Changchun wrote:

quoted

Hi Yuanhan,

quoted

-----Original Message-----
From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
Sent: Wednesday, August 19, 2015 11:53 AM
To: Ouyang, Changchun
Cc: dev@dpdk.org; Xie, Huawei
Subject: Re: [dpdk-dev] [PATCH v4 02/12] vhost: support multiple queues in
virtio dev

Hi Changchun,

On Wed, Aug 12, 2015 at 04:02:37PM +0800, Ouyang Changchun wrote:

quoted

Each virtio device could have multiple queues, say 2 or 4, at most 8.
Enabling this feature allows virtio device/port on guest has the
ability to use different vCPU to receive/transmit packets from/to each

queue.

quoted

In multiple queues mode, virtio device readiness means all queues of
this virtio device are ready, cleanup/destroy a virtio device also
requires clearing all queues belong to it.

Signed-off-by: Changchun Ouyang <redacted>
---

[snip ..]

quoted

 /*
+ *  Initialise all variables in vring queue pair.
+ */
+static void
+init_vring_queue_pair(struct virtio_net *dev, uint16_t qp_idx) {
+	uint32_t virt_rx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_RXQ;
+	uint32_t virt_tx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_TXQ;
+	memset(dev->virtqueue[virt_rx_q_idx], 0, sizeof(struct

vhost_virtqueue));

quoted

+	memset(dev->virtqueue[virt_tx_q_idx], 0, sizeof(struct
+vhost_virtqueue));
+
+	dev->virtqueue[virt_rx_q_idx]->kickfd = (eventfd_t)-1;
+	dev->virtqueue[virt_rx_q_idx]->callfd = (eventfd_t)-1;
+	dev->virtqueue[virt_tx_q_idx]->kickfd = (eventfd_t)-1;
+	dev->virtqueue[virt_tx_q_idx]->callfd = (eventfd_t)-1;
+
+	/* Backends are set to -1 indicating an inactive device. */
+	dev->virtqueue[virt_rx_q_idx]->backend = VIRTIO_DEV_STOPPED;
+	dev->virtqueue[virt_tx_q_idx]->backend = VIRTIO_DEV_STOPPED; }
+
+/*
  *  Initialise all variables in device structure.
  */
 static void

@@ -258,17 +294,34 @@ init_device(struct virtio_net *dev)
 	/* Set everything to 0. */

There is a trick here. Let me fill the context first:

283 static void
284 init_device(struct virtio_net *dev)
285 {
286         uint64_t vq_offset;
287
288         /*
289          * Virtqueues have already been malloced so
290          * we don't want to set them to NULL.
291          */
292         vq_offset = offsetof(struct virtio_net, mem);
293
294         /* Set everything to 0. */
295         memset((void *)(uintptr_t)((uint64_t)(uintptr_t)dev + vq_offset), 0,
296                 (sizeof(struct virtio_net) - (size_t)vq_offset));
297
298         init_vring_queue_pair(dev, 0);

This piece of code's intention is to memset everything to zero, except the
`virtqueue' field, for, as the comment stated, we have already allocated
virtqueue.

It works only when `virtqueue' field is before `mem' field, and it was
before:

    struct virtio_net {
            struct vhost_virtqueue  *virtqueue[VIRTIO_QNUM];        /**< Contains
all virtqueue information. */
            struct virtio_memory    *mem;           /**< QEMU memory and memory
region information. */
            ...

After this patch, it becomes:

    struct virtio_net {
            struct virtio_memory    *mem;           /**< QEMU memory and memory
region information. */
            struct vhost_virtqueue  **virtqueue;    /**< Contains all virtqueue
information. */
            ...


Which actually wipes all stuff inside `struct virtio_net`, resulting to setting
`virtqueue' to NULL as well.

While reading the code(without you patch applied), I thought that it's error-
prone, as it is very likely that someone else besides the author doesn't know
such undocumented rule. And you just gave me an example :)

Huawei, I'm proposing a fix to call rte_zmalloc() for allocating new_ll_dev to
get rid of such issue. What do you think?

	--yliu

Suggest you finish the latter patch review:
[PATCH v4 04/12] vhost: set memory layout for multiple queues mode,
After you finish reviewing this patch, I think you will change your mind :-)

This patch resolve what you concern.

Sorry that I hadn't gone that far yet. And yes, I see. I found that you
moved the barrier to `features' field, which puts the `virtqueue' field
back to the "do not set to zero" zone.

It's still an undocumented rule, and hence, error prone, IMO. But, you
reminded me that init_device() will be invoked at other place else(reset_owner()).
Hence, my solution won't work, either.

I'm wondering saving the `virtqueue'(and `mem_arr' from patch 04/12)
explicitly before memset() and restoring them after that, to get rid of
the undocumented rule. It may become uglier with more and more fields
need to be saved this way. But judging that we have two fields so far,
I'm kind of okay to that.

I thought it again. Nah.., let's forget that. It's not flexible, for
array fields like "virtqueue[]".

	--yliu

What do you think then? If that doesn't work, we should add some comments
inside the virtio_net struct at least, or even add a build time check.

	--yliu

quoted

quoted

quoted

 	memset((void *)(uintptr_t)((uint64_t)(uintptr_t)dev + vq_offset), 0,
 		(sizeof(struct virtio_net) - (size_t)vq_offset));
-	memset(dev->virtqueue[VIRTIO_RXQ], 0, sizeof(struct

vhost_virtqueue));

quoted

-	memset(dev->virtqueue[VIRTIO_TXQ], 0, sizeof(struct

vhost_virtqueue));

quoted

-	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
-	dev->virtqueue[VIRTIO_RXQ]->callfd = (eventfd_t)-1;
-	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
-	dev->virtqueue[VIRTIO_TXQ]->callfd = (eventfd_t)-1;
+	init_vring_queue_pair(dev, 0);
+	dev->virt_qp_nb = 1;
+}
+
+/*
+ *  Alloc mem for vring queue pair.
+ */
+int
+alloc_vring_queue_pair(struct virtio_net *dev, uint16_t qp_idx) {
+	struct vhost_virtqueue *virtqueue = NULL;
+	uint32_t virt_rx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_RXQ;
+	uint32_t virt_tx_q_idx = qp_idx * VIRTIO_QNUM + VIRTIO_TXQ;

-	/* Backends are set to -1 indicating an inactive device. */
-	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
-	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
+	virtqueue = rte_malloc(NULL, sizeof(struct vhost_virtqueue) *

VIRTIO_QNUM, 0);

quoted

+	if (virtqueue == NULL) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"Failed to allocate memory for virt qp:%d.\n",

qp_idx);

quoted

+		return -1;
+	}
+
+	dev->virtqueue[virt_rx_q_idx] = virtqueue;
+	dev->virtqueue[virt_tx_q_idx] = virtqueue + VIRTIO_TXQ;
+
+	init_vring_queue_pair(dev, qp_idx);
+
+	return 0;
 }

 /*

@@ -280,7 +333,6 @@ static int
 new_device(struct vhost_device_ctx ctx)  {
 	struct virtio_net_config_ll *new_ll_dev;
-	struct vhost_virtqueue *virtqueue_rx, *virtqueue_tx;

 	/* Setup device and virtqueues. */
 	new_ll_dev = rte_malloc(NULL, sizeof(struct virtio_net_config_ll),

0); @@ -291,28 +343,22 @@ new_device(struct vhost_device_ctx ctx)
 		return -1;
 	}

-	virtqueue_rx = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
-	if (virtqueue_rx == NULL) {
-		rte_free(new_ll_dev);
+	new_ll_dev->dev.virtqueue =
+		rte_malloc(NULL, VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX *

sizeof(struct vhost_virtqueue *), 0);

quoted

+	if (new_ll_dev->dev.virtqueue == NULL) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"(%"PRIu64") Failed to allocate memory for rxq.\n",
+			"(%"PRIu64") Failed to allocate memory for

dev.virtqueue.\n",

quoted

 			ctx.fh);
+		rte_free(new_ll_dev);
 		return -1;
 	}

-	virtqueue_tx = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
-	if (virtqueue_tx == NULL) {
-		rte_free(virtqueue_rx);
+	if (alloc_vring_queue_pair(&new_ll_dev->dev, 0) == -1) {
+		rte_free(new_ll_dev->dev.virtqueue);
 		rte_free(new_ll_dev);
-		RTE_LOG(ERR, VHOST_CONFIG,
-			"(%"PRIu64") Failed to allocate memory for txq.\n",
-			ctx.fh);
 		return -1;
 	}

-	new_ll_dev->dev.virtqueue[VIRTIO_RXQ] = virtqueue_rx;
-	new_ll_dev->dev.virtqueue[VIRTIO_TXQ] = virtqueue_tx;
-
 	/* Initialise device and virtqueues. */
 	init_device(&new_ll_dev->dev);

@@ -396,7 +442,7 @@ set_owner(struct vhost_device_ctx ctx)
  * Called from CUSE IOCTL: VHOST_RESET_OWNER
  */
 static int
-reset_owner(struct vhost_device_ctx ctx)
+reset_owner(__rte_unused struct vhost_device_ctx ctx)
 {
 	struct virtio_net_config_ll *ll_dev;

@@ -434,6 +480,7 @@ static int
 set_features(struct vhost_device_ctx ctx, uint64_t *pu)  {
 	struct virtio_net *dev;
+	uint32_t q_idx;

 	dev = get_device(ctx);
 	if (dev == NULL)

@@ -445,22 +492,26 @@ set_features(struct vhost_device_ctx ctx,

uint64_t *pu)

quoted

 	dev->features = *pu;

 	/* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF

is set. */

quoted

-	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
-		LOG_DEBUG(VHOST_CONFIG,
-			"(%"PRIu64") Mergeable RX buffers enabled\n",
-			dev->device_fh);
-		dev->virtqueue[VIRTIO_RXQ]->vhost_hlen =
-			sizeof(struct virtio_net_hdr_mrg_rxbuf);
-		dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =
-			sizeof(struct virtio_net_hdr_mrg_rxbuf);
-	} else {
-		LOG_DEBUG(VHOST_CONFIG,
-			"(%"PRIu64") Mergeable RX buffers disabled\n",
-			dev->device_fh);
-		dev->virtqueue[VIRTIO_RXQ]->vhost_hlen =
-			sizeof(struct virtio_net_hdr);
-		dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =
-			sizeof(struct virtio_net_hdr);
+	for (q_idx = 0; q_idx < dev->virt_qp_nb; q_idx++) {
+		uint32_t virt_rx_q_idx = q_idx * VIRTIO_QNUM +

VIRTIO_RXQ;

quoted

+		uint32_t virt_tx_q_idx = q_idx * VIRTIO_QNUM +

VIRTIO_TXQ;

quoted

+		if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
+			LOG_DEBUG(VHOST_CONFIG,
+				"(%"PRIu64") Mergeable RX buffers

enabled\n",

quoted

+				dev->device_fh);
+			dev->virtqueue[virt_rx_q_idx]->vhost_hlen =
+				sizeof(struct virtio_net_hdr_mrg_rxbuf);
+			dev->virtqueue[virt_tx_q_idx]->vhost_hlen =
+				sizeof(struct virtio_net_hdr_mrg_rxbuf);
+		} else {
+			LOG_DEBUG(VHOST_CONFIG,
+				"(%"PRIu64") Mergeable RX buffers

disabled\n",

quoted

+				dev->device_fh);
+			dev->virtqueue[virt_rx_q_idx]->vhost_hlen =
+				sizeof(struct virtio_net_hdr);
+			dev->virtqueue[virt_tx_q_idx]->vhost_hlen =
+				sizeof(struct virtio_net_hdr);
+		}
 	}
 	return 0;
 }

@@ -826,6 +877,14 @@ int rte_vhost_feature_enable(uint64_t

feature_mask)

quoted

 	return -1;
 }

+uint16_t rte_vhost_qp_num_get(struct virtio_net *dev) {
+	if (dev == NULL)
+		return 0;
+
+	return dev->virt_qp_nb;
+}
+
 /*
  * Register ops so that we can add/remove device to data core.
  */
--
1.8.4.2