From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tetsuya Mukawa Subject: Re: [PATCH v4 2/2] vhost: Add VHOST PMD Date: Tue, 24 Nov 2015 12:44:28 +0900 Message-ID: <5653DD1C.4090804@igel.co.jp> 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> <20151124034057.GG2325@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, ann.zhuangyanying@huawei.com To: Yuanhan Liu , "Xie, Huawei" Return-path: Received: from mail-pa0-f44.google.com (mail-pa0-f44.google.com [209.85.220.44]) by dpdk.org (Postfix) with ESMTP id B8E838E8F for ; Tue, 24 Nov 2015 04:44:30 +0100 (CET) Received: by pacdm15 with SMTP id dm15so6805333pac.3 for ; Mon, 23 Nov 2015 19:44:30 -0800 (PST) In-Reply-To: <20151124034057.GG2325@yliu-dev.sh.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2015/11/24 12:40, Yuanhan Liu wrote: > 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? Yes, I guess so. > > 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). Sure, it's nice idea. I will split the patch. Tetsuya > --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