From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id E435F5677 for ; Mon, 8 Feb 2016 10:42:54 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 08 Feb 2016 01:42:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,415,1449561600"; d="scan'208";a="910704717" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by fmsmga002.fm.intel.com with ESMTP; 08 Feb 2016 01:42:52 -0800 Received: from sivlogin002.ir.intel.com (sivlogin002.ir.intel.com [10.237.217.37]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id u189gqHa012154; Mon, 8 Feb 2016 09:42:52 GMT Received: from sivlogin002.ir.intel.com (localhost [127.0.0.1]) by sivlogin002.ir.intel.com with ESMTP id u189gnMV026995; Mon, 8 Feb 2016 09:42:49 GMT Received: (from fyigit@localhost) by sivlogin002.ir.intel.com with œ id u189gm5Q026991; Mon, 8 Feb 2016 09:42:48 GMT X-Authentication-Warning: sivlogin002.ir.intel.com: fyigit set sender to ferruh.yigit@intel.com using -f Date: Mon, 8 Feb 2016 09:42:48 +0000 From: Ferruh Yigit To: Tetsuya Mukawa Message-ID: <20160208094248.GA25651@sivlogin002.ir.intel.com> Mail-Followup-To: Tetsuya Mukawa , dev@dpdk.org, yuanhan.liu@intel.com, ann.zhuangyanying@huawei.com References: <1448355603-21275-2-git-send-email-mukawa@igel.co.jp> <1454570791-19131-3-git-send-email-mukawa@igel.co.jp> <20160204111735.GA30426@sivlogin002.ir.intel.com> <56B44115.6090808@igel.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56B44115.6090808@igel.co.jp> User-Agent: Mutt/1.5.17 (2007-11-01) Cc: dev@dpdk.org, ann.zhuangyanying@huawei.com, yuanhan.liu@intel.com Subject: Re: [dpdk-dev] [PATCH v7 2/2] vhost: Add VHOST PMD 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: Mon, 08 Feb 2016 09:42:55 -0000 On Fri, Feb 05, 2016 at 03:28:37PM +0900, Tetsuya Mukawa wrote: > On 2016/02/04 20:17, Ferruh Yigit wrote: > > On Thu, Feb 04, 2016 at 04:26:31PM +0900, Tetsuya Mukawa wrote: > > > > Hi Tetsuya, > > > >> The patch introduces a new PMD. This PMD is implemented as thin wrapper > >> of librte_vhost. It means librte_vhost is also needed to compile the PMD. > >> The vhost messages will be handled only when a port is started. So start > >> a port first, then invoke QEMU. > >> > >> The PMD has 2 parameters. > >> - iface: The parameter is used to specify a path to connect to a > >> virtio-net device. > >> - queues: The parameter is used to specify the number of the queues > >> virtio-net device has. > >> (Default: 1) > >> > >> Here is an example. > >> $ ./testpmd -c f -n 4 --vdev 'eth_vhost0,iface=/tmp/sock0,queues=1' -- -i > >> > >> To connect above testpmd, here is qemu command example. > >> > >> $ qemu-system-x86_64 \ > >> > >> -chardev socket,id=chr0,path=/tmp/sock0 \ > >> -netdev vhost-user,id=net0,chardev=chr0,vhostforce,queues=1 \ > >> -device virtio-net-pci,netdev=net0,mq=on > >> > >> Signed-off-by: Tetsuya Mukawa > > Please find some more comments, mostly minor nits, > > > > please feel free to add my ack for next version of this patch: > > Acked-by: Ferruh Yigit > > > > <...> > >> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c > >> new file mode 100644 > >> index 0000000..b2305c2 > >> --- /dev/null > >> +++ b/drivers/net/vhost/rte_eth_vhost.c > > <...> > >> + > >> +struct pmd_internal { > >> + TAILQ_ENTRY(pmd_internal) next; > >> + char *dev_name; > >> + char *iface_name; > >> + uint8_t port_id; > > You can also get rid of port_id too, if you keep list of rte_eth_dev. > > But this is not so important, keep as it is if you want to. > > Thank you so much for checking and good suggestions. > I will follow your comments without below. > > >> + > >> + volatile uint16_t once; > >> +}; > >> + > > <...> > >> + > >> +static int > >> +new_device(struct virtio_net *dev) > >> +{ > > <...> > >> + > >> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { > >> + vq = eth_dev->data->rx_queues[i]; > >> + if (vq == NULL) > > can vq be NULL? It is allocated in rx/tx_queue_setup() and there is already a NULL check there? > > I doubt user may not setup all queues. > In this case, we need above checking. > > > > >> + continue; > >> + vq->device = dev; > >> + vq->internal = internal; > >> + rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0); > >> + } > >> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > >> + vq = eth_dev->data->tx_queues[i]; > >> + if (vq == NULL) > >> + continue; > > Same here. > > >> + vq->device = dev; > >> + vq->internal = internal; > >> + rte_vhost_enable_guest_notification(dev, vq->virtqueue_id, 0); > >> + } > >> + > >> + dev->flags |= VIRTIO_DEV_RUNNING; > >> + dev->priv = eth_dev; > >> + eth_dev->data->dev_link.link_status = 1; > >> + > >> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { > >> + vq = eth_dev->data->rx_queues[i]; > >> + if (vq == NULL) > >> + continue; > > But we can remove this. If in above loop, vq can be NULL because user not setup the queue, it will be NULL in here too, isn't it? Why we can remove NULL check here? > > >> + rte_atomic32_set(&vq->allow_queuing, 1); > >> + } > >> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > >> + vq = eth_dev->data->tx_queues[i]; > >> + if (vq == NULL) > >> + continue; > > And this. > > > <...> > >> + if (dev->data->rx_queues[i] == NULL) > >> + continue; > >> + vq = dev->data->rx_queues[i]; > >> + stats->q_ipackets[i] = vq->rx_pkts; > >> + rx_total += stats->q_ipackets[i]; > >> + > >> + stats->q_ibytes[i] = vq->rx_bytes; > >> + rx_total_bytes += stats->q_ibytes[i]; > >> + } > >> + > >> + for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && > >> + i < dev->data->nb_tx_queues; i++) { > >> + if (dev->data->tx_queues[i] == NULL) > > more queue NULL check here, I am not sure if these are necessary > > Same here, in the case user doesn't setup all queues, I will leave this. > > Anyway, I will fix below code. > - Manage ether devices list instead of internal structures list. > - Remove needless NULL checking. > - Replace "pthread_exit" to "return NULL". > - Replace rte_panic to RTE_LOG, also add error handling. > - Remove duplicated lines. > - Remove needless casting. > - Follow coding style. > - Remove needless parenthesis. > > And leave below. > - some NULL checking before accessing a queue. > > Tetsuya