From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 2CBE08E81 for ; Tue, 24 Nov 2015 04:39:01 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP; 23 Nov 2015 19:39:01 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,338,1444719600"; d="scan'208";a="693090959" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga003.jf.intel.com with ESMTP; 23 Nov 2015 19:38:59 -0800 Date: Tue, 24 Nov 2015 11:40:57 +0800 From: Yuanhan Liu To: Tetsuya Mukawa , "Xie, Huawei" Message-ID: <20151124034057.GG2325@yliu-dev.sh.intel.com> References: <1447046221-20811-3-git-send-email-mukawa@igel.co.jp> <1447392031-24970-1-git-send-email-mukawa@igel.co.jp> <1447392031-24970-3-git-send-email-mukawa@igel.co.jp> <20151120114304.GB2325@yliu-dev.sh.intel.com> <5653CFE4.8010405@igel.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5653CFE4.8010405@igel.co.jp> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: dev@dpdk.org, ann.zhuangyanying@huawei.com Subject: Re: [dpdk-dev] [PATCH v4 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, 24 Nov 2015 03:39:01 -0000 On Tue, Nov 24, 2015 at 11:48:04AM +0900, Tetsuya Mukawa wrote: > On 2015/11/20 20:43, Yuanhan Liu wrote: > > On Fri, Nov 13, 2015 at 02:20:31PM +0900, Tetsuya Mukawa wrote: > > .... > >> +static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER; > >> + > >> +static rte_atomic16_t nb_started_ports; > >> +pthread_t session_th; > > static? > > Hi Yuanhan, > > I appreciate your carefully reviewing. > I will fix issues you commented, and submit it again. > > I added below 2 comments. > Could you please check it? > > >> + > >> +static uint16_t > >> +eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs) > >> +{ > >> + struct vhost_queue *r = q; > >> + uint16_t i, nb_tx = 0; > >> + > >> + if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0)) > >> + return 0; > >> + > >> + rte_atomic32_set(&r->while_queuing, 1); > >> + > >> + if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0)) > >> + goto out; > >> + > >> + /* Enqueue packets to guest RX queue */ > >> + nb_tx = rte_vhost_enqueue_burst(r->device, > >> + r->virtqueue_id, bufs, nb_bufs); > >> + > >> + r->tx_pkts += nb_tx; > >> + r->err_pkts += nb_bufs - nb_tx; > >> + > >> + for (i = 0; likely(i < nb_tx); i++) > >> + rte_pktmbuf_free(bufs[i]); > > We should free upto nb_bufs here, but not nb_tx, right? > > I guess we don't need to free all packet buffers. > Could you please check l2fwd_send_burst() in l2fwd example? > It seems DPDK application frees packet buffers that failed to send. Yes, you are right. I was thinking it's just a vhost app, and forgot that this is for rte_eth_tx_burst, sigh ... > > >> +static struct rte_driver pmd_vhost_drv = { > >> + .name = "eth_vhost", > >> + .type = PMD_VDEV, > >> + .init = rte_pmd_vhost_devinit, > >> + .uninit = rte_pmd_vhost_devuninit, > >> +}; > >> + > >> +struct > >> +virtio_net *rte_eth_vhost_portid2vdev(uint16_t port_id) > > struct virtio_net * > > rte_eth_vhost_portid2vdev() > > > > BTW, why making a speical eth API for virtio? This doesn't make too much > > sense to me. > > This is a kind of helper function. Yeah, I know that. I was thinking that an API prefixed with rte_eth_ should be a common interface for all eth drivers. Here this one is for vhost PMD only, though. I then had a quick check of DPDK code, and found a similar example, bond, such as rte_eth_bond_create(). So, it might be okay to introduce PMD specific eth APIs? Anyway, I would suggest you to put it into another patch, so that it can be reworked (or even dropped) if someone else doesn't like it (or doesn't think it's necessary). --yliu > > I assume that DPDK applications want to know relation between port_id > and virtio device structure. > But, in "new" callback handler that DPDK application registers, > application can receive virtio device structure, but it doesn't tell > which port is. > > To know it, probably here are steps that DPDK application needs to do. > > 1. Store interface name that is specified when vhost pmd is invoked. > (For example, store information like /tmp/socket0 is for port0, and > /tmp/socket1 is for port1) > 2. Compare above interface name and dev->ifname that is stored in virtio > device structure, then DPDK application can know which port is. > > If DPDK application uses Port Hotplug, I guess above steps are easy. > But if they don't, interface name will be specified in "--vdev" EAL > command line option. > So probably it's not so easy to handle interface name in DPDK application. > This is why I added the function. > > Thanks, > Tetsuya