From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id D59D68D9E for ; Wed, 19 Aug 2015 08:39:51 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP; 18 Aug 2015 23:39:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,708,1432623600"; d="scan'208";a="771546993" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.64]) by fmsmga001.fm.intel.com with ESMTP; 18 Aug 2015 23:39:50 -0700 Date: Wed, 19 Aug 2015 14:39:47 +0800 From: Yuanhan Liu To: "Ouyang, Changchun" Message-ID: <20150819063947.GC2438@yliu-dev.sh.intel.com> References: <1434355006-30583-1-git-send-email-changchun.ouyang@intel.com> <1439366567-3402-1-git-send-email-changchun.ouyang@intel.com> <1439366567-3402-3-git-send-email-changchun.ouyang@intel.com> <20150819035244.GA2438@yliu-dev.sh.intel.com> <20150819062851.GB2438@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150819062851.GB2438@yliu-dev.sh.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v4 02/12] vhost: support multiple queues in virtio dev X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Aug 2015 06:39:52 -0000 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: > > Hi Yuanhan, > > > > > -----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: > > > > 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 > > > > --- > > > [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. > > 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 > > > > > > > > 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 > > > >