From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f171.google.com (mail-pf0-f171.google.com [209.85.192.171]) by dpdk.org (Postfix) with ESMTP id 238A491BF for ; Tue, 9 Feb 2016 02:54:53 +0100 (CET) Received: by mail-pf0-f171.google.com with SMTP id e127so23603686pfe.3 for ; Mon, 08 Feb 2016 17:54:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=igel-co-jp.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-type:content-transfer-encoding; bh=onjWar7M0HbDSE1F4E1L4fsMGE1t6Ql4JSVe9gSMbW4=; b=OTr4+6bSlAUtRiSal60Cd7hx3ko0iaIQP2S7fgEAkthNlNTlwO0eQLwWplPt71HdVq KiLuoWCtah1F2i0jXKDybCFj6ZI3nN1wvXDyZ4aCFhzxuz/7W6k4ovA7TDB/m4Ujn+nV UYeVPDrBl/ewpyxOTDMmBUAtOL3UMPWKXVPwop+WCcCAakc+QvznSbYOs7OcOALI+KnJ szmXSLDTi4P3lSYka4ySnKaPdrf9hEkfH1iUep4RQMzZlq0po0DhFgD8u/pJDiruTsmf wYDiAUvOhbXt7xsXviMVeMcmhhWnD9xCAPZlJLDAFtUFpIvlEHqQjonWlAjB4gysYOxB 3BRw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=onjWar7M0HbDSE1F4E1L4fsMGE1t6Ql4JSVe9gSMbW4=; b=FsNLFzfz69ifwxY6ZphvKQLR+6tOcBpJfTNwMBo8LIY2mXoDLq4KJR9kuWqBCYwzXB WSYvnR895BryHvHw0IslP1ufw2HDjJfiG96aHu7/wbdrhkhgEo2vQwK+ss39wr7pC8KJ SJi401ZmaQ8pUbc9Aq6Z1TZ36PDw34w9454kJB1hbFvrOxATY6r6VbKVd0jxHWBuxhZ0 tPuMkwmopPa54EzwjQr9Y/+g7v7cxZWolfgnTNtJsaxHR6SKwCRzeZUWu2uuXyzqHT3C RN+Hr98g2HnWAc/vjF4LMaTVQFQGvidmuIdpY/cXSpEtpGmNDdB47QKHfIbG6wEjVS3Z 1WOA== X-Gm-Message-State: AG10YOS8DjN3Uc3kNnf4GfVcMEpAuRMgIHT+WempaPmnBwhcxvnd1DFCCpW8igGhfSJ4Eg== X-Received: by 10.98.70.211 with SMTP id o80mr46456339pfi.124.1454982892430; Mon, 08 Feb 2016 17:54:52 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by smtp.googlemail.com with ESMTPSA id s90sm40571995pfa.49.2016.02.08.17.54.50 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 08 Feb 2016 17:54:51 -0800 (PST) To: 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> <20160208094248.GA25651@sivlogin002.ir.intel.com> From: Tetsuya Mukawa Message-ID: <56B946EA.7050505@igel.co.jp> Date: Tue, 9 Feb 2016 10:54:50 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160208094248.GA25651@sivlogin002.ir.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Tue, 09 Feb 2016 01:54:53 -0000 On 2016/02/08 18:42, Ferruh Yigit wrote: > 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? Yes, you are right. Will fix it and submit again. Thanks, Tetsuya >>>> + 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