From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 5A24C8D9E for ; Wed, 19 Aug 2015 07:54:40 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 18 Aug 2015 22:54:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,707,1432623600"; d="scan'208";a="544541233" Received: from pgsmsx102.gar.corp.intel.com ([10.221.44.80]) by FMSMGA003.fm.intel.com with ESMTP; 18 Aug 2015 22:54:38 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.110.15) by PGSMSX102.gar.corp.intel.com (10.221.44.80) with Microsoft SMTP Server (TLS) id 14.3.224.2; Wed, 19 Aug 2015 13:54:10 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.210]) with mapi id 14.03.0224.002; Wed, 19 Aug 2015 13:54:09 +0800 From: "Ouyang, Changchun" To: Yuanhan Liu Thread-Topic: [dpdk-dev] [PATCH v4 02/12] vhost: support multiple queues in virtio dev Thread-Index: AQHQ1NVUMORckLG1YUiFAf5S5kQh2p4SNToAgACnB9A= Date: Wed, 19 Aug 2015 05:54:09 +0000 Message-ID: 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> In-Reply-To: <20150819035244.GA2438@yliu-dev.sh.intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 05:54:41 -0000 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 i= n > virtio dev >=20 > Hi Changchun, >=20 > 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 =3D qp_idx * VIRTIO_QNUM + VIRTIO_RXQ; > > + uint32_t virt_tx_q_idx =3D 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 =3D (eventfd_t)-1; > > + dev->virtqueue[virt_rx_q_idx]->callfd =3D (eventfd_t)-1; > > + dev->virtqueue[virt_tx_q_idx]->kickfd =3D (eventfd_t)-1; > > + dev->virtqueue[virt_tx_q_idx]->callfd =3D (eventfd_t)-1; > > + > > + /* Backends are set to -1 indicating an inactive device. */ > > + dev->virtqueue[virt_rx_q_idx]->backend =3D VIRTIO_DEV_STOPPED; > > + dev->virtqueue[virt_tx_q_idx]->backend =3D 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. */ >=20 > There is a trick here. Let me fill the context first: >=20 > 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 =3D offsetof(struct virtio_net, mem); > 293 > 294 /* Set everything to 0. */ > 295 memset((void *)(uintptr_t)((uint64_t)(uintptr_t)dev + vq_offs= et), 0, > 296 (sizeof(struct virtio_net) - (size_t)vq_offset)); > 297 > 298 init_vring_queue_pair(dev, 0); >=20 > This piece of code's intention is to memset everything to zero, except th= e > `virtqueue' field, for, as the comment stated, we have already allocated > virtqueue. >=20 > It works only when `virtqueue' field is before `mem' field, and it was > before: >=20 > struct virtio_net { > struct vhost_virtqueue *virtqueue[VIRTIO_QNUM]; /**< = Contains > all virtqueue information. */ > struct virtio_memory *mem; /**< QEMU memory and = memory > region information. */ > ... >=20 > After this patch, it becomes: >=20 > struct virtio_net { > struct virtio_memory *mem; /**< QEMU memory and = memory > region information. */ > struct vhost_virtqueue **virtqueue; /**< Contains all vir= tqueue > information. */ > ... >=20 >=20 > Which actually wipes all stuff inside `struct virtio_net`, resulting to s= etting > `virtqueue' to NULL as well. >=20 > While reading the code(without you patch applied), I thought that it's er= ror- > 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 :) >=20 > Huawei, I'm proposing a fix to call rte_zmalloc() for allocating new_ll_d= ev to > get rid of such issue. What do you think? >=20 > --yliu >=20 >=20 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. >=20 > > 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 =3D (eventfd_t)-1; > > - dev->virtqueue[VIRTIO_RXQ]->callfd =3D (eventfd_t)-1; > > - dev->virtqueue[VIRTIO_TXQ]->kickfd =3D (eventfd_t)-1; > > - dev->virtqueue[VIRTIO_TXQ]->callfd =3D (eventfd_t)-1; > > + init_vring_queue_pair(dev, 0); > > + dev->virt_qp_nb =3D 1; > > +} > > + > > +/* > > + * Alloc mem for vring queue pair. > > + */ > > +int > > +alloc_vring_queue_pair(struct virtio_net *dev, uint16_t qp_idx) { > > + struct vhost_virtqueue *virtqueue =3D NULL; > > + uint32_t virt_rx_q_idx =3D qp_idx * VIRTIO_QNUM + VIRTIO_RXQ; > > + uint32_t virt_tx_q_idx =3D qp_idx * VIRTIO_QNUM + VIRTIO_TXQ; > > > > - /* Backends are set to -1 indicating an inactive device. */ > > - dev->virtqueue[VIRTIO_RXQ]->backend =3D VIRTIO_DEV_STOPPED; > > - dev->virtqueue[VIRTIO_TXQ]->backend =3D VIRTIO_DEV_STOPPED; > > + virtqueue =3D rte_malloc(NULL, sizeof(struct vhost_virtqueue) * > VIRTIO_QNUM, 0); > > + if (virtqueue =3D=3D 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] =3D virtqueue; > > + dev->virtqueue[virt_tx_q_idx] =3D 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 =3D 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 =3D rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); > > - if (virtqueue_rx =3D=3D NULL) { > > - rte_free(new_ll_dev); > > + new_ll_dev->dev.virtqueue =3D > > + rte_malloc(NULL, VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX * > sizeof(struct vhost_virtqueue *), 0); > > + if (new_ll_dev->dev.virtqueue =3D=3D 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 =3D rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0); > > - if (virtqueue_tx =3D=3D NULL) { > > - rte_free(virtqueue_rx); > > + if (alloc_vring_queue_pair(&new_ll_dev->dev, 0) =3D=3D -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] =3D virtqueue_rx; > > - new_ll_dev->dev.virtqueue[VIRTIO_TXQ] =3D 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 =3D get_device(ctx); > > if (dev =3D=3D NULL) > > @@ -445,22 +492,26 @@ set_features(struct vhost_device_ctx ctx, > uint64_t *pu) > > dev->features =3D *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 =3D > > - sizeof(struct virtio_net_hdr_mrg_rxbuf); > > - dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =3D > > - 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 =3D > > - sizeof(struct virtio_net_hdr); > > - dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =3D > > - sizeof(struct virtio_net_hdr); > > + for (q_idx =3D 0; q_idx < dev->virt_qp_nb; q_idx++) { > > + uint32_t virt_rx_q_idx =3D q_idx * VIRTIO_QNUM + > VIRTIO_RXQ; > > + uint32_t virt_tx_q_idx =3D 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 =3D > > + sizeof(struct virtio_net_hdr_mrg_rxbuf); > > + dev->virtqueue[virt_tx_q_idx]->vhost_hlen =3D > > + 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 =3D > > + sizeof(struct virtio_net_hdr); > > + dev->virtqueue[virt_tx_q_idx]->vhost_hlen =3D > > + 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 =3D=3D NULL) > > + return 0; > > + > > + return dev->virt_qp_nb; > > +} > > + > > /* > > * Register ops so that we can add/remove device to data core. > > */ > > -- > > 1.8.4.2 > >