[dpdk-dev] [PATCH v4 02/12] vhost: support multiple queues in virtio dev
Ouyang, Changchun
changchun.ouyang at intel.com
Wed Aug 19 07:54:09 CEST 2015
Hi Yuanhan,
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com]
> Sent: Wednesday, August 19, 2015 11:53 AM
> To: Ouyang, Changchun
> Cc: dev at 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:
> > 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.
> >
> > 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 <changchun.ouyang at intel.com>
> > ---
> [snip ..]
> > /*
> > + * 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));
> > + 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.
>
> > 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));
> > - memset(dev->virtqueue[VIRTIO_TXQ], 0, sizeof(struct
> vhost_virtqueue));
> >
> > - 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);
> > + if (virtqueue == NULL) {
> > + RTE_LOG(ERR, VHOST_CONFIG,
> > + "Failed to allocate memory for virt qp:%d.\n",
> qp_idx);
> > + 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);
> > + 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",
> > 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)
> > dev->features = *pu;
> >
> > /* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF
> is set. */
> > - 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;
> > + uint32_t virt_tx_q_idx = q_idx * VIRTIO_QNUM +
> VIRTIO_TXQ;
> > + if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> > + LOG_DEBUG(VHOST_CONFIG,
> > + "(%"PRIu64") Mergeable RX buffers
> enabled\n",
> > + 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",
> > + 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)
> > 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
> >
More information about the dev
mailing list